@samitouri / QOS-React-2 / commits / 822386f252

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.

Andrew Clark committed Jun 27, 2023 at 13:26 UTC 822386f252fd1f0e949efa904a1ed790133329f7

4 files changed +207 -14

packages/react-dom/src/__tests__/ReactUpdates-test.js

+58

@@ -1620,6 +1620,64 @@ describe('ReactUpdates', () => {

1620 });

1621 });

1622

1623 + it("does not infinite loop if there's a synchronous render phase update on another component", () => {

1624 + let setState;

1625 + function App() {

1626 + const [, _setState] = React.useState(0);

1627 + setState = _setState;

1628 + return <Child />;

1629 + }

1630 +

1631 + function Child(step) {

1632 + // This will cause an infinite update loop, and a warning in dev.

1633 + setState(n => n + 1);

1634 + return null;

1635 + }

1636 +

1637 + const container = document.createElement('div');

1638 + const root = ReactDOMClient.createRoot(container);

1639 +

1640 + expect(() => {

1641 + expect(() => ReactDOM.flushSync(() => root.render(<App />))).toThrow(

1642 + 'Maximum update depth exceeded',

1643 + );

1644 + }).toErrorDev(

1645 + 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',

1646 + );

1647 + });

1648 +

1649 + it("does not infinite loop if there's an async render phase update on another component", async () => {

1650 + let setState;

1651 + function App() {

1652 + const [, _setState] = React.useState(0);

1653 + setState = _setState;

1654 + return <Child />;

1655 + }

1656 +

1657 + function Child(step) {

1658 + // This will cause an infinite update loop, and a warning in dev.

1659 + setState(n => n + 1);

1660 + return null;

1661 + }

1662 +

1663 + const container = document.createElement('div');

1664 + const root = ReactDOMClient.createRoot(container);

1665 +

1666 + await expect(async () => {

1667 + let error;

1668 + try {

1669 + await act(() => {

1670 + React.startTransition(() => root.render(<App />));

1671 + });

1672 + } catch (e) {

1673 + error = e;

1674 + }

1675 + expect(error.message).toMatch('Maximum update depth exceeded');

1676 + }).toErrorDev(

1677 + 'Warning: Cannot update a component (`App`) while rendering a different component (`Child`)',

1678 + );

1679 + });

1680 +

1681 // TODO: Replace this branch with @gate pragmas

1682 if (__DEV__) {

1683 it('warns about a deferred infinite update loop with useEffect', async () => {

packages/react-reconciler/src/ReactFiberRootScheduler.js

+49

@@ -139,6 +139,18 @@ export function ensureRootIsScheduled(root: FiberRoot): void {

139 }

140 }

141

142 +function unscheduleAllRoots() {

143 + // This is only done in a fatal error situation, as a last resort to prevent

144 + // an infinite render loop.

145 + let root = firstScheduledRoot;

146 + while (root !== null) {

147 + const next = root.next;

148 + root.next = null;

149 + root = next;

150 + }

151 + firstScheduledRoot = lastScheduledRoot = null;

152 +}

153 +

154 export function flushSyncWorkOnAllRoots() {

155 // This is allowed to be called synchronously, but the caller should check

156 // the execution context first.

@@ -169,10 +181,47 @@ function flushSyncWorkAcrossRoots_impl(onlyLegacy: boolean) {

181

182 // There may or may not be synchronous work scheduled. Let's check.

183 let didPerformSomeWork;

184 + let nestedUpdatePasses = 0;

185 let errors: Array<mixed> | null = null;

186 isFlushingWork = true;

187 do {

188 didPerformSomeWork = false;

189 +

190 + // This outer loop re-runs if performing sync work on a root spawns

191 + // additional sync work. If it happens too many times, it's very likely

192 + // caused by some sort of infinite update loop. We already have a loop guard

193 + // in place that will trigger an error on the n+1th update, but it's

194 + // possible for that error to get swallowed if the setState is called from

195 + // an unexpected place, like during the render phase. So as an added

196 + // precaution, we also use a guard here.

197 + //

198 + // Ideally, there should be no known way to trigger this synchronous loop.

199 + // It's really just here as a safety net.

200 + //

201 + // This limit is slightly larger than the one that throws inside setState,

202 + // because that one is preferable because it includes a componens stack.

203 + if (++nestedUpdatePasses > 60) {

204 + // This is a fatal error, so we'll unschedule all the roots.

205 + unscheduleAllRoots();

206 + // TODO: Change this error message to something different to distinguish

207 + // it from the one that is thrown from setState. Those are less fatal

208 + // because they usually will result in the bad component being unmounted,

209 + // and an error boundary being triggered, rather than us having to

210 + // forcibly stop the entire scheduler.

211 + const infiniteUpdateError = new Error(

212 + 'Maximum update depth exceeded. This can happen when a component ' +

213 + 'repeatedly calls setState inside componentWillUpdate or ' +

214 + 'componentDidUpdate. React limits the number of nested updates to ' +

215 + 'prevent infinite loops.',

216 + );

217 + if (errors === null) {

218 + errors = [infiniteUpdateError];

219 + } else {

220 + errors.push(infiniteUpdateError);

221 + }

222 + break;

223 + }

224 +

225 let root = firstScheduledRoot;

226 while (root !== null) {

227 if (onlyLegacy && root.tag !== LegacyRoot) {

packages/react-reconciler/src/ReactFiberWorkLoop.js

+91 -9

@@ -141,9 +141,9 @@ import {

141 includesExpiredLane,

142 getNextLanes,

143 getLanesToRetrySynchronouslyOnError,

144 - markRootUpdated,

145 - markRootSuspended as markRootSuspended_dontCallThisOneDirectly,

146 - markRootPinged,

144 + markRootSuspended as _markRootSuspended,

145 + markRootUpdated as _markRootUpdated,

146 + markRootPinged as _markRootPinged,

147 markRootEntangled,

148 markRootFinished,

149 addFiberToLanesMap,

@@ -370,6 +370,13 @@ let workInProgressRootConcurrentErrors: Array<CapturedValue<mixed>> | null =

370 let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | null =

371 null;

372

373 +// Tracks when an update occurs during the render phase.

374 +let workInProgressRootDidIncludeRecursiveRenderUpdate: boolean = false;

375 +// Thacks when an update occurs during the commit phase. It's a separate

376 +// variable from the one for renders because the commit phase may run

377 +// concurrently to a render phase.

378 +let didIncludeCommitPhaseUpdate: boolean = false;

379 +

380 // The most recent time we either committed a fallback, or when a fallback was

381 // filled in with the resolved UI. This lets us throttle the appearance of new

382 // content as it streams in, to minimize jank.

@@ -1114,6 +1121,7 @@ function finishConcurrentRender(

1121 root,

1122 workInProgressRootRecoverableErrors,

1123 workInProgressTransitions,

1124 + workInProgressRootDidIncludeRecursiveRenderUpdate,

1125 );

1126 } else {

1127 if (

@@ -1148,6 +1156,7 @@ function finishConcurrentRender(

1156 finishedWork,

1157 workInProgressRootRecoverableErrors,

1158 workInProgressTransitions,

1159 + workInProgressRootDidIncludeRecursiveRenderUpdate,

1160 lanes,

1161 ),

1162 msUntilTimeout,

@@ -1160,6 +1169,7 @@ function finishConcurrentRender(

1169 finishedWork,

1170 workInProgressRootRecoverableErrors,

1171 workInProgressTransitions,

1172 + workInProgressRootDidIncludeRecursiveRenderUpdate,

1173 lanes,

1174 );

1175 }

@@ -1170,6 +1180,7 @@ function commitRootWhenReady(

1180 finishedWork: Fiber,

1181 recoverableErrors: Array<CapturedValue<mixed>> | null,

1182 transitions: Array<Transition> | null,

1183 + didIncludeRenderPhaseUpdate: boolean,

1184 lanes: Lanes,

1185 ) {

1186 // TODO: Combine retry throttling with Suspensey commits. Right now they run

@@ -1196,7 +1207,13 @@ function commitRootWhenReady(

1207 // us that it's ready. This will be canceled if we start work on the

1208 // root again.

1209 root.cancelPendingCommit = schedulePendingCommit(

1199 - commitRoot.bind(null, root, recoverableErrors, transitions),

1210 + commitRoot.bind(

1211 + null,

1212 + root,

1213 + recoverableErrors,

1214 + transitions,

1215 + didIncludeRenderPhaseUpdate,

1216 + ),

1217 );

1218 markRootSuspended(root, lanes);

1219 return;

@@ -1204,7 +1221,7 @@ function commitRootWhenReady(

1221 }

1222

1223 // Otherwise, commit immediately.

1207 - commitRoot(root, recoverableErrors, transitions);

1224 + commitRoot(root, recoverableErrors, transitions, didIncludeRenderPhaseUpdate);

1225 }

1226

1227 function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {

@@ -1260,17 +1277,51 @@ function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {

1277 return true;

1278 }

1279

1280 +// The extra indirections around markRootUpdated and markRootSuspended is

1281 +// needed to avoid a circular dependency between this module and

1282 +// ReactFiberLane. There's probably a better way to split up these modules and

1283 +// avoid this problem. Perhaps all the root-marking functions should move into

1284 +// the work loop.

1285 +

1286 +function markRootUpdated(root: FiberRoot, updatedLanes: Lanes) {

1287 + _markRootUpdated(root, updatedLanes);

1288 +

1289 + // Check for recursive updates

1290 + if (executionContext & RenderContext) {

1291 + workInProgressRootDidIncludeRecursiveRenderUpdate = true;

1292 + } else if (executionContext & CommitContext) {

1293 + didIncludeCommitPhaseUpdate = true;

1294 + }

1295 +

1296 + throwIfInfiniteUpdateLoopDetected();

1297 +}

1298 +

1299 +function markRootPinged(root: FiberRoot, pingedLanes: Lanes) {

1300 + _markRootPinged(root, pingedLanes);

1301 +

1302 + // Check for recursive pings. Pings are conceptually different from updates in

1303 + // other contexts but we call it an "update" in this context because

1304 + // repeatedly pinging a suspended render can cause a recursive render loop.

1305 + // The relevant property is that it can result in a new render attempt

1306 + // being scheduled.

1307 + if (executionContext & RenderContext) {

1308 + workInProgressRootDidIncludeRecursiveRenderUpdate = true;

1309 + } else if (executionContext & CommitContext) {

1310 + didIncludeCommitPhaseUpdate = true;

1311 + }

1312 +

1313 + throwIfInfiniteUpdateLoopDetected();

1314 +}

1315 +

1316 function markRootSuspended(root: FiberRoot, suspendedLanes: Lanes) {

1317 // When suspending, we should always exclude lanes that were pinged or (more

1318 // rarely, since we try to avoid it) updated during the render phase.

1266 - // TODO: Lol maybe there's a better way to factor this besides this

1267 - // obnoxiously named function :)

1319 suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);

1320 suspendedLanes = removeLanes(

1321 suspendedLanes,

1322 workInProgressRootInterleavedUpdatedLanes,

1323 );

1273 - markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);

1324 + _markRootSuspended(root, suspendedLanes);

1325 }

1326

1327 // This is the entry point for synchronous tasks that don't go

@@ -1341,6 +1392,7 @@ export function performSyncWorkOnRoot(root: FiberRoot): null {

1392 root,

1393 workInProgressRootRecoverableErrors,

1394 workInProgressTransitions,

1395 + workInProgressRootDidIncludeRecursiveRenderUpdate,

1396 );

1397

1398 // Before exiting, make sure there's a callback scheduled for the next

@@ -1555,6 +1607,7 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes): Fiber {

1607 workInProgressRootPingedLanes = NoLanes;

1608 workInProgressRootConcurrentErrors = null;

1609 workInProgressRootRecoverableErrors = null;

1610 + workInProgressRootDidIncludeRecursiveRenderUpdate = false;

1611

1612 finishQueueingConcurrentUpdates();

1613

@@ -2577,6 +2630,7 @@ function commitRoot(

2630 root: FiberRoot,

2631 recoverableErrors: null | Array<CapturedValue<mixed>>,

2632 transitions: Array<Transition> | null,

2633 + didIncludeRenderPhaseUpdate: boolean,

2634 ) {

2635 // TODO: This no longer makes any sense. We already wrap the mutation and

2636 // layout phases. Should be able to remove.

@@ -2590,6 +2644,7 @@ function commitRoot(

2644 root,

2645 recoverableErrors,

2646 transitions,

2647 + didIncludeRenderPhaseUpdate,

2648 previousUpdateLanePriority,

2649 );

2650 } finally {

@@ -2604,6 +2659,7 @@ function commitRootImpl(

2659 root: FiberRoot,

2660 recoverableErrors: null | Array<CapturedValue<mixed>>,

2661 transitions: Array<Transition> | null,

2662 + didIncludeRenderPhaseUpdate: boolean,

2663 renderPriorityLevel: EventPriority,

2664 ) {

2665 do {

@@ -2683,6 +2739,9 @@ function commitRootImpl(

2739

2740 markRootFinished(root, remainingLanes);

2741

2742 + // Reset this before firing side effects so we can detect recursive updates.

2743 + didIncludeCommitPhaseUpdate = false;

2744 +

2745 if (root === workInProgressRoot) {

2746 // We can reset these now that they are finished.

2747 workInProgressRoot = null;

@@ -2929,7 +2988,19 @@ function commitRootImpl(

2988

2989 // Read this again, since a passive effect might have updated it

2990 remainingLanes = root.pendingLanes;

2932 - if (includesSyncLane(remainingLanes)) {

2991 + if (

2992 + // Check if there was a recursive update spawned by this render, in either

2993 + // the render phase or the commit phase. We track these explicitly because

2994 + // we can't infer from the remaining lanes alone.

2995 + didIncludeCommitPhaseUpdate ||

2996 + didIncludeRenderPhaseUpdate ||

2997 + // As an additional precaution, we also check if there's any remaining sync

2998 + // work. Theoretically this should be unreachable but if there's a mistake

2999 + // in React it helps to be overly defensive given how hard it is to debug

3000 + // those scenarios otherwise. This won't catch recursive async updates,

3001 + // though, which is why we check the flags above first.

3002 + includesSyncLane(remainingLanes)

3003 + ) {

3004 if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {

3005 markNestedUpdateScheduled();

3006 }

@@ -3471,6 +3542,17 @@ export function throwIfInfiniteUpdateLoopDetected() {

3542 rootWithNestedUpdates = null;

3543 rootWithPassiveNestedUpdates = null;

3544

3545 + if (executionContext & RenderContext && workInProgressRoot !== null) {

3546 + // We're in the render phase. Disable the concurrent error recovery

3547 + // mechanism to ensure that the error we're about to throw gets handled.

3548 + // We need it to trigger the nearest error boundary so that the infinite

3549 + // update loop is broken.

3550 + workInProgressRoot.errorRecoveryDisabledLanes = mergeLanes(

3551 + workInProgressRoot.errorRecoveryDisabledLanes,

3552 + workInProgressRootRenderLanes,

3553 + );

3554 + }

3555 +

3556 throw new Error(

3557 'Maximum update depth exceeded. This can happen when a component ' +

3558 'repeatedly calls setState inside componentWillUpdate or ' +

scripts/jest/matchers/toWarnDev.js

+9 -5

@@ -69,12 +69,16 @@ const createMatcherFor = (consoleMethod, matcherName) =>

69 (message.includes('\n in ') || message.includes('\n at '));

71 const consoleSpy = (format, ...args) => {

72 - // Ignore uncaught errors reported by jsdom

73 - // and React addendums because they're too noisy.

72 if (

75 - !logAllErrors &&

76 - consoleMethod === 'error' &&

77 - shouldIgnoreConsoleError(format, args)

73 + // Ignore uncaught errors reported by jsdom

74 + // and React addendums because they're too noisy.

75 + (!logAllErrors &&

76 + consoleMethod === 'error' &&

77 + shouldIgnoreConsoleError(format, args)) ||

78 + // Ignore error objects passed to console.error, which we sometimes

79 + // use as a fallback behavior, like when reportError

80 + // isn't available.

81 + typeof format !== 'string'

82 ) {

83 return;

84 }