Skip to content

Commit

Permalink
Add featureflag to not re-order mount items in FabricMountingManager
Browse files Browse the repository at this point in the history
Summary:
In facebook#44188, we've started combining multiple transactions in a single transaction, to meet React's atomicity requirements, while also dealing with the constraints of Android's Fabric implementation.

This revealed a bug where in some scenarios (especially when using transitions), a node may be deleted and created during the same transaction. The current implementation of FabricMountingManager assumes it can safely reorder some operations, which it does to optimize the size of IntBufferBatch mount items. This is however incorrect and unsafe when multiple transactions are merged.

**Example:**

Differentiator output:

```
# Transaction 1
Remove facebook#100 from facebook#11
Delete facebook#100

# Transaction 2
Create facebook#100
Insert facebook#100 into facebook#11
```
FabricMountingManager output
```
Remove facebook#100 from facebook#11
Insert facebook#100 into facebook#11
Delete facebook#100
```

Note that the create action is also skipped, because we only update `allocatedViewTags` after processing all mutations, leading FabricMountingManager to assume creation is not required.

This leads to an invalid state in SurfaceMountingManager, which will be surfaced as a crash in `getViewState` on the next mutation that interacts with these views.

Changelog: [Android][Fixed] Fix crash in getViewState when using suspense fallbacks.

Differential Revision: D63148523
  • Loading branch information
javache authored and facebook-github-bot committed Sep 27, 2024
1 parent 04ebbe0 commit 6e3d15f
Show file tree
Hide file tree
Showing 20 changed files with 263 additions and 122 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<bce0548a9e58299746c75aad95a25275>>
* @generated SignedSource<<b4d0cfe00218add9f769183c4f741261>>
*/

/**
Expand Down Expand Up @@ -58,6 +58,12 @@ public object ReactNativeFeatureFlags {
@JvmStatic
public fun destroyFabricSurfacesInReactInstanceManager(): Boolean = accessor.destroyFabricSurfacesInReactInstanceManager()

/**
* Prevent FabricMountingManager from reordering mountitems, which may lead to invalid state on the UI thread
*/
@JvmStatic
public fun disableMountItemReorderingAndroid(): Boolean = accessor.disableMountItemReorderingAndroid()

/**
* Kill-switch to turn off support for aling-items:baseline on Fabric iOS.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<19dca512d93d689e927ee5988a43e646>>
* @generated SignedSource<<457a8f0213f00a9e074b8b735edbd421>>
*/

/**
Expand All @@ -25,6 +25,7 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
private var batchRenderingUpdatesInEventLoopCache: Boolean? = null
private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
private var destroyFabricSurfacesInReactInstanceManagerCache: Boolean? = null
private var disableMountItemReorderingAndroidCache: Boolean? = null
private var enableAlignItemsBaselineOnFabricIOSCache: Boolean? = null
private var enableAndroidLineHeightCenteringCache: Boolean? = null
private var enableAndroidMixBlendModePropCache: Boolean? = null
Expand Down Expand Up @@ -120,6 +121,15 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
return cached
}

override fun disableMountItemReorderingAndroid(): Boolean {
var cached = disableMountItemReorderingAndroidCache
if (cached == null) {
cached = ReactNativeFeatureFlagsCxxInterop.disableMountItemReorderingAndroid()
disableMountItemReorderingAndroidCache = cached
}
return cached
}

override fun enableAlignItemsBaselineOnFabricIOS(): Boolean {
var cached = enableAlignItemsBaselineOnFabricIOSCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<76eebf045692e945d39a4ea27a63ae02>>
* @generated SignedSource<<aa67b584ff2469b2c7addaacbaecc1bb>>
*/

/**
Expand Down Expand Up @@ -38,6 +38,8 @@ public object ReactNativeFeatureFlagsCxxInterop {

@DoNotStrip @JvmStatic public external fun destroyFabricSurfacesInReactInstanceManager(): Boolean

@DoNotStrip @JvmStatic public external fun disableMountItemReorderingAndroid(): Boolean

@DoNotStrip @JvmStatic public external fun enableAlignItemsBaselineOnFabricIOS(): Boolean

@DoNotStrip @JvmStatic public external fun enableAndroidLineHeightCentering(): Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<8155a9c1309145fefdb19feb33c241db>>
* @generated SignedSource<<c12808c9b5a206d2671fb42b296bd469>>
*/

/**
Expand Down Expand Up @@ -33,6 +33,8 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi

override fun destroyFabricSurfacesInReactInstanceManager(): Boolean = false

override fun disableMountItemReorderingAndroid(): Boolean = false

override fun enableAlignItemsBaselineOnFabricIOS(): Boolean = true

override fun enableAndroidLineHeightCentering(): Boolean = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<5e4e474b62996caec15bbf8af9622a0a>>
* @generated SignedSource<<3f5ada38e8e379f0a74ecbd9f7fe58c6>>
*/

/**
Expand All @@ -29,6 +29,7 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
private var batchRenderingUpdatesInEventLoopCache: Boolean? = null
private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
private var destroyFabricSurfacesInReactInstanceManagerCache: Boolean? = null
private var disableMountItemReorderingAndroidCache: Boolean? = null
private var enableAlignItemsBaselineOnFabricIOSCache: Boolean? = null
private var enableAndroidLineHeightCenteringCache: Boolean? = null
private var enableAndroidMixBlendModePropCache: Boolean? = null
Expand Down Expand Up @@ -129,6 +130,16 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
return cached
}

override fun disableMountItemReorderingAndroid(): Boolean {
var cached = disableMountItemReorderingAndroidCache
if (cached == null) {
cached = currentProvider.disableMountItemReorderingAndroid()
accessedFeatureFlags.add("disableMountItemReorderingAndroid")
disableMountItemReorderingAndroidCache = cached
}
return cached
}

override fun enableAlignItemsBaselineOnFabricIOS(): Boolean {
var cached = enableAlignItemsBaselineOnFabricIOSCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<97eddbbd75ff7cfd0f1c905d72e9eafd>>
* @generated SignedSource<<a7beea440e1bedc035b71eff1903292f>>
*/

/**
Expand Down Expand Up @@ -33,6 +33,8 @@ public interface ReactNativeFeatureFlagsProvider {

@DoNotStrip public fun destroyFabricSurfacesInReactInstanceManager(): Boolean

@DoNotStrip public fun disableMountItemReorderingAndroid(): Boolean

@DoNotStrip public fun enableAlignItemsBaselineOnFabricIOS(): Boolean

@DoNotStrip public fun enableAndroidLineHeightCentering(): Boolean
Expand Down
Loading

0 comments on commit 6e3d15f

Please sign in to comment.