Transaction merging can cause `CREATE` mutations to be effectively dropped

[j-piasecki]

Description

Related to #38743

Transaction merging introduced in #44188 may cause CREATE mutations to be effectively dropped.

When two transactions, where the first one contains a DELETE mutation for a view with a view tag X and the other contains a CREATE mutation for a view with a view tag X, get merged the result is a transaction containing both mutations. When that transaction is executed, the DELETE operations are run after CREATE operations, so the view will not exist after the transaction finishes which results in the host tree diverging from the shadow tree.

Inside FabricUIManagerBinding pendingTransactions_ field tracks transactions to be executed but only one per surface:

react-native/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h

Lines 164 to 166 in a6b2355

	// Track pending transactions, one per surfaceId
	std::mutex pendingTransactionsMutex_;
	std::vector<MountingTransaction> pendingTransactions_;

Inside schedulerDidFinishTransaction transactions are merged when a transaction for a given surface is already pending:

react-native/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp

Lines 546 to 550 in a6b2355

	if (pendingTransaction != pendingTransactions_.end()) {
	pendingTransaction->mergeWith(std::move(*mountingTransaction));
	} else {
	pendingTransactions_.push_back(std::move(*mountingTransaction));
	}

If instead of merging transactions into one, multiple ones could be queued for a single surface and executed in order, the issue would be fixed. I can open a PR with that change but before that, I'd like to know whether that could have any unwanted side effects, as I'm lacking context on why merging was chosen there.

It also seems like this may be a known problem:

react-native/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp

Lines 828 to 829 in 69356b7

	// FIXME: this optimization is incorrect when multiple transactions are
	// merged together

Steps to reproduce

1. Open the application (the code may be pasted to RNTester)
2. Click the Click this first button
3. Click the Click this second button

React Native Version

0.76.2

Affected Platforms

Runtime - Android

Areas

Fabric - The New Renderer

Output of npx react-native info

Details

System:
  OS: macOS 14.7.1
  CPU: (12) arm64 Apple M3 Pro
  Memory: 96.86 MB / 18.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.18.0
    path: ~/.nvm/versions/node/v20.18.0/bin/node
  Yarn:
    version: 1.22.19
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.8.2
    path: ~/.nvm/versions/node/v20.18.0/bin/npm
  Watchman:
    version: 2024.09.09.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.15.2
    path: /Users/jakubpiasecki/.rvm/gems/ruby-2.7.5/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK:
    API Levels:
      - "24"
      - "26"
      - "28"
      - "29"
      - "30"
      - "31"
      - "32"
      - "33"
      - "34"
      - "35"
    Build Tools:
      - 26.0.3
      - 28.0.3
      - 29.0.2
      - 29.0.3
      - 30.0.2
      - 30.0.3
      - 31.0.0
      - 32.0.0
      - 32.1.0
      - 33.0.0
      - 33.0.1
      - 34.0.0
      - 35.0.0
      - 35.0.0
    System Images:
      - android-28 | Google ARM64-V8a Play ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
      - android-34 | Google Play ARM 64 v8a
      - android-35 | Google Play ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2024.2 AI-242.23339.11.2421.12550806
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.2
    path: /usr/bin/javac
  Ruby:
    version: 2.7.5
    path: /Users/jakubpiasecki/.rvm/rubies/ruby-2.7.5/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react: Not Found
  react-native: Not Found
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: false
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

Exception in native call
com.facebook.react.bridge.RetryableMountingLayerException: Unable to find viewState for tag 4. Surface stopped: false
	at com.facebook.react.fabric.mounting.SurfaceMountingManager.getViewState(SurfaceMountingManager.java:1103)
	at com.facebook.react.fabric.mounting.SurfaceMountingManager.updateProps(SurfaceMountingManager.java:687)
	at com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.execute(IntBufferBatchMountItem.java:157)
	at com.facebook.react.fabric.mounting.MountItemDispatcher.executeOrEnqueue(MountItemDispatcher.java:370)
	at com.facebook.react.fabric.mounting.MountItemDispatcher.dispatchMountItems(MountItemDispatcher.java:265)
	at com.facebook.react.fabric.mounting.MountItemDispatcher.tryDispatchMountItems(MountItemDispatcher.java:122)
	at com.facebook.react.fabric.FabricUIManager$DispatchUIFrameCallback.doFrameGuarded(FabricUIManager.java:1392)
	at com.facebook.react.fabric.GuardedFrameCallback.doFrame(GuardedFrameCallback.kt:22)
	at com.facebook.react.modules.core.ReactChoreographer.frameCallback$lambda$1(ReactChoreographer.kt:59)
	at com.facebook.react.modules.core.ReactChoreographer.$r8$lambda$nSkFhrr5T7rop_XKwzlLov4NLLw(Unknown Source:0)
	at com.facebook.react.modules.core.ReactChoreographer$$ExternalSyntheticLambda0.doFrame(D8$$SyntheticClass:0)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1404)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1415)
	at android.view.Choreographer.doCallbacks(Choreographer.java:1015)
	at android.view.Choreographer.doFrame(Choreographer.java:941)
	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1389)
	at android.os.Handler.handleCallback(Handler.java:959)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loopOnce(Looper.java:232)
	at android.os.Looper.loop(Looper.java:317)
	at android.app.ActivityThread.main(ActivityThread.java:8699)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:886)

Reproducer

https://snack.expo.dev/@jpiasecki/unable_to_find_viewstate_repro

Screenshots and Videos

Screen.Recording.2024-11-26.at.13.18.17.mov

[react-native-bot]

Tip

Newer version available: You are on a supported minor version, but it looks like there's a newer patch available - 0.76.3. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

[react-native-bot]

Tip

Newer version available: You are on a supported minor version, but it looks like there's a newer patch available - undefined. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

[javache]

Thanks for the report! I’m working on a fix under the disable_mount_item_reordering, please try it out. We’ve hit some issues rolling that out internally, but we can try and prioritise this.

[javache]

Note that transaction merging is fairly recent, but the underlying crash has been around for a while.

[j-piasecki]

I’m working on a fix under the disable_mount_item_reordering, please try it out.

Thanks for pointing it out! I've been testing on the Expensify App so I missed it 🤦. I've tried enabling the flag on the repro from this issue and unfortunately, it doesn't help - the crash happens when clicking the first button (which changes collapsable prop to true and back to false inside a layout effect).

As a side note, do you see any obvious problems with disabling the transaction merging and queuing more than one instead?

[javache]

Transaction merging (which just appends them really) is required to correctly implement cascading renders from effects in the new architecture. Otherwise, it's possible to observe the intermediate states which is not correct.

[javache]

I've narrowed down this repro to a mounting unit test in #48002