Skip to content

Keep the iOS renderer scheduler alive through instance shutdown - #59754

Open
sorinc03 wants to merge 2 commits into
react:mainfrom
sorinc03:fix/ios-reload-queued-commands
Open

sorinc03 wants to merge 2 commits into
react:mainfrom
sorinc03:fix/ios-reload-queued-commands

Conversation

@sorinc03

@sorinc03 sorinc03 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #59721.

During a normal bridgeless reload, RCTInstance::invalidate suspends the surface presenter before JS-thread cleanup runs. Suspension can destroy the renderer scheduler and its delegate proxy while the current JS task still has rendering updates capturing the proxy by raw pointer.

Retain the scheduler through that cleanup, and clear its Objective-C delegate after stopping surfaces. Before dispatching cleanup, permanently remove its layout-animation run-loop observer on main. Scheduler destruction then stays in the same JS cleanup block as runtime destruction, with the scheduler released first. This prevents both an observer callout racing teardown and queued JSI handles outliving the runtime. It adds no per-rendering-callback guard, synchronous main wait, or JS-to-main-to-JS destruction round trip.

Add three native lifecycle tests, the required CocoaPods Yoga header dependency for the scheduler import, and regenerated Apple API snapshots.

Changelog

[IOS] [FIXED] - Prevent a queued view command from accessing a destroyed scheduler delegate during a normal bridgeless reload

Test Plan

Used the issue's public Nezz/expo-repro branch repro/ReloadSchedulerCrash (React Native 0.88.1), Xcode 27.1, and a dedicated iOS 27.1 iPhone Duo simulator.

  • Initial reproduction: unmodified prebuilt crashed in 20/20 reloads; unmodified source crashed in 10/10. An actual EXC_BAD_ACCESS report contained Scheduler::uiManagerDidDispatchCommand and RuntimeScheduler_Modern::updateRendering on the JS thread.
  • Final observer-detachment fix: 20/20 normal reloads acknowledged after removing all diagnostics, with no crashes or timeouts.
  • Final observer-detachment fix: 20/20 instrumented reloads during a five-second layout animation acknowledged, with no crashes or timeouts. All 20 processes recorded real animation ticks, observer removal on main, scheduler destruction on JS, then runtime teardown. No ticks occurred after observer removal.
  • After removing the diagnostics and rebuilding, another 5/5 animated reloads acknowledged with no crashes or timeouts.
  • Three native lifecycle tests pass in a clean build without diagnostics. They exercise real instance invalidation/presenter suspension with scheduler and JS-thread-manager collaborators replaced, covering observer removal on main, no callbacks while JS cleanup is pending, scheduler retention/release, and invalidation without a scheduler. Runtime/JSI behavior is covered by the app trials, not by these probes alone.
  • All 12 API snapshot validations pass with Doxygen 1.16.1; clang-format 21.1.2, Ruby podspec syntax, and git diff --check pass.

An unpublished intermediate approach that released the whole scheduler on main after JS cleanup crashed in 1/20 animated trials while destroying queued JSI handles. The final fix instead detaches only the observer on main and preserves scheduler-before-runtime destruction in one JS cleanup block.

Validation stages the production changes into the report's source-built 0.88.1 fixture; it is not a complete checkout-HEAD build or full RNTester suite. The reporter's iOS 26.5 environment, physical devices, and Release builds were not tested. This addresses normal off-JS-thread bridgeless reload, not a new policy for inline JS invalidation, standalone presenter suspension, nested main run loops, repeated invalidation, the legacy bridge, or the separate shared-animation backend.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 2, 2026
@sorinc03

sorinc03 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

@Nezz I reproduced the JS-thread queued-command crash in your fixture: 20/20 crashes with the published prebuilt and 10/10 with an unmodified source build. Applying this instance-shutdown fix to the 0.88.1 source build gave 20/20 survived-reload acknowledgements, with no crashes or timeouts. Two native ownership tests also pass. This was iOS 27.1 on an iPhone Duo simulator; could you retest on your iOS 26.5 setup? The PR leaves legacy bridge and inline JS-thread invalidation outside its scope.

@facebook-github-tools facebook-github-tools Bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 2, 2026
@Nezz

Nezz commented Nov 2, 2026

Copy link
Copy Markdown

Thank you, I verified the fix and it's all good!

@meta-codesync

meta-codesync Bot commented Nov 3, 2026

Copy link
Copy Markdown

@javache has imported this pull request. If you are a Meta employee, you can view this in D129319362.

@javache

javache commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Internal feedback:

Before this change, reload called invalidate on the main thread (RCTHostHostTargetDelegate::onReload asserts RCTAssertMainQueue() before didReceiveReloadCommand → _reloadWithShouldRestartSurfaces:). -[RCTSurfacePresenter suspend] dropped the last strong reference there, so -[RCTScheduler dealloc] and its .cxx_destruct ran on main. Now they always run on the JS thread.

RCTScheduler owns _uiRunLoopObserver, a MainRunLoopObserver. Its CF handler block captures the raw this and runs this->activityDidChange → LayoutAnimationDelegateProxy::activityDidChange → [(_bridge RCTScheduler *)scheduler animationTick] → scheduler->animationTick() → uiManager->animationTick(). The owner.lock() in that handler only keeps _layoutAnimationDelegateProxy alive. It does not keep the observer or the RCTScheduler alive.

The observer stays enabled after suspend when layout animations were in flight.

LayoutAnimationKeyFrameManager::stopSurface only queues the surface ID, and onAllAnimationsComplete (the call that disables the observer) only runs from a later pullTransaction, which never comes once surfaces are stopped. So the main thread keeps running the observer callout on every BeforeWaiting.
The JS thread's .cxx_destruct then destroys _uiRunLoopObserver, and after it scheduler (the std::unique_ptr that owns the UIManager). CFRunLoopRemoveObserver from another thread does not wait for a callout already running.

A main-thread callout that overlaps this teardown reads enabled_ on a freed PlatformRunLoopObserver and messages a deallocated RCTScheduler whose Scheduler/UIManager is being destroyed. The result is EXC_BAD_ACCESS on reload. No lock synchronizes the two threads. Keep the release on the main thread, for example by hopping the release back to main after the JS task drains, or disable and remove the observer on main inside suspend.

@sorinc03

sorinc03 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor Author

@javache Addressed in 4e1097f by permanently removing the scheduler's layout-animation observer on main before dispatching JS cleanup. The scheduler remains retained until that cleanup block, where it is released before the runtime. This keeps both lifetimes ordered without a synchronous main wait or another JS event-loop turn between scheduler and runtime teardown.

I tested the alternative of releasing the whole scheduler on main after cleanup; an animated trial crashed while destroying queued JSI handles after the runtime had gone. That approach is not in this update.

The final observer-detachment version passes three native lifecycle tests, all 12 API snapshot validations, and 20 normal plus 25 animated reloads with explicit post-reload acknowledgements and no crashes/timeouts. Twenty animation trials were instrumented: logs confirm main-thread observer removal, no subsequent ticks, then JS-thread scheduler destruction before runtime teardown. Diagnostics were removed, the clean native suite was rerun, and the 20 normal plus five additional animated trials used that clean build.

Runtime validation uses the source-built 0.88.1 issue fixture on an iOS 27.1 simulator, not a full checkout-HEAD/RNTester run or device/Release validation. The PR description has the detailed scope and results.

The Meta Internal-Only Changes Check currently reports that the PR and imported diff are out of sync; this commit needs re-importing into D129319362.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[iOS] Reload crashes the app while a view command is still queued

3 participants