Fix race condition in key event handling on Android

[gspencergoog]

Description

This fixes a problem in Android key event handling where, because I was only using a single bool to indicate that we were re-dispatching, there was a race condition when multiple keys were pending (sent to the framework, awaiting responses).

This fixes that by switching to a mechanism that uses a fingerprint calculated from the event information itself (a combination of event action (up/down), scan code, and event timestamp).

In doing this, I realized that because key events can come from either the dispatchEvent call, or through the InputConnectionAdaptor, I needed to handle both routes properly so that the events would all be handled, and all go through the same mechanism on the framework side.

Related Issues

• Android key event handling is racy flutter#70908

Tests

• Added tests to make sure that key events received through the InputConnectionAdaptor get sent to the AndroidKeyProcessor, but when the key processor send the events to the InputConnection, it doesn't get them back again.

Breaking Change

• No, no existing tests failed, so this is not a breaking change.

[LongCatIsLooong]

getEventTime() seems to return a long, and according to this stackoverflow question the bit mask is not going to work because of sign extension.

[LongCatIsLooong]

Do we still need an id? Is it possible to use == instead since we are re-dispatching the same KeyEvent instance?

[gspencergoog]

Hmm. I hadn't considered that, but perhaps we can do that. I'll give it a try.

[gspencergoog]

Actually, we do need some kind of ID, because I need to be able to identify the event when it comes back from the framework, but I can probably just use event.hashCode() instead of figuring out my own ID.

[LongCatIsLooong]

I thought the framework response only contains a "handled" field, so we actually have access to the original FlutterKeyEvent object?

engine/shell/platform/android/io/flutter/embedding/engine/systemchannels/KeyEventChannel.java

Lines 76 to 119 in 7846f29

	BasicMessageChannel.Reply<Object> createReplyHandler(long eventId) {
	return message -> {
	if (eventResponseHandler == null) {
	return;
	}
	try {
	if (message == null) {
	eventResponseHandler.onKeyEventNotHandled(eventId);
	return;
	}
	final JSONObject annotatedEvent = (JSONObject) message;
	final boolean handled = annotatedEvent.getBoolean("handled");
	if (handled) {
	eventResponseHandler.onKeyEventHandled(eventId);
	} else {
	eventResponseHandler.onKeyEventNotHandled(eventId);
	}
	} catch (JSONException e) {
	Log.e(TAG, "Unable to unpack JSON message: " + e);
	eventResponseHandler.onKeyEventNotHandled(eventId);
	}
	};
	}
	@NonNull public final BasicMessageChannel<Object> channel;
	public void keyUp(@NonNull FlutterKeyEvent keyEvent) {
	Map<String, Object> message = new HashMap<>();
	message.put("type", "keyup");
	message.put("keymap", "android");
	encodeKeyEvent(keyEvent, message);
	channel.send(message, createReplyHandler(keyEvent.eventId));
	}
	public void keyDown(@NonNull FlutterKeyEvent keyEvent) {
	Map<String, Object> message = new HashMap<>();
	message.put("type", "keydown");
	message.put("keymap", "android");
	encodeKeyEvent(keyEvent, message);
	channel.send(message, createReplyHandler(keyEvent.eventId));
	}

[gspencergoog]

Ahh, good point. The FlutterKeyEvent doesn't include the original Android KeyEvent, so I'll switch to using the FlutterKeyEvent as the ID.

[gspencergoog]

I simplified the FlutterKeyEvent to just have what it needed and keep the Android KeyEvent it was created with.

[LongCatIsLooong]

In the case that the even is consumed by some other view before reaching our FlutterView (not sure if possible, maybe in add-to-app?), it will never be removed from the queue?

[gspencergoog]

Hmm. I was assuming that it wasn't possible: the event should be given to the view again, but I suppose it might be possible to construct an add2app scenario where a view was inserted before this view, and then have it handle the event before the view received it again.

I'm not sure how to prevent that. At the moment, it will assert after that happens, since the events will begin to arrive out of order. Perhaps that assert should just be a log message instead of an assertion?

One way to address this would be to just keep track of at most the last 1000 events, and not do any checking to make sure that they arrive in order. Then, even if this happens, there is a limited amount of memory it can waste. It's unlikely in normal operation that 1000 key events would be produced before the framework can respond to any of them. It would be slightly slower, since it would have to do a hash map lookup to find the event instead of assuming that it was at the head of the deque.

[LongCatIsLooong]

Is it possible to directly deliver the event to the FlutterView since this is a re-dispatch?

[gspencergoog]

I wish, but no. If I don't deliver it to the root view, then the back button doesn't get processed.

[LongCatIsLooong]

Oh I see. The root view dispatches the event to the activity.

[LongCatIsLooong]

Oh I see. The root view dispatches the event to the activity.