From 05865c32f5995e0cb42bb052088993a4b03bfc09 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 27 Jun 2023 13:26:35 -0400 Subject: [PATCH] Fix: Detect infinite update loops caused by render phase updates (#26625) This PR contains a regression test and two separate fixes: a targeted fix, and a more general one that's designed as a last-resort guard against these types of bugs (both bugs in app code and bugs in React). I confirmed that each of these fixes separately are sufficient to fix the regression test I added. We can't realistically detect all infinite update loop scenarios because they could be async; even a single microtask can foil our attempts to detect a cycle. But this improves our strategy for detecting the most common kind. See commit messages for more details. --- .../src/__tests__/ReactUpdates-test.js | 58 +++++++++++ .../src/ReactFiberRootScheduler.js | 49 ++++++++++ .../src/ReactFiberWorkLoop.js | 96 +++++++++++++++++-- scripts/jest/matchers/toWarnDev.js | 14 ++- 4 files changed, 204 insertions(+), 13 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 67abeb6dd0ad6..9b1478cf3967a 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1620,6 +1620,64 @@ describe('ReactUpdates', () => { }); }); + it("does not infinite loop if there's a synchronous render phase update on another component", () => { + 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 () => { + 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/ReactFiberRootScheduler.js b/packages/react-reconciler/src/ReactFiberRootScheduler.js index a8542d0a0ea06..faff2cc954367 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -139,6 +139,18 @@ export function ensureRootIsScheduled(root: FiberRoot): void { } } +function unscheduleAllRoots() { + // This is only done in a fatal error situation, as a last resort to prevent + // an infinite render loop. + let root = firstScheduledRoot; + while (root !== null) { + const next = root.next; + root.next = null; + root = next; + } + firstScheduledRoot = lastScheduledRoot = null; +} + export function flushSyncWorkOnAllRoots() { // This is allowed to be called synchronously, but the caller should check // the execution context first. @@ -166,10 +178,47 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) { // There may or may not be synchronous work scheduled. Let's check. let didPerformSomeWork; + let nestedUpdatePasses = 0; let errors: Array | null = null; isFlushingWork = true; do { didPerformSomeWork = false; + + // This outer loop re-runs if performing sync work on a root spawns + // additional sync work. If it happens too many times, it's very likely + // caused by some sort of infinite update loop. We already have a loop guard + // in place that will trigger an error on the n+1th update, but it's + // possible for that error to get swallowed if the setState is called from + // an unexpected place, like during the render phase. So as an added + // precaution, we also use a guard here. + // + // Ideally, there should be no known way to trigger this synchronous loop. + // It's really just here as a safety net. + // + // This limit is slightly larger than the one that throws inside setState, + // because that one is preferable because it includes a componens stack. + if (++nestedUpdatePasses > 60) { + // This is a fatal error, so we'll unschedule all the roots. + unscheduleAllRoots(); + // TODO: Change this error message to something different to distinguish + // it from the one that is thrown from setState. Those are less fatal + // because they usually will result in the bad component being unmounted, + // and an error boundary being triggered, rather than us having to + // forcibly stop the entire scheduler. + const infiniteUpdateError = new Error( + 'Maximum update depth exceeded. This can happen when a component ' + + 'repeatedly calls setState inside componentWillUpdate or ' + + 'componentDidUpdate. React limits the number of nested updates to ' + + 'prevent infinite loops.', + ); + if (errors === null) { + errors = [infiniteUpdateError]; + } else { + errors.push(infiniteUpdateError); + } + break; + } + let root = firstScheduledRoot; while (root !== null) { if (onlyLegacy && root.tag !== LegacyRoot) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index b3953369803be..d7a99b1af3ce7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -142,9 +142,9 @@ import { includesExpiredLane, getNextLanes, getLanesToRetrySynchronouslyOnError, - markRootUpdated, - markRootSuspended as markRootSuspended_dontCallThisOneDirectly, - markRootPinged, + markRootSuspended as _markRootSuspended, + markRootUpdated as _markRootUpdated, + markRootPinged as _markRootPinged, markRootEntangled, markRootFinished, addFiberToLanesMap, @@ -373,6 +373,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. @@ -1095,6 +1102,7 @@ function finishConcurrentRender( root, workInProgressRootRecoverableErrors, workInProgressTransitions, + workInProgressRootDidIncludeRecursiveRenderUpdate, ); } else { if ( @@ -1129,6 +1137,7 @@ function finishConcurrentRender( finishedWork, workInProgressRootRecoverableErrors, workInProgressTransitions, + workInProgressRootDidIncludeRecursiveRenderUpdate, lanes, ), msUntilTimeout, @@ -1141,6 +1150,7 @@ function finishConcurrentRender( finishedWork, workInProgressRootRecoverableErrors, workInProgressTransitions, + workInProgressRootDidIncludeRecursiveRenderUpdate, lanes, ); } @@ -1151,6 +1161,7 @@ function commitRootWhenReady( finishedWork: Fiber, recoverableErrors: Array> | null, transitions: Array | null, + didIncludeRenderPhaseUpdate: boolean, lanes: Lanes, ) { // TODO: Combine retry throttling with Suspensey commits. Right now they run @@ -1177,7 +1188,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); return; @@ -1185,7 +1202,7 @@ function commitRootWhenReady( } // Otherwise, commit immediately. - commitRoot(root, recoverableErrors, transitions); + commitRoot(root, recoverableErrors, transitions, didIncludeRenderPhaseUpdate); } function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean { @@ -1241,17 +1258,51 @@ 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); + + // 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); + + // 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) { // 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); + _markRootSuspended(root, suspendedLanes); } // This is the entry point for synchronous tasks that don't go @@ -1324,6 +1375,7 @@ export function performSyncWorkOnRoot(root: FiberRoot, lanes: Lanes): null { root, workInProgressRootRecoverableErrors, workInProgressTransitions, + workInProgressRootDidIncludeRecursiveRenderUpdate, ); // Before exiting, make sure there's a callback scheduled for the next @@ -1538,6 +1590,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRootPingedLanes = NoLanes; workInProgressRootConcurrentErrors = null; workInProgressRootRecoverableErrors = null; + workInProgressRootDidIncludeRecursiveRenderUpdate = false; finishQueueingConcurrentUpdates(); @@ -2582,6 +2635,7 @@ function commitRoot( root: FiberRoot, recoverableErrors: null | Array>, transitions: Array | null, + didIncludeRenderPhaseUpdate: boolean, ) { // TODO: This no longer makes any sense. We already wrap the mutation and // layout phases. Should be able to remove. @@ -2595,6 +2649,7 @@ function commitRoot( root, recoverableErrors, transitions, + didIncludeRenderPhaseUpdate, previousUpdateLanePriority, ); } finally { @@ -2609,6 +2664,7 @@ function commitRootImpl( root: FiberRoot, recoverableErrors: null | Array>, transitions: Array | null, + didIncludeRenderPhaseUpdate: boolean, renderPriorityLevel: EventPriority, ) { do { @@ -2688,6 +2744,9 @@ function commitRootImpl( markRootFinished(root, remainingLanes); + // 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; @@ -2940,6 +2999,16 @@ 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. + didIncludeCommitPhaseUpdate || + didIncludeRenderPhaseUpdate || + // As an additional precaution, we also check if there's any remaining sync + // work. Theoretically this should be unreachable but if there's a mistake + // in React it helps to be overly defensive given how hard it is to debug + // those scenarios otherwise. This won't catch recursive async updates, + // though, which is why we check the flags above first. // Was the finished render the result of an update (not hydration)? includesSomeLane(lanes, UpdateLanes) && // Did it schedule a sync update? @@ -3486,6 +3555,17 @@ export function throwIfInfiniteUpdateLoopDetected() { rootWithNestedUpdates = null; rootWithPassiveNestedUpdates = null; + 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/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 88ae8661e9964..992cedcf6e262 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -69,12 +69,16 @@ const createMatcherFor = (consoleMethod, matcherName) => (message.includes('\n in ') || message.includes('\n at ')); const consoleSpy = (format, ...args) => { - // Ignore uncaught errors reported by jsdom - // and React addendums because they're too noisy. if ( - !logAllErrors && - consoleMethod === 'error' && - shouldIgnoreConsoleError(format, args) + // Ignore uncaught errors reported by jsdom + // and React addendums because they're too noisy. + (!logAllErrors && + consoleMethod === 'error' && + shouldIgnoreConsoleError(format, args)) || + // Ignore error objects passed to console.error, which we sometimes + // use as a fallback behavior, like when reportError + // isn't available. + typeof format !== 'string' ) { return; }