Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[engine] always force platform channel responses to schedule a task. #54975

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

jonahwilliams
Copy link
Member

If we use runNowOrPostTask on platform channel responses, then we may not wake up the dart message loop. If nothing else wakes it up, then the embedder may hang on platform channel responses.

This fixes several google3 integration tests when run in merged thread mode.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

@@ -1076,10 +1076,10 @@ void Shell::OnPlatformViewDispatchPlatformMessage(

// The static leak checker gets confused by the use of fml::MakeCopyable.
// NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks)
fml::TaskRunner::RunNowOrPostTask(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this conditional on merged thread mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Before merged thread mode, RunNowOrPost task would always force a task to be posted because it was not possible for platform channel responses to come from the UI thread - only from plugins on the platform thread. In merged thread mode, RunNowOrPostTask is immediately invoking the closure without scheduling a task - which means the dart event loop isn't worken up.

So all I'm doing here is removing the conditional behavior for merged thread mode which is always incorrect.

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 5, 2024
@auto-submit auto-submit bot merged commit 015f3b1 into flutter:main Sep 5, 2024
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 5, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 5, 2024
…154691)

flutter/engine@c50eb8a...015f3b1

2024-09-05 [email protected] [engine] always force platform channel responses to schedule a task. (flutter/engine#54975)
2024-09-05 [email protected] Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. (flutter/engine#54965)

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],[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
auto-submit bot added a commit to flutter/flutter that referenced this pull request Sep 6, 2024
…isions) (#154691)" (#154726)

Reverts: #154691
Initiated by: jtmcdole
Reason for reverting: breaking flutter-dashboard
Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:

flutter/engine@c50eb8a...015f3b1

2024-09-05 [email protected] [engine] always force platform channel responses to schedule a task. (flutter/engine#54975)
2024-09-05 [email protected] Fix unexpected ViewFocus events when Text Editing utilities change focus in the middle of a blur call. (flutter/engine#54965)

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],[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
@jtmcdole
Copy link
Member

jtmcdole commented Sep 6, 2024

Reason for revert: I believe this change caused flutter/flutter to break in an engine roll. It was one of two changes - the other being a webui change.

Tests broken:

module_test_ios: testDualCold

router test:

[2024-09-05 17:43:20.837343] [STDOUT] stderr: [ +135 ms] VMServiceFlutterDriver: Connected to Flutter application.
[2024-09-05 17:43:20.841927] [STDOUT] stdout: [   +4 ms] 00:00 �[32m+0�[0m: flutter run test --route sanity check flutter drive --route�[0m
[2024-09-05 17:43:20.875760] [STDOUT] stdout: [  +33 ms] 00:00 �[32m+0�[0m�[31m -1�[0m: flutter run test --route sanity check flutter drive --route �[1m�[31m[E]�[0m�[0m
[2024-09-05 17:43:20.877108] [STDOUT] stdout: [   +1 ms]   Expected: '/smuggle-it'
[2024-09-05 17:43:20.877135] [STDOUT] stdout: [        ]     Actual: '/'
[2024-09-05 17:43:20.877172] [STDOUT] stdout: [        ]      Which: is different. Both strings start the same, but the actual value is missing the following trailing characters: smuggle-it ...

@jtmcdole jtmcdole added the revert Label used to revert changes in a closed and merged pull request. label Sep 6, 2024
auto-submit bot pushed a commit that referenced this pull request Sep 6, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Sep 6, 2024
auto-submit bot added a commit that referenced this pull request Sep 6, 2024
… a task. (#54975)" (#55000)

Reverts: #54975
Initiated by: jtmcdole
Reason for reverting: I believe this change caused flutter/flutter to break in an engine roll. It was one of two changes - the other being a webui change.

Tests broken:

module_test_ios:  testDualCold

router test:

```
[2024-09-05 17:43:20.837343] [STDOUT] stderr: [ +135 ms] VMServiceFlutterDriver: Connected to Flutter application.
[2024-09-05 17:43:20.841927] [STDOUT] stdout: [   +4 ms] 00:00 �[32m+0�[0m: 
Original PR Author: jonahwilliams

Reviewed By: {jason-simmons, johnmccutchan}

This change reverts the following previous change:
If we use runNowOrPostTask on platform channel responses, then we may not wake up the dart message loop. If nothing else wakes it up, then the embedder may hang on platform channel responses.

This fixes several google3 integration tests when run in merged thread mode.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 6, 2024
zanderso added a commit to flutter/flutter that referenced this pull request Sep 6, 2024
flutter/engine@c50eb8a...419fb8c

2024-09-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[engine] always force platform channel responses to schedule a task.
(#54975)"
(flutter/engine#55000)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Skia from b6bab0fde426 to 6ad117bd2efe (2 revisions)
(flutter/engine#54999)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Fuchsia Test Scripts from D9INMR2u4wcyiZ750... to
5dqcFlKzRjJb6V95W... (flutter/engine#54998)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Skia from a09312b70d37 to b6bab0fde426 (3 revisions)
(flutter/engine#54997)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Skia from 368f209ccca5 to a09312b70d37 (1 revision)
(flutter/engine#54995)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Skia from aec11ae18bb6 to 368f209ccca5 (3 revisions)
(flutter/engine#54992)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Fuchsia Linux SDK from xNv47d1TZmK9XgTxu... to PBeI0gGvgFdXV6hCg...
(flutter/engine#54990)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Skia from 809f868ded1c to aec11ae18bb6 (22 revisions)
(flutter/engine#54988)
2024-09-06
[[email protected]](mailto:[email protected])
Removes the int storage from Color
(flutter/engine#54714)
2024-09-06 [[email protected]](mailto:[email protected]) iOS,macOS: Add
logging of duplicate codesign binaries
(flutter/engine#54987)
2024-09-06
[[email protected]](mailto:[email protected])
Roll Fuchsia Test Scripts from k4lKsecg0pdIp-U7c... to
D9INMR2u4wcyiZ750... (flutter/engine#54984)
2024-09-05
[[email protected]](mailto:[email protected])
Manual roll of Dart. (flutter/engine#54983)
2024-09-05 [[email protected]](mailto:[email protected]) iOS,macOS: add
unsigned_binaries.txt (flutter/engine#54977)
2024-09-05
[[email protected]](mailto:[email protected])
Manual Skia roll to 809f868ded1c
(flutter/engine#54972)
2024-09-05
[[email protected]](mailto:[email protected])
[canvaskit] Fix incorrect calculation of ImageFilter paint bounds
(flutter/engine#54980)
2024-09-05 [[email protected]](mailto:[email protected])
[engine] always force platform channel responses to schedule a task.
(flutter/engine#54975)
2024-09-05
[[email protected]](mailto:[email protected])
Fix unexpected ViewFocus events when Text Editing utilities change focus
in the middle of a blur call.
(flutter/engine#54965)

Also rolling transitive DEPS:
fuchsia/sdk/core/linux-amd64 from xNv47d1TZmK9 to PBeI0gGvgFdX

---------

Co-authored-by: Christopher Fujino <[email protected]>
Co-authored-by: Zachary Anderson <[email protected]>
auto-submit bot pushed a commit that referenced this pull request Sep 6, 2024
…55006)

Reland of #54975

Fixes the initial route behavior for iOS. Previously the initial route setting would _always_ be posted as a task, which after merging the threads would fire after isolate creation, thus it would not actually update the initial route setting. Fixed Engine constructor so that it reads the initial route from the settings.
auto-submit bot added a commit that referenced this pull request Sep 7, 2024
…onses. (#55006)" (#55022)

Reverts: #55006
Initiated by: jonahwilliams
Reason for reverting: still failling mac module test
Original PR Author: jonahwilliams

Reviewed By: {jason-simmons}

This change reverts the following previous change:
Reland of #54975

Fixes the initial route behavior for iOS. Previously the initial route setting would _always_ be posted as a task, which after merging the threads would fire after isolate creation, thus it would not actually update the initial route setting. Fixed Engine constructor so that it reads the initial route from the settings.
jesswrd pushed a commit to jesswrd/engine that referenced this pull request Sep 11, 2024
…lutter#55006)

Reland of flutter#54975

Fixes the initial route behavior for iOS. Previously the initial route setting would _always_ be posted as a task, which after merging the threads would fire after isolate creation, thus it would not actually update the initial route setting. Fixed Engine constructor so that it reads the initial route from the settings.
jesswrd pushed a commit to jesswrd/engine that referenced this pull request Sep 11, 2024
…onses. (flutter#55006)" (flutter#55022)

Reverts: flutter#55006
Initiated by: jonahwilliams
Reason for reverting: still failling mac module test
Original PR Author: jonahwilliams

Reviewed By: {jason-simmons}

This change reverts the following previous change:
Reland of flutter#54975

Fixes the initial route behavior for iOS. Previously the initial route setting would _always_ be posted as a task, which after merging the threads would fire after isolate creation, thus it would not actually update the initial route setting. Fixed Engine constructor so that it reads the initial route from the settings.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

4 participants