iOS: Fix flake when backgrounding platform views - #190816
Conversation
Previously, `bringLayersIntoView:withCompositionOrder:` started with a `FML_DCHECK(self.flutterView)`, which aborts when an app with platform views on screen is backgrounded. Before flutter-team-archive/engine#53826, `SubmitFrame` ran entirely on the platform thread. It started with `FML_DCHECK([[NSThread currentThread] isMainThread])` and bailed out early when `flutter_view_` was null. `BringLayersIntoView` sat in the same (synchronous) scope as that check, which ensured the assertion passed. That change split the method in two: the null check stayed with the raster thread work in `SubmitFrame`, and the UIKit work moved into `PerformSubmit`, posted to the platform thread. The isMainThread assertion moved over with it; the `flutter_view_` one didn't, so ended up being reached on a different thread at a later point than the check that was guarding it. Since `flutterView` is a weak ref that `surfaceUpdated:NO` clears on backgrounding, it might be gone by the time the posted task runs. We now check and bail out early instead, before `_previousCompositionOrder` is cleared, since nothing was attached this frame. `applyMutators` already does this in the same deferred task. This doesn't change opt builds (including debug opt builds), which drop `DCHECK`s at compile time. No new tests: this fixes a flake that `MultiplePlatformViewsBackgroundForegroundTest` in the iOS Scenario app keeps hitting in my local testing.
|
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. |
There was a problem hiding this comment.
Code Review
This pull request modifies the bringLayersIntoView:withCompositionOrder: method in FlutterPlatformViewsController.mm to replace the FML_DCHECK(self.flutterView) assertion with a null check on flutterView, returning early if it is nil. There are no review comments, and I have no feedback to provide.
Previously,
bringLayersIntoView:withCompositionOrder:started with aFML_DCHECK(self.flutterView), which aborts when an app with platform views on screen is backgrounded.Before flutter-team-archive/engine#53826,
SubmitFrameran entirely on the platform thread. It started withFML_DCHECK([[NSThread currentThread] isMainThread])and bailed out early whenflutter_view_was null.BringLayersIntoViewsat in the same (synchronous) scope as that check, which ensured the assertion passed.That change split the method in two: the null check stayed with the raster thread work in
SubmitFrame, and the UIKit work moved intoPerformSubmit, posted to the platform thread. The isMainThread assertion moved over with it; theflutter_view_one didn't, so ended up being reached on a different thread at a later point than the check that was guarding it. SinceflutterViewis a weak ref thatsurfaceUpdated:NOclears on backgrounding, it might be gone by the time the posted task runs.We now check and bail out early instead, before
_previousCompositionOrderis cleared, since nothing was attached this frame.applyMutatorsalready does this in the same deferred task.This doesn't change opt builds (including debug opt builds), which drop
DCHECKs at compile time.No new tests: this fixes a flake that
MultiplePlatformViewsBackgroundForegroundTestin the iOS Scenario app keeps hitting in my local testing.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.