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