Guard Scheduler deferred lambdas against delegate teardown (#56680)

[fkgozali]

Summary:

Scheduler::uiManagerDidFinishTransaction and Scheduler::uiManagerDidDispatchCommand queue lambdas via runtimeScheduler_->scheduleRenderingUpdate that capture the raw delegate_ pointer by value. With RuntimeScheduler_Modern (the bridgeless implementation), the lambda runs asynchronously, so if the SchedulerDelegate is destroyed between enqueue and execution (surface teardown, Scheduler destruction, or setDelegate flip), the lambda dereferences dangling memory → EXC_BAD_ACCESS / KERN_INVALID_ADDRESS.

Add a per-delegate-identity invalidation token (shared_ptr<atomic<bool>>) owned by Scheduler and captured by-value into the deferred lambdas. The destructor and setDelegate flip the flag to true; lambdas check it (acquire ordering) before dereferencing the captured raw delegate.

The shared_ptr keeps the atomic alive for any outstanding lambdas. Public API is unchanged (private member only), so C++ API snapshots are unaffected.

Changelog:
[General][Fixed] - Prevent Scheduler use-after-free crash when surfaces tear down with pending rendering updates

Reviewed By: mdvacca

Differential Revision: D103727974

[meta-codesync]

@fkgozali has exported this pull request. If you are a Meta employee, you can view the originating Diff in D103727974.

[meta-codesync]

This pull request has been merged in aadbe96.

[delekta]

Hi @fkgozali, Kamil from Software Mansion here. We’re seeing what looks like the same issue in one of our client's project. I really like this solution, but I was wondering about one edge case.

Have you considered this issue?

// Thread A: queued rendering update
if (guardEnabled && *invalidated) { // reads false
return;
}

// Thread B: Scheduler::setDelegate(...)
*delegateInvalidated_ = true;
delegate_ = newDelegate;

// old delegate is freed

// Thread A resumes
delegate->schedulerShouldRenderTransactions(...); // possible use-after-free?

I like your solution because of how small and non-invasive it is. I was also wondering whether you considered more ownership/synchronization-heavy alternatives, like moving the delegate to shared_ptr / weak_ptr, or using a mutex / in-flight tracking around delegate calls. Those seem like they would close the lifetime race more completely, but they also look much more invasive because they would change the delegate ownership model or add locking around this path.

[fkgozali]

@delekta Thanks for the info! Note that this fix is a "workaround" because it's hiding the actual problem: "why is the queue not flushing things during teardown?". I expect some more iteration on this, but for now, at least the app should not hard crash, though it might show some more unpredictable behavior after teardown.

On moving the delegate to shared_ptr, yes that's one of the idea we're considering, but didn't pursue it yet because of the API change. The current focus is to isolate the root cause and find a robust fix, before cleaning up the API ergonomics etc. Hope that helps.

Do you happen to have solid repro for this btw? If so would be helpful to see if this featureflag/fix helps in your scenario.

[delekta]

@fkgozali Thanks, that makes sense. Unfortunately we do not have a repro. That has been the biggest issue for us with this bug. Without a repro, the best signal we have is crash volume after enabling the fix. I can try your solution in our scenario and watch whether the crash rate changes.

The closest mitigation we tried before was flushing the runtimeScheduler queue during Scheduler destructor here: discord#169. That drastically reduced the number of crashes for us, but did not eliminate them completely.

[fkgozali]

The closest mitigation we tried before was flushing the runtimeScheduler queue during Scheduler destructor

I see, I think there are a few scenarios overall that we need to identify:

• pending calls during teardown --> should be flushed
• some early failures that cause the runtime to be in a "bad state", hence attempting to flushing may cause other undefined behavior
• dependencies (e.g. delegate) that somehow got dealloc earlier, maybe due to (2), causing hard crash due to invoking a freed memory block

I suspect we need a combo of approaches, not just purely flushing, or dropping them like what I did in this PR

[fkgozali]

@delekta FYI, we're adding a test to better simulate the crash scenario: #56800. With that, it should be easier to explore proper fixes