Fix crash from withheld exiting layout animations in experimental proxy - #10073
Conversation
When React re-creates a tag whose exiting removal is still withheld (e.g. a Suspense boundary re-suspending over animating views), the experimental proxy either overwrote or missed the stale bookkeeping, letting the old still-mounted view corrupt the mounting layer. Port of the reconcile fix from #9821 to LayoutAnimationsProxy_Experimental, extended to its bookkeeping model: reconcile contradicted removals for nodes tracked in lightNodes_ and deadNodes, register WAITING subtree members in lightNodes_ so the reconcile can find them, erase lightNodes_ entries when withheld nodes are deleted, and guard the endLayoutAnimation lookup against a release-mode null deref. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe experimental layout animation proxy reconciles re-created tags with withheld removals during mounting transactions and keeps ChangesLayout removal reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReactMounting
participant LayoutAnimationsProxy_Experimental
participant lightNodes_
participant deadNodes
participant LightNodeParent
ReactMounting->>LayoutAnimationsProxy_Experimental: pullTransaction(mutations)
LayoutAnimationsProxy_Experimental->>lightNodes_: reconcile Create/Insert tags
LayoutAnimationsProxy_Experimental->>deadNodes: check DEAD nodes
LayoutAnimationsProxy_Experimental->>LightNodeParent: remove contradicted node
LayoutAnimationsProxy_Experimental->>LayoutAnimationsProxy_Experimental: end subtree animations and drop ancestors
LayoutAnimationsProxy_Experimental-->>ReactMounting: filtered mutations
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpp (1)
123-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider regression test coverage for this crash path.
This is a targeted fix for a hard-to-hit, timing-dependent native crash (Suspense re-suspension + exiting layout animations + shared element transitions). No test exercising
reconcileContradictedRemovals/the recreate-while-withheld scenario is included in the reviewed files. If there's an existing native/mounting-layer test harness for this proxy, a regression test would materially reduce risk of silent reintroduction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpp` around lines 123 - 172, Add a regression test using the existing native or mounting-layer harness for LayoutAnimationsProxy_Experimental and exercise reconcileContradictedRemovals when a tag is recreated while its removal is withheld, including the exiting/shared-transition timing path. Assert the stale removal is flushed and the recreated view remains correctly registered without crashing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpp`:
- Around line 123-172: Add a regression test using the existing native or
mounting-layer harness for LayoutAnimationsProxy_Experimental and exercise
reconcileContradictedRemovals when a tag is recreated while its removal is
withheld, including the exiting/shared-transition timing path. Assert the stale
removal is flushed and the recreated view remains correctly registered without
crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31024595-4185-4564-8145-e498872f0c94
📒 Files selected for processing (2)
packages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.cpppackages/react-native-reanimated/Common/cpp/reanimated/LayoutAnimations/LayoutAnimationsProxy_Experimental.h
…xy (#10073) ## Summary Closes #9822. On iOS release builds with `ENABLE_SHARED_ELEMENT_TRANSITIONS: true` (the default on `main`), the Suspense + Layout Animation Crash example crashes within ~1–2 seconds with `EXC_BAD_ACCESS` inside `LayoutAnimationsProxy_Experimental::updateLightTree`. This is the experimental-proxy counterpart of #9820/#9821: when React re-creates a tag whose exiting removal the proxy is still withholding (a re-suspending Suspense boundary deletes a subtree and later re-creates it with the same fiber tags), the proxy's bookkeeping is contradicted and corrupts both the light tree and the mounting layer's view registry. Unlike the legacy proxy — whose `nodeForTag_` tracks every node of a withheld subtree, so the #9821 reconcile could catch any re-created tag — the experimental proxy only kept `ANIMATING` roots in `lightNodes_`. Two whole classes of withheld views were invisible to any reconcile: - **`WAITING` interior nodes** (e.g. FlatList cell containers kept alive under an exiting item) — tracked in neither `lightNodes_` nor `deadNodes`, - **settled `DEAD` nodes** — already moved to `deadNodes` and erased from `lightNodes_`, but still mounted until the end-of-transaction cleanup, which runs *after* an incoming `Create` would have re-registered the tag. A `Create` for such a tag passed straight through against a still-registered old view. Diagnosing with a mutation-stream replay showed the resulting "double Create" (an iOS `RCTComponentViewRegistry` overwrite) as the single divergence source; the wrong view then got recycled by the old node's deferred `Delete`, and subsequent transactions faulted on null light nodes / stale indices. The fix mirrors #9821, adapted to the experimental proxy's bookkeeping: - `reconcileContradictedRemovals` runs at the start of `pullTransaction`: an incoming `Create`/`Insert` targeting a withheld tag — found in `lightNodes_` (any non-live state) or in `deadNodes` — flushes that node's withheld Remove/Delete immediately, before the tag is re-registered, mirroring the `deadNodes` cleanup in `handleRemovals`. - `WAITING` subtree members are now registered in `lightNodes_` (both the kept-subview branch of `startAnimationsRecursively` and the kept-root branch of `handleRemovals`), so the reconcile can find them — matching the legacy proxy's per-node bookkeeping. - `endAnimationsRecursively` and `maybeDropAncestors` erase the deleted node's `lightNodes_` entry (guarded so a re-registered live node is never touched). This also fixes a pre-existing stale-entry leak for animating descendants force-ended together with an exiting ancestor. - `endLayoutAnimation` replaces an unchecked `lightNodes_[tag]` lookup with a guarded `find` — the old code would default-insert a null node and dereference it in release, where the assert is compiled out. ## Test plan Uses the Suspense + Layout Animation Crash example added in #9821, with `ENABLE_SHARED_ELEMENT_TRANSITIONS` left at its default (`true`): 1. build `fabric-example` for iOS in Release, 2. open the example and tap Start stress. Before this change the app crashes within ~1–2 seconds; three consecutive runs on `main` produced the exact stack reported in #9822 (`SIGSEGV` in `updateLightTree` ← `pullTransaction` ← `RCTMountingManager performTransaction`). After it, the same stress ran through hundreds of mode switches (several minutes) with no crash, and a replay of ~525k logged mutations from an instrumented run showed no registry overwrites and no index divergence between the emitted mutations and the mounted hierarchy. (cherry picked from commit 7225372) Co-authored-by: Tomasz Żelawski <tzelawski@gmail.com>
Summary
Closes #9822.
On iOS release builds with
ENABLE_SHARED_ELEMENT_TRANSITIONS: true(the default onmain), the Suspense + Layout Animation Crash example crashes within ~1–2 seconds withEXC_BAD_ACCESSinsideLayoutAnimationsProxy_Experimental::updateLightTree. This is the experimental-proxy counterpart of #9820/#9821: when React re-creates a tag whose exiting removal the proxy is still withholding (a re-suspending Suspense boundary deletes a subtree and later re-creates it with the same fiber tags), the proxy's bookkeeping is contradicted and corrupts both the light tree and the mounting layer's view registry.Unlike the legacy proxy — whose
nodeForTag_tracks every node of a withheld subtree, so the #9821 reconcile could catch any re-created tag — the experimental proxy only keptANIMATINGroots inlightNodes_. Two whole classes of withheld views were invisible to any reconcile:WAITINGinterior nodes (e.g. FlatList cell containers kept alive under an exiting item) — tracked in neitherlightNodes_nordeadNodes,DEADnodes — already moved todeadNodesand erased fromlightNodes_, but still mounted until the end-of-transaction cleanup, which runs after an incomingCreatewould have re-registered the tag.A
Createfor such a tag passed straight through against a still-registered old view. Diagnosing with a mutation-stream replay showed the resulting "double Create" (an iOSRCTComponentViewRegistryoverwrite) as the single divergence source; the wrong view then got recycled by the old node's deferredDelete, and subsequent transactions faulted on null light nodes / stale indices.The fix mirrors #9821, adapted to the experimental proxy's bookkeeping:
reconcileContradictedRemovalsruns at the start ofpullTransaction: an incomingCreate/Inserttargeting a withheld tag — found inlightNodes_(any non-live state) or indeadNodes— flushes that node's withheld Remove/Delete immediately, before the tag is re-registered, mirroring thedeadNodescleanup inhandleRemovals.WAITINGsubtree members are now registered inlightNodes_(both the kept-subview branch ofstartAnimationsRecursivelyand the kept-root branch ofhandleRemovals), so the reconcile can find them — matching the legacy proxy's per-node bookkeeping.endAnimationsRecursivelyandmaybeDropAncestorserase the deleted node'slightNodes_entry (guarded so a re-registered live node is never touched). This also fixes a pre-existing stale-entry leak for animating descendants force-ended together with an exiting ancestor.endLayoutAnimationreplaces an uncheckedlightNodes_[tag]lookup with a guardedfind— the old code would default-insert a null node and dereference it in release, where the assert is compiled out.Test plan
Uses the Suspense + Layout Animation Crash example added in #9821, with
ENABLE_SHARED_ELEMENT_TRANSITIONSleft at its default (true):fabric-examplefor iOS in Release,Before this change the app crashes within ~1–2 seconds; three consecutive runs on
mainproduced the exact stack reported in #9822 (SIGSEGVinupdateLightTree←pullTransaction←RCTMountingManager performTransaction). After it, the same stress ran through hundreds of mode switches (several minutes) with no crash, and a replay of ~525k logged mutations from an instrumented run showed no registry overwrites and no index divergence between the emitted mutations and the mounted hierarchy.🤖 Generated with Claude Code