Skip to content

Commit

Permalink
Add featureflag to not re-order mount items in FabricMountingManager (f…
Browse files Browse the repository at this point in the history
…acebook#46702)

Summary:
Pull Request resolved: facebook#46702

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 microsoft#100 from microsoft#11
Delete microsoft#100

# Transaction 2
Create microsoft#100
Insert microsoft#100 into microsoft#11
```
FabricMountingManager output
```
Remove microsoft#100 from microsoft#11
Insert microsoft#100 into microsoft#11
Delete microsoft#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.

Reviewed By: sammy-SC

Differential Revision: D63148523

fbshipit-source-id: 07ae26b2f7b7eba1b9784041dd3059b0956c035e
  • Loading branch information
javache authored and facebook-github-bot committed Oct 28, 2024
1 parent dd43279 commit bd133b5
Show file tree
Hide file tree
Showing 20 changed files with 257 additions and 113 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<<f98a6ddd268fb59b5ee5edb4997b3a63>>
* @generated SignedSource<<575eeb1e291c1a372eba7aabcdd948e3>>
*/

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

/**
* 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<<6b3d3512d88c836dd809204cad636211>>
* @generated SignedSource<<f93759a639dbbb95d3307003e4907c86>>
*/

/**
Expand All @@ -24,6 +24,7 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
private var disableEventLoopOnBridgelessCache: Boolean? = null
private var disableMountItemReorderingAndroidCache: Boolean? = null
private var enableAlignItemsBaselineOnFabricIOSCache: Boolean? = null
private var enableAndroidLineHeightCenteringCache: Boolean? = null
private var enableBridgelessArchitectureCache: Boolean? = null
Expand Down Expand Up @@ -104,6 +105,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<<0ef9a66ccabeb0357f4b15c3e897f8fb>>
* @generated SignedSource<<44d0fe9a36e5e51816e10b8799d451fe>>
*/

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

@DoNotStrip @JvmStatic public external fun disableEventLoopOnBridgeless(): 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<<ee5400241a72652a2f8ff343a06e4e8c>>
* @generated SignedSource<<917a6effbfd0a476cc05d90abee3c80b>>
*/

/**
Expand All @@ -31,6 +31,8 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi

override fun disableEventLoopOnBridgeless(): 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<<949f5cdf6d0a4015fbd680ba718dce6d>>
* @generated SignedSource<<2ad36465b1a411cb55d85416bd8ba823>>
*/

/**
Expand All @@ -28,6 +28,7 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
private var completeReactInstanceCreationOnBgThreadOnAndroidCache: Boolean? = null
private var disableEventLoopOnBridgelessCache: Boolean? = null
private var disableMountItemReorderingAndroidCache: Boolean? = null
private var enableAlignItemsBaselineOnFabricIOSCache: Boolean? = null
private var enableAndroidLineHeightCenteringCache: Boolean? = null
private var enableBridgelessArchitectureCache: Boolean? = null
Expand Down Expand Up @@ -112,6 +113,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<<4e42e76c98b7434273e4f11212f0527b>>
* @generated SignedSource<<9770a9f125b8bcb4b1daef9e3458433f>>
*/

/**
Expand All @@ -31,6 +31,8 @@ public interface ReactNativeFeatureFlagsProvider {

@DoNotStrip public fun disableEventLoopOnBridgeless(): Boolean

@DoNotStrip public fun disableMountItemReorderingAndroid(): Boolean

@DoNotStrip public fun enableAlignItemsBaselineOnFabricIOS(): Boolean

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

0 comments on commit bd133b5

Please sign in to comment.