Skip to content

Commit cf926a1

Browse files
sammy-SCfacebook-github-bot
authored andcommitted
decouple commit from mount on Android (facebook#44236)
Summary: Pull Request resolved: facebook#44236 ## Changelog: [Android] [Fixed] - fix a case when view preallocation or props forwarding on Android lead to dropped props update # What does this fix This fixes a bug where prop change is not delivered to Android mounting layer if the prop change was initiated from state update inside of `useLayoutEffect`, `componentDidMount` or `componentDidUpdate`. This affects android only and when batched rendering is enabled. There are two root causes of this problem: 1. View preallocation on Android: https://fburl.com/code/r62p3vot 2. Prop forwarding on Android: https://fburl.com/code/644f1ppk Minimal repro : ``` import React, {useLayoutEffect, useState} from 'react'; import {Button, SafeAreaView, View} from 'react-native'; function Foo() { const [bgColor, setBgColor] = React.useState('red'); useLayoutEffect(() => { console.log('useLayoutEffect'); setBgColor('blue'); }, []); return ( <View style={{ backgroundColor: bgColor, width: '100%', height: '100%', }} /> ); } function RNTesterApp() { const [show, setShow] = useState(false); return ( <SafeAreaView> <Button title="Toggle" onPress={() => setShow(!show)} /> {show && <Foo />} </SafeAreaView> ); } export default RNTesterApp; ``` # The underlaying problem The problem is combination of view preallocation and batched rendering updates. Here is a step by step what happens in the repro above: 1. React issues asks Fabric to create new shadow node A with background colour **red**. 2. Fabric asks Android to allocate a view for shadow node A with background colour **red**. 3. React commits tree **T1** and calls layout effects. Meanwhile Fabric waits, without trying to mount the tree **T1**, to prevent painting state that is about to be updated and prevent flickering. 4. React clones node A, changing the background colour to **blue** and commits the new tree **T2**. 5. Fabric, will now go ahead and mount the latest tree **T2**. While creating mount instructions, it will drop prop updates because it believes prop updates where delivered already as part of step 2. # The fix The fix is to change two things: 1. Ignore view preallocation for shadow nodes which were cloned with new props. 2. Set hasBeenMounted flag on ShadowNode later in the Fabric pipeline to fix it. Both of these are hidden behind a single feature flag: `fixMountedFlagAndFixPreallocationClone` ## Performance implication: I estimate that this will impact around 3% of views. Reviewed By: rubennorte Differential Revision: D56353589 fbshipit-source-id: 651d3cd2d0f78bfbbe9c05aa1ae1b1690c15e4ea
1 parent 132563d commit cf926a1

35 files changed

+348
-94
lines changed

packages/react-native/React/Fabric/RCTScheduler.mm

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ void schedulerDidRequestPreliminaryViewAllocation(SurfaceId surfaceId, const Sha
4343
// This delegate method is not currently used on iOS.
4444
}
4545

46+
void schedulerDidRequestUpdateToPreallocatedView(const ShadowNode &shadowNode) override
47+
{
48+
// Does nothing.
49+
// This delegate method is not currently used on iOS.
50+
}
51+
4652
void schedulerDidDispatchCommand(
4753
const ShadowView &shadowView,
4854
const std::string &commandName,

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<f3d26e8e345068bc7ef4e156be460e09>>
7+
* @generated SignedSource<<4adf12f2d6a102944b0fa922aa1c3e84>>
88
*/
99

1010
/**
@@ -94,6 +94,12 @@ public object ReactNativeFeatureFlags {
9494
@JvmStatic
9595
public fun enableUIConsistency(): Boolean = accessor.enableUIConsistency()
9696

97+
/**
98+
* Splits hasBeenMounted and promoted.
99+
*/
100+
@JvmStatic
101+
public fun fixMountedFlagAndFixPreallocationClone(): Boolean = accessor.fixMountedFlagAndFixPreallocationClone()
102+
97103
/**
98104
* Forces the mounting layer on Android to always batch mount items instead of dispatching them immediately. This might fix some crashes related to synchronous state updates, where some views dispatch state updates during mount.
99105
*/

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxAccessor.kt

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<40668dcd951123da7c0b4ddde23f94c9>>
7+
* @generated SignedSource<<24c24962f08ba7c52c296a5ac9abdbbc>>
88
*/
99

1010
/**
@@ -31,6 +31,7 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
3131
private var enableSpannableBuildingUnificationCache: Boolean? = null
3232
private var enableSynchronousStateUpdatesCache: Boolean? = null
3333
private var enableUIConsistencyCache: Boolean? = null
34+
private var fixMountedFlagAndFixPreallocationCloneCache: Boolean? = null
3435
private var forceBatchingMountItemsOnAndroidCache: Boolean? = null
3536
private var inspectorEnableCxxInspectorPackagerConnectionCache: Boolean? = null
3637
private var inspectorEnableModernCDPRegistryCache: Boolean? = null
@@ -138,6 +139,15 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
138139
return cached
139140
}
140141

142+
override fun fixMountedFlagAndFixPreallocationClone(): Boolean {
143+
var cached = fixMountedFlagAndFixPreallocationCloneCache
144+
if (cached == null) {
145+
cached = ReactNativeFeatureFlagsCxxInterop.fixMountedFlagAndFixPreallocationClone()
146+
fixMountedFlagAndFixPreallocationCloneCache = cached
147+
}
148+
return cached
149+
}
150+
141151
override fun forceBatchingMountItemsOnAndroid(): Boolean {
142152
var cached = forceBatchingMountItemsOnAndroidCache
143153
if (cached == null) {

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsCxxInterop.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<c3f02dff409c10aa7922141cef3a6990>>
7+
* @generated SignedSource<<0ceccc453595057ca96d9ae49c7f4637>>
88
*/
99

1010
/**
@@ -50,6 +50,8 @@ public object ReactNativeFeatureFlagsCxxInterop {
5050

5151
@DoNotStrip @JvmStatic public external fun enableUIConsistency(): Boolean
5252

53+
@DoNotStrip @JvmStatic public external fun fixMountedFlagAndFixPreallocationClone(): Boolean
54+
5355
@DoNotStrip @JvmStatic public external fun forceBatchingMountItemsOnAndroid(): Boolean
5456

5557
@DoNotStrip @JvmStatic public external fun inspectorEnableCxxInspectorPackagerConnection(): Boolean

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsDefaults.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<d12888990ebca7c6199f4b51802ee59b>>
7+
* @generated SignedSource<<29720cb2aa02ebdbe8e5efbe9e3a4b01>>
88
*/
99

1010
/**
@@ -45,6 +45,8 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi
4545

4646
override fun enableUIConsistency(): Boolean = false
4747

48+
override fun fixMountedFlagAndFixPreallocationClone(): Boolean = false
49+
4850
override fun forceBatchingMountItemsOnAndroid(): Boolean = false
4951

5052
override fun inspectorEnableCxxInspectorPackagerConnection(): Boolean = false

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsLocalAccessor.kt

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<c06b3b34cea24459f6ade0ec5665dae7>>
7+
* @generated SignedSource<<977c8d88557a37c750ec59e67c94878c>>
88
*/
99

1010
/**
@@ -35,6 +35,7 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
3535
private var enableSpannableBuildingUnificationCache: Boolean? = null
3636
private var enableSynchronousStateUpdatesCache: Boolean? = null
3737
private var enableUIConsistencyCache: Boolean? = null
38+
private var fixMountedFlagAndFixPreallocationCloneCache: Boolean? = null
3839
private var forceBatchingMountItemsOnAndroidCache: Boolean? = null
3940
private var inspectorEnableCxxInspectorPackagerConnectionCache: Boolean? = null
4041
private var inspectorEnableModernCDPRegistryCache: Boolean? = null
@@ -153,6 +154,16 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
153154
return cached
154155
}
155156

157+
override fun fixMountedFlagAndFixPreallocationClone(): Boolean {
158+
var cached = fixMountedFlagAndFixPreallocationCloneCache
159+
if (cached == null) {
160+
cached = currentProvider.fixMountedFlagAndFixPreallocationClone()
161+
accessedFeatureFlags.add("fixMountedFlagAndFixPreallocationClone")
162+
fixMountedFlagAndFixPreallocationCloneCache = cached
163+
}
164+
return cached
165+
}
166+
156167
override fun forceBatchingMountItemsOnAndroid(): Boolean {
157168
var cached = forceBatchingMountItemsOnAndroidCache
158169
if (cached == null) {

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlagsProvider.kt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
* This source code is licensed under the MIT license found in the
55
* LICENSE file in the root directory of this source tree.
66
*
7-
* @generated SignedSource<<dceb20e62a9ddbb98c872a24fab9765c>>
7+
* @generated SignedSource<<a76816db2d7d7d3f0463103387e2a049>>
88
*/
99

1010
/**
@@ -45,6 +45,8 @@ public interface ReactNativeFeatureFlagsProvider {
4545

4646
@DoNotStrip public fun enableUIConsistency(): Boolean
4747

48+
@DoNotStrip public fun fixMountedFlagAndFixPreallocationClone(): Boolean
49+
4850
@DoNotStrip public fun forceBatchingMountItemsOnAndroid(): Boolean
4951

5052
@DoNotStrip public fun inspectorEnableCxxInspectorPackagerConnection(): Boolean

packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,6 +523,17 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
523523
mountingManager->preallocateShadowView(surfaceId, ShadowView(shadowNode));
524524
}
525525

526+
void Binding::schedulerDidRequestUpdateToPreallocatedView(
527+
const ShadowNode& shadowNode) {
528+
auto mountingManager =
529+
getMountingManager("schedulerDidRequestUpdateToPreallocatedView");
530+
if (!mountingManager) {
531+
return;
532+
}
533+
534+
mountingManager->updatePreallocatedShadowNode(shadowNode);
535+
}
536+
526537
void Binding::schedulerDidDispatchCommand(
527538
const ShadowView& shadowView,
528539
const std::string& commandName,

packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,9 @@ class Binding : public jni::HybridClass<Binding, JBinding>,
109109
const SurfaceId surfaceId,
110110
const ShadowNode& shadowNode) override;
111111

112+
void schedulerDidRequestUpdateToPreallocatedView(
113+
const ShadowNode& shadowNode) override;
114+
112115
void schedulerDidDispatchCommand(
113116
const ShadowView& shadowView,
114117
const std::string& commandName,

packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include "MountItem.h"
1212
#include "StateWrapperImpl.h"
1313

14+
#include <react/featureflags/ReactNativeFeatureFlags.h>
1415
#include <react/jni/ReadableNativeMap.h>
1516
#include <react/renderer/components/scrollview/ScrollViewProps.h>
1617
#include <react/renderer/core/conversions.h>
@@ -826,6 +827,29 @@ void FabricMountingManager::preallocateShadowView(
826827
isLayoutableShadowNode);
827828
}
828829

830+
void FabricMountingManager::updatePreallocatedShadowNode(
831+
const ShadowNode& shadowNode) {
832+
if (ReactNativeFeatureFlags::fixMountedFlagAndFixPreallocationClone()) {
833+
// When batched rendering is enabled, React may do
834+
// multiple commits in a row but only the last one is mounted.
835+
// View preallocation does not account for this scenario and
836+
// a prop update may be dropped because view is marked as preallocated.
837+
// To work around this, we can detect when a view was cloned with different
838+
// props, and remove the view from `allocatedViewRegistry_`.
839+
std::lock_guard lock(allocatedViewsMutex_);
840+
auto allocatedViewsIterator =
841+
allocatedViewRegistry_.find(shadowNode.getSurfaceId());
842+
if (allocatedViewsIterator == allocatedViewRegistry_.end()) {
843+
// The surface does not exist, nothing to do.
844+
return;
845+
}
846+
auto& allocatedViews = allocatedViewsIterator->second;
847+
if (allocatedViews.find(shadowNode.getTag()) != allocatedViews.end()) {
848+
allocatedViews.erase(shadowNode.getTag());
849+
}
850+
}
851+
}
852+
829853
void FabricMountingManager::dispatchCommand(
830854
const ShadowView& shadowView,
831855
const std::string& commandName,

0 commit comments

Comments
 (0)