[Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect (#22277)

* Add test that triggers infinite update loop

In 18, passive effects are flushed synchronously if they are the
result of a synchronous update. We have a guard for infinite update
loops that occur in the layout phase, but it doesn't currently work for
synchronous updates from a passive effect.

The reason this probably hasn't come up yet is because synchronous
updates inside the passive effect phase are relatively rare: you either
have to imperatively dispatch a discrete event, like `el.focus`, or you
have to call `ReactDOM.flushSync`, which triggers a warning. (In
general, updates inside a passive effect are not encouraged.)

I discovered this because `useSyncExternalStore` does sometimes
trigger updates inside the passive effect phase.

This commit adds a failing test to prove the issue exists. I will fix
it in the next commit.

* Fix failing test added in previous commit

The way we detect a "nested update" is if there's synchronous work
remaining at the end of the commit phase.

Currently this check happens before we synchronously flush the passive
effects. I moved it to after the effects are fired, so that it detects
whether synchronous work was scheduled in that phase.

Author: acdlite

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

@@ -1594,6 +1594,7 @@ describe('ReactUpdates', () => {
     });
   });
 
+  // TODO: Replace this branch with @gate pragmas
   if (__DEV__) {
     it('warns about a deferred infinite update loop with useEffect', () => {
       function NonTerminating() {
@@ -1684,4 +1685,35 @@ describe('ReactUpdates', () => {
       expect(container.textContent).toBe('1000');
     });
   }
+
+  it('prevents infinite update loop triggered by synchronous updates in useEffect', () => {
+    // Ignore flushSync warning
+    spyOnDev(console, 'error');
+
+    function NonTerminating() {
+      const [step, setStep] = React.useState(0);
+      React.useEffect(() => {
+        // Other examples of synchronous updates in useEffect are imperative
+        // event dispatches like `el.focus`, or `useSyncExternalStore`, which
+        // may schedule a synchronous update upon subscribing if it detects
+        // that the store has been mutated since the initial render.
+        //
+        // (Originally I wrote this test using `el.focus` but those errors
+        // get dispatched in a JSDOM event and I don't know how to "catch" those
+        // so that they don't fail the test.)
+        ReactDOM.flushSync(() => {
+          setStep(step + 1);
+        });
+      }, [step]);
+      return step;
+    }
+
+    const container = document.createElement('div');
+    const root = ReactDOM.createRoot(container);
+    expect(() => {
+      ReactDOM.flushSync(() => {
+        root.render(<NonTerminating />);
+      });
+    }).toThrow('Maximum update depth exceeded');
+  });
 });

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
   remainingLanes = root.pendingLanes;
 
   // Check if there's remaining work on this root
+  // TODO: This is part of the `componentDidCatch` implementation. Its purpose
+  // is to detect whether something might have called setState inside
+  // `componentDidCatch`. The mechanism is known to be flawed because `setState`
+  // inside `componentDidCatch` is itself flawed — that's why we recommend
+  // `getDerivedStateFromError` instead. However, it could be improved by
+  // checking if remainingLanes includes Sync work, instead of whether there's
+  // any work remaining at all (which would also include stuff like Suspense
+  // retries or transitions). It's been like this for a while, though, so fixing
+  // it probably isn't that urgent.
   if (remainingLanes === NoLanes) {
     // If there's no remaining work, we can clear the set of already failed
     // error boundaries.
@@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
   }
 
-  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
-    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-      markNestedUpdateScheduled();
-    }
-
-    // Count the number of times the root synchronously re-renders without
-    // finishing. If there are too many, it indicates an infinite update loop.
-    if (root === rootWithNestedUpdates) {
-      nestedUpdateCount++;
-    } else {
-      nestedUpdateCount = 0;
-      rootWithNestedUpdates = root;
-    }
-  } else {
-    nestedUpdateCount = 0;
-  }
-
   onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);
 
   if (enableUpdaterTracking) {
@@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
     flushPassiveEffects();
   }
 
+  // Read this again, since a passive effect might have updated it
+  remainingLanes = root.pendingLanes;
+  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
+    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+      markNestedUpdateScheduled();
+    }
+
+    // Count the number of times the root synchronously re-renders without
+    // finishing. If there are too many, it indicates an infinite update loop.
+    if (root === rootWithNestedUpdates) {
+      nestedUpdateCount++;
+    } else {
+      nestedUpdateCount = 0;
+      rootWithNestedUpdates = root;
+    }
+  } else {
+    nestedUpdateCount = 0;
+  }
+
   // If layout work was scheduled, flush it now.
   flushSyncCallbacks();
 

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -1897,6 +1897,15 @@ function commitRootImpl(root, renderPriorityLevel) {
   remainingLanes = root.pendingLanes;
 
   // Check if there's remaining work on this root
+  // TODO: This is part of the `componentDidCatch` implementation. Its purpose
+  // is to detect whether something might have called setState inside
+  // `componentDidCatch`. The mechanism is known to be flawed because `setState`
+  // inside `componentDidCatch` is itself flawed — that's why we recommend
+  // `getDerivedStateFromError` instead. However, it could be improved by
+  // checking if remainingLanes includes Sync work, instead of whether there's
+  // any work remaining at all (which would also include stuff like Suspense
+  // retries or transitions). It's been like this for a while, though, so fixing
+  // it probably isn't that urgent.
   if (remainingLanes === NoLanes) {
     // If there's no remaining work, we can clear the set of already failed
     // error boundaries.
@@ -1909,23 +1918,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     }
   }
 
-  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
-    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
-      markNestedUpdateScheduled();
-    }
-
-    // Count the number of times the root synchronously re-renders without
-    // finishing. If there are too many, it indicates an infinite update loop.
-    if (root === rootWithNestedUpdates) {
-      nestedUpdateCount++;
-    } else {
-      nestedUpdateCount = 0;
-      rootWithNestedUpdates = root;
-    }
-  } else {
-    nestedUpdateCount = 0;
-  }
-
   onCommitRootDevTools(finishedWork.stateNode, renderPriorityLevel);
 
   if (enableUpdaterTracking) {
@@ -1964,6 +1956,25 @@ function commitRootImpl(root, renderPriorityLevel) {
     flushPassiveEffects();
   }
 
+  // Read this again, since a passive effect might have updated it
+  remainingLanes = root.pendingLanes;
+  if (includesSomeLane(remainingLanes, (SyncLane: Lane))) {
+    if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
+      markNestedUpdateScheduled();
+    }
+
+    // Count the number of times the root synchronously re-renders without
+    // finishing. If there are too many, it indicates an infinite update loop.
+    if (root === rootWithNestedUpdates) {
+      nestedUpdateCount++;
+    } else {
+      nestedUpdateCount = 0;
+      rootWithNestedUpdates = root;
+    }
+  } else {
+    nestedUpdateCount = 0;
+  }
+
   // If layout work was scheduled, flush it now.
   flushSyncCallbacks();