Repository navigation
Conversation
Contributor
|
By analyzing the blame information on this pull request, we identified @hramos and @JoelMarcey to be potential reviewers. |
Contributor
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
Contributor
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Contributor
|
@facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
Import Started
meta-codesync Bot
pushed a commit
that referenced
this pull request
Nov 4, 2026
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. --> ## Summary: <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> 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: ```text 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`](https://github.com/software-mansion/react-native-screens/blob/4.27.0/android/src/main/java/com/swmansion/rnscreens/Screen.kt), [`NativeProxy.notifyScreenRemoved`](https://github.com/software-mansion/react-native-screens/blob/4.27.0/android/src/fabric/java/com/swmansion/rnscreens/NativeProxy.kt)). Reanimated shared element transitions will soon move the source view out of such a screen into the surface root ([react-native-reanimated#11054](software-mansion/react-native-reanimated#11054)), and the app crashes when going back. Analysis: [comment on #11054](software-mansion/react-native-reanimated#11054 (comment)). Reanimated is getting a workaround that releases the holds before the mount batch ([react-native-reanimated#11101](software-mansion/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`](https://developer.android.com/reference/android/view/ViewGroup#endViewTransition(android.view.View)) 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: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [ANDROID] [FIXED] - Fix `addViewAt` crash when a view that a parent still holds with `startViewTransition` is inserted into another parent ## Test Plan: <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. --> New Robolectric tests in `SurfaceMountingManagerViewTransitionTest`, with the real `SurfaceMountingManager` and real Android `ViewGroup`s: ```sh ./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: ```sh ./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:** https://github.com/user-attachments/assets/40cd095f-909e-47b4-b741-16cdc9751bba **After:** https://github.com/user-attachments/assets/db6d35b5-c9e7-40ec-8632-2b96f79188f6 Differential Revision: D129333127 Pull Request resolved: #59776
fabriziocucci
pushed a commit
that referenced
this pull request
Nov 8, 2026
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. --> ## Summary: <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> 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: ```text 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`](https://github.com/software-mansion/react-native-screens/blob/4.27.0/android/src/main/java/com/swmansion/rnscreens/Screen.kt), [`NativeProxy.notifyScreenRemoved`](https://github.com/software-mansion/react-native-screens/blob/4.27.0/android/src/fabric/java/com/swmansion/rnscreens/NativeProxy.kt)). Reanimated shared element transitions will soon move the source view out of such a screen into the surface root ([react-native-reanimated#11054](software-mansion/react-native-reanimated#11054)), and the app crashes when going back. Analysis: [comment on #11054](software-mansion/react-native-reanimated#11054 (comment)). Reanimated is getting a workaround that releases the holds before the mount batch ([react-native-reanimated#11101](software-mansion/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`](https://developer.android.com/reference/android/view/ViewGroup#endViewTransition(android.view.View)) 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: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [ANDROID] [FIXED] - Fix `addViewAt` crash when a view that a parent still holds with `startViewTransition` is inserted into another parent ## Test Plan: <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. --> New Robolectric tests in `SurfaceMountingManagerViewTransitionTest`, with the real `SurfaceMountingManager` and real Android `ViewGroup`s: ```sh ./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: ```sh ./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:** https://github.com/user-attachments/assets/40cd095f-909e-47b4-b741-16cdc9751bba **After:** https://github.com/user-attachments/assets/db6d35b5-c9e7-40ec-8632-2b96f79188f6 Differential Revision: D129333127 Pull Request resolved: #59776 (cherry picked from commit 45152f1)
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.
Line 367 said throughly when it should say thoroughly