Fix: Detect infinite update loops caused by render phase updates (fac…

…ebook#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.

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 <Child />;
+    }
+
+    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(<App />))).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 <Child />;
+    }
+
+    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(<App />));
+        });
+      } 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 () => {

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<CapturedValue<mixed>> | null =
 let workInProgressRootRecoverableErrors: Array<CapturedValue<mixed>> | 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<CapturedValue<mixed>> | null,
   transitions: Array<Transition> | null,
+  didIncludeRenderPhaseUpdate: boolean,
   lanes: Lanes,
   spawnedLane: Lane,
 ) {
@@ -1240,15 +1252,27 @@ 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;
     }
   }
 
   // Otherwise, commit immediately.
-  commitRoot(root, recoverableErrors, transitions, spawnedLane);
+  commitRoot(
+    root,
+    recoverableErrors,
+    transitions,
+    didIncludeRenderPhaseUpdate,
+    spawnedLane,
+  );
 }
 
 function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {
@@ -1304,21 +1328,59 @@ 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,
   spawnedLane: Lane,
 ) {
   // 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<CapturedValue<mixed>>,
   transitions: Array<Transition> | 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<CapturedValue<mixed>>,
   transitions: Array<Transition> | 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 ' +

packages/shared/ReactFeatureFlags.js

@@ -169,6 +169,12 @@ export const enableFilterEmptyStringAttributesDOM = __NEXT_MAJOR__;
 // Disabled caching behavior of `react/cache` in client runtimes.
 export const disableClientCache = false;
 
+/**
+ * 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
 //

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);

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 = true;
+export const enableInfiniteRenderLoopDetection = false;
 
 export const enableAsyncActions = false;
 

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);

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);

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__;

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.