From 172e07b061b46f008504f1821662d7da2697c6e9 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Mon, 30 Mar 2026 12:35:58 -0700 Subject: [PATCH] Remove tag from allocatedViewRegistry in destroyUnmountedShadowNode (#56274) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/56274 When a ShadowNode is destroyed (e.g. from a superseded concurrent render or React Transition), destroyUnmountedShadowNode calls Java destroyUnmountedView which deletes the view from tagToViewState. However, the C++ allocatedViewRegistry was not updated, leaving a stale entry. This causes a crash when a subsequent executeMount encounters a Create mutation for the same tag: allocatedViewTags.contains(tag) returns true (stale entry from preallocate), so the Create mount item is skipped. But the Java view was already destroyed. When subsequent mount items (Insert, UpdateState, etc.) try to reference the tag, getViewState() throws RetryableMountingLayerException because the tag is not in tagToViewState. The fix mirrors what the normal Delete mutation path already does (line 638: allocatedViewTags.erase(tag)) - ensuring allocatedViewRegistry stays in sync with the Java-side tagToViewState. Changelog: [Android][Fixed] - Fix crash from stale preallocated view registry after concurrent render cancellation Differential Revision: D98729251 --- .../react/fabric/FabricMountingManager.cpp | 14 +++ .../fabric/SurfaceMountingManagerTest.kt | 118 ++++++++++++++++++ 2 files changed, 132 insertions(+) create mode 100644 packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerTest.kt diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp index 95ddd80e6d9c..0cdf3513c3b6 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp @@ -1026,6 +1026,20 @@ void FabricMountingManager::destroyUnmountedShadowNode( auto tag = family.getTag(); auto surfaceId = family.getSurfaceId(); + // Remove from allocatedViewRegistry so that executeMount does not skip + // the Create mount item for this tag. Without this, if the view was + // preallocated and then destroyed (e.g. due to a superseded concurrent + // render), executeMount would skip the Create because allocatedViewTags + // still contains the tag, but the Java side no longer has the view in + // tagToViewState (it was deleted by destroyUnmountedView below). + { + std::lock_guard allocatedViewsLock(allocatedViewsMutex_); + auto allocatedViewsIterator = allocatedViewRegistry_.find(surfaceId); + if (allocatedViewsIterator != allocatedViewRegistry_.end()) { + allocatedViewsIterator->second.erase(tag); + } + } + // ThreadScope::WithClassLoader is necessary because // destroyUnmountedShadowNode is being called from a destructor thread jni::ThreadScope::WithClassLoader([&]() { diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerTest.kt new file mode 100644 index 000000000000..be7b6f0c9d22 --- /dev/null +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerTest.kt @@ -0,0 +1,118 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +@file:Suppress("DEPRECATION") + +package com.facebook.react.fabric + +import com.facebook.react.ReactRootView +import com.facebook.react.bridge.JavaOnlyMap +import com.facebook.react.bridge.ReactTestHelper +import com.facebook.react.fabric.mounting.MountingManager +import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor +import com.facebook.react.fabric.mounting.SurfaceMountingManager +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.ThemedReactContext +import com.facebook.react.uimanager.ViewManager +import com.facebook.react.uimanager.ViewManagerRegistry +import com.facebook.react.views.view.ReactViewManager +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Tests for [SurfaceMountingManager] view lifecycle around preallocateView, deleteView, and + * createView. + * + * The key scenario is the interaction between view preallocation and destroyUnmountedShadowNode + * (D98729251): when a preallocated view is destroyed (e.g. from a superseded concurrent render), + * and then a new Create mount item is emitted for the same tag, the view must be recreated. + * + * NOTE: These tests verify Java-side behavior only. The C++ fix in FabricMountingManager (erasing + * from allocatedViewRegistry_ in destroyUnmountedShadowNode) ensures the Create mount item is + * emitted in the first place. That behavior requires native code and is not testable via + * Robolectric. + * + * See T223288217. + */ +@RunWith(RobolectricTestRunner::class) +class SurfaceMountingManagerTest { + private lateinit var mountingManager: MountingManager + private lateinit var themedReactContext: ThemedReactContext + private val surfaceId = 1 + + @Before + fun setUp() { + ReactNativeFeatureFlagsForTests.setUp() + val reactContext = ReactTestHelper.createCatalystContextForTest() + themedReactContext = ThemedReactContext(reactContext, reactContext, null, -1) + mountingManager = + MountingManager( + ViewManagerRegistry(listOf>(ReactViewManager())), + MountItemExecutor {}, + ) + } + + private fun startSurface(): SurfaceMountingManager { + val rootView = ReactRootView(themedReactContext) + mountingManager.startSurface(surfaceId, themedReactContext, rootView) + return mountingManager.getSurfaceManagerEnforced(surfaceId, "test") + } + + @Test + fun preallocateView_createsView() { + val smm = startSurface() + smm.preallocateView("RCTView", 42, JavaOnlyMap.of(), null, true) + assertThat(smm.getViewExists(42)).isTrue() + } + + @Test + fun deleteView_removesPreallocatedView() { + val smm = startSurface() + smm.preallocateView("RCTView", 42, JavaOnlyMap.of(), null, true) + smm.deleteView(42) + assertThat(smm.getViewExists(42)).isFalse() + } + + /** + * Verifies the Java side of the scenario fixed by D98729251: + * 1. preallocateView creates the view + * 2. deleteView removes it (simulating destroyUnmountedShadowNode) + * 3. createView recreates it (the C++ fix ensures this Create is actually emitted) + * 4. addViewAt succeeds with the recreated view + */ + @Test + fun createView_succeedsAfterPreallocateAndDelete() { + val smm = startSurface() + + smm.preallocateView("RCTView", 42, JavaOnlyMap.of(), null, true) + assertThat(smm.getViewExists(42)).isTrue() + + smm.deleteView(42) + assertThat(smm.getViewExists(42)).isFalse() + + smm.createView("RCTView", 42, JavaOnlyMap.of(), null, null, true) + assertThat(smm.getViewExists(42)).isTrue() + + smm.addViewAt(surfaceId, 42, 0) + assertThat(smm.getView(42)).isNotNull() + } + + /** createView is a no-op when the view already exists from preallocate (normal case). */ + @Test + fun createView_isIdempotentWhenPreallocated() { + val smm = startSurface() + + smm.preallocateView("RCTView", 42, JavaOnlyMap.of(), null, true) + val viewBefore = smm.getView(42) + + smm.createView("RCTView", 42, JavaOnlyMap.of(), null, null, true) + assertThat(smm.getView(42)).isSameAs(viewBefore) + } +}