Repository navigation
iOS: Fix use-after-free race during shell teardown - #190132
Conversation
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 introduces a mechanism to cancel and unblock WaitForFirstFrame calls during Shell destruction, preventing potential use-after-free issues when background threads access the shell. It adds CancelWaitForFirstFrame to the Shell class, implements synchronization using a dispatch group in FlutterEngine.mm on iOS, and includes a unit test to verify the behavior. The review feedback suggests simplifying wait_for_first_frame_cancelled_ from std::atomic<bool> to a regular bool since it is always accessed under the protection of waiting_for_first_frame_mutex_, which would also simplify its usage in shell.cc.
|
|
||
| bool first_frame_rasterized_ = false; | ||
| std::atomic<bool> waiting_for_first_frame_ = true; | ||
| std::atomic<bool> wait_for_first_frame_cancelled_ = false; |
There was a problem hiding this comment.
The variable wait_for_first_frame_cancelled_ is always accessed and modified while holding waiting_for_first_frame_mutex_ (both in WaitForFirstFrame and CancelWaitForFirstFrame). Therefore, making it a std::atomic<bool> is redundant and introduces unnecessary atomic overhead. It can be a regular bool instead.
| std::atomic<bool> wait_for_first_frame_cancelled_ = false; | |
| bool wait_for_first_frame_cancelled_ = false; |
There was a problem hiding this comment.
Hrm. Poking at all three of the current cases:
- CancelWaitForFirstFrame: yes the mutex is held via scoped_lock.
- WaitForFirstFrame: also held via scoped_lock, and we pass it to the lambda, so both are covered.
This seems technically reasonable. I worry about the future-proofing side of things where someone comes along and refactors (which I've just mentioned we might want to do) and assumes these two are equivalent.
I could maybe add a comment right above it saying osmething like:
// All uses must be guarded by waiting_for_first_frame_mutex_.Tempted to leave as-is because the overhead is near-zero, but I'm fine tacking on a comment and then making it a plain bool if you prefer, @jason-simmons. I mean there are a dozen other ways future humans could mess this up regardless of whether or not it's marked atomic. I think that's probably enough to change my mind; going with the plain bool :P. Shout if you disagree.
There was a problem hiding this comment.
Hmm this got me looking at the other (pre-existing) atomic bool waiting_for_first_frame_. This is entirely unrelated to this fix but I'm a bit puzzled by something.
Looks like it's used in:
- WaitForFirstFrame: As noted in the comment above guarded by the mutex which was needed for the condition variable. Good. Called on a background thread for the deep link case we're fixing here. (Looks like we assert that it CANNOT be called on UI or Raster thread at the top)
- OnPlatformViewCreated: we reset the waiting state to true there. Called on the raster thread. Not guarded by the mutex but fine since this isn't triggering an unblock for any waiters.
- OnAnimatorDraw: we're NOT locking the mutex, but we are calling
notify_all()on the condition variable. That seems incorrect. This work is done on the raster thread.
Historically we've had a policy of avoiding any blocking at all on the raster thread so this may be an intentional risk/reward tradeoff.
Thinking through a potential sequence of events though:
On the background thread:
- WaitForFirstFrame: Lock the mutex.
- in wait_until: Check the condition. No frame has arrived. But let's pretend the thread suspended before unlock mutex/sleep.
Meanwhile... over on the raster thread:
- OnAnimatorDraw: The frame is ready. Store false to the bool.
- Raster thread: Call notify_all(). Because the background thread isn't technically asleep on the OS wait queue yet, the notification fires into
the void and wakes nobody up.
Meanwhile... over on the background thread:
- in wait_until: Resume exactly where we left off. Finish entering the condition variable block, unlock the mutex, and goes to sleep.
In theory, shouldn't we be acquiring the mutex in OnAnimatorDraw so we can't fire notify_all() in the gap between the condvar check and when it sleeps + releases the mutex. Seems like there's a teensy chance that threads waiting on the first frame never wake up.
I think?
That's totally unrelated to this patch and definitely NOT something I want to try to touch here, but probably worth understanding properly, particularly if we look at changing how the first frame waiting is implemented internally in a followup.
| bool success = waiting_for_first_frame_condition_.wait_until( | ||
| lock, duration, [&waiting_for_first_frame = waiting_for_first_frame_] { | ||
| return !waiting_for_first_frame.load(); | ||
| lock, duration, | ||
| [&waiting_for_first_frame = waiting_for_first_frame_, | ||
| &cancelled = wait_for_first_frame_cancelled_] { | ||
| return !waiting_for_first_frame.load() || cancelled.load(); | ||
| }); | ||
| if (success) { | ||
| if (wait_for_first_frame_cancelled_.load()) { | ||
| return fml::Status(fml::StatusCode::kAborted, "Shell is shutting down."); | ||
| } else if (success) { | ||
| return fml::Status(); | ||
| } else { | ||
| return fml::Status(fml::StatusCode::kDeadlineExceeded, "timeout"); | ||
| } | ||
| } | ||
|
|
||
| void Shell::CancelWaitForFirstFrame() { | ||
| { | ||
| std::scoped_lock lock(waiting_for_first_frame_mutex_); | ||
| wait_for_first_frame_cancelled_.store(true); | ||
| } | ||
| waiting_for_first_frame_condition_.notify_all(); | ||
| } |
There was a problem hiding this comment.
Since wait_for_first_frame_cancelled_ can be a regular bool instead of std::atomic<bool>, we can simplify its usage by removing the .load() and .store() calls.
bool success = waiting_for_first_frame_condition_.wait_until(
lock, duration,
[&waiting_for_first_frame = waiting_for_first_frame_,
&cancelled = wait_for_first_frame_cancelled_] {
return !waiting_for_first_frame.load() || cancelled;
});
if (wait_for_first_frame_cancelled_) {
return fml::Status(fml::StatusCode::kAborted, "Shell is shutting down.");
} else if (success) {
return fml::Status();
} else {
return fml::Status(fml::StatusCode::kDeadlineExceeded, "timeout");
}
}
void Shell::CancelWaitForFirstFrame() {
{
std::scoped_lock lock(waiting_for_first_frame_mutex_);
wait_for_first_frame_cancelled_ = true;
}
waiting_for_first_frame_condition_.notify_all();
}There was a problem hiding this comment.
See discussion above. Pending reviewer feedback on how paranoid we feel about future-proofing.
|
@jason-simmons not sure if you have better ideas here. Another approach could be to make make Another approach could be for the shell to track a count of active waiters and we'd block waiting for that count to drop to zero, but that would block the platform thread (which is also the UI thread for iOS and others now) which seems like potential for deadlock. In theory we could make shell held by a shared_ptr but that feels a bit gross too -- I'd rather we had fewer shared_ptrs that we have now... not more. Philosophically it should really be uniquely owned by the embedder and no one else should know about it. This is internal-only/non-public and gets a fix in that doesn't have the timeout issue that the band-aid fix in b/521830222 has, and we can always follow-up with nicer fixes (e.g. the async callback registration approach). Would love to hear any other ideas you might have though. |
|
I think it may be worth looking at the other embedders to see if any of them have a similar usage issue for this. I assume Android and others may also have deep-link handling and may have taken the same approach, but I also think those can be separate patches. |
aa484c2 to
daec753
Compare
-[FlutterEngine waitForFirstFrame:callback:] dispatches a block to a QOS_CLASS_BACKGROUND queue that dereferences the `_shell` unique_ptr and blocks in `Shell::WaitForFirstFrame()` on Shell-owned `waiting_for_first_frame_mutex_`/`waiting_for_first_frame_condition_`. Previously, nothing prevented `-destroyContext` from running concurrently on the main thread and calling `_shell.reset()` while that background block was still running, or hadn't started yet. `Shell::~Shell` only joins the platform/UI/raster/IO task runners. It has no idea a separate GCD thread might be parked (or about to park) inside the object, so `_shell.reset()` can free the Shell out from under the block, which then reads and locks/unlocks freed memory. Specifically: * We dispatch the background block in `-[FlutterEngine waitForFirstFrame:callback:]`, which captures `weakSelf`. * Before the block runs, or while it's blocked on the condition variable, the main thread calls `destroyContext`, which resets `_shell`. * The block resolves `strongSelf` which is still valid -- that's the FlutterEngine wrapper, not `_shell`, and calls `strongSelf.shell.WaitForFirstFrame(...)` on a Shell that's already been freed. This is reachable via -sendDeepLinkToFramework:completionHandler:, which calls waitForFirstFrame: with a 3s timeout, racing any concurrent lifecycle teardown (scene disconnect, view controller dealloc). This adds `Shell::CancelWaitForFirstFrame()`, which flips a cancelled flag under `waiting_for_first_frame_mutex_` and notifies the condition var, causing any current/upcoming `WaitForFirstFrame()` call to return `kAborted` promptly instead of blocking out its timeout. We call this before resetting the shell in `-destroyContext`, then wait on a dispatch_group for that block to actually finish touching `_shell` before freeing it. This is a separate dispatch_group to the one that already blocks the completion callback, so that concurrent waitForFirstFrame: calls don't have their callbacks coupled to each other. The unit tests was copied (near-)verbatim from repro on the bug. Issue: b/521830222
daec753 to
209dc20
Compare
| return !waiting_for_first_frame.load(); | ||
| lock, duration, | ||
| [&waiting_for_first_frame = waiting_for_first_frame_, | ||
| &cancelled = wait_for_first_frame_cancelled_] { |
There was a problem hiding this comment.
I approve of this cancelled spelling 😂
There was a problem hiding this comment.
Haha oh no, I'm going to have to dig out my script again!
Agreed. We want to move iOS to the embedder API anyways, so refactoring now might be throwaway work. This simple fix is pragmatic and very reasonable 👍 |
I think this is the best option.
It should be possible to replace AFAICT |
…12318) Manual roll requested by [email protected] flutter/flutter@0f02463...c83f80b 2026-07-29 [email protected] Use super-parameter in two_dimensional_utils (flutter/flutter#189957) 2026-07-29 [email protected] Scope SemanticsTester per test in scrollable_semantics_test (flutter/flutter#189800) 2026-07-29 [email protected] Roll Skia from d78865e708ad to 70733f74d415 (2 revisions) (flutter/flutter#190168) 2026-07-28 [email protected] iOS: Fix use-after-free race during shell teardown (flutter/flutter#190132) 2026-07-28 [email protected] Roll Skia from 62442d6cf0ec to d78865e708ad (30 revisions) (flutter/flutter#190152) 2026-07-28 [email protected] Made linter more robust to non-utf8 files (flutter/flutter#190012) 2026-07-28 [email protected] Roll Packages from 6969329 to 3e63635 (6 revisions) (flutter/flutter#190142) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
`Shell::WaitForFirstFrame` violates the shell's threading model: the platform thread owns the shell's lifetime, but the wait parks arbitrary caller threads on a condition variable inside the shell. That made the use-after-free fixed in flutter#190132 possible, and the fix required teardown to block until in-flight waiters drained. As discussed on that PR, an asynchronous callback registration is the API this should have been from the start. This adds `Shell::AddFirstFrameCallback(fml::closure)`: registered on the platform thread, invoked on the raster thread at the point `OnAnimatorDraw` previously notified the condition variable, or synchronously at registration if a first frame has already been presented. Closures still pending at shell destruction are dropped. `-waitForFirstFrame:callback:` (the deep-link wait, and the sole remaining first-frame wait after the previous change) is reimplemented on top of it: the background-queue block now parks on a `FlutterFirstFrameWaiter` that holds no reference to the engine or shell, making the use-after-free structurally impossible rather than carefully avoided, and `-destroyContext` aborts outstanding waiters instead of blocking teardown on a dispatch_group until they drain. This deliberately does not reuse `Rasterizer::SetNextFrameCallback` (the mechanism behind `-installFirstFrameCallback:` and Android's onFirstFrame): that fires only after an actual draw to a live surface, while the `OnAnimatorDraw` site fires on any non-empty frame delivery -- including on headless engines and while the GPU is unavailable -- which is the semantics the deep-link wait has always had. Also fixes a lost wakeup spotted in review of flutter#190132: `OnAnimatorDraw` stored the first-frame flag and notified the condition variable without holding the mutex, so the notify could fire between a waiter's predicate check and its wait, stranding the waiter for its full timeout. The store now happens under the mutex. One behavioural change: waiters aborted by teardown report didTimeout YES (previously the cancelled wait reported NO), so a deep link raced by engine destruction now invokes its completion handler with failure rather than pushing a route into a dead engine and never completing. Tests cover delivery on the raster thread, releasing a blocked platform thread without depending on its own task queue, synchronous delivery once a frame has been presented, the zero-size-frame case, dropping of pending closures at shell destruction, and prompt unblocking of -waitForFirstFrame:callback: during teardown. `Shell::WaitForFirstFrame` and `CancelWaitForFirstFrame` are now unused outside shell unit tests; a follow-up removes them. Issue: b/521830222
Removes `Shell::WaitForFirstFrame` and `CancelWaitForFirstFrame`, along with the condition variable and cancelled flag backing them. The iOS embedder -- the only production caller -- now registers callbacks via `Shell::AddFirstFrameCallback`, so nothing referenced these outside shell unit tests. The condition variable was the mechanism that allowed arbitrary caller threads to block inside the shell with no synchronisation against its destruction, the root cause of the use-after-free fixed in flutter#190132. With it gone, first-frame tracking in the shell reduces to the `waiting_for_first_frame_` flag and the registered callback list, both guarded by a single mutex. The `WaitForFirstFrame` unit tests are superseded by the `AddFirstFrameCallback` tests added in the previous change: the wait/already-presented/timeout/zero-size cases map directly onto callback registration. `WaitForFirstFrameInlined`, which asserted `kFailedPrecondition` on single-threaded configurations, is dropped along with that status: no caller blocks any more, so there is no can't-block-here precondition to enforce. `OnPlatformViewDestroyDisablesThreadMerger`, which used the wait as a frame barrier, now uses a registered callback for the same purpose. Also removes the now-unused `fml/status.h` include from `shell.h`; no remaining includer relied on it transitively.
…s the Shell threading model The Shell::WaitForFirstFrame API would block until the engine renders the first frame. This could potentially cause issues if the shell is destroyed while a thread is waiting within a call to WaitForFirstFrame. This PR replaces WaitForFirstFrame with an AddFirstFrameCallback API that invokes a callback when the first frame has been rendered. If the shell is destroyed before the first frame, then the callback will never be executed. See flutter#190132
…s the Shell threading model (flutter#191841) The Shell::WaitForFirstFrame API would block until the engine renders the first frame. This could potentially cause issues if the shell is destroyed while a thread is waiting within a call to WaitForFirstFrame. This PR replaces WaitForFirstFrame with an AddFirstFrameCallback API that invokes a callback when the first frame has been rendered. If the shell is destroyed before the first frame, then the callback will never be executed. See flutter#190132
3 participants
-[FlutterEngine waitForFirstFrame:callback:] dispatches a block to a QOS_CLASS_BACKGROUND queue that dereferences the
_shellunique_ptr and blocks inShell::WaitForFirstFrame()on Shell-ownedwaiting_for_first_frame_mutex_/waiting_for_first_frame_condition_.Previously, nothing prevented
-destroyContextfrom running concurrently on the main thread and calling_shell.reset()while that background block was still running, or hadn't started yet.Shell::~Shellonly joins the platform/UI/raster/IO task runners. It has no idea a separate GCD thread might be parked (or about to park) inside the object, so_shell.reset()can free the Shell out from under the block, which then reads and locks/unlocks freed memory.Specifically:
-[FlutterEngine waitForFirstFrame:callback:], which capturesweakSelf.destroyContext, which resets_shell.strongSelfwhich is still valid -- that's the FlutterEngine wrapper, not_shell, and callsstrongSelf.shell.WaitForFirstFrame(...)on a Shell that's already been freed.This is reachable via -sendDeepLinkToFramework:completionHandler:, which calls waitForFirstFrame: with a 3s timeout, racing any concurrent lifecycle teardown (scene disconnect, view controller dealloc).
This adds
Shell::CancelWaitForFirstFrame(), which flips a cancelled flag underwaiting_for_first_frame_mutex_and notifies the condition var, causing any current/upcomingWaitForFirstFrame()call to returnkAbortedpromptly instead of blocking out its timeout. We call this before resetting the shell in-destroyContext, then wait on a dispatch_group for that block to actually finish touching_shellbefore freeing it.This is a separate dispatch_group to the one that already blocks the completion callback, so that concurrent waitForFirstFrame: calls don't have their callbacks coupled to each other.
The unit tests was copied (near-)verbatim from repro on the bug.
Issue: b/521830222
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.