Repository: facebook/react
Pull request: [Bugfix] Prevent infinite update loop caused by a synchronous update in a passive effect

## The bug

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, which means if this scenario happens, the main thread will lock up and crash the tab.

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.

## The fix

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.

Changed files:
filename: packages/react-dom/src/__tests__/ReactUpdates-test.js
patch: @@ -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');
+  });
 });

filename: packages/react-reconciler/src/ReactFiberWorkLoop.new.js
patch: @@ -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();
 

filename: packages/react-reconciler/src/ReactFiberWorkLoop.old.js
patch: @@ -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();