Modify assert keyboard condition on Windows to account for violation of invariant #36129
Conversation
cbracken
left a comment
There was a problem hiding this comment.
Can you add a test for this?
|
@cbracken Test what, in particular? The only edge case where this matters that I am aware of is the simulated caps lock toggle issued by Narrator, so what is it that we would actually be testing? |
Ideally we'd have an integration test that would crash without this fix. With this fix, the test would complete without triggering any assertions. This test would ensure we don't run into this crash again in the future. |
cbracken
left a comment
There was a problem hiding this comment.
lgtm modulo @loic-sharma 's nits. Thanks for fixing this one!
dkwingsmt
left a comment
There was a problem hiding this comment.
How come the bug is fixed by relaxing the assertion condition? I thought it should be done by changing how physical keys are minted when scan code is 0.
| // key is currently recorded as pressed. | ||
| if (record_iter != pressingRecords_.end()) { | ||
| pressingRecords_.erase(record_iter); | ||
| } |
There was a problem hiding this comment.
| } | |
| } else { | |
| assert(record_iter != pressingRecords_.end()); | |
| } |
There was a problem hiding this comment.
Why have this? The condition will always be false just by virtue of the fact that in enters this else statement.
There was a problem hiding this comment.
We'd still like it to assert in debug mode, but prevent erase from crashing by segfault in release mode since the error wouldn't be caught by assert. See https://discord.com/channels/608014603317936148/608020180177780791/1019033994286866433
There was a problem hiding this comment.
Are you certain? Would that assert not fail in exactly the same case that caused this issue in the first place? If we receive a keydown event while a key's state is not pressed, the synchronization will erase its pressing record before the next keyup event is processed, which will then reach and fail this assert. We would no longer crash in release mode, but I had thought our plan was to discard the redundant keyup events in these cases, not fail on them.
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
e7a866a to
11248af
Compare
| // 4 total events should be fired: | ||
| // Pre-synchronization toggle, pre-sync press, | ||
| // main event, and post-sync press. |
There was a problem hiding this comment.
You might as well expand it into detailed comparison of events. (Not necessarily for all fields, but to let people know what the events are.)
| SendEvent(key_data, KeyboardKeyEmbedderHandler::HandleResponse, | ||
| reinterpret_cast<void*>(pending_responses_[response_id].get())); | ||
|
|
||
| SynchronizeCritialPressedStates(key, physical_key, is_event_down, |
There was a problem hiding this comment.
I was thinking if it will be too slow. But it shouldn't matter since we're not comparing all keys. But I liked your previous idea where you do explicit comparison, since the only key that might have post-event synchronization is the event key. Why did you decide against that?
There was a problem hiding this comment.
The exact scenario when we want to update the pressed state and synthesize an event is more complicated than just whether the key state is pressed or not. As a method already exists to handle these conditions and perform the action we need for post sync, I am calling it
50d12dc to
cde4afc
Compare
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
…olation of invariant (flutter/engine#36129)
…of invariant (flutter#36129) * Skip assert on critical keys * Comment motivation * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Unit test keyboard change * Format * Ditto * Update shell/platform/windows/keyboard_unittests.cc Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> * Discard repeat keyup messages * Initial * Post synchronization * Explain assertion * Reassess pressing records * Formatting * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> * One space * Update shell/platform/windows/keyboard_key_embedder_handler.cc Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com> * Comment spacing Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
There is at least one edge case where an assert statement in the keyboard handler for Windows would fail in debug mode, and an absent entry in a
mapwould be erased in release, causing a segfault.When CAPS is pressed twice quickly while Narrator is open, it sends a keydown and keyup event consecutively to the window, but because the
get_key_state_handler cannot account for this simulated key event, it would be cleared from the record of pressed physical keys twice.This change checks that a key record is present before attempting to erase it, and asserts that only critical keys (including caps lock) may be absent, as only these keys can be erased prematurely.
Addresses #flutter/flutter/108731
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.