From 2cd3c424ea9637cb15858db4353b5a71e9b505c6 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Mon, 12 May 2025 17:39:20 +0100 Subject: [PATCH] Add eager alternate.stateNode cleanup (#33161) This is a fix for a problem where React retains shadow nodes longer than it needs to. The behaviour is shown in React Native test: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169 When React commits a new shadow tree, old shadow nodes are stored inside `fiber.alternate.stateNode`. This is not cleared up until React clones the node again. This may be problematic if mutation deletes a subtree, in that case `fiber.alternate.stateNode` will retain entire subtree until next update. In case of image nodes, this means retaining entire images. So when React goes from revision A: `` to revision B: ``, `fiber.alternate.stateNode` will be pointing to Shadow Node that represents revision A.. ![image](https://github.com/user-attachments/assets/076b677e-d152-4763-8c9d-4f923212b424) To fix this, this PR adds a new feature flag `enableEagerAlternateStateNodeCleanup`. When enabled, `alternate.stateNode` is proactively pointed towards finishedWork's stateNode, releasing resources sooner. I have verified this fixes the issue [demonstrated by React Native tests](https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169). All existing React tests pass when the flag is enabled. --- .../react-reconciler/src/ReactFiberCommitWork.js | 15 +++++++++++++++ packages/shared/ReactFeatureFlags.js | 2 ++ .../forks/ReactFeatureFlags.native-fb-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.native-fb.js | 1 + .../shared/forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native-fb.js | 1 + .../forks/ReactFeatureFlags.test-renderer.www.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 2 ++ 9 files changed, 25 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index e73b3fa8fb..8a4530c630 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -57,6 +57,7 @@ import { enableComponentPerformanceTrack, enableViewTransition, enableFragmentRefs, + enableEagerAlternateStateNodeCleanup, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -1947,6 +1948,20 @@ function commitMutationEffectsOnFiber( } } } + } else { + if (enableEagerAlternateStateNodeCleanup) { + if (supportsPersistence) { + if (finishedWork.alternate !== null) { + // `finishedWork.alternate.stateNode` is pointing to a stale shadow + // node at this point, retaining it and its subtree. To reclaim + // memory, point `alternate.stateNode` to new shadow node. This + // prevents shadow node from staying in memory longer than it + // needs to. The correct behaviour of this is checked by test in + // React Native: ShadowNodeReferenceCounter-itest.js#L150 + finishedWork.alternate.stateNode = finishedWork.stateNode; + } + } + } } break; } diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index a3bb0f9a5d..c2bcb31b46 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -135,6 +135,8 @@ export const enableShallowPropDiffing = false; export const enableSiblingPrerendering = true; +export const enableEagerAlternateStateNodeCleanup = true; + /** * Enables an expiration time for retry lanes to avoid starvation. */ diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index 089693d679..923190f950 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -22,6 +22,7 @@ export const enableObjectFiber = __VARIANT__; export const enableHiddenSubtreeInsertionEffectCleanup = __VARIANT__; export const enablePersistedModeClonedFlag = __VARIANT__; export const enableShallowPropDiffing = __VARIANT__; +export const enableEagerAlternateStateNodeCleanup = __VARIANT__; export const passChildrenWhenCloningPersistedNodes = __VARIANT__; export const enableSiblingPrerendering = __VARIANT__; export const enableUseEffectCRUDOverload = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index a77ef6a38a..17fe4ae98f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -25,6 +25,7 @@ export const { enablePersistedModeClonedFlag, enableShallowPropDiffing, enableUseEffectCRUDOverload, + enableEagerAlternateStateNodeCleanup, passChildrenWhenCloningPersistedNodes, enableSiblingPrerendering, enableFastAddPropertiesInDiffing, diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index c1dea3b37f..2187acea6e 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -49,6 +49,7 @@ export const enableSchedulingProfiler = __PROFILE__; export const enableComponentPerformanceTrack = false; export const enableScopeAPI = false; export const enableShallowPropDiffing = false; +export const enableEagerAlternateStateNodeCleanup = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = false; export const enableTaint = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index ea8c78abb5..f39e18d95f 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -65,6 +65,7 @@ export const enableShallowPropDiffing = false; export const enableSiblingPrerendering = true; export const enableUseEffectCRUDOverload = false; +export const enableEagerAlternateStateNodeCleanup = false; export const enableYieldingBeforePassive = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index 8bcdb5e82e..e0f5962a0d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -47,6 +47,7 @@ export const enableSchedulingProfiler = __PROFILE__; export const enableComponentPerformanceTrack = false; export const enableScopeAPI = false; export const enableShallowPropDiffing = false; +export const enableEagerAlternateStateNodeCleanup = false; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseCallback = false; export const enableTaint = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index f45d6a6f66..3e1aa94e03 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -74,6 +74,7 @@ export const enableShallowPropDiffing = false; export const enableSiblingPrerendering = true; export const enableUseEffectCRUDOverload = false; +export const enableEagerAlternateStateNodeCleanup = false; export const enableHydrationLaneScheduling = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 27bf13230e..9cc46b11f7 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -110,6 +110,8 @@ export const disableLegacyMode = true; export const enableShallowPropDiffing = false; +export const enableEagerAlternateStateNodeCleanup = false; + export const enableLazyPublicInstanceInFabric = false; export const enableSwipeTransition = false;