Fix crash from withheld exiting layout animations in legacy proxy#9821
Open
tomekzaw wants to merge 1 commit into
Open
Fix crash from withheld exiting layout animations in legacy proxy#9821tomekzaw wants to merge 1 commit into
tomekzaw wants to merge 1 commit into
Conversation
The legacy LayoutAnimationsProxy withholds Remove/Delete mutations for views animating out. When React re-creates or re-inserts a tag whose exiting removal is still withheld, the stale removal later fired against a hierarchy that no longer matched and unmounted the wrong, still-live view, crashing the mounting layer in iOS release builds. Reconcile such contradicted removals at the start of each transaction by flushing the withheld Remove/Delete before the tag is re-registered, and replace an unchecked MutationNode cast in endLayoutAnimation with a runtime guard (the assert guarding it is compiled out in release). Closes #9820. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a release-only iOS crash in the legacy LayoutAnimationsProxy_Legacy when React reuses a tag that still has a withheld exiting Remove/Delete, by reconciling (flushing) contradicted removals at the start of each mounting transaction and hardening endLayoutAnimation against an unsafe node cast.
Changes:
- Add
reconcileContradictedRemovalsto flush withheld exiting removals when an incomingCreate/Inserttargets the same tag. - Replace a release-unsafe
react_native_assert+ cast inendLayoutAnimationwith a runtime guard. - Add a Suspense + layout animation crash repro example and register it in the common app; adjust
fabric-examplestatic flag to exercise the legacy proxy.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Legacy.h | Declares the new reconciliation helper for contradicted removals. |
| packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Legacy.cpp | Implements reconciliation at transaction start and hardens endLayoutAnimation against invalid node type assumptions in release. |
| apps/fabric-example/package.json | Sets ENABLE_SHARED_ELEMENT_TRANSITIONS to false to run the legacy proxy path in the example app. |
| apps/common-app/src/apps/reanimated/examples/SuspenseLayoutAnimationCrashExample.tsx | Adds a minimal repro/stress example for the Suspense + exiting layout animation crash scenario. |
| apps/common-app/src/apps/reanimated/examples/index.ts | Registers the new crash repro example in the examples list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+118
to
+138
| export default function SuspenseLayoutAnimationCrashExample() { | ||
| const [mode, nextMode] = useReducer((m: number) => m + 1, 0); | ||
| const [swapped, setSwapped] = useState(false); | ||
| const started = useRef(false); | ||
|
|
||
| const start = () => { | ||
| if (started.current) { | ||
| return; | ||
| } | ||
| started.current = true; | ||
| let tick = 0; | ||
| // never cleared — the app crashes before cleanup matters | ||
| setInterval(() => { | ||
| tick += 1; | ||
| if (tick % 2 === 0) { | ||
| nextMode(); | ||
| } else { | ||
| setSwapped((s) => !s); | ||
| } | ||
| }, 200); | ||
| }; |
Comment on lines
+41
to
+48
| // If React re-creates or re-inserts a tag whose exiting removal we are still | ||
| // withholding, it has contradicted that withheld Remove/Delete. Flush it now | ||
| // instead of letting it fire later against a stale hierarchy (which would | ||
| // unmount the wrong, still-live view and crash the mounting layer). | ||
| // | ||
| // This must run before addOngoingAnimations (which would otherwise emit an | ||
| // Update for a tag we are about to Delete this frame) and before | ||
| // parseRemoveMutations, so the rest of the pipeline sees clean bookkeeping. |
Contributor
There was a problem hiding this comment.
Move this comment out of pullTransaction. It can be instead a doc comment on the reconcile method
Comment on lines
+167
to
+170
| // The node was replaced by a plain Node (e.g. by a reparenting move) after | ||
| // the exiting animation started, so there is no withheld removal to | ||
| // finalize. Casting it to MutationNode here would corrupt the heap, and the | ||
| // react_native_assert that used to guard this is compiled out in release. |
Contributor
There was a problem hiding this comment.
Suggested change
| // The node was replaced by a plain Node (e.g. by a reparenting move) after | |
| // the exiting animation started, so there is no withheld removal to | |
| // finalize. Casting it to MutationNode here would corrupt the heap, and the | |
| // react_native_assert that used to guard this is compiled out in release. | |
| // The node was re-added (probably by Suspense), we can't finish the exiting animation |
| @@ -127,7 +163,13 @@ std::optional<SurfaceId> LayoutAnimationsProxy_Legacy::endLayoutAnimation(int ta | |||
| } | |||
|
|
|||
| auto node = nodeForTag_[tag]; | |||
Contributor
There was a problem hiding this comment.
This access here is wrong probably and only works due to [] creating the node, maybe the condition below should be if contains and isMutationNode, because:
endAnimationsRecursivelyremoves fromnodeForTag_- an exiting animation on this view's child could put node back in
nodeForTag_, but it wouldn't be a mutation node.
On the other hand, why does this run after an animation got canceled? I think this needs to be verified
Contributor
There was a problem hiding this comment.
It seems that we are still emitting endLayoutAnimation after it got cancelled. I think we could either skip it, or pass a flag here so that we know it was cancelled and can react accordingly, instead of making the defensive checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #9820.
On iOS release builds, an app that runs exiting layout animations under a Suspense boundary that keeps re-suspending crashes inside React Native's mount transaction within a few seconds. This happens with ENABLE_SHARED_ELEMENT_TRANSITIONS set to false, which is where the legacy LayoutAnimationsProxy is active.
The legacy proxy withholds Remove and Delete mutations for views that are animating out. When React later re-creates or re-inserts a tag whose exiting removal is still being withheld, the proxy kept the stale bookkeeping instead of reconciling it. The withheld removal then fired in a later frame against a hierarchy that no longer matched, unmounting the wrong, still-mounted view and corrupting the mounting layer.
This reconciles those contradicted removals at the start of each transaction: if an incoming Create or Insert targets a tag that still has a withheld exiting removal, that removal is flushed right away, before the tag is re-registered, instead of firing later against a stale hierarchy. It also replaces an unchecked MutationNode cast in endLayoutAnimation with a runtime guard, since the assert that used to guard it is compiled out in release builds and the node can legitimately be a plain Node after a reparenting move.
Test plan
Adds the Suspense + Layout Animation Crash example to fabric-example. To reproduce:
Before this change the app crashes within a few seconds. After it, the example keeps running and animating under the same stress.
🤖 Generated with Claude Code