Remove tag from allocatedViewRegistry in destroyUnmountedShadowNode (#56274)

Summary:
Pull Request resolved: #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

Reviewed By: lenaic

Differential Revision: D98729251

fbshipit-source-id: 1a837138622b29bb62e31fb67aacb2c9ff4e9e5d

Author: javache

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([&]() {

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<ViewManager<*, *>>(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)
+  }
+}