Fix race condition in key event handling on Android (flutter#22658)

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 the event itself to tell if it was redispatched.

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.

shell/platform/android/io/flutter/embedding/android/AndroidKeyProcessor.java

@@ -12,10 +12,8 @@
 import io.flutter.Log;
 import io.flutter.embedding.engine.systemchannels.KeyEventChannel;
 import io.flutter.plugin.editing.TextInputPlugin;
-import java.util.AbstractMap.SimpleImmutableEntry;
 import java.util.ArrayDeque;
 import java.util.Deque;
-import java.util.Map.Entry;
 
 /**
  * A class to process key events from Android, passing them to the framework as messages using
@@ -33,7 +31,6 @@
  */
 public class AndroidKeyProcessor {
   private static final String TAG = "AndroidKeyProcessor";
-  private static long eventIdSerial = 0;
 
   @NonNull private final KeyEventChannel keyEventChannel;
   @NonNull private final TextInputPlugin textInputPlugin;
@@ -50,8 +47,8 @@ public class AndroidKeyProcessor {
    * <p>It is possible that that in the middle of the async round trip, the focus chain could
    * change, and instead of the native widget that was "next" when the event was fired getting the
    * event, it may be the next widget when the event is synthesized that gets it. In practice, this
-   * shouldn't be a huge problem, as this is an unlikely occurance to happen without user input, and
-   * it may actually be desired behavior, but it is possible.
+   * shouldn't be a huge problem, as this is an unlikely occurrence to happen without user input,
+   * and it may actually be desired behavior, but it is possible.
    *
    * @param view takes the activity to use for re-dispatching of events that were not handled by the
    *     framework.
@@ -96,23 +93,39 @@ public boolean onKeyEvent(@NonNull KeyEvent keyEvent) {
       // case the theory is wrong.
       return false;
     }
-    if (eventResponder.dispatchingKeyEvent) {
-      // Don't handle it if it is from our own delayed event dispatch.
+    if (eventResponder.isHeadEvent(keyEvent)) {
+      // If the keyEvent is at the head of the queue of pending events we've seen,
+      // and has the same id, then we know that this is a re-dispatched keyEvent, and
+      // we shouldn't respond to it, but we should remove it from tracking now.
+      eventResponder.removeHeadEvent();
       return false;
     }
 
     Character complexCharacter = applyCombiningCharacterToBaseCharacter(keyEvent.getUnicodeChar());
     KeyEventChannel.FlutterKeyEvent flutterEvent =
-        new KeyEventChannel.FlutterKeyEvent(keyEvent, complexCharacter, eventIdSerial++);
+        new KeyEventChannel.FlutterKeyEvent(keyEvent, complexCharacter);
+
+    eventResponder.addEvent(keyEvent);
     if (action == KeyEvent.ACTION_DOWN) {
       keyEventChannel.keyDown(flutterEvent);
     } else {
       keyEventChannel.keyUp(flutterEvent);
     }
-    eventResponder.addEvent(flutterEvent.eventId, keyEvent);
     return true;
   }
 
+  /**
+   * Returns whether or not the given event is currently being processed by this key processor. This
+   * is used to determine if a new key event sent to the {@link InputConnectionAdaptor} originates
+   * from a hardware key event, or a soft keyboard editing event.
+   *
+   * @param event the event to check for being the current event.
+   * @return
+   */
+  public boolean isCurrentEvent(@NonNull KeyEvent event) {
+    return eventResponder.isHeadEvent(event);
+  }
+
   /**
    * Applies the given Unicode character in {@code newCharacterCodePoint} to a previously entered
    * Unicode combining character and returns the combination of these characters if a combination
@@ -176,65 +189,63 @@ private static class EventResponder implements KeyEventChannel.EventResponseHand
     // The maximum number of pending events that are held before starting to
     // complain.
     private static final long MAX_PENDING_EVENTS = 1000;
-    final Deque<Entry<Long, KeyEvent>> pendingEvents = new ArrayDeque<Entry<Long, KeyEvent>>();
+    final Deque<KeyEvent> pendingEvents = new ArrayDeque<KeyEvent>();
     @NonNull private final View view;
     @NonNull private final TextInputPlugin textInputPlugin;
-    boolean dispatchingKeyEvent = false;
 
     public EventResponder(@NonNull View view, @NonNull TextInputPlugin textInputPlugin) {
       this.view = view;
       this.textInputPlugin = textInputPlugin;
     }
 
-    /**
-     * Removes the pending event with the given id from the cache of pending events.
-     *
-     * @param id the id of the event to be removed.
-     */
-    private KeyEvent removePendingEvent(long id) {
-      if (pendingEvents.getFirst().getKey() != id) {
+    /** Removes the first pending event from the cache of pending events. */
+    private KeyEvent removeHeadEvent() {
+      return pendingEvents.removeFirst();
+    }
+
+    private KeyEvent checkIsHeadEvent(KeyEvent event) {
+      if (pendingEvents.size() == 0) {
+        throw new AssertionError(
+            "Event response received when no events are in the queue. Received event " + event);
+      }
+      if (pendingEvents.getFirst() != event) {
         throw new AssertionError(
             "Event response received out of order. Should have seen event "
-                + pendingEvents.getFirst().getKey()
+                + pendingEvents.getFirst()
                 + " first. Instead, received "
-                + id);
+                + event);
       }
-      return pendingEvents.removeFirst().getValue();
+      return pendingEvents.getFirst();
+    }
+
+    private boolean isHeadEvent(KeyEvent event) {
+      return pendingEvents.size() > 0 && pendingEvents.getFirst() == event;
     }
 
     /**
      * Called whenever the framework responds that a given key event was handled by the framework.
      *
-     * @param id the event id of the event to be marked as being handled by the framework. Must not
-     *     be null.
+     * @param event the event to be marked as being handled by the framework. Must not be null.
      */
     @Override
-    public void onKeyEventHandled(long id) {
-      removePendingEvent(id);
+    public void onKeyEventHandled(KeyEvent event) {
+      removeHeadEvent();
     }
 
     /**
      * Called whenever the framework responds that a given key event wasn't handled by the
      * framework.
      *
-     * @param id the event id of the event to be marked as not being handled by the framework. Must
-     *     not be null.
+     * @param event the event to be marked as not being handled by the framework. Must not be null.
      */
     @Override
-    public void onKeyEventNotHandled(long id) {
-      dispatchKeyEvent(removePendingEvent(id));
+    public void onKeyEventNotHandled(KeyEvent event) {
+      redispatchKeyEvent(checkIsHeadEvent(event));
     }
 
-    /** Adds an Android key event with an id to the event responder to wait for a response. */
-    public void addEvent(long id, @NonNull KeyEvent event) {
-      if (pendingEvents.size() > 0 && pendingEvents.getFirst().getKey() >= id) {
-        throw new AssertionError(
-            "New events must have ids greater than the most recent pending event. New id "
-                + id
-                + " is less than or equal to the last event id of "
-                + pendingEvents.getFirst().getKey());
-      }
-      pendingEvents.addLast(new SimpleImmutableEntry<Long, KeyEvent>(id, event));
+    /** Adds an Android key event to the event responder to wait for a response. */
+    public void addEvent(@NonNull KeyEvent event) {
+      pendingEvents.addLast(event);
       if (pendingEvents.size() > MAX_PENDING_EVENTS) {
         Log.e(
             TAG,
@@ -250,27 +261,21 @@ public void addEvent(long id, @NonNull KeyEvent event) {
      *
      * @param event the event to be dispatched to the activity.
      */
-    public void dispatchKeyEvent(KeyEvent event) {
+    private void redispatchKeyEvent(KeyEvent event) {
       // If the textInputPlugin is still valid and accepting text, then we'll try
       // and send the key event to it, assuming that if the event can be sent,
       // that it has been handled.
-      if (textInputPlugin.getLastInputConnection() != null
-          && textInputPlugin.getInputMethodManager().isAcceptingText()) {
-        dispatchingKeyEvent = true;
-        boolean handled = textInputPlugin.getLastInputConnection().sendKeyEvent(event);
-        dispatchingKeyEvent = false;
-        if (handled) {
-          return;
-        }
+      if (textInputPlugin.getInputMethodManager().isAcceptingText()
+          && textInputPlugin.getLastInputConnection() != null
+          && textInputPlugin.getLastInputConnection().sendKeyEvent(event)) {
+        // The event was handled, so we can remove it from the queue.
+        removeHeadEvent();
+        return;
       }
 
       // Since the framework didn't handle it, dispatch the event again.
       if (view != null) {
-        // Turn on dispatchingKeyEvent so that we don't dispatch to ourselves and
-        // send it to the framework again.
-        dispatchingKeyEvent = true;
         view.getRootView().dispatchKeyEvent(event);
-        dispatchingKeyEvent = false;
       }
     }
   }

shell/platform/android/io/flutter/embedding/android/FlutterView.java

@@ -739,11 +739,6 @@ public boolean dispatchKeyEvent(KeyEvent event) {
     } else if (event.getAction() == KeyEvent.ACTION_UP) {
       // Stop tracking the event.
       getKeyDispatcherState().handleUpEvent(event);
-      if (!event.isTracking() || event.isCanceled()) {
-        // Don't send the event to the key processor if it was canceled, or no
-        // longer being tracked.
-        return super.dispatchKeyEvent(event);
-      }
     }
     // If the key processor doesn't handle it, then send it on to the
     // superclass. The key processor will typically handle all events except