[Windows, Keyboard] Fix crash on Up-only IME event (flutter#33746)

dkwingsmt · web-flow · commit ee71e31c36ce · 2022-06-16T16:13:55.000-07:00
* Impl and test

* Format and some doc

* Rewrite algorithm

* Format

* Simplify

* Fix

* Fix params
diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc
@@ -162,25 +162,60 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl(
   const uint64_t logical_key = GetLogicalKey(key, extended, scancode);
   assert(action == WM_KEYDOWN || action == WM_KEYUP ||
          action == WM_SYSKEYDOWN || action == WM_SYSKEYUP);
-  const bool is_physical_down = action == WM_KEYDOWN || action == WM_SYSKEYDOWN;
 
   auto last_logical_record_iter = pressingRecords_.find(physical_key);
   const bool had_record = last_logical_record_iter != pressingRecords_.end();
   const uint64_t last_logical_record =
       had_record ? last_logical_record_iter->second : 0;
 
+  // The logical key for the current "tap sequence".
+  //
+  // Key events are formed in tap sequences: down, repeats, up. The logical key
+  // stays consistent throughout a tap sequence, which is this value.
+  uint64_t sequence_logical_key =
+      had_record ? last_logical_record : logical_key;
+
+  if (sequence_logical_key == VK_PROCESSKEY) {
+    // VK_PROCESSKEY means that the key press is used by an IME. These key
+    // presses are considered handled and not sent to Flutter. These events must
+    // be filtered by result_logical_key because the key up event of such
+    // presses uses the "original" logical key.
+    callback(true);
+    return;
+  }
+
+  const bool is_event_down = action == WM_KEYDOWN || action == WM_SYSKEYDOWN;
+
+  UpdateLastSeenCritialKey(key, physical_key, sequence_logical_key);
+  // Synchronize the toggled states of critical keys (such as whether CapsLocks
+  // is enabled). Toggled states can only be changed upon a down event, so if
+  // the recorded toggled state does not match the true state, this function
+  // will synthesize (an up event if the key is recorded pressed, then) a down
+  // event.
+  //
+  // After this function, all critical keys will have their toggled state
+  // updated to the true state, while the critical keys whose toggled state have
+  // been changed will be pressed regardless of their true pressed state.
+  // Updating the pressed state will be done by SynchronizeCritialPressedStates.
+  SynchronizeCritialToggledStates(key, is_event_down);
+  // Synchronize the pressed states of critical keys (such as whether CapsLocks
+  // is pressed).
+  //
+  // After this function, all critical keys except for the target key will have
+  // their toggled state and pressed state matched with their true states. The
+  // target key's pressed state will be updated immediately after this.
+  SynchronizeCritialPressedStates(key, physical_key, is_event_down);
+
   // The resulting event's `type`.
   FlutterKeyEventType type;
-  // The resulting event's `logical_key`.
-  uint64_t result_logical_key;
+  character = UndeadChar(character);
+  char character_bytes[kCharacterCacheSize];
   // What pressingRecords_[physical_key] should be after the KeyboardHookImpl
   // returns (0 if the entry should be removed).
   uint64_t eventual_logical_record;
-  char character_bytes[kCharacterCacheSize];
-
-  character = UndeadChar(character);
+  uint64_t result_logical_key;
 
-  if (is_physical_down) {
+  if (is_event_down) {
     if (had_record) {
       if (was_down) {
         // A normal repeated key.
@@ -223,35 +258,6 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl(
     }
   }
 
-  if (result_logical_key == VK_PROCESSKEY) {
-    // VK_PROCESSKEY means that the key press is used by an IME. These key
-    // presses are considered handled and not sent to Flutter. These events must
-    // be filtered by result_logical_key because the key up event of such
-    // presses uses the "original" logical key.
-    callback(true);
-    return;
-  }
-
-  UpdateLastSeenCritialKey(key, physical_key, result_logical_key);
-  // Synchronize the toggled states of critical keys (such as whether CapsLocks
-  // is enabled). Toggled states can only be changed upon a down event, so if
-  // the recorded toggled state does not match the true state, this function
-  // will synthesize (an up event if the key is recorded pressed, then) a down
-  // event.
-  //
-  // After this function, all critical keys will have their toggled state
-  // updated to the true state, while the critical keys whose toggled state have
-  // been changed will be pressed regardless of their true pressed state.
-  // Updating the pressed state will be done by SynchronizeCritialPressedStates.
-  SynchronizeCritialToggledStates(key, type == kFlutterKeyEventTypeDown);
-  // Synchronize the pressed states of critical keys (such as whether CapsLocks
-  // is pressed).
-  //
-  // After this function, all critical keys except for the target key will have
-  // their toggled state and pressed state matched with their true states. The
-  // target key's pressed state will be updated immediately after this.
-  SynchronizeCritialPressedStates(key, type != kFlutterKeyEventTypeRepeat);
-
   if (eventual_logical_record != 0) {
     pressingRecords_[physical_key] = eventual_logical_record;
   } else {
@@ -333,8 +339,10 @@ void KeyboardKeyEmbedderHandler::UpdateLastSeenCritialKey(
 }
 
 void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates(
-    int this_virtual_key,
-    bool is_down_event) {
+    int event_virtual_key,
+    bool is_event_down) {
+  //    NowState ---------------->  PreEventState --------------> TrueState
+  //              Synchronization                      Event
   for (auto& kv : critical_keys_) {
     UINT virtual_key = kv.first;
     CriticalKey& key_info = kv.second;
@@ -346,21 +354,26 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates(
 
     // Check toggling state first, because it might alter pressing state.
     if (key_info.check_toggled) {
+      const bool target_is_pressed =
+          pressingRecords_.find(key_info.physical_key) !=
+          pressingRecords_.end();
       // The togglable keys observe a 4-phase cycle:
       //
       //  Phase#   0          1          2         3
       //   Event       Down        Up        Down      Up
       // Pressed   0          1          0         1
       // Toggled   0          1          1         0
-      SHORT state = get_key_state_(virtual_key);
-      bool should_toggled = state & kStateMaskToggled;
-      if (virtual_key == this_virtual_key && is_down_event) {
-        key_info.toggled_on = !key_info.toggled_on;
+      const bool true_toggled = get_key_state_(virtual_key) & kStateMaskToggled;
+      bool pre_event_toggled = true_toggled;
+      // Check if the main event's key is the key being checked. If it's the
+      // non-repeat down event, toggle the state.
+      if (virtual_key == event_virtual_key && !target_is_pressed &&
+          is_event_down) {
+        pre_event_toggled = !pre_event_toggled;
       }
-      if (key_info.toggled_on != should_toggled) {
+      if (key_info.toggled_on != pre_event_toggled) {
         // If the key is pressed, release it first.
-        if (pressingRecords_.find(key_info.physical_key) !=
-            pressingRecords_.end()) {
+        if (target_is_pressed) {
           SendEvent(SynthesizeSimpleEvent(
                         kFlutterKeyEventTypeUp, key_info.physical_key,
                         key_info.logical_key, empty_character),
@@ -373,14 +386,26 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialToggledStates(
                                         key_info.logical_key, empty_character),
                   nullptr, nullptr);
       }
-      key_info.toggled_on = should_toggled;
+      key_info.toggled_on = true_toggled;
     }
   }
 }
 
 void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates(
-    int this_virtual_key,
-    bool pressed_state_will_change) {
+    int event_virtual_key,
+    int event_physical_key,
+    bool is_event_down) {
+  // During an incoming event, there might be a synthesized Flutter event for
+  // each key of each pressing goal, followed by an eventual main Flutter
+  // event.
+  //
+  //    NowState ---------------->  PreEventState --------------> TrueState
+  //              Synchronization                      Event
+  //
+  // The goal of the synchronization algorithm is to derive a pre-event state
+  // that can satisfy the true state (`true_pressed`) after the event, and that
+  // requires as few synthesized events based on the current state
+  // (`now_pressed`) as possible.
   for (auto& kv : critical_keys_) {
     UINT virtual_key = kv.first;
     CriticalKey& key_info = kv.second;
@@ -390,25 +415,42 @@ void KeyboardKeyEmbedderHandler::SynchronizeCritialPressedStates(
     }
     assert(key_info.logical_key != 0);
     if (key_info.check_pressed) {
-      SHORT state = get_key_state_(virtual_key);
-      auto recorded_pressed_iter = pressingRecords_.find(key_info.physical_key);
-      bool recorded_pressed = recorded_pressed_iter != pressingRecords_.end();
-      bool should_pressed = state & kStateMaskPressed;
-      if (virtual_key == this_virtual_key && pressed_state_will_change) {
-        should_pressed = !should_pressed;
+      SHORT true_pressed = get_key_state_(virtual_key) & kStateMaskPressed;
+      auto pressing_record_iter = pressingRecords_.find(key_info.physical_key);
+      bool now_pressed = pressing_record_iter != pressingRecords_.end();
+      bool pre_event_pressed = true_pressed;
+      // Check if the main event is the key being checked to get the correct
+      // target state.
+      if (is_event_down) {
+        // For down events, this key is the event key if they have the same
+        // virtual key, because virtual key represents "functionality."
+        if (virtual_key == event_virtual_key) {
+          pre_event_pressed = false;
+        }
+      } else {
+        // For up events, this key is the event key if they have the same
+        // physical key, because it is necessary to ensure that the physical
+        // key is correctly released.
+        //
+        // In that case, although the previous state should be pressed, don't
+        // synthesize a down event even if it's not. The later code will handle
+        // such cases by skipping abrupt up events. Obviously don't synthesize
+        // up events either.
+        if (event_physical_key == key_info.physical_key) {
+          continue;
+        }
       }
-      if (recorded_pressed != should_pressed) {
-        if (recorded_pressed) {
-          pressingRecords_.erase(recorded_pressed_iter);
+      if (now_pressed != pre_event_pressed) {
+        if (now_pressed) {
+          pressingRecords_.erase(pressing_record_iter);
         } else {
           pressingRecords_[key_info.physical_key] = key_info.logical_key;
         }
         const char* empty_character = "";
         SendEvent(
-            SynthesizeSimpleEvent(recorded_pressed ? kFlutterKeyEventTypeUp
-                                                   : kFlutterKeyEventTypeDown,
-                                  key_info.physical_key, key_info.logical_key,
-                                  empty_character),
+            SynthesizeSimpleEvent(
+                now_pressed ? kFlutterKeyEventTypeUp : kFlutterKeyEventTypeDown,
+                key_info.physical_key, key_info.logical_key, empty_character),
             nullptr, nullptr);
       }
     }
diff --git a/shell/platform/windows/keyboard_key_embedder_handler.h b/shell/platform/windows/keyboard_key_embedder_handler.h
@@ -114,10 +114,13 @@ class KeyboardKeyEmbedderHandler
                                 uint64_t logical_key);
   // Check each key's state from |get_key_state_| and synthesize events
   // if their toggling states have been desynchronized.
-  void SynchronizeCritialToggledStates(int virtual_key, bool is_down);
+  void SynchronizeCritialToggledStates(int event_virtual_key,
+                                       bool is_event_down);
   // Check each key's state from |get_key_state_| and synthesize events
   // if their pressing states have been desynchronized.
-  void SynchronizeCritialPressedStates(int virtual_key, bool was_down);
+  void SynchronizeCritialPressedStates(int event_virtual_key,
+                                       int event_physical_key,
+                                       bool is_event_down);
 
   // Wraps perform_send_event_ with state tracking. Use this instead of
   // |perform_send_event_| to send events to the framework.
diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc
@@ -46,6 +46,7 @@ constexpr uint64_t kScanCodeKeyO = 0x18;
 constexpr uint64_t kScanCodeKeyQ = 0x10;
 constexpr uint64_t kScanCodeKeyW = 0x11;
 constexpr uint64_t kScanCodeDigit1 = 0x02;
+constexpr uint64_t kScanCodeDigit2 = 0x03;
 constexpr uint64_t kScanCodeDigit6 = 0x07;
 // constexpr uint64_t kScanCodeNumpad1 = 0x4f;
 // constexpr uint64_t kScanCodeNumLock = 0x45;
@@ -1915,6 +1916,44 @@ TEST(KeyboardTest, ImeExtendedEventsAreIgnored) {
   EXPECT_EQ(tester.RedispatchedMessageCountAndClear(), 0);
 }
 
+// Ensures that synthesization works correctly when a Shift key is pressed and
+// (only) its up event is labeled as an IME event (VK_PROCESSKEY).
+//
+// Regression test for https://github.com/flutter/flutter/issues/104169. These
+// are real messages recorded when pressing Shift-2 using Microsoft Pinyin IME
+// on Win 10 Enterprise, which crashed the app before the fix.
+TEST(KeyboardTest, UpOnlyImeEventsAreCorrectlyHandled) {
+  KeyboardTester tester;
+  tester.Responding(true);
+
+  // US Keyboard layout.
+
+  // Press CtrlRight in IME mode.
+  tester.InjectKeyboardChanges(std::vector<KeyboardChange>{
+      KeyStateChange{VK_LSHIFT, true, false},
+      WmKeyDownInfo{VK_SHIFT, kScanCodeShiftLeft, kNotExtended, kWasUp}.Build(
+          kWmResultZero),
+      WmKeyDownInfo{VK_PROCESSKEY, kScanCodeDigit2, kNotExtended, kWasUp}.Build(
+          kWmResultZero),
+      KeyStateChange{VK_LSHIFT, false, true},
+      WmKeyUpInfo{VK_PROCESSKEY, kScanCodeShiftLeft, kNotExtended}.Build(
+          kWmResultZero),
+      WmKeyUpInfo{'2', kScanCodeDigit2, kNotExtended, kWasUp}.Build(
+          kWmResultZero)});
+
+  EXPECT_EQ(key_calls.size(), 4);
+  EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown,
+                       kPhysicalShiftLeft, kLogicalShiftLeft, "",
+                       kNotSynthesized);
+  EXPECT_CALL_IS_EVENT(key_calls[1], kFlutterKeyEventTypeDown, 0, 0, "",
+                       kNotSynthesized);
+  EXPECT_CALL_IS_EVENT(key_calls[2], kFlutterKeyEventTypeUp, kPhysicalShiftLeft,
+                       kLogicalShiftLeft, "", kNotSynthesized);
+  EXPECT_CALL_IS_EVENT(key_calls[3], kFlutterKeyEventTypeDown, 0, 0, "",
+                       kNotSynthesized);
+  clear_key_calls();
+}
+
 TEST(KeyboardTest, DisorderlyRespondedEvents) {
   KeyboardTester tester;
 
