{"commit":{"authoredAt":"2024-02-09T11:14:37-05:00","shortRevision":"d8c1fa6b0b","revision":"d8c1fa6b0b8da0512cb5acab9cd4f242451392f3","authorName":"Jan Kassens","subject":"Add infinite update loop detection (#28279)","authorEmail":"jkassens@meta.com"},"files":[{"status":"M","path":"packages\/react-dom\/src\/__tests__\/ReactUpdates-test.js"},{"status":"M","path":"packages\/react-reconciler\/src\/ReactFiberWorkLoop.js"},{"status":"M","path":"packages\/shared\/ReactFeatureFlags.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.native-fb.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.native-oss.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.test-renderer.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.test-renderer.native.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.test-renderer.www.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.www-dynamic.js"},{"status":"M","path":"packages\/shared\/forks\/ReactFeatureFlags.www.js"}],"body":"This is a partial redo of https:\/\/github.com\/facebook\/react\/pull\/26625.\r\nSince that was unlanded due to some detected breakages. This now\r\nincludes a feature flag to be careful in rolling this out.","parents":["03d6f7cf007cb3b2a3a65e6ec29be02aafd87870"],"patch":"diff --git a\/packages\/react-dom\/src\/__tests__\/ReactUpdates-test.js b\/packages\/react-dom\/src\/__tests__\/ReactUpdates-test.js\nindex cd37619374..cf6d0ccb9e 100644\n--- a\/packages\/react-dom\/src\/__tests__\/ReactUpdates-test.js\n+++ b\/packages\/react-dom\/src\/__tests__\/ReactUpdates-test.js\n@@ -1709,6 +1709,70 @@ describe('ReactUpdates', () => {\n expect(subscribers.length).toBe(limit);\n });\n \n+ it(\"does not infinite loop if there's a synchronous render phase update on another component\", () => {\n+ if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) {\n+ return;\n+ }\n+ let setState;\n+ function App() {\n+ const [, _setState] = React.useState(0);\n+ setState = _setState;\n+ return ;\n+ }\n+\n+ function Child(step) {\n+ \/\/ This will cause an infinite update loop, and a warning in dev.\n+ setState(n => n + 1);\n+ return null;\n+ }\n+\n+ const container = document.createElement('div');\n+ const root = ReactDOMClient.createRoot(container);\n+\n+ expect(() => {\n+ expect(() => ReactDOM.flushSync(() => root.render())).toThrow(\n+ 'Maximum update depth exceeded',\n+ );\n+ }).toErrorDev(\n+ 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',\n+ );\n+ });\n+\n+ it(\"does not infinite loop if there's an async render phase update on another component\", async () => {\n+ if (gate(flags => !flags.enableInfiniteRenderLoopDetection)) {\n+ return;\n+ }\n+ let setState;\n+ function App() {\n+ const [, _setState] = React.useState(0);\n+ setState = _setState;\n+ return ;\n+ }\n+\n+ function Child(step) {\n+ \/\/ This will cause an infinite update loop, and a warning in dev.\n+ setState(n => n + 1);\n+ return null;\n+ }\n+\n+ const container = document.createElement('div');\n+ const root = ReactDOMClient.createRoot(container);\n+\n+ await expect(async () => {\n+ let error;\n+ try {\n+ await act(() => {\n+ React.startTransition(() => root.render());\n+ });\n+ } catch (e) {\n+ error = e;\n+ }\n+ expect(error.message).toMatch('Maximum update depth exceeded');\n+ }).toErrorDev(\n+ 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',\n+ );\n+ });\n+\n \/\/ TODO: Replace this branch with @gate pragmas\n if (__DEV__) {\n it('warns about a deferred infinite update loop with useEffect', async () => {\ndiff --git a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.js b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.js\nindex 597d008994..5fafd8de5c 100644\n--- a\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.js\n+++ b\/packages\/react-reconciler\/src\/ReactFiberWorkLoop.js\n@@ -40,6 +40,7 @@ import {\n useModernStrictMode,\n disableLegacyContext,\n alwaysThrottleRetries,\n+ enableInfiniteRenderLoopDetection,\n } from 'shared\/ReactFeatureFlags';\n import ReactSharedInternals from 'shared\/ReactSharedInternals';\n import is from 'shared\/objectIs';\n@@ -147,10 +148,10 @@ import {\n getNextLanes,\n getEntangledLanes,\n getLanesToRetrySynchronouslyOnError,\n- markRootUpdated,\n- markRootSuspended as markRootSuspended_dontCallThisOneDirectly,\n- markRootPinged,\n upgradePendingLanesToSync,\n+ markRootSuspended as _markRootSuspended,\n+ markRootUpdated as _markRootUpdated,\n+ markRootPinged as _markRootPinged,\n markRootFinished,\n addFiberToLanesMap,\n movePendingFibersToMemoized,\n@@ -381,6 +382,13 @@ let workInProgressRootConcurrentErrors: Array> | null =\n let workInProgressRootRecoverableErrors: Array> | null =\n null;\n \n+\/\/ Tracks when an update occurs during the render phase.\n+let workInProgressRootDidIncludeRecursiveRenderUpdate: boolean = false;\n+\/\/ Thacks when an update occurs during the commit phase. It's a separate\n+\/\/ variable from the one for renders because the commit phase may run\n+\/\/ concurrently to a render phase.\n+let didIncludeCommitPhaseUpdate: boolean = false;\n+\n \/\/ The most recent time we either committed a fallback, or when a fallback was\n \/\/ filled in with the resolved UI. This lets us throttle the appearance of new\n \/\/ content as it streams in, to minimize jank.\n@@ -1154,6 +1162,7 @@ function finishConcurrentRender(\n root,\n workInProgressRootRecoverableErrors,\n workInProgressTransitions,\n+ workInProgressRootDidIncludeRecursiveRenderUpdate,\n workInProgressDeferredLane,\n );\n } else {\n@@ -1189,6 +1198,7 @@ function finishConcurrentRender(\n finishedWork,\n workInProgressRootRecoverableErrors,\n workInProgressTransitions,\n+ workInProgressRootDidIncludeRecursiveRenderUpdate,\n lanes,\n workInProgressDeferredLane,\n ),\n@@ -1202,6 +1212,7 @@ function finishConcurrentRender(\n finishedWork,\n workInProgressRootRecoverableErrors,\n workInProgressTransitions,\n+ workInProgressRootDidIncludeRecursiveRenderUpdate,\n lanes,\n workInProgressDeferredLane,\n );\n@@ -1213,6 +1224,7 @@ function commitRootWhenReady(\n finishedWork: Fiber,\n recoverableErrors: Array> | null,\n transitions: Array | null,\n+ didIncludeRenderPhaseUpdate: boolean,\n lanes: Lanes,\n spawnedLane: Lane,\n ) {\n@@ -1240,7 +1252,13 @@ function commitRootWhenReady(\n \/\/ us that it's ready. This will be canceled if we start work on the\n \/\/ root again.\n root.cancelPendingCommit = schedulePendingCommit(\n- commitRoot.bind(null, root, recoverableErrors, transitions),\n+ commitRoot.bind(\n+ null,\n+ root,\n+ recoverableErrors,\n+ transitions,\n+ didIncludeRenderPhaseUpdate,\n+ ),\n );\n markRootSuspended(root, lanes, spawnedLane);\n return;\n@@ -1248,7 +1266,13 @@ function commitRootWhenReady(\n }\n \n \/\/ Otherwise, commit immediately.\n- commitRoot(root, recoverableErrors, transitions, spawnedLane);\n+ commitRoot(\n+ root,\n+ recoverableErrors,\n+ transitions,\n+ didIncludeRenderPhaseUpdate,\n+ spawnedLane,\n+ );\n }\n \n function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {\n@@ -1304,6 +1328,46 @@ function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {\n return true;\n }\n \n+\/\/ The extra indirections around markRootUpdated and markRootSuspended is\n+\/\/ needed to avoid a circular dependency between this module and\n+\/\/ ReactFiberLane. There's probably a better way to split up these modules and\n+\/\/ avoid this problem. Perhaps all the root-marking functions should move into\n+\/\/ the work loop.\n+\n+function markRootUpdated(root: FiberRoot, updatedLanes: Lanes) {\n+ _markRootUpdated(root, updatedLanes);\n+\n+ if (enableInfiniteRenderLoopDetection) {\n+ \/\/ Check for recursive updates\n+ if (executionContext & RenderContext) {\n+ workInProgressRootDidIncludeRecursiveRenderUpdate = true;\n+ } else if (executionContext & CommitContext) {\n+ didIncludeCommitPhaseUpdate = true;\n+ }\n+\n+ throwIfInfiniteUpdateLoopDetected();\n+ }\n+}\n+\n+function markRootPinged(root: FiberRoot, pingedLanes: Lanes) {\n+ _markRootPinged(root, pingedLanes);\n+\n+ if (enableInfiniteRenderLoopDetection) {\n+ \/\/ Check for recursive pings. Pings are conceptually different from updates in\n+ \/\/ other contexts but we call it an \"update\" in this context because\n+ \/\/ repeatedly pinging a suspended render can cause a recursive render loop.\n+ \/\/ The relevant property is that it can result in a new render attempt\n+ \/\/ being scheduled.\n+ if (executionContext & RenderContext) {\n+ workInProgressRootDidIncludeRecursiveRenderUpdate = true;\n+ } else if (executionContext & CommitContext) {\n+ didIncludeCommitPhaseUpdate = true;\n+ }\n+\n+ throwIfInfiniteUpdateLoopDetected();\n+ }\n+}\n+\n function markRootSuspended(\n root: FiberRoot,\n suspendedLanes: Lanes,\n@@ -1311,14 +1375,12 @@ function markRootSuspended(\n ) {\n \/\/ When suspending, we should always exclude lanes that were pinged or (more\n \/\/ rarely, since we try to avoid it) updated during the render phase.\n- \/\/ TODO: Lol maybe there's a better way to factor this besides this\n- \/\/ obnoxiously named function :)\n suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);\n suspendedLanes = removeLanes(\n suspendedLanes,\n workInProgressRootInterleavedUpdatedLanes,\n );\n- markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes, spawnedLane);\n+ _markRootSuspended(root, suspendedLanes, spawnedLane);\n }\n \n \/\/ This is the entry point for synchronous tasks that don't go\n@@ -1391,6 +1453,7 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null {\n root,\n workInProgressRootRecoverableErrors,\n workInProgressTransitions,\n+ workInProgressRootDidIncludeRecursiveRenderUpdate,\n workInProgressDeferredLane,\n );\n \n@@ -1607,6 +1670,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {\n workInProgressDeferredLane = NoLane;\n workInProgressRootConcurrentErrors = null;\n workInProgressRootRecoverableErrors = null;\n+ workInProgressRootDidIncludeRecursiveRenderUpdate = false;\n \n \/\/ Get the lanes that are entangled with whatever we're about to render. We\n \/\/ track these separately so we can distinguish the priority of the render\n@@ -2675,6 +2739,7 @@ function commitRoot(\n root: FiberRoot,\n recoverableErrors: null | Array>,\n transitions: Array | null,\n+ didIncludeRenderPhaseUpdate: boolean,\n spawnedLane: Lane,\n ) {\n \/\/ TODO: This no longer makes any sense. We already wrap the mutation and\n@@ -2689,6 +2754,7 @@ function commitRoot(\n root,\n recoverableErrors,\n transitions,\n+ didIncludeRenderPhaseUpdate,\n previousUpdateLanePriority,\n spawnedLane,\n );\n@@ -2704,6 +2770,7 @@ function commitRootImpl(\n root: FiberRoot,\n recoverableErrors: null | Array>,\n transitions: Array | null,\n+ didIncludeRenderPhaseUpdate: boolean,\n renderPriorityLevel: EventPriority,\n spawnedLane: Lane,\n ) {\n@@ -2784,6 +2851,9 @@ function commitRootImpl(\n \n markRootFinished(root, remainingLanes, spawnedLane);\n \n+ \/\/ Reset this before firing side effects so we can detect recursive updates.\n+ didIncludeCommitPhaseUpdate = false;\n+\n if (root === workInProgressRoot) {\n \/\/ We can reset these now that they are finished.\n workInProgressRoot = null;\n@@ -3036,10 +3106,15 @@ function commitRootImpl(\n \/\/ hydration lanes in this check, because render triggered by selective\n \/\/ hydration is conceptually not an update.\n if (\n+ \/\/ Check if there was a recursive update spawned by this render, in either\n+ \/\/ the render phase or the commit phase. We track these explicitly because\n+ \/\/ we can't infer from the remaining lanes alone.\n+ (enableInfiniteRenderLoopDetection &&\n+ (didIncludeRenderPhaseUpdate || didIncludeCommitPhaseUpdate)) ||\n \/\/ Was the finished render the result of an update (not hydration)?\n- includesSomeLane(lanes, UpdateLanes) &&\n- \/\/ Did it schedule a sync update?\n- includesSomeLane(remainingLanes, SyncUpdateLanes)\n+ (includesSomeLane(lanes, UpdateLanes) &&\n+ \/\/ Did it schedule a sync update?\n+ includesSomeLane(remainingLanes, SyncUpdateLanes))\n ) {\n if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {\n markNestedUpdateScheduled();\n@@ -3582,6 +3657,19 @@ export function throwIfInfiniteUpdateLoopDetected() {\n rootWithNestedUpdates = null;\n rootWithPassiveNestedUpdates = null;\n \n+ if (enableInfiniteRenderLoopDetection) {\n+ if (executionContext & RenderContext && workInProgressRoot !== null) {\n+ \/\/ We're in the render phase. Disable the concurrent error recovery\n+ \/\/ mechanism to ensure that the error we're about to throw gets handled.\n+ \/\/ We need it to trigger the nearest error boundary so that the infinite\n+ \/\/ update loop is broken.\n+ workInProgressRoot.errorRecoveryDisabledLanes = mergeLanes(\n+ workInProgressRoot.errorRecoveryDisabledLanes,\n+ workInProgressRootRenderLanes,\n+ );\n+ }\n+ }\n+\n throw new Error(\n 'Maximum update depth exceeded. This can happen when a component ' +\n 'repeatedly calls setState inside componentWillUpdate or ' +\ndiff --git a\/packages\/shared\/ReactFeatureFlags.js b\/packages\/shared\/ReactFeatureFlags.js\nindex 6cd8795052..67862a4f14 100644\n--- a\/packages\/shared\/ReactFeatureFlags.js\n+++ b\/packages\/shared\/ReactFeatureFlags.js\n@@ -170,6 +170,12 @@ export const disableClientCache = false;\n \/\/ Changes Server Components Reconciliation when they have keys\n export const enableServerComponentKeys = __NEXT_MAJOR__;\n \n+\/**\n+ * Enables a new error detection for infinite render loops from updates caused\n+ * by setState or similar outside of the component owning the state.\n+ *\/\n+export const enableInfiniteRenderLoopDetection = true;\n+\n \/\/ -----------------------------------------------------------------------------\n \/\/ Chopping Block\n \/\/\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.native-fb.js b\/packages\/shared\/forks\/ReactFeatureFlags.native-fb.js\nindex 3d0c70ef5f..497a5b6bcf 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.native-fb.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.native-fb.js\n@@ -94,6 +94,7 @@ export const enableUseDeferredValueInitialArg = true;\n export const disableClientCache = true;\n \n export const enableServerComponentKeys = true;\n+export const enableInfiniteRenderLoopDetection = false;\n \n \/\/ Flow magic to verify the exports of this file match the original version.\n ((((null: any): ExportsType): FeatureFlagsType): ExportsType);\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.native-oss.js b\/packages\/shared\/forks\/ReactFeatureFlags.native-oss.js\nindex b9705b2762..67b220974b 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.native-oss.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.native-oss.js\n@@ -75,6 +75,7 @@ export const useModernStrictMode = false;\n export const enableDO_NOT_USE_disableStrictPassiveEffect = false;\n export const enableFizzExternalRuntime = false;\n export const enableDeferRootSchedulingToMicrotask = false;\n+export const enableInfiniteRenderLoopDetection = false;\n \n export const enableAsyncActions = false;\n \ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.js b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.js\nindex a09d5f1e2b..63025e79fb 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.js\n@@ -86,6 +86,7 @@ export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__;\n export const disableClientCache = true;\n \n export const enableServerComponentKeys = true;\n+export const enableInfiniteRenderLoopDetection = false;\n \n \/\/ Flow magic to verify the exports of this file match the original version.\n ((((null: any): ExportsType): FeatureFlagsType): ExportsType);\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.native.js b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.native.js\nindex 51a47ee52f..4baed7ddd2 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.native.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.native.js\n@@ -50,6 +50,7 @@ export const enableUseMemoCacheHook = true;\n export const enableUseEffectEventHook = false;\n export const enableClientRenderFallbackOnTextMismatch = true;\n export const enableUseRefAccessWarning = false;\n+export const enableInfiniteRenderLoopDetection = false;\n \n export const enableRetryLaneExpiration = false;\n export const retryLaneExpirationMs = 5000;\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.www.js b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.www.js\nindex c5e13f95e3..0105b2bed9 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.www.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.test-renderer.www.js\n@@ -86,6 +86,7 @@ export const enableUseDeferredValueInitialArg = true;\n export const disableClientCache = true;\n \n export const enableServerComponentKeys = true;\n+export const enableInfiniteRenderLoopDetection = false;\n \n \/\/ Flow magic to verify the exports of this file match the original version.\n ((((null: any): ExportsType): FeatureFlagsType): ExportsType);\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.www-dynamic.js b\/packages\/shared\/forks\/ReactFeatureFlags.www-dynamic.js\nindex 775dced6d1..9b15bddded 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.www-dynamic.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.www-dynamic.js\n@@ -43,6 +43,8 @@ export const enableDebugTracing = __EXPERIMENTAL__;\n \n export const enableSchedulingProfiler = __VARIANT__;\n \n+export const enableInfiniteRenderLoopDetection = __VARIANT__;\n+\n \/\/ These are already tested in both modes using the build type dimension,\n \/\/ so we don't need to use __VARIANT__ to get extra coverage.\n export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;\ndiff --git a\/packages\/shared\/forks\/ReactFeatureFlags.www.js b\/packages\/shared\/forks\/ReactFeatureFlags.www.js\nindex 7ae2da95cc..3a3f0f86a6 100644\n--- a\/packages\/shared\/forks\/ReactFeatureFlags.www.js\n+++ b\/packages\/shared\/forks\/ReactFeatureFlags.www.js\n@@ -36,6 +36,7 @@ export const {\n retryLaneExpirationMs,\n syncLaneExpirationMs,\n transitionLaneExpirationMs,\n+ enableInfiniteRenderLoopDetection,\n } = dynamicFeatureFlags;\n \n \/\/ On WWW, __EXPERIMENTAL__ is used for a new modern build.\n"}

dillon1000/react

Files changed