diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 9b1478cf3967a..67abeb6dd0ad6 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1620,64 +1620,6 @@ 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 6b4036f71df03..8e0c65b8552df 100644 --- a/packages/react-reconciler/src/ReactFiberRootScheduler.js +++ b/packages/react-reconciler/src/ReactFiberRootScheduler.js @@ -139,18 +139,6 @@ 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. @@ -181,47 +169,10 @@ 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 104616527fe97..1b3e274191d35 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -141,9 +141,9 @@ import { includesExpiredLane, getNextLanes, getLanesToRetrySynchronouslyOnError, - markRootSuspended as _markRootSuspended, - markRootUpdated as _markRootUpdated, - markRootPinged as _markRootPinged, + markRootUpdated, + markRootSuspended as markRootSuspended_dontCallThisOneDirectly, + markRootPinged, markRootEntangled, markRootFinished, addFiberToLanesMap, @@ -370,13 +370,6 @@ 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. @@ -1121,7 +1114,6 @@ function finishConcurrentRender( root, workInProgressRootRecoverableErrors, workInProgressTransitions, - workInProgressRootDidIncludeRecursiveRenderUpdate, ); } else { if ( @@ -1156,7 +1148,6 @@ function finishConcurrentRender( finishedWork, workInProgressRootRecoverableErrors, workInProgressTransitions, - workInProgressRootDidIncludeRecursiveRenderUpdate, lanes, ), msUntilTimeout, @@ -1169,7 +1160,6 @@ function finishConcurrentRender( finishedWork, workInProgressRootRecoverableErrors, workInProgressTransitions, - workInProgressRootDidIncludeRecursiveRenderUpdate, lanes, ); } @@ -1180,7 +1170,6 @@ 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 @@ -1207,13 +1196,7 @@ 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, - didIncludeRenderPhaseUpdate, - ), + commitRoot.bind(null, root, recoverableErrors, transitions), ); markRootSuspended(root, lanes); return; @@ -1221,7 +1204,7 @@ function commitRootWhenReady( } // Otherwise, commit immediately. - commitRoot(root, recoverableErrors, transitions, didIncludeRenderPhaseUpdate); + commitRoot(root, recoverableErrors, transitions); } function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean { @@ -1277,51 +1260,17 @@ 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(root, suspendedLanes); + markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes); } // This is the entry point for synchronous tasks that don't go @@ -1392,7 +1341,6 @@ export function performSyncWorkOnRoot(root: FiberRoot): null { root, workInProgressRootRecoverableErrors, workInProgressTransitions, - workInProgressRootDidIncludeRecursiveRenderUpdate, ); // Before exiting, make sure there's a callback scheduled for the next @@ -1607,7 +1555,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber { workInProgressRootPingedLanes = NoLanes; workInProgressRootConcurrentErrors = null; workInProgressRootRecoverableErrors = null; - workInProgressRootDidIncludeRecursiveRenderUpdate = false; finishQueueingConcurrentUpdates(); @@ -2649,7 +2596,6 @@ 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. @@ -2663,7 +2609,6 @@ function commitRoot( root, recoverableErrors, transitions, - didIncludeRenderPhaseUpdate, previousUpdateLanePriority, ); } finally { @@ -2678,7 +2623,6 @@ function commitRootImpl( root: FiberRoot, recoverableErrors: null | Array>, transitions: Array | null, - didIncludeRenderPhaseUpdate: boolean, renderPriorityLevel: EventPriority, ) { do { @@ -2758,9 +2702,6 @@ 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; @@ -3007,19 +2948,7 @@ function commitRootImpl( // Read this again, since a passive effect might have updated it remainingLanes = root.pendingLanes; - 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. - includesSyncLane(remainingLanes) - ) { + if (includesSyncLane(remainingLanes)) { if (enableProfilerTimer && enableProfilerNestedUpdatePhase) { markNestedUpdateScheduled(); } @@ -3561,17 +3490,6 @@ 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 992cedcf6e262..88ae8661e9964 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -69,16 +69,12 @@ 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 ( - // 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' + !logAllErrors && + consoleMethod === 'error' && + shouldIgnoreConsoleError(format, args) ) { return; }