Repository navigation
iOS: Ensure engine is run on the main thread - #191626
Conversation
Previously, running a `FlutterEngine` from a background queue silently gave us an engine whose platform thread was not the main thread. The first symptom an app sees is an assertion deep inside `DisplayLinkManager.shared`, which is accurate but confusing and looks like a bug rather than an intentional precondition. `MakeThreadHost` adopts the calling thread as the engine's platform/UI thread. That thread is required to be the main thread for several reasons: * it reads UIKit state and runs every plugin and platform channel callback * nothing else pumps its run loop We now assert this via `FML_CHECK` in: * `createShell:libraryURI:initialRoute:`: covers the `run*` methods and `FlutterViewController`'s implicit engine * `spawnWithEntrypoint:libraryURI:initialRoute:entrypointArgs:`: covers the second and later engines in a `FlutterEngineGroup`. A spawned engine inherits its spawner's task runners rather than adopting the calling thread, so the check in `createShell` constrains every engine in a group. This also adds an `FML_DCHECK` during engine construction since it reads `UIApplication` lifecycle state and registers UIKit observers. This also annotates the Swift-visible entry points `NS_SWIFT_UI_ACTOR` so that Swift callers get a compile-time error rather than a runtime abort. `FlutterViewController` doesn't need an annotation since, like all view controllers, it already inherits main actor isolation from `UIViewController`. Issue: flutter#190909 Related: flutter#190725 Related: flutter#165904 No test changes, since these are assertions and unlike GoogleTest, XCUITest and Swift Testing tests have no death-test. This doesn't add any additional runtime aborts since `DisplayLinkManager.shared` already does this; it just pushes them up to where they're actually actionable by the user, and provides better error messages.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. |
github-actions
Bot
added
platform-ios
![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 enforces that FlutterEngine instances, groups, and headless runners are created and run on the main thread by adding documentation, NS_SWIFT_UI_ACTOR annotations, @MainActor isolation, and runtime main-thread checks (FML_DCHECK and FML_CHECK). The feedback suggests simplifying the runtime checks in FlutterEngine.mm by using the more direct and idiomatic class method [NSThread isMainThread] instead of querying the current thread instance.
|
test-exempt: annotation change that is compiler-enforced |
3 participants
Previously, running a
FlutterEnginefrom a background queue silently gave us an engine whose platform thread was not the main thread. The first symptom an app sees is an assertion deep insideDisplayLinkManager.shared, which is accurate but confusing and looks like a bug rather than an intentional precondition.MakeThreadHostadopts the calling thread as the engine's platform/UI thread. That thread is required to be the main thread for several reasons:We now assert this via
FML_CHECKin:createShell:libraryURI:initialRoute:: covers therun*methods andFlutterViewController's implicit enginespawnWithEntrypoint:libraryURI:initialRoute:entrypointArgs:: covers the second and later engines in aFlutterEngineGroup.A spawned engine inherits its spawner's task runners rather than adopting the calling thread, so the check in
createShellconstrains every engine in a group.This also adds an
FML_DCHECKduring engine construction since it readsUIApplicationlifecycle state and registers UIKit observers.This also annotates the Swift-visible entry points
NS_SWIFT_UI_ACTORso that Swift callers get a compile-time error rather than a runtime abort.FlutterViewControllerdoesn't need an annotation since, like all view controllers, it already inherits main actor isolation fromUIViewController.Issue: #190909
Related: #190725
Related: #165904
No test changes, since these are assertions and unlike GoogleTest, XCUITest and Swift Testing tests have no death-test. This doesn't add any additional runtime aborts since
DisplayLinkManager.sharedalready does this; it just pushes them up to where they're actually actionable by the user, and provides better error messages.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.