Repository navigation
Fix Android Fabric mount crashes from merged mounting transactions (APP-7AR / APP-H7A) - #101057
tylerkaraszewski merged 10 commits into
Conversation
Patch +015 made canMergeWith able to refuse a merge, which broke two invariants stock React Native relies on: that a surface has exactly one pending transaction, and that a merged batch is safe to reorder. - canMergeWith now also refuses an incoming Create/Insert of a tag the pending transaction deletes, and any mutation parented under such a tag - the merge target is looked up from the back of the pending queue - the queue is drained entry by entry instead of being swapped out, so a re-entrant commit cannot jump ahead of what is still pending - merged transactions keep differ mutation order when mounting Fixes the divergence behind Sentry APP-7AR and APP-H7A.
|
|
|
@linhvovan29546 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@linhvovan29546 regarding the HybridApp warning above: this PR doesn't update any library versions. It adds one new React Native patch, The patch touches I don't have access to |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d6cd8cdcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Taking entries off the pending queue one at a time let a concurrent drain (JS thread vs UI thread) take a later transaction and mount it first. Only one call now drains the queue; a call that finds a drain in progress, re-entrant or on another thread, leaves its transaction queued for it. The test now models MountItemDispatcher's FIFO and re-entrancy guard, raises a re-entrant commit mid-batch, and adds a concurrent drain.
|
@oqildev Honestly, I don't think we need to include the test in the |
Requested in review: the patch does not need to ship its own test under patches/react-native.
|
@linhvovan29546 Done, I removed |
|
@codex review! |
|
@codex review |
|
@oqildev Codex isn’t responding for me. Could you please request a Codex review? Thanks! |
|
@codex review |
|
@codex @MelvinBot Review |
|
🤖 Code review — the diagnosis is right, but I'd push back on the shape of the fix. Two things: 1. Hunk 4 makes hunks 1–3 unnecessary
Hunk 4 turns that reordering off for every merged transaction. Once it does, the divergence So the minimal fix for APP-7AR and APP-H7A is hunk 4 plus reverting The counter-argument would be that refusing a merge preserves mount-item batching for the refused transaction. But hunk 4 already forfeits batching for every transaction that did merge, and merging is the common case here — that's the premise of the bug. The batching you keep by refusing merges is marginal. Suggestion: land hunk 4 on its own (hunk 1 is fine to keep as belt-and-braces), and either drop hunks 2–3 with 2. Hunk 3 silently turns synchronous mounts into asynchronous ones
Note that cross-thread mounting was already mutually exclusive: Suggestion: if you keep hunk 3, make it a lock rather than a skip flag, and only skip for same-thread re-entrancy. Store the draining thread id instead of a bool: same thread → return (correct, this is the re-entrant case you need to defer); different thread → wait for the drain, then drain your own. That preserves ordering and the synchronous contract, and it can't deadlock, since the drainer holds no lock while mounting. Things I checked that are fine
Not verified: whether Next Steps: Reply with |
Taking the pending queue one entry at a time under a boolean flag made a synchronous commit on another thread return without mounting: the caller asked for the transaction to be mounted before it returned, which is what commitMutex_ used to give it. The queue now records the draining thread. A re-entrant call on that thread returns and lets the outer drain finish in commit order, while a call on another thread waits for the drain and then drains itself.
Point 2 is right, fixed in 13d6ebc: the queue now records the draining thread instead of a bool, so a re-entrant call returns while a call on another thread waits for the drain and then drains itself, keeping the synchronous contract On point 1, I agree hunk 4 subsumes the hazard So it's either reverting |
…unt-transaction-order # Conflicts: # patches/react-native/details.md
|
@MelvinBot Review |
|
🤖 Re-review of 13d6ebc. The thread-id drain is better than the skip flag, but it still doesn't deliver the synchronous contract — and I'd correct the premise you used to rule out the smaller fix. 1.
|
Patch +015 stopped the CREATE -> INSERT reordering hazard by refusing to merge a transaction that created a tag the pending one deleted. It only inspected Create, and a refused merge left a surface holding several pending transactions, which upstream's merge-target lookup and its swap-then-mount drain do not expect. Setting maintainMutationOrder for merged transactions removes the hazard for every mutation pairing, so +015 and the queue machinery built around it are both deleted.
|
Both points land, and I've taken option A: You're right that You're also right about the drain. The loop runs until the queue is empty, so a JS-thread drainer mounts the UI thread's transaction off the UI thread, and The Android evidence on this PR asserted on |
|
@MelvinBot Review |
|
🤖 Re-review of 81d4eb8. This is the right shape now — 4 hunks, 3 files, no 1.
|
|
The change is risky, so I’m checking and reviewing it carefully to make sure it doesn’t cause any other issues. |
|
Both taken in 964acc4, documentation only.
Remaining, as you say: the Android evidence re-recorded against merged transactions mounting in differ order, and the AdHoc HybridApp build for |
There was a problem hiding this comment.
Since we removed the +015 patch, I think we should update this patch name to +015.
There was a problem hiding this comment.
Done in 3234cd5 — the patch is now react-native+0.86.0+015+fix-merged-transaction-mount-order.patch, and its details.md entry moved up between +014 and +016.
One thing the rename changes: +015 is applied before +027, which also touches FabricMountingManager.cpp, so I regenerated the patch against the pre-+027 tree. The hunk there moves from line 581 to 580 and the pre-image hashes change with it. I checked both orders end up byte-identical: applying +015 then +027 to a pristine 0.86.0 tree produces exactly the same three files as the old +027 then +044 order, and +027 still applies cleanly on top.
|
@oqildev Could you please merge main again? Thanks |
|
@linhvovan29546 Merged |
The old +015 is deleted by this PR, so its number is free and keeping +044 would leave a gap. Regenerated against the pre-+027 tree, since +015 is applied before it: the hunk in FabricMountingManager.cpp moves from line 581 to 580 and the pre-image hashes change accordingly. Applying +015 then +027 produces byte-identical sources to the previous +027 then +044 order.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppUploading Screen Recording 2026-09-23 at 19.05.54.mov… Android: mWeb ChromeThe change only for android/iOS iOS: HybridAppScreen.Recording.2026-09-23.at.19.51.12.moviOS: mWeb SafariThe change only for android/iOS MacOS: Chrome / SafariThe change only for android/iOS |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 tylerkaraszewski has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/tylerkaraszewski in version: 9.4.93-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.93-3 🚀
Bundle Size Analysis (Sentry): |
The doc comment implied every cross-commit mutation list is covered. LayoutAnimationKeyFrameManager::pullTransaction appends mutations deferred from earlier commits and returns a freshly constructed transaction, so merged_ is false there; it is dormant in App, which uses Reanimated rather than LayoutAnimation.configureNext. details.md now also records the Update half: bucketing hoists an Update(X) from one commit ahead of a Create(X) from the next, so narrowing this to deletes would reintroduce that case.
…unt-transaction-order
5 participants
Explanation of Change
On Android,
FabricUIManagerBinding::schedulerDidFinishTransactionmerges a finished mounting transaction into the one still pending for the same surface whenever commits outpace UI-thread mounting, andFabricMountingManager::executeMountthen writesDeletemount items last and splitsUpdates into their own buckets. React Native's own comment on that block says the optimization "is incorrect when multiple transactions are merged together": a mergedREMOVE -> DELETE | CREATE -> INSERTsequence is mounted asINSERT -> REMOVE -> DELETE, which destroys a view the shadow tree still considers mounted. Every index computed for that parent afterwards is stale, and that is what APP-7AR (addViewAt: failed to insert view, underlyingIndexOutOfBoundsException: index=1 count=0) and APP-H7A (Unable to remove a view from a view that is not a ViewGroup) report.This PR replaces patch
+015withreact-native+0.86.0+015+fix-merged-transaction-mount-order.patch.+015guarded the same hazard from the other side, by refusing the merge when the incoming transaction created a tag the pending one deleted. It only inspectedType::Create, so pairings such asDelete(tag)+Insert(same tag)still merged, and refusing a merge left a surface holding several pending transactions - a state upstream's merge-target lookup and its swap-then-mount drain do not expect.The new patch sets
maintainMutationOrderwhen the transaction is a merge of several commits (MountingTransaction::wasMerged()), so a merged batch is mounted in differ order and no mutation pairing can be reordered. Transactions that were never merged keep the delete-batching optimization exactly as it is, and differ-order mounting is already a supported mode - it is whatdisableMountItemReorderingAndroidturns on globally. With no refused merges, a surface holds one pending transaction again, which is upstream's own invariant.The change is limited to Android mount ordering plus two small additions to the shared
MountingTransaction, which iOS compiles but never callswasMerged()on. There are no JS/TS changes.Fixed Issues
$ #98791
PROPOSAL: #98791 (comment)
Tests
A. Android on-device
node_modules/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp, right afterauto revisionNumber = telemetry.getRevisionNumber();, add:cd android && ./gradlew assembleDevelopmentDebug -PpatchedArtifacts.forceBuildFromSource=true.adb logcat | grep APP98791ORDER.APP98791ORDER MERGEDline reportsmaintainOrder=1, so every merged batch is mounted in differ order.addViewAt: failed to insert vieworUnable to remove a view from a view that is not a ViewGroup.mainwith the same logging added. The lines there reportmaintainOrder=0, which is the batch reordering this patch removes.B. Regression, all platforms
Note: on Android Native in development, React Navigation logs two dev-only warnings,
The 'navigation' object hasn't been initialized yetat startup andThe 'state' or 'screen' passed in the params couldn't be applied to the navigator with the screens: 'Inbox', 'Report'when returning to the Inbox tab. This PR has no JS changes, so both come frommainand are unrelated.Offline tests
QA Steps
The crash is a timing-dependent race in native mounting (roughly one event per affected user), so it cannot be triggered on demand on staging. QA should verify there is no regression in everyday navigation:
The fix itself should be confirmed after release by APP-7AR and APP-H7A event counts dropping on app versions that include this PR.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android.native.-.fabric.crash_.mp4
Android: mWeb Chrome
mWeb.android.mp4
iOS: Native
ios-native_.mp4
iOS: mWeb Safari
mWeb.ios.mp4
MacOS: Chrome / Safari
web.chrome.mp4
web.safari.mp4