react-native icon indicating copy to clipboard operation
react-native copied to clipboard

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

Open j-piasecki opened this issue 1 year ago • 10 comments

Description

Related to https://github.com/facebook/react-native/issues/38743

Transaction merging introduced in https://github.com/facebook/react-native/pull/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: https://github.com/facebook/react-native/blob/a6b2355b8d559aba2f1f13394c91f563f9b9e1a4/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h#L164-L166

Inside schedulerDidFinishTransaction transactions are merged when a transaction for a given surface is already pending: https://github.com/facebook/react-native/blob/a6b2355b8d559aba2f1f13394c91f563f9b9e1a4/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp#L546-L550

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: https://github.com/facebook/react-native/blob/69356b7f2144283fa6be7e9998e25a0681e159f0/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp#L828-L829

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

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

https://github.com/user-attachments/assets/7df6bc47-a741-48cc-85e3-398239415b11

j-piasecki avatar Nov 26 '24 14:11 j-piasecki

[!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 avatar Nov 26 '24 14:11 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.

react-native-bot avatar Nov 26 '24 14:11 react-native-bot

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 avatar Nov 26 '24 20:11 javache

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

javache avatar Nov 26 '24 20:11 javache

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?

j-piasecki avatar Nov 27 '24 10:11 j-piasecki

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 avatar Nov 27 '24 13:11 javache

I've narrowed down this repro to a mounting unit test in https://github.com/facebook/react-native/pull/48002

javache avatar Nov 28 '24 11:11 javache

I have my app migrated to EXPO SDK 52 recently and this bug happens ALL the times on a given screen, I can't release my app until this is fixed ?

jacargentina avatar Nov 28 '24 21:11 jacargentina

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot avatar May 28 '25 05:05 react-native-bot

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot avatar May 28 '25 05:05 react-native-bot

This seems to be fixed on latest versions, i'm using EXPO SDK 53 and the error has gone.

jacargentina avatar Jun 02 '25 13:06 jacargentina

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot avatar Dec 02 '25 05:12 react-native-bot