From a476277fc54a5595995016268696e407299e8e87 Mon Sep 17 00:00:00 2001 From: Sorin Cioban Date: Mon, 2 Nov 2026 11:54:50 +0000 Subject: [PATCH 1/2] Preserve mount order when merged transactions recreate a view --- .../react/fabric/FabricMountingManager.cpp | 14 +-- .../jni/react/fabric/MountItemReordering.h | 45 +++++++ .../fabric/test/MountItemReorderingTest.cpp | 113 ++++++++++++++++++ 3 files changed, 164 insertions(+), 8 deletions(-) create mode 100644 packages/react-native/ReactAndroid/src/main/jni/react/fabric/MountItemReordering.h create mode 100644 packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp 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 b5f1eb569b2..2ab6635ef4f 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 @@ -10,6 +10,7 @@ #include "AnimatedPropBufferEncoder.h" #include "EventEmitterWrapper.h" #include "MountItem.h" +#include "MountItemReordering.h" #include "StateWrapperImpl.h" #include @@ -606,7 +607,8 @@ void FabricMountingManager::executeMount( auto& mutations = transaction.getMutations(); bool maintainMutationOrder = - ReactNativeFeatureFlags::disableMountItemReorderingAndroid(); + ReactNativeFeatureFlags::disableMountItemReorderingAndroid() || + requiresStrictMountItemOrder(mutations); bool updateViewsOnDensityChange = ReactNativeFeatureFlags::fixTextLayoutOnPixelDensityChange(); @@ -1026,13 +1028,9 @@ void FabricMountingManager::executeMount( } } - // Write deletes last - so that all prop updates, etc, for the tag in the same - // batch don't fail. Without additional machinery, moving deletes here - // requires that the differ never produces "DELETE...CREATE" in that order for - // the same tag. It's nice to be able to batch all similar operations together - // for space efficiency. - // FIXME: this optimization is incorrect when multiple transactions are - // merged together + // Write deletes last so updates for the same tag do not fail. Batches that + // recreate a deleted tag preserve mutation order in cppCommonMountItems + // instead, keeping updates and deletes attached to the correct native view. if (!cppDeleteMountItems.empty()) { writeMountItemPreamble( buffer, CppMountItem::Type::Delete, cppDeleteMountItems.size()); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/MountItemReordering.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/MountItemReordering.h new file mode 100644 index 00000000000..d3754f0570a --- /dev/null +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/MountItemReordering.h @@ -0,0 +1,45 @@ +/* + * 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. + */ + +#pragma once + +#include + +#include +#include +#include + +namespace facebook::react { +namespace { + +inline bool requiresStrictMountItemOrder(const ShadowViewMutationList &mutations) +{ + auto firstDelete = std::find_if(mutations.begin(), mutations.end(), [](const auto &mutation) { + return mutation.type == ShadowViewMutation::Delete; + }); + if (firstDelete == mutations.end() || std::none_of(std::next(firstDelete), mutations.end(), [](const auto &mutation) { + return mutation.type == ShadowViewMutation::Create; + })) { + return false; + } + + // Do not allocate a set for ordinary mount-only or unmount-only batches. + std::unordered_set deletedTags; + for (auto it = firstDelete; it != mutations.end(); ++it) { + const auto &mutation = *it; + if (mutation.type == ShadowViewMutation::Delete) { + deletedTags.insert(mutation.oldChildShadowView.tag); + } else if (mutation.type == ShadowViewMutation::Create && deletedTags.contains(mutation.newChildShadowView.tag)) { + // Moving deletes to the end would delete this tag's new native view. + return true; + } + } + return false; +} + +} // namespace +} // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp new file mode 100644 index 00000000000..78045b005ac --- /dev/null +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp @@ -0,0 +1,113 @@ +/* + * 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. + */ + +#include +#include + +#include + +namespace facebook::react { +namespace { + +ShadowView viewWithTag(Tag tag) { + ShadowView view; + view.tag = tag; + view.layoutMetrics.frame.size = {100, 100}; + return view; +} + +} // namespace + +TEST(MountItemReorderingTest, emptyAndUpdateOnlyBatchesCanBeReordered) { + EXPECT_FALSE(requiresStrictMountItemOrder({})); + auto view = viewWithTag(42); + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::UpdateMutation(view, view, 1)})); +} + +TEST(MountItemReorderingTest, distinctLifetimesCanBeReordered) { + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::DeleteMutation(viewWithTag(42)), + ShadowViewMutation::CreateMutation(viewWithTag(43))})); +} + +TEST(MountItemReorderingTest, deletionOnlyBatchesCanBeReordered) { + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::DeleteMutation(viewWithTag(42)), + ShadowViewMutation::DeleteMutation(viewWithTag(43))})); +} + +TEST(MountItemReorderingTest, creationBeforeFinalDeletionCanBeReordered) { + auto view = viewWithTag(42); + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::CreateMutation(view), + ShadowViewMutation::DeleteMutation(view)})); +} + +TEST(MountItemReorderingTest, reparentingWithoutDeletionCanBeReordered) { + auto view = viewWithTag(42); + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::RemoveMutation(1, view, 0), + ShadowViewMutation::InsertMutation(2, view, 0)})); +} + +TEST(MountItemReorderingTest, recreationRequiresOriginalOrder) { + auto view = viewWithTag(42); + EXPECT_TRUE(requiresStrictMountItemOrder( + {ShadowViewMutation::DeleteMutation(view), + ShadowViewMutation::CreateMutation(view)})); +} + +TEST(MountItemReorderingTest, virtualViewRecreationRequiresOriginalOrder) { + auto view = viewWithTag(42); + view.layoutMetrics = EmptyLayoutMetrics; + EXPECT_TRUE(requiresStrictMountItemOrder( + {ShadowViewMutation::DeleteMutation(view), + ShadowViewMutation::CreateMutation(view)})); +} + +TEST(MountItemReorderingTest, multipleLifetimesRequireOriginalOrder) { + auto view = viewWithTag(42); + EXPECT_TRUE(requiresStrictMountItemOrder( + {ShadowViewMutation::CreateMutation(view), + ShadowViewMutation::DeleteMutation(view), + ShadowViewMutation::CreateMutation(view), + ShadowViewMutation::DeleteMutation(view)})); +} + +TEST(MountItemReorderingTest, interleavedTagsDoNotHideRecreation) { + EXPECT_TRUE(requiresStrictMountItemOrder( + {ShadowViewMutation::DeleteMutation(viewWithTag(42)), + ShadowViewMutation::DeleteMutation(viewWithTag(43)), + ShadowViewMutation::CreateMutation(viewWithTag(44)), + ShadowViewMutation::CreateMutation(viewWithTag(42))})); +} + +TEST(MountItemReorderingTest, mergedTransactionsRetainLifetimeBoundaries) { + auto oldView = viewWithTag(42); + auto newView = viewWithTag(42); + newView.layoutMetrics.frame.size = {100, 100}; + MountingTransaction transaction{ + 1, + 1, + {ShadowViewMutation::UpdateMutation(oldView, oldView, 1), + ShadowViewMutation::RemoveMutation(1, oldView, 0), + ShadowViewMutation::DeleteMutation(oldView)}, + TransactionTelemetry{}}; + transaction.mergeWith( + MountingTransaction{ + 1, + 2, + {ShadowViewMutation::CreateMutation(newView), + ShadowViewMutation::InsertMutation(1, newView, 0), + ShadowViewMutation::UpdateMutation(newView, newView, 1)}, + TransactionTelemetry{}}); + + EXPECT_TRUE(requiresStrictMountItemOrder(transaction.getMutations())); +} + +} // namespace facebook::react From 83a4269d520a9709bcd09596f97f5493dcc55edf Mon Sep 17 00:00:00 2001 From: Sorin Cioban Date: Mon, 2 Nov 2026 16:45:19 +0000 Subject: [PATCH 2/2] test(android): cover skipped inserts after merged view recreation --- .../fabric/test/MountItemReorderingTest.cpp | 29 +++ ...aceMountingManagerMergedTransactionTest.kt | 219 ++++++++++++++++++ 2 files changed, 248 insertions(+) create mode 100644 packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerMergedTransactionTest.kt diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp index 78045b005ac..92c6b1cc786 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test/MountItemReorderingTest.cpp @@ -110,4 +110,33 @@ TEST(MountItemReorderingTest, mergedTransactionsRetainLifetimeBoundaries) { EXPECT_TRUE(requiresStrictMountItemOrder(transaction.getMutations())); } +TEST( + MountItemReorderingTest, + selectsStrictOrderForRecreationBeforeLaterReparenting) { + auto child = viewWithTag(480); + auto sibling = viewWithTag(1712); + MountingTransaction transaction{ + 1, + 1, + {ShadowViewMutation::RemoveMutation(10, child, 0), + ShadowViewMutation::DeleteMutation(child)}, + TransactionTelemetry{}}; + transaction.mergeWith( + MountingTransaction{ + 1, + 2, + {ShadowViewMutation::CreateMutation(child), + ShadowViewMutation::InsertMutation(10, child, 0)}, + TransactionTelemetry{}}); + + // A trailing delete would lose this child before a later batch moves it to + // index 0 and inserts its next sibling at index 1 in the destination parent. + EXPECT_TRUE(requiresStrictMountItemOrder(transaction.getMutations())); + EXPECT_FALSE(requiresStrictMountItemOrder( + {ShadowViewMutation::RemoveMutation(10, child, 0), + ShadowViewMutation::InsertMutation(482, child, 0), + ShadowViewMutation::CreateMutation(sibling), + ShadowViewMutation::InsertMutation(482, sibling, 1)})); +} + } // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerMergedTransactionTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerMergedTransactionTest.kt new file mode 100644 index 00000000000..a74b344c94c --- /dev/null +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerMergedTransactionTest.kt @@ -0,0 +1,219 @@ +/* + * 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.ReactSoftExceptionLogger +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.fabric.mounting.mountitems.IntBufferBatchMountItem +import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.Companion.INSTRUCTION_CREATE +import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.Companion.INSTRUCTION_DELETE +import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.Companion.INSTRUCTION_INSERT +import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem.Companion.INSTRUCTION_REMOVE +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.ReactViewGroup +import com.facebook.react.views.view.ReactViewManager +import com.facebook.testutils.shadows.ShadowSoLoader +import org.assertj.core.api.Assertions.assertThat +import org.junit.After +import org.junit.Assert.assertThrows +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Replays both native encoder orders against real Java mounting classes. The C++ order-selection + * regression is in MountItemReorderingTest; these tests do not execute the C++ encoder through JNI. + */ +@RunWith(RobolectricTestRunner::class) +@Config(shadows = [ShadowSoLoader::class]) +class SurfaceMountingManagerMergedTransactionTest { + private lateinit var mountingManager: MountingManager + private lateinit var surface: SurfaceMountingManager + private lateinit var viewManager: ReactViewManager + private val softExceptions = mutableListOf() + private val listener = + ReactSoftExceptionLogger.ReactSoftExceptionListener { _, cause -> softExceptions.add(cause) } + + @Before + fun setUp() { + ReactNativeFeatureFlagsForTests.setUp() + viewManager = ReactViewManager() + val context = ReactTestHelper.createTestReactApplicationContext() + val themedContext = ThemedReactContext(context, context, null, SURFACE) + mountingManager = + MountingManager( + ViewManagerRegistry(listOf>(viewManager)), + MountItemExecutor {}, + ) + mountingManager.startSurface(SURFACE, themedContext, ReactRootView(themedContext)) + surface = mountingManager.getSurfaceManagerEnforced(SURFACE, "test") + ReactSoftExceptionLogger.addListener(listener) + execute( + intArrayOf( + INSTRUCTION_CREATE, + SOURCE_PARENT, + 1, + INSTRUCTION_INSERT, + SOURCE_PARENT, + SURFACE, + 0, + INSTRUCTION_CREATE, + DESTINATION_PARENT, + 1, + INSTRUCTION_INSERT, + DESTINATION_PARENT, + SURFACE, + 1, + INSTRUCTION_CREATE, + RECREATED_CHILD, + 1, + INSTRUCTION_INSERT, + RECREATED_CHILD, + SOURCE_PARENT, + 0, + ), + createCount = 3, + ) + } + + @After + fun tearDown() { + ReactSoftExceptionLogger.removeListener(listener) + mountingManager.stopSurface(SURFACE) + } + + @Test + fun trailingDeleteMakesTheNextSiblingInsertOutOfBounds() { + assertTrailingDeleteFails(removeClippedSubviews = false) + } + + @Test + fun trailingDeleteMakesTheNextClippedSiblingInsertOutOfBounds() { + assertTrailingDeleteFails(removeClippedSubviews = true) + } + + @Test + fun mutationOrderPreservesTheReparentedChildAndNextSibling() { + assertMutationOrderSucceeds(removeClippedSubviews = false) + } + + @Test + fun mutationOrderPreservesTheReparentedChildAndNextClippedSibling() { + assertMutationOrderSucceeds(removeClippedSubviews = true) + } + + private fun assertTrailingDeleteFails(removeClippedSubviews: Boolean) { + destination().removeClippedSubviews = removeClippedSubviews + assertThat(destination().removeClippedSubviews).isEqualTo(removeClippedSubviews) + recreateChild(keepMutationOrder = false) + assertThat(surface.getViewExists(RECREATED_CHILD)).isFalse() + assertThat(viewManager.getChildCount(surface.getView(SOURCE_PARENT) as ReactViewGroup)) + .isEqualTo(1) + + val failure = assertThrows(IllegalStateException::class.java) { reparentAndInsertSibling() } + + assertThat(failure) + .hasMessageContaining("view [$NEXT_SIBLING] into parent [$DESTINATION_PARENT] at index 1") + assertThat(failure.cause).isInstanceOf(IndexOutOfBoundsException::class.java) + assertThat(viewManager.getChildCount(destination())).isZero() + assertThat(softExceptions.map { it.message }) + .contains("Unable to find viewState for tag: [$RECREATED_CHILD] for addViewAt") + } + + private fun assertMutationOrderSucceeds(removeClippedSubviews: Boolean) { + destination().removeClippedSubviews = removeClippedSubviews + assertThat(destination().removeClippedSubviews).isEqualTo(removeClippedSubviews) + recreateChild(keepMutationOrder = true) + assertThat(surface.getViewExists(RECREATED_CHILD)).isTrue() + + reparentAndInsertSibling() + + assertThat(viewManager.getChildCount(surface.getView(SOURCE_PARENT) as ReactViewGroup)).isZero() + assertThat(viewManager.getChildCount(destination())).isEqualTo(2) + assertThat(viewManager.getChildAt(destination(), 0)).isSameAs(surface.getView(RECREATED_CHILD)) + assertThat(viewManager.getChildAt(destination(), 1)).isSameAs(surface.getView(NEXT_SIBLING)) + assertThat(softExceptions).isEmpty() + } + + private fun recreateChild(keepMutationOrder: Boolean) { + val remove = intArrayOf(INSTRUCTION_REMOVE, RECREATED_CHILD, SOURCE_PARENT, 0) + val delete = intArrayOf(INSTRUCTION_DELETE, RECREATED_CHILD) + val createAndInsert = + intArrayOf( + INSTRUCTION_CREATE, + RECREATED_CHILD, + 1, + INSTRUCTION_INSERT, + RECREATED_CHILD, + SOURCE_PARENT, + 0, + ) + execute( + if (keepMutationOrder) remove + delete + createAndInsert + else remove + createAndInsert + delete, + createCount = 1, + ) + } + + private fun reparentAndInsertSibling() { + execute( + intArrayOf( + INSTRUCTION_REMOVE, + RECREATED_CHILD, + SOURCE_PARENT, + 0, + INSTRUCTION_INSERT, + RECREATED_CHILD, + DESTINATION_PARENT, + 0, + INSTRUCTION_CREATE, + NEXT_SIBLING, + 1, + INSTRUCTION_INSERT, + NEXT_SIBLING, + DESTINATION_PARENT, + 1, + ), + createCount = 1, + ) + } + + private fun destination(): ReactViewGroup = surface.getView(DESTINATION_PARENT) as ReactViewGroup + + private fun execute(instructions: IntArray, createCount: Int) { + val objects = mutableListOf() + repeat(createCount) { + objects.add("RCTView") + objects.add(JavaOnlyMap.of()) + objects.add(null) + objects.add(null) + } + IntBufferBatchMountItem(SURFACE, instructions, objects.toTypedArray(), 0) + .execute(mountingManager) + } + + private companion object { + const val SURFACE = 1 + const val SOURCE_PARENT = 10 + const val RECREATED_CHILD = 480 + const val DESTINATION_PARENT = 482 + const val NEXT_SIBLING = 1712 + } +}