Skip to content

Commit

Permalink
[Windows, Keyboard] Fix crash on Up-only IME event (flutter#33746)
Browse files Browse the repository at this point in the history 
* Impl and test

* Format and some doc

* Rewrite algorithm

* Format

* Simplify

* Fix

* Fix params
  • Loading branch information
@dkwingsmt
dkwingsmt authored Jun 16, 2022
1 parent b22cdd4 commit ee71e31
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 63 deletions.
164 changes: 103 additions & 61 deletions shell/platform/windows/keyboard_key_embedder_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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),
Expand All @@ -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;
Expand All @@ -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);
}
}
Expand Down
7 changes: 5 additions & 2 deletions shell/platform/windows/keyboard_key_embedder_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
39 changes: 39 additions & 0 deletions shell/platform/windows/keyboard_win32_unittests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down

0 comments on commit ee71e31

Please sign in to comment.