Fix race condition in key event handling on Android.

Author: gspencergoog

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

@@ -11,6 +11,7 @@
 import androidx.annotation.Nullable;
 import io.flutter.Log;
 import io.flutter.embedding.engine.systemchannels.KeyEventChannel;
+import io.flutter.embedding.engine.systemchannels.KeyEventChannel.FlutterKeyEvent;
 import io.flutter.plugin.editing.TextInputPlugin;
 import java.util.AbstractMap.SimpleImmutableEntry;
 import java.util.ArrayDeque;
@@ -33,7 +34,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 +50,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.
@@ -88,31 +88,49 @@ public void destroy() {
    * @return true if the key event should not be propagated to other Android components. Delayed
    *     synthesis events will return false, so that other components may handle them.
    */
-  public boolean onKeyEvent(@NonNull KeyEvent keyEvent) {
-    int action = keyEvent.getAction();
-    if (action != KeyEvent.ACTION_DOWN && action != KeyEvent.ACTION_UP) {
+  public boolean onKeyEvent(@NonNull KeyEvent event) {
+    int action = event.getAction();
+    if (action != event.ACTION_DOWN && action != event.ACTION_UP) {
       // There is theoretically a KeyEvent.ACTION_MULTIPLE, but theoretically
       // that isn't sent by Android anymore, so this is just for protection in
       // case the theory is wrong.
       return false;
     }
-    if (eventResponder.dispatchingKeyEvent) {
-      // Don't handle it if it is from our own delayed event dispatch.
+    long eventId = FlutterKeyEvent.computeEventId(event);
+    if (eventResponder.isHeadEvent(eventId)) {
+      // If the event 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 event, and
+      // we shouldn't respond to it, but we should remove it from tracking now.
+      eventResponder.removePendingEvent(eventId);
       return false;
     }
 
-    Character complexCharacter = applyCombiningCharacterToBaseCharacter(keyEvent.getUnicodeChar());
+    Character complexCharacter = applyCombiningCharacterToBaseCharacter(event.getUnicodeChar());
     KeyEventChannel.FlutterKeyEvent flutterEvent =
-        new KeyEventChannel.FlutterKeyEvent(keyEvent, complexCharacter, eventIdSerial++);
+        new KeyEventChannel.FlutterKeyEvent(event, complexCharacter);
+
+    eventResponder.addEvent(flutterEvent.eventId, event);
     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) {
+    long id = FlutterKeyEvent.computeEventId(event);
+    return eventResponder.isHeadEvent(id);
+  }
+
   /**
    * 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
@@ -179,7 +197,6 @@ private static class EventResponder implements KeyEventChannel.EventResponseHand
     final Deque<Entry<Long, KeyEvent>> pendingEvents = new ArrayDeque<Entry<Long, 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;
@@ -202,6 +219,25 @@ private KeyEvent removePendingEvent(long id) {
       return pendingEvents.removeFirst().getValue();
     }
 
+    private KeyEvent findPendingEvent(long id) {
+      if (pendingEvents.size() == 0) {
+        throw new AssertionError(
+            "Event response received when no events are in the queue. Received id " + id);
+      }
+      if (pendingEvents.getFirst().getKey() != id) {
+        throw new AssertionError(
+            "Event response received out of order. Should have seen event "
+                + pendingEvents.getFirst().getKey()
+                + " first. Instead, received "
+                + id);
+      }
+      return pendingEvents.getFirst().getValue();
+    }
+
+    private boolean isHeadEvent(long id) {
+      return pendingEvents.size() > 0 && pendingEvents.getFirst().getKey() == id;
+    }
+
     /**
      * Called whenever the framework responds that a given key event was handled by the framework.
      *
@@ -222,18 +258,11 @@ public void onKeyEventHandled(long id) {
      */
     @Override
     public void onKeyEventNotHandled(long id) {
-      dispatchKeyEvent(removePendingEvent(id));
+      dispatchKeyEvent(findPendingEvent(id), id);
     }
 
     /** 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));
       if (pendingEvents.size() > MAX_PENDING_EVENTS) {
         Log.e(
@@ -250,27 +279,21 @@ public void addEvent(long id, @NonNull KeyEvent event) {
      *
      * @param event the event to be dispatched to the activity.
      */
-    public void dispatchKeyEvent(KeyEvent event) {
+    public void dispatchKeyEvent(KeyEvent event, long id) {
       // 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.
+        removePendingEvent(id);
+        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

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

@@ -225,16 +225,29 @@ public static class FlutterKeyEvent {
      * The unique id for this Flutter key event.
      *
      * <p>This id is used to identify pending events when results are received from the framework.
-     * This ID does not come from Android.
+     * This ID does not come from Android, but instead is derived from attributes of this event that
+     * can be recognized if the event is re-dispatched.
      */
     public final long eventId;
 
-    public FlutterKeyEvent(@NonNull KeyEvent androidKeyEvent, long eventId) {
-      this(androidKeyEvent, null, eventId);
+    /**
+     * Creates a unique id for a Flutter key event from an Android KeyEvent.
+     *
+     * <p>This id is used to identify pending key events when results are received from the
+     * framework.
+     */
+    public static long computeEventId(@NonNull KeyEvent event) {
+      return (event.getEventTime() & 0xffffffff)
+          | ((long) (event.getAction() == KeyEvent.ACTION_DOWN ? 0x1 : 0x0)) << 32
+          | ((long) (event.getScanCode() & 0xffff)) << 33;
+    }
+
+    public FlutterKeyEvent(@NonNull KeyEvent androidKeyEvent) {
+      this(androidKeyEvent, null);
     }
 
     public FlutterKeyEvent(
-        @NonNull KeyEvent androidKeyEvent, @Nullable Character complexCharacter, long eventId) {
+        @NonNull KeyEvent androidKeyEvent, @Nullable Character complexCharacter) {
       this(
           androidKeyEvent.getDeviceId(),
           androidKeyEvent.getFlags(),
@@ -246,7 +259,7 @@ public FlutterKeyEvent(
           androidKeyEvent.getMetaState(),
           androidKeyEvent.getSource(),
           androidKeyEvent.getRepeatCount(),
-          eventId);
+          computeEventId(androidKeyEvent));
     }
 
     public FlutterKeyEvent(

shell/platform/android/io/flutter/plugin/editing/InputConnectionAdaptor.java

@@ -285,13 +285,22 @@ private static int clampIndexToEditable(int index, Editable editable) {
     return clamped;
   }
 
+  // This function is called both when hardware key events occur and aren't
+  // handled by the framework, as well as when soft keyboard editing events
+  // occur, and need a chance to be handled by the framework.
   @Override
   public boolean sendKeyEvent(KeyEvent event) {
-    // Give the key processor a chance to process this event.  It will send it
-    // to the framework to be handled and return true. If the framework ends up
-    // not handling it, the processor will re-send the event, this time
-    // returning false so that it can be processed here.
-    if (keyProcessor != null && keyProcessor.onKeyEvent(event)) {
+    // This gives the key processor a chance to process this event if it came
+    // from a soft keyboard. It will send it to the framework to be handled and
+    // return true. If the framework ends up not handling it, the processor will
+    // re-send the event to this function. Only do this if the event is not the
+    // current event, since that indicates that the key processor sent it to us,
+    // and we only want to call the key processor for events that it doesn't
+    // already know about (i.e. when events arrive here from a soft keyboard and
+    // not a hardware keyboard), to avoid a loop.
+    if (keyProcessor != null
+        && !keyProcessor.isCurrentEvent(event)
+        && keyProcessor.onKeyEvent(event)) {
       return true;
     }
 

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

@@ -21,7 +21,6 @@
 import android.util.AttributeSet;
 import android.util.SparseArray;
 import android.view.DisplayCutout;
-import android.view.KeyEvent;
 import android.view.MotionEvent;
 import android.view.PointerIcon;
 import android.view.Surface;
@@ -268,12 +267,6 @@ public DartExecutor getDartExecutor() {
     return dartExecutor;
   }
 
-  @Override
-  public boolean dispatchKeyEventPreIme(KeyEvent event) {
-    return (isAttached() && androidKeyProcessor.onKeyEvent(event))
-        || super.dispatchKeyEventPreIme(event);
-  }
-
   public FlutterNativeView getFlutterNativeView() {
     return mNativeView;
   }

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/KeyEventChannelTest.java

@@ -62,7 +62,7 @@ public void onKeyEventNotHandled(@NonNull long id) {
 
     KeyEvent event = new FakeKeyEvent(KeyEvent.ACTION_DOWN, 65);
     KeyEventChannel.FlutterKeyEvent flutterKeyEvent =
-        new KeyEventChannel.FlutterKeyEvent(event, null, 10);
+        new KeyEventChannel.FlutterKeyEvent(event, null);
     keyEventChannel.keyDown(flutterKeyEvent);
     ArgumentCaptor<ByteBuffer> byteBufferArgumentCaptor = ArgumentCaptor.forClass(ByteBuffer.class);
     ArgumentCaptor<BinaryMessenger.BinaryReply> replyArgumentCaptor =
@@ -103,7 +103,7 @@ public void onKeyEventNotHandled(long id) {
 
     KeyEvent event = new FakeKeyEvent(KeyEvent.ACTION_UP, 65);
     KeyEventChannel.FlutterKeyEvent flutterKeyEvent =
-        new KeyEventChannel.FlutterKeyEvent(event, null, 10);
+        new KeyEventChannel.FlutterKeyEvent(event, null);
     keyEventChannel.keyUp(flutterKeyEvent);
     ArgumentCaptor<ByteBuffer> byteBufferArgumentCaptor = ArgumentCaptor.forClass(ByteBuffer.class);
     ArgumentCaptor<BinaryMessenger.BinaryReply> replyArgumentCaptor =

shell/platform/android/test/io/flutter/plugin/editing/InputConnectionAdaptorTest.java

@@ -9,6 +9,7 @@
 import static org.mockito.Mockito.anyString;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -1028,19 +1029,47 @@ public void testCursorAnchorInfo() {
     assertNull(testImm.lastCursorAnchorInfo);
   }
 
+  @Test
+  public void testSendKeyEvent_sendSoftKeyEvents() {
+    ListenableEditingState editable = sampleEditable(5, 5);
+    AndroidKeyProcessor mockKeyProcessor = mock(AndroidKeyProcessor.class);
+    when(mockKeyProcessor.isCurrentEvent(any())).thenReturn(true);
+    InputConnectionAdaptor adaptor = sampleInputConnectionAdaptor(editable, mockKeyProcessor);
+
+    KeyEvent shiftKeyDown = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_SHIFT_LEFT);
+
+    boolean didConsume = adaptor.sendKeyEvent(shiftKeyDown);
+    assertFalse(didConsume);
+    verify(mockKeyProcessor, never()).onKeyEvent(shiftKeyDown);
+  }
+
+  @Test
+  public void testSendKeyEvent_sendHardwareKeyEvents() {
+    ListenableEditingState editable = sampleEditable(5, 5);
+    AndroidKeyProcessor mockKeyProcessor = mock(AndroidKeyProcessor.class);
+    when(mockKeyProcessor.isCurrentEvent(any())).thenReturn(false);
+    when(mockKeyProcessor.onKeyEvent(any())).thenReturn(true);
+    InputConnectionAdaptor adaptor = sampleInputConnectionAdaptor(editable, mockKeyProcessor);
+
+    KeyEvent shiftKeyDown = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_SHIFT_LEFT);
+
+    boolean didConsume = adaptor.sendKeyEvent(shiftKeyDown);
+    assertTrue(didConsume);
+    verify(mockKeyProcessor, times(1)).onKeyEvent(shiftKeyDown);
+  }
+
   @Test
   public void testSendKeyEvent_delKeyNotConsumed() {
-    int selStart = 29;
-    ListenableEditingState editable = sampleEditable(selStart, selStart, SAMPLE_RTL_TEXT);
+    ListenableEditingState editable = sampleEditable(5, 5);
     InputConnectionAdaptor adaptor = sampleInputConnectionAdaptor(editable);
 
     KeyEvent downKeyDown = new KeyEvent(KeyEvent.ACTION_DOWN, KeyEvent.KEYCODE_DEL);
 
-    for (int i = 0; i < 10; i++) {
+    for (int i = 0; i < 4; i++) {
       boolean didConsume = adaptor.sendKeyEvent(downKeyDown);
       assertFalse(didConsume);
     }
-    assertEquals(29, Selection.getSelectionStart(editable));
+    assertEquals(5, Selection.getSelectionStart(editable));
   }
 
   @Test
@@ -1097,11 +1126,15 @@ private static ListenableEditingState sampleEditable(int selStart, int selEnd, S
 
   private static InputConnectionAdaptor sampleInputConnectionAdaptor(
       ListenableEditingState editable) {
+    return sampleInputConnectionAdaptor(editable, mock(AndroidKeyProcessor.class));
+  }
+
+  private static InputConnectionAdaptor sampleInputConnectionAdaptor(
+      ListenableEditingState editable, AndroidKeyProcessor mockKeyProcessor) {
     View testView = new View(RuntimeEnvironment.application);
     int client = 0;
     TextInputChannel textInputChannel = mock(TextInputChannel.class);
     FlutterJNI mockFlutterJNI = mock(FlutterJNI.class);
-    AndroidKeyProcessor mockKeyProcessor = mock(AndroidKeyProcessor.class);
     when(mockFlutterJNI.nativeFlutterTextUtilsIsEmoji(anyInt()))
         .thenAnswer((invocation) -> Emoji.isEmoji((int) invocation.getArguments()[0]));
     when(mockFlutterJNI.nativeFlutterTextUtilsIsEmojiModifier(anyInt()))