[pawicao]

Summary:

On Android, SurfaceMountingManager.addViewAt fails when it inserts a view that a parent still holds with ViewGroup.startViewTransition.

A held child can leave its old parent's child list while child.parent still points to that old parent:

oldParent.indexOfChild(view) == -1
view.parent == oldParent

The existing recovery in addViewAt calls removeView on the old parent. That does nothing for a view that is no longer in the child list, so addView throws addViewAt: failed to insert view ... into parent. The recovery also adds the tag to erroneouslyReaddedReactTags, so a later Remove of that tag returns early and the view is never removed.

React Native Screens holds the content of a removed screen this way during its exit animation (Screen.startRemovalTransition, NativeProxy.notifyScreenRemoved).
Reanimated shared element transitions will soon move the source view out of such a screen into the surface root (react-native-reanimated#11054), and the app crashes when going back.
Analysis: comment on #11054.
Reanimated is getting a workaround that releases the holds before the mount batch (react-native-reanimated#11101), but addViewAt is the better place for the fix.

The fix

Before the existing recovery, if the old parent no longer lists the view, call endViewTransition on it, then read view.parent again. If the parent is cleared, the view is inserted normally and the tag is not marked as re-added. If the parent is still set, the existing recovery and its diagnostics run as before.

• Holds of removed views that are not inserted again stay in place, so normal exit animations do not change.
• A view held more than once, for example by nested screens, is released by one call, because endViewTransition clears the parent when it removes the view from the disappearing children.
• Nested carriers each go through the same check on their own insert.
• ReactViewGroup.endViewTransition also clears its own childrenRemovedWhileTransitioning entry, so RN's clipping state stays in sync.

Changelog:

[ANDROID] [FIXED] - Fix addViewAt crash when a view that a parent still holds with startViewTransition is inserted into another parent

Test Plan:

New Robolectric tests in SurfaceMountingManagerViewTransitionTest, with the real SurfaceMountingManager and real Android ViewGroups:

./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest \
  --tests 'com.facebook.react.fabric.SurfaceMountingManagerViewTransitionTest'

Test	Without fix	With fix
held child moves to root, then final removal	fails: addViewAt: failed to insert view [12] into parent [1] at index 1	passes
nested held children (each parent holds twice, carrier in carrier) move, then final removal	fails (same error)	passes
held child moves with the root attached to a window	fails (same error)	passes
held child is put back into its old parent	fails: ... into parent [10] at index 0	passes
control: held exit with no reinsert keeps its hold	passes	passes
control: unheld move	passes	passes
control: existing recovery for a child that another parent still lists	passes	passes

Other checks:

./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests 'com.facebook.react.fabric.*' -PenableWarningsAsErrors=true  # 89 tests, 0 failures
./gradlew :packages:react-native:ReactAndroid:lintDebug -PenableWarningsAsErrors=true  # no issues in changed files
yarn format-check-kotlin  # pass

Device test: Reanimated apps/fabric-example (branch of #11054, without #11101), Android emulator, Debug, --active-arch-only, React Native built from source. The example app does not build against main (it needs RN 0.86 to 0.88), so this used v0.88.0 with only this SurfaceMountingManager.kt change applied.

• Without the fix: Shared Element Transitions → Fade Content → Sun, wait for the push to end, tap the content to go back. The app crashes with addViewAt: failed to insert view [666] into parent [1] at index 3. The specified child already has a parent.
• With the fix: the same path (3 times), the hardware Back button (4 times), a quick pop during the push, nested shared tags, and nested stack navigation. No crash, no addViewAt or "already has a parent" lines in logcat, and no shared view is left on the root after the pop.

Recordings

Before:

before-fade-content-tap-back.mp4

After:

after-fade-content-tap-back.mp4

[javache]

Can you add this logic inline here, so we can avoid doing the same null / class checks?

[pawicao]

Thanks for the review! I did inline it in d7addde. As a result view.parent is now read once, and read again only after endViewTransition. Is this what you had in mind here? I could also potentially fold the two older is ViewGroup checks in the recovery block onto one as? ViewGroup value, so the parent's class is checked only once if you prefer

[meta-codesync]

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

[mangeshraut712]

In SurfaceMountingManager.addViewAt (SurfaceMountingManager.kt ~339–341), a held child is released with a single endViewTransition(view) when indexOfChild is -1.

ViewGroup.startViewTransition can be nested: each call appends the view to mTransitioningViews, and endViewTransition only removes one occurrence. The new nested test (addViewAt_movesNestedHeldChildren...) calls startViewTransition twice per parent via repeat(2) before removeViewAt, then relies on this one endViewTransition during addViewAt.

On a real ViewGroup that still leaves a hold, view.parent can stay non-null after the move/remove path and the “final removal” assertions can fail outside Robolectric. Loop endViewTransition while indexOfChild(view) == -1 && parent === currParentView (or while the view remains in the transitioning set), matching the number of outstanding starts.