Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle out-of-order add/remove pointer events #55740

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

hbatagelo
Copy link
Contributor

@hbatagelo hbatagelo commented Oct 8, 2024

Fixes #146251.

On Windows, a "pointer add" event to a new view is sometimes sent before the "pointer remove" event from the old view. When PointerDataPacketConverter handles the add event, it asserts that the device's pointer state has been cleared. Since the pointer state is cleared only when the remove event is processed, the assertion fails if the events arrive out of order.

The solution proposed here is to synthesize a remove event if an add event is received while the pointer state hasn't been cleared (i.e., if the pointer is added without being removed from the previous view).

To avoid duplicate remove events, the view ID is now tracked in PointerState. If the original "remove" arrives after the synthesized one, the state's view ID will differ from the pointer data's view ID, allowing it to be safely ignored.

When synthesizing the remove event, it's possible that the old view has already been destroyed, meaning no remove event will be received later. For example, this occurs when a window is destroyed while the pointer is inside it. However, the framework expects an "add" event to always follow a "remove" event, and "remove" events with invalid views are ignored. To meet the framework's expectations, the view ID of the "add" event will be used for the "remove" event if the old view has been destroyed.

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 and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

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

@chinmaygarde
Copy link
Member

Triage: @loic-sharma, do you how to direct this to reviewers?

UpdatePointerIdentifier(synthesized_cancel_event, state, false);

state.is_down = false;
converted_pointers.push_back(synthesized_cancel_event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with this code. Could you explain why the cancel event is synthesized only if the view exists? That's not immediately obvious to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replicated the behavior of the non-synthesized/original remove event. When a non-synthesized "remove" is handled, a "cancel" is synthesized if the pointer is down, and a "hover" is synthesized if the pointer location has changed. Here, I replicate the synthesized "cancel" event, but not the "hover" event, since the pointer location is unchanged.

My reasoning for synthesizing the "cancel" event only for valid views is that the framework will ignore events for non-existing views.

@loic-sharma
Copy link
Member

Thanks for the excellent contribution! This patch looks good to me, but I'm not very familiar with this area. @dkwingsmt or @moffatman could you give this a review?

@loic-sharma
Copy link
Member

FYI, your PR failed the following checks:

Errors...
[5735/6381] ACTION //flutter/lib/snapshot:generate_snapshot_bin(//build/toolchain/mac:ios_clang_arm)
FAILED: gen/flutter/lib/snapshot/vm_isolate_snapshot.bin gen/flutter/lib/snapshot/vm_snapshot_instructions.bin gen/flutter/lib/snapshot/isolate_snapshot.bin gen/flutter/lib/snapshot/isolate_snapshot_instructions.bin 
vpython3 ../../build/gn_run_binary.py clang_arm64/gen_snapshot --snapshot_kind=core --enable_mirrors=false --vm_snapshot_data=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/vm_isolate_snapshot.bin --vm_snapshot_instructions=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/vm_snapshot_instructions.bin --isolate_snapshot_data=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/isolate_snapshot.bin --isolate_snapshot_instructions=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/isolate_snapshot_instructions.bin /Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/flutter_patched_sdk/platform_strong.dill
Command failed: ./clang_arm64/gen_snapshot --snapshot_kind=core --enable_mirrors=false --vm_snapshot_data=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/vm_isolate_snapshot.bin --vm_snapshot_instructions=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/vm_snapshot_instructions.bin --isolate_snapshot_data=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/isolate_snapshot.bin --isolate_snapshot_instructions=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/lib/snapshot/isolate_snapshot_instructions.bin /Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/flutter_patched_sdk/platform_strong.dill
exitCode: -9

[5736/6381] ACTION //flutter/testing/scenario_app:_scenario_app_snapshot_snapshot(//build/toolchain/mac:ios_clang_arm)
FAILED: gen/flutter/testing/scenario_app/ios/snapshot_assembly.S 
vpython3 ../../build/gn_run_binary.py clang_arm64/gen_snapshot --deterministic --snapshot_kind=app-aot-assembly --assembly=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/testing/scenario_app/ios/snapshot_assembly.S /Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/testing/scenario_app/kernel_blob.bin
Command failed: ./clang_arm64/gen_snapshot --deterministic --snapshot_kind=app-aot-assembly --assembly=/Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/testing/scenario_app/ios/snapshot_assembly.S /Volumes/Work/s/w/ir/cache/builder/src/out/ios_profile/gen/flutter/testing/scenario_app/kernel_blob.bin
exitCode: -9
[19/104] ShellTest.EncodeImageRetryOverflows (986 ms)
[INFO:flutter/testing/test_timeout_listener.cc(76)] Test timeout of 300 seconds per test case will be enforced.
�[0;33mNote: Google Test filter = ShellTest.EncodeImageRetryOverflows
�[m�[0;32m[==========] �[mRunning 1 test from 1 test suite.
�[0;32m[----------] �[mGlobal test environment set-up.
�[0;32m[----------] �[m1 test from ShellTest
�[0;32m[ RUN      ] �[mShellTest.EncodeImageRetryOverflows
[19/104] ShellTest.EncodeImageRetryOverflows returned/aborted with exit code -11 (986 ms)

These failures don't look related to your change. I've retried these failing checks. If the re-runs fail, I would try rebasing your changes off the latest main commit.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome change with awesome doc, thank you so much!

Copy link
Member

@loic-sharma loic-sharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the wonderful contribution!

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 16, 2024
@auto-submit auto-submit bot merged commit a5f2a1a into flutter:main Oct 16, 2024
31 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 16, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 17, 2024
…ions) (#157066)

Manual roll requested by [email protected]

Cannot build log URL because revision "f12e0d385f4d" is invalid: Luci builds of "Linux Web Framework tests" for f12e0d385f4d64765ed46c7c3ac8fa7fa8bc3c43 was FAILURE

2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "update the repo references for package:file (#55906)" (flutter/engine#55916)
2024-10-16 [email protected] Roll Skia from 09917e06158b to 3a081993e2a7 (3 revisions) (flutter/engine#55911)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from d916a5f69a48 to 2bf0f2b8d391 (24 revisions) (#55884)" (flutter/engine#55915)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from 2bf0f2b8d391 to 7fce3544047c (4 revisions) (#55896)" (flutter/engine#55914)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7fce3544047c to 148b7c530657 (3 revisions) (#55903)" (flutter/engine#55913)
2024-10-16 [email protected] update the repo references for package:file (flutter/engine#55906)
2024-10-16 [email protected] Handle out-of-order add/remove pointer events (flutter/engine#55740)
2024-10-16 [email protected] Starts looking for the bdf fast path in relation to the snapshot_entity's transform (flutter/engine#55890)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Lurchfresser pushed a commit to Lurchfresser/flutter that referenced this pull request Oct 17, 2024
…ions) (flutter#157066)

Manual roll requested by [email protected]

Cannot build log URL because revision "f12e0d385f4d" is invalid: Luci builds of "Linux Web Framework tests" for f12e0d385f4d64765ed46c7c3ac8fa7fa8bc3c43 was FAILURE

2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "update the repo references for package:file (flutter#55906)" (flutter/engine#55916)
2024-10-16 [email protected] Roll Skia from 09917e06158b to 3a081993e2a7 (3 revisions) (flutter/engine#55911)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from d916a5f69a48 to 2bf0f2b8d391 (24 revisions) (flutter#55884)" (flutter/engine#55915)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Manual roll Dart SDK from 2bf0f2b8d391 to 7fce3544047c (4 revisions) (flutter#55896)" (flutter/engine#55914)
2024-10-16 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 7fce3544047c to 148b7c530657 (3 revisions) (flutter#55903)" (flutter/engine#55913)
2024-10-16 [email protected] update the repo references for package:file (flutter/engine#55906)
2024-10-16 [email protected] Handle out-of-order add/remove pointer events (flutter/engine#55740)
2024-10-16 [email protected] Starts looking for the bdf fast path in relation to the snapshot_entity's transform (flutter/engine#55890)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix multi-window pointer event crash
4 participants