Skip to content

Commit 4d36e57

Browse files
jingjing2222meta-codesync[bot]
authored andcommitted
Fix Android Fabric Native Animated null prop crash (#56913)
Summary: This fixes an Android Fabric crash in the `overrideBySynchronousMountPropsAtMountingAndroid` path when Native Animated has stored a synchronous `transform` / `opacity` override and a later regular Fabric commit contains `null` for the same prop. | As-is | To-be | | --- | --- | | <img src="https://raw.githubusercontent.com/jingjing2222/react-native-fabric-transform-repro/main/crashed.gif" alt="Baseline Android Fabric transform crash repro" width="320" /> | <img src="https://raw.githubusercontent.com/jingjing2222/react-native-fabric-transform-repro/main/patched.gif" alt="Patched Android Fabric transform result" width="320" /> | | [crashed.gif](https://github.com/jingjing2222/react-native-fabric-transform-repro/blob/main/crashed.gif) | [patched.gif](https://github.com/jingjing2222/react-native-fabric-transform-repro/blob/main/patched.gif) | Native Animated can synchronously update `transform` / `opacity` on the UI thread. A regular Fabric commit for the same view may arrive later with stale props, including `transform: null` or `opacity: null`. Those regular Fabric null values should not clear the stored synchronous override, because the override may still represent the fresher Native Animated value. Before this patch, the override merge path assumed that the incoming Fabric prop had the same non-null type as the stored override. For example, a stored `transform` override is an array, but a regular Fabric update may contain `transform: null`. That mismatch could crash in `SurfaceMountingManager.overridePropsReadableMap`. This patch: - Allows regular Fabric `null` updates to be overridden by stored synchronous `transform` / `opacity` values instead of crashing. - Keeps regular Fabric `transform: null` / `opacity: null` from clearing the stored synchronous override. - Scopes synchronous null clearing to `transform` / `opacity` only. - Leaves null values for other props untouched, since other view prop nulls may be meaningful. This PR intentionally does not change Java Native Animated `restoreDefaultValues()` behavior. That restore implementation can be handled separately because changing it may affect existing apps. Repro repository: https://github.com/jingjing2222/react-native-fabric-transform-repro ## Why this is correct A regular Fabric `null` update is not enough to prove user intent to clear the native animated value, because it can be the stale commit produced after Native Animated already wrote a fresher value on the UI thread. In that case, the stored synchronous override should continue to win. If the synchronous Native Animated path explicitly sends `transform: null` or `opacity: null`, this patch treats that as a clear signal for that stored override key. The clear behavior is intentionally scoped to the props currently stored by this override path. ```text regular Fabric null: possible stale commit -> stored synchronous override wins synchronous transform / opacity null: explicit clear -> stored override key is removed other null props: left untouched ``` ## Changelog: [ANDROID] [FIXED] - Fix Android Fabric Native Animated crash when regular Fabric null props collide with stored synchronous transform or opacity overrides. Pull Request resolved: #56913 Test Plan: Added regression coverage for stale regular Fabric null commits and scoped synchronous null clears: - `updateProps_withNullOpacity_keepsStoredSynchronousProp` - `updateProps_withNullTransform_keepsStoredSynchronousProp` - `updatePropsSynchronously_withNullOpacity_removesStoredSynchronousProp` - `updatePropsSynchronously_withNullTransform_removesStoredSynchronousProp` Ran: ```sh ./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests com.facebook.react.fabric.SurfaceMountingManagerSynchronousMountPropsTest -Preact.internal.useHermesStable=true ``` Result: ```text BUILD SUCCESSFUL ``` Ran: ```sh ./gradlew :packages:react-native:ReactAndroid:testDebugUnitTest --tests com.facebook.react.animated.NativeAnimatedNodeTraversalTest -Preact.internal.useHermesStable=true ``` Result: ```text BUILD SUCCESSFUL ``` Ran: ```sh ./gradlew :packages:react-native:ReactAndroid:ktfmtCheck -Preact.internal.useHermesStable=true ``` Result: ```text BUILD SUCCESSFUL ``` Reviewed By: javache Differential Revision: D105974289 Pulled By: zeyap fbshipit-source-id: 7688cfa6b89f2d107c7ae58356864835d41f6add
1 parent 54b9af1 commit 4d36e57

2 files changed

Lines changed: 135 additions & 12 deletions

File tree

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/SurfaceMountingManager.kt

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -625,14 +625,15 @@ internal constructor(
625625

626626
public fun storeSynchronousMountPropsOverride(reactTag: Int, props: ReadableMap): Unit {
627627
if (ReactNativeFeatureFlags.overrideBySynchronousMountPropsAtMountingAndroid()) {
628-
val propsMap = getMapFromPropsReadableMap(props)
629-
var synchronousMountProps = tagToSynchronousMountProps[reactTag]
630-
if (synchronousMountProps != null) {
631-
synchronousMountProps.putAll(propsMap)
628+
val propsMap = getAnimatedPropsMap(props)
629+
val synchronousMountProps = tagToSynchronousMountProps[reactTag] ?: mutableMapOf()
630+
clearAnimatedNullPropsMapKeys(props, synchronousMountProps)
631+
synchronousMountProps.putAll(propsMap)
632+
if (synchronousMountProps.isEmpty()) {
633+
tagToSynchronousMountProps.remove(reactTag)
632634
} else {
633-
synchronousMountProps = propsMap
635+
tagToSynchronousMountProps[reactTag] = synchronousMountProps
634636
}
635-
tagToSynchronousMountProps[reactTag] = synchronousMountProps
636637
}
637638
}
638639

@@ -1332,7 +1333,11 @@ internal constructor(
13321333
for ((propKey, propValue) in patchMap) {
13331334
if (outputReadableMap.hasKey(propKey)) {
13341335
if (propKey == PROP_TRANSFORM) {
1335-
assert(outputReadableMap.getType(propKey) == ReadableType.Array && propValue is List<*>)
1336+
val outputType = outputReadableMap.getType(propKey)
1337+
assert(
1338+
(outputType == ReadableType.Array || outputType == ReadableType.Null) &&
1339+
propValue is List<*>
1340+
)
13361341
val array = WritableNativeArray()
13371342
for (item in propValue as List<*>) {
13381343
if (item is Map<*, *>) {
@@ -1350,14 +1355,18 @@ internal constructor(
13501355
}
13511356
outputReadableMap.putArray(propKey, array)
13521357
} else if (propKey == PROP_OPACITY) {
1353-
assert(outputReadableMap.getType(propKey) == ReadableType.Number && propValue is Number)
1358+
val outputType = outputReadableMap.getType(propKey)
1359+
assert(
1360+
(outputType == ReadableType.Number || outputType == ReadableType.Null) &&
1361+
propValue is Number
1362+
)
13541363
outputReadableMap.putDouble(propKey, (propValue as Number).toDouble())
13551364
}
13561365
}
13571366
}
13581367
}
13591368

1360-
private fun getMapFromPropsReadableMap(readableMap: ReadableMap): MutableMap<String, Any> {
1369+
private fun getAnimatedPropsMap(readableMap: ReadableMap): MutableMap<String, Any> {
13611370
val outputMap = mutableMapOf<String, Any>()
13621371

13631372
if (
@@ -1387,6 +1396,25 @@ internal constructor(
13871396
return outputMap
13881397
}
13891398

1399+
private fun clearAnimatedNullPropsMapKeys(
1400+
readableMap: ReadableMap,
1401+
outputMap: MutableMap<String, Any>,
1402+
) {
1403+
// Native Animated uses synchronous null updates to restore animated-managed props.
1404+
// Keep this scoped to props stored by this override path today.
1405+
if (
1406+
readableMap.hasKey(PROP_TRANSFORM) &&
1407+
readableMap.getType(PROP_TRANSFORM) == ReadableType.Null
1408+
) {
1409+
outputMap.remove(PROP_TRANSFORM)
1410+
}
1411+
if (
1412+
readableMap.hasKey(PROP_OPACITY) && readableMap.getType(PROP_OPACITY) == ReadableType.Null
1413+
) {
1414+
outputMap.remove(PROP_OPACITY)
1415+
}
1416+
}
1417+
13901418
// prevents unchecked conversion warn of the <ViewGroup> type
13911419
private fun getViewGroupManager(viewState: ViewState): IViewGroupManager<View> {
13921420
val viewManager =

packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/SurfaceMountingManagerSynchronousMountPropsTest.kt

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,17 @@
1010
package com.facebook.react.fabric
1111

1212
import com.facebook.react.ReactRootView
13+
import com.facebook.react.bridge.JavaOnlyArray
1314
import com.facebook.react.bridge.JavaOnlyMap
1415
import com.facebook.react.bridge.ReactTestHelper
16+
import com.facebook.react.bridge.ReadableArray
1517
import com.facebook.react.fabric.mounting.MountingManager
16-
import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor
1718
import com.facebook.react.fabric.mounting.SurfaceMountingManager
1819
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests
1920
import com.facebook.react.uimanager.ThemedReactContext
2021
import com.facebook.react.uimanager.ViewManager
2122
import com.facebook.react.uimanager.ViewManagerRegistry
23+
import com.facebook.react.views.view.ReactViewGroup
2224
import com.facebook.react.views.view.ReactViewManager
2325
import com.facebook.testutils.shadows.ShadowNativeLoader
2426
import com.facebook.testutils.shadows.ShadowNativeMap
@@ -67,8 +69,8 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
6769
themedReactContext = ThemedReactContext(reactContext, reactContext, null, -1)
6870
mountingManager =
6971
MountingManager(
70-
ViewManagerRegistry(listOf<ViewManager<*, *>>(ReactViewManager())),
71-
MountItemExecutor {},
72+
ViewManagerRegistry(listOf<ViewManager<*, *>>(TestReactViewManager())),
73+
{},
7274
)
7375
}
7476

@@ -83,6 +85,25 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
8385
smm.addViewAt(surfaceId, tag, 0)
8486
}
8587

88+
private class TestReactViewManager : ReactViewManager() {
89+
override fun setTransformProperty(
90+
view: ReactViewGroup,
91+
transforms: ReadableArray?,
92+
transformOrigin: ReadableArray?,
93+
) {
94+
view.translationY = 0.0f
95+
if (transforms == null) {
96+
return
97+
}
98+
for (i in 0..<transforms.size()) {
99+
val transform = transforms.getMap(i) ?: continue
100+
if (transform.hasKey("translateY")) {
101+
view.translationY = transform.getDouble("translateY").toFloat()
102+
}
103+
}
104+
}
105+
}
106+
86107
/** Stored synchronous opacity should override a stale Fabric mount update. */
87108
@Test
88109
fun storeSynchronousProps_overridesStaleOpacityInUpdateProps() {
@@ -137,6 +158,80 @@ class SurfaceMountingManagerSynchronousMountPropsTest {
137158
assertThat(smm.getView(tag).alpha).isEqualTo(0.2f)
138159
}
139160

161+
/** A null Fabric prop update should not clear the stored synchronous opacity override. */
162+
@Test
163+
fun updateProps_withNullOpacity_keepsStoredSynchronousProp() {
164+
val smm = startSurface()
165+
val tag = 42
166+
createAndAttachView(smm, tag)
167+
168+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", 0.3))
169+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", 0.3))
170+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
171+
172+
smm.updateProps(tag, JavaOnlyMap.of("opacity", null))
173+
174+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
175+
}
176+
177+
/** A synchronous null update should clear the stored synchronous opacity override. */
178+
@Test
179+
fun updatePropsSynchronously_withNullOpacity_removesStoredSynchronousProp() {
180+
val smm = startSurface()
181+
val tag = 42
182+
createAndAttachView(smm, tag)
183+
184+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", 0.3))
185+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", 0.3))
186+
assertThat(smm.getView(tag).alpha).isEqualTo(0.3f)
187+
188+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("opacity", null))
189+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("opacity", null))
190+
assertThat(smm.getView(tag).alpha).isEqualTo(1.0f)
191+
192+
smm.updateProps(tag, JavaOnlyMap.of("opacity", null))
193+
194+
assertThat(smm.getView(tag).alpha).isEqualTo(1.0f)
195+
}
196+
197+
/** A null Fabric prop update should not clear the stored synchronous transform override. */
198+
@Test
199+
fun updateProps_withNullTransform_keepsStoredSynchronousProp() {
200+
val smm = startSurface()
201+
val tag = 42
202+
val transform = JavaOnlyArray.of(JavaOnlyMap.of("translateY", 40.0))
203+
createAndAttachView(smm, tag)
204+
205+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", transform))
206+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", transform))
207+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
208+
209+
smm.updateProps(tag, JavaOnlyMap.of("transform", null))
210+
211+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
212+
}
213+
214+
/** A synchronous null update should clear the stored synchronous transform override. */
215+
@Test
216+
fun updatePropsSynchronously_withNullTransform_removesStoredSynchronousProp() {
217+
val smm = startSurface()
218+
val tag = 42
219+
val transform = JavaOnlyArray.of(JavaOnlyMap.of("translateY", 40.0))
220+
createAndAttachView(smm, tag)
221+
222+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", transform))
223+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", transform))
224+
assertThat(smm.getView(tag).translationY).isEqualTo(40.0f)
225+
226+
smm.storeSynchronousMountPropsOverride(tag, JavaOnlyMap.of("transform", null))
227+
smm.updatePropsSynchronously(tag, JavaOnlyMap.of("transform", null))
228+
assertThat(smm.getView(tag).translationY).isEqualTo(0.0f)
229+
230+
smm.updateProps(tag, JavaOnlyMap.of("transform", null))
231+
232+
assertThat(smm.getView(tag).translationY).isEqualTo(0.0f)
233+
}
234+
140235
/**
141236
* When a view is deleted, stored synchronous props should be cleaned up. A recreated view with
142237
* the same tag should not be affected by the old stored props.

0 commit comments

Comments
 (0)