Repository navigation
[Windows] Fix PostPlatformThreadTask cancelling tasks before they run - #193412
auto-submit[bot] merged 6 commits into
Conversation
FlutterDesktopEnginePostPlatformThreadTask aggregate-initialized its Context as a temporary and passed it to std::make_shared, which copied it into the shared allocation. When the temporary was destroyed at the end of the statement, its destructor saw invoked == false and called on_cancel immediately, even though the task was still queued and would run later. Give Context a constructor so std::make_shared builds it in place, and disallow copying it with FML_DISALLOW_COPY_AND_ASSIGN. Any future change that copies the context now fails to compile instead of silently cancelling the task. Add PostPlatformThreadTaskNotCancelledWhenRun, which verifies that on_cancel is called neither when the task is posted nor after it has run and the engine is destroyed.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Context struct within FlutterDesktopEnginePostPlatformThreadTask to use an explicit constructor and adds a unit test to verify that the cancel callback is not invoked when a task runs. The review feedback recommends replacing the legacy FML_DISALLOW_COPY_AND_ASSIGN macro with explicit = delete declarations for the copy constructor and copy assignment operator to align with the Google C++ Style Guide, which also enables the removal of the newly added flutter/fml/macros.h header.
|
cc @loic-sharma |
|
This seems to make sense. Interesting that gemini didn't catch the destructor on temporary invoking the on_cancel callback. The context must definitely be made non copyable. |
| PumpMessage(); | ||
| } | ||
|
|
||
| engine.reset(); |
There was a problem hiding this comment.
Is this line needed? I imagine this test would've failed before the fix even without this line? (I suspect this might've been copied from the PostPlatformThreadTaskCancelledOnEngineDestroy test, but I suspect it's not actually needed here.)
There was a problem hiding this comment.
this test would've failed before the fix even without this line?
Yes it would, but it additionally ensures that the cancel callback is not called, even if the engine was shut down after running the task, which was not tested before (as far as I can see).
And EXPECT_FALSE(captures.cancel_called); above the while loop tests the first case, so that cancel callback is not called immediately as soon as the context is destroyed. After the loop it makes less sense, as it would wait forever to finish, as the task would never have been called, when the test would have failed (before this PR fix).
I added an additional comment to explain the engine.reset line.
There was a problem hiding this comment.
Gotcha. I don't feel strongly either ways, so sounds good to me 👍
…tests.cc Co-authored-by: Loïc Sharma <[email protected]>
|
Thank you for the feedback @loic-sharma . Most of the comments were for my own understanding, but if its common practice, its totally fine to remove them (I'm not quite around in the C++ game ^^). Ready for a rereview now :D |
|
Kindly asking you to do the second review @mattkae thank you for your time! |
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193412, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
autosubmit label was removed for flutter/flutter/193412, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193412, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193412, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
loic-sharma
added
the
autosubmit
This comment was marked as outdated.
This comment was marked as outdated.
flutter-dashboard
Bot
removed
the
autosubmit
…526) FlutterDesktopEnginePostPlatformThreadTask in current stable Flutter builds its task context from a temporary whose destructor invokes on_cancel right away, so the plugin freed each queued FFI task before it ran and freed it again afterwards. This affected every Dart -> native call and every native -> Dart reply on Windows. Pass no on_cancel and let the callback own the task. The engine is fixed upstream in flutter/flutter#193412; until the minimum Flutter version includes it, a task discarded at engine shutdown leaks instead.
…flutter#193412) FlutterDesktopEnginePostPlatformThreadTask aggregate-initialized its Context as a temporary and passed it to std::make_shared, which copied it into the shared allocation. When the temporary was destroyed at the end of the statement, its destructor saw invoked == false and called on_cancel immediately, even though the task was still queued and would run later. Give Context a constructor so std::make_shared builds it in place, and disallow copying it with FML_DISALLOW_COPY_AND_ASSIGN. Any future change that copies the context now fails to compile instead of silently cancelling the task. Add PostPlatformThreadTaskNotCancelledWhenRun, which verifies that on_cancel is called neither when the task is posted nor after it has run and the engine is destroyed. Fixes flutter#193411 For transparency: PR was partially created with Claude. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [flutter/website]: https://github.com/flutter/website [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Loïc Sharma <[email protected]> Co-authored-by: Matthew Kosarek <[email protected]>
FlutterDesktopEnginePostPlatformThreadTask aggregate-initialized its Context as a temporary and passed it to std::make_shared, which copied it into the shared allocation. When the temporary was destroyed at the end of the statement, its destructor saw invoked == false and called on_cancel immediately, even though the task was still queued and would run later.
Give Context a constructor so std::make_shared builds it in place, and disallow copying it with FML_DISALLOW_COPY_AND_ASSIGN. Any future change that copies the context now fails to compile instead of silently cancelling the task.
Add PostPlatformThreadTaskNotCancelledWhenRun, which verifies that on_cancel is called neither when the task is posted nor after it has run and the engine is destroyed.
Fixes #193411
For transparency: PR was partially created with Claude.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.