Skip to content

Preserve mount order when merged transactions recreate a view - #59756

Open
sorinc03 wants to merge 2 commits into
react:mainfrom
sorinc03:fix/android-merged-mount-order
Open

sorinc03 wants to merge 2 commits into
react:mainfrom
sorinc03:fix/android-merged-mount-order

Conversation

@sorinc03

@sorinc03 sorinc03 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #59724.

MountingTransaction::mergeWith can concatenate a delete and a later create for the same native tag. Android normally moves deletes to the end of the encoded batch, so the create can be skipped while the old view is registered and the trailing delete then unregisters the newly reinserted view. Subsequent FlatList updates fail with missing view state, an existing parent, and finally ReactViewGroup.isViewClipped.

Detect delete-then-create lifetimes before collecting mount items and use the existing order-preserving encoder for those batches. Preserve the old grouping for safe batches, and skip hash-set allocation for batches without a delete followed by any create. Virtual views are included because their registry state also has a lifetime. The experimental global flag and Java crash checks are unchanged.

Changelog

[ANDROID] [FIXED] - Preserve native view lifetimes when merged Fabric transactions delete and recreate the same tag

Test Plan

  • The report's release reproduction crashes on the published 0.88.1 native library. Its two-task control continued through more than 500 updates without the one-task failure.
  • Built the same reproduction against this branch's unmodified native source at debaf42 before enabling the fix: three one-task trials all failed, two with the reported fatal isViewClipped stack and one with mounting errors/stalled updates.
  • Patched release: all three 20-second trials completed without crashes or mounting errors, advancing through 39, 40 and 40 updates. A separate 60-second soak completed 122 updates without crashes or mounting errors.
  • Eleven C++ regression tests pass using the actual ShadowViewMutation and MountingTransaction implementations: safe batches, reparenting, virtual views, same-tag recreation, repeated/interleaved lifetimes, merged transactions, and recreation followed by reparenting into an empty container. These test the encoder's order-selection helper, not a mock copy of it.
  • Four Java-side characterization tests replay the old grouped order and preserved mutation order through the real IntBufferBatchMountItem, SurfaceMountingManager and ReactViewManager. Moving Delete last loses the recreated child's ViewState, skips its subsequent insert at index 0, then throws addViewAt ... index 1 with an IndexOutOfBoundsException cause. Preserved order succeeds with both clipping enabled and disabled, keeps both children in order, and emits no soft exceptions.
  • Full Android Debug JVM task with the new coverage: 861 tests, 120 suites, zero failures/errors/skips.
  • All 12 native API snapshots match, checked with CI-pinned Doxygen 1.16.1 in a clean validation checkout. Changed C++ formatting and git diff --check pass.

The release reproduction ran on a dedicated arm64 API 35 Pixel 6 emulator. It is the report's JS fixture with only current-source integration adjustments: the current activity delegate API, matching NDK 28.2 and source-built Hermes compiler. The original report used API 36. No physical-device, all-ABI, or full renderer GTest-suite coverage is claimed. The eleven host GTests used a focused external CMake harness because the existing Android JNI instrumentation test target is not available in the OSS Gradle build.

The new Java-side tests run under Robolectric with manually specified mount-instruction buffers; they do not execute the C++ encoder through JNI. Together with the C++ order-selection test, they validate the proposed skipped-insert mechanism, not the root cause of every production addViewAt crash or an RN 0.86.3 backport.

@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 Confirmed against current native source on an API 35 Pixel 6 emulator: the unpatched one-task case fails in all three trials (two fatal isViewClipped exceptions and one mounting-error/stalled-update run). The patched build passes three 20-second trials (39/40/40 updates) and a separate 60-second run (122 updates), with no crashes or mounting errors. The two-task control also continued past 500 updates. Could you retest this targeted fallback on your API 36 setup?

@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
@GersonRocha9

Copy link
Copy Markdown

Adding a production data point in case it helps prioritize this. We see a related signature on Android:

java.lang.IllegalStateException: addViewAt: failed to insert view into parent at index 1
  at com.facebook.react.uimanager.ReactClippingViewManager.addView
Caused by: java.lang.IndexOutOfBoundsException
  • 12 events / 7 users since August, React Native 0.86.3 (New Architecture), Android 11 to 17 on Samsung, POCO and Nokia devices.
  • Fatal, with no app frames in the stack, so there is nothing we can work around in JS. We don't set removeClippedSubviews ourselves.

It looks like the same merged-transaction ordering problem as #57017 (delete + recreate of a tag ending up out of order). If this PR is expected to cover the addViewAt variant as well, that would close it for us.

@sorinc03

sorinc03 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

@GersonRocha9 This is expected to cover addViewAt failures when they are caused by the same delete-then-recreate reordering, but I cannot confirm that from the top-level exception alone.

Our recorded unpatched, current-source reproduction also produced addViewAt: cannot insert view ... View already has a parent before the clipping crash. With this patch, the three 20-second runs and separate 60-second run had no mounting errors. That supports this family of failures, but it is not verification of your exact IndexOutOfBoundsException variant on 0.86.3. The reports in #57017 do not by themselves establish one shared root cause.

This patch preserves native view lifetimes; it does not clamp insertion indices or suppress exceptions. Could you share the full Caused by stack and nearby MissingViewState / View already has a parent messages, if available? A minimal reproducer or retest with the native patch would help confirm coverage.

This PR targets main; a 0.86.3 backport has not been validated. I would keep your production case unconfirmed until that evidence or retest is available.

@sorinc03

sorinc03 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

@GersonRocha9 Follow-up: I have now validated the proposed index-1 failure chain locally, without needing an app reproducer from you. The added coverage is in 83a4269.

Using React Native's real Java mounting classes, a merged delete/recreate with Delete moved to the end loses the child's ViewState. Moving that child into an empty parent then skips the insert at index 0; inserting its sibling at index 1 throws the wrapped IndexOutOfBoundsException. I reproduced this with removeClippedSubviews both enabled and disabled.

Preserving the mutation order, as this PR does, makes the same sequence succeed: both children remain registered and correctly ordered, the source parent is empty, and no soft exceptions are emitted. I added four Java-side characterization tests plus a C++ test confirming that this merged lifetime requests strict order. No additional production change or bounds-clamping workaround was needed for this mechanism.

The full Android JVM suite passes (861 tests), as do all 11 C++ order-selection tests. The Java tests use explicit instruction buffers under Robolectric, not a full C++/JNI pipeline or your 0.86.3 app. They establish this causal mechanism rather than proving that every similar production stack has the same cause.

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.

[Android] FlatList crashes when a View loses its native view and gets it back

2 participants