Fix: Pressing and releasing a key before the framework has started should not throw #149790

dkwingsmt · 2024-06-06T05:22:27Z

Fixes #125975, which fixes #87391.

Background

Flutter requires that the key event stream conforms "the key event rule". Simply speaking, a pointer repeat event or a pointer up event must occur only when key is considered held down according to Flutter's record, while a pointer down event must occur only when the key is considered released. In debug mode, Flutter throws an assertion on violation.

Problem

The key event rule might be broken during the hot restart, because the event is transmitted and processed asynchronously through the message channel, and at the time the framework reinitializes the keyboard record, the engine might have sent events to the framework.

During hot restart, the Dart VM is recreated but the engine isn't. The engine retains the keyboard state from the last session, meaning there can be keys pressed, but the framework's state starts from zero. Therefore the framework must query the latest keyboard state (i.e. "what keys are being pressed right now?") before assigning the keyboard handler.

However, if any key events occur between when the channelBuffers is created and when getKeyboardState returns, these keys will have been sent out by the engine, unprocessed by any channel handlers and therefore buffered, but are also included in the returned engine state, causing the error.

For example:
When Dart VM hot restarts while key A is pressed, therefore engine.pressedKeys = { 'A' }
While framework is being initialized, key F on the physical keyboard is pressed.
Engine processes the physical event, records engine.pressedKeys = {'A', 'F'} and sends out keyboard event PointerData(down, 'F')
There are currently no handlers for flutter/keyevent, therefore this key event is buffered.
The framework finally initializes the binding and _initKeyboard sends out getKeyboardState and gets the pressed state {'A', 'F'}.
The framework assigns handler to flutter/keyevent, and received PointerData(down, 'F'), and finds that F is already pressed according to its record, and asserts.

An earlier attempt #122885 tried to fix the issue by making the framework query the keyboard state right before it sets the key event handlers. This resolves most, but not all, of the issue, because the message queue can still buffer 1 event, which has been included in the init state sent by the engine and will also be sent to the event handler, causing a crash.

Solution

This PR fixes this issue by clearing the method channels before assigning channel handlers.

Remaining question

Currently there are two problems I'm not sure:

One step of the test requires waiting until the callbacks are set (before sending the 3rd event, otherwise the 3rd event will cause the event buffers to pop the 2nd event). I can't think of an elegant way other than polling the status in an infinite loop. The poll shouldn't last very long, since the process should only be a _keyboard.syncKeyboardState().then and a scheduleMicrotask, but it's still ugly.
Resizing the channel buffers currently occur in the framework. It works, but according to ChannelBuffer's doc it seems the resizing is designed to occur from the engine side. And also, since my intention is to clear the buffer (I don't care about buffer size afterwards though), should we add a clearBuffer method instead?

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

bleroux

This PR looks very promising. I'm eager to see it merged because it is a great improvement for dev experience.
Just spotted two nits.

packages/flutter/test/services/hardware_keyboard_init_test.dart

Co-authored-by: Bruno Leroux <bruno.leroux@gmail.com>

Hixie · 2024-08-01T21:38:08Z

packages/flutter/test/services/hardware_keyboard_init_test.dart

+    while (true) {
+      await Future<void>.delayed(const Duration(milliseconds: 20));


we should definitely never write code like this :-)

doesn't the framework synchronously add its listeners? (if not, why not?)

Hixie · 2024-08-01T21:39:31Z

can't we just buffer all the events and not discard them? (by having the platform side configure the channel to not discard events, by sending it the message that increases the buffer size)

Hixie · 2024-08-01T21:41:40Z

(or alternatively, if we do want to just discard all messages, can't the platform side set the buffer size to zero?)

dkwingsmt · 2024-08-01T21:45:41Z

can't we just buffer all the events and not discard them? (by having the platform side configure the channel to not discard events, by sending it the message that increases the buffer size)

I've thought about it as well. There are two reasons I don't think it's as good:

Theoretically we don't know how many events should be buffered. For example, all keys can be pressed altogether right at the hot restart, and there will be over 100 events to be buffered.
In the end the framework just wants the keyboard state, instead of past events.

dkwingsmt · 2024-08-01T21:46:19Z

(or alternatively, if we do want to just discard all messages, can't the platform side set the buffer size to zero?)

We can, but isn't this the same process? It's just moving the two lines from the framework to the engine.

dkwingsmt · 2024-08-01T21:50:02Z

I've just come up with an alternative approach: maybe we just don't recreate KeyboardManager at all so that the keyboard state never needs to be rebuilt.

Edit: no it doesn't work, since during a hot restart all states are cleared.

Hixie · 2024-08-01T23:08:27Z

(or alternatively, if we do want to just discard all messages, can't the platform side set the buffer size to zero?)

We can, but isn't this the same process? It's just moving the two lines from the framework to the engine.

The difference is that if the engine does it, there's never anything buffered, so there's nothing for the framework to worry about. Conceptually it should always be the sending side sending these buffer size messages, not the receiving side.

bleroux · 2024-08-02T06:59:52Z

The difference is that if the engine does it, there's never anything buffered, so there's nothing for the framework to worry about. Conceptually it should always be the sending side sending these buffer size messages, not the receiving side.

We tried doing that on the engine side, but there is a core issue. Here is a part of the summary I wrote in #125975 (comment)

"
Unfortunately, it turned out that this solution (setting channel buffer to 0) is not applicable to very core channels such as lifecycle and keyboard because those channels are created very early on the engine side (before the Dart isolate has started and this is a blocker because the channel resizing logic is done on the Dart side).
When experimenting with it, I reached the same issue that Greg described in #131452 (comment).
"

And I think this also applies to

can't we just buffer all the events and not discard them? (by having the platform side configure the channel to not discard events, by sending it the message that increases the buffer size)

The engine can receive some key events before the Dart isolate has started and at that time the buffering logic is not available because it is written in Dart.

doesn't the framework synchronously add its listeners? (if not, why not?)

The framework add the listeners asynchronously because it is waiting for the result of a method channel invocation to get the initial pressed state, see #122885.

Hixie · 2024-08-02T21:35:53Z

Maybe we should move the channel buffers logic up to C++? It sounds like there's a race condition between when the messages can start getting sent, and when the engine side is ready to start buffering them.

The framework add the listeners asynchronously because it is waiting for the result of a method channel invocation to get the initial pressed state, see #122885.

We probably should have some sort of callback or Future or binding API to indicate that the keyboard subsystem is ready. This all gets triggered by the binding, so maybe the test should override the binding and hook into the appropriate API to know when it's ready. (This may require some rejiggering of how the keyboard subsystem bootstraps, but that's ok. The key is to do it in a way that doesn't reserve memory in every Flutter app that's for no purpose other than serving this test, of course!)

dkwingsmt · 2024-10-07T20:05:24Z

Closing it for now since I don't have the bandwidth. Let me revisit it in the future.

Impl and test

4428e3f

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 6, 2024

bleroux reviewed Jul 31, 2024

View reviewed changes

packages/flutter/test/services/hardware_keyboard_init_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/services/hardware_keyboard_init_test.dart Outdated Show resolved Hide resolved

dkwingsmt and others added 2 commits August 1, 2024 14:25

Apply suggestions from code review

07cfd97

Co-authored-by: Bruno Leroux <bruno.leroux@gmail.com>

Merge branch 'master' into fix-key-init-2

ef7dbbd

Hixie reviewed Aug 1, 2024

View reviewed changes

Merge branch 'master' into fix-key-init-2

0ec1280

dkwingsmt closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Pressing and releasing a key before the framework has started should not throw #149790

Fix: Pressing and releasing a key before the framework has started should not throw #149790

		while (true) {
		await Future<void>.delayed(const Duration(milliseconds: 20));

Fix: Pressing and releasing a key before the framework has started should not throw #149790

Fix: Pressing and releasing a key before the framework has started should not throw #149790

Conversation

Background

Problem

Solution

Remaining question

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment