Bugfix: Nested useOpaqueIdentifier calls

Fixes an issue where multiple useOpaqueIdentifier hooks are upgraded
to client ids within the same render.

The way the upgrade works is that useOpaqueIdentifier schedules a
render phase update then throws an error to trigger React's error
recovery mechanism.

The normal error recovery mechanism is designed for errors that occur
as a result of interleaved mutations, so we usually only retry a single
time, synchronously, before giving up.

useOpaqueIdentifier is different because the error its throws when
upgrading is not caused by an interleaved mutation. Rather, it happens
when an ID is referenced for the first time inside a client-rendered
tree (i.e. sommething that wasn't part of the initial server render).
The fact that it relies on the error recovery mechanism is an
implementation detail. And a single recovery attempt may be
insufficient. For example, if a parent and a child component may
reference different ids, and both are mounted as a result of the same
client update, that will trigger two separate error recovery attempts.

Because render phase updates are not allowed when triggered from
userspace — we log a warning in developement to prevent them —
we can assume that if something does update during the render phase, it
is one of our "legit" implementation details like useOpaqueIdentifier.
So we can keep retrying until we succeed — up to a limit, to protect
against inifite loops. I chose 50 since that's the limit we use for
commit phase updates.

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

@@ -1975,5 +1975,55 @@ describe('ReactDOMServerHooks', () => {
         container.getElementsByTagName('span')[0].getAttribute('id'),
       ).not.toBeNull();
     });
+
+    it('useOpaqueIdentifier with multiple ids in nested components', async () => {
+      function DivWithId({id, children}) {
+        return <div id={id}>{children}</div>;
+      }
+
+      let setShowMore;
+      function App() {
+        Scheduler.unstable_yieldValue('App');
+        const outerId = useOpaqueIdentifier();
+        const innerId = useOpaqueIdentifier();
+        const [showMore, _setShowMore] = useState(false);
+        setShowMore = _setShowMore;
+        return showMore ? (
+          <DivWithId id={outerId}>
+            <DivWithId id={innerId} />
+          </DivWithId>
+        ) : null;
+      }
+
+      const container = document.createElement('div');
+      container.innerHTML = ReactDOMServer.renderToString(<App />);
+      expect(Scheduler).toHaveYielded(['App']);
+
+      await act(async () => {
+        ReactDOM.hydrateRoot(container, <App />);
+      });
+      expect(Scheduler).toHaveYielded(['App']);
+
+      // Show additional content that wasn't part of the initial server-
+      // rendered repsonse.
+      await act(async () => {
+        setShowMore(true);
+      });
+      expect(Scheduler).toHaveYielded([
+        // First render. This one doesn't finish because the outerId is
+        // referenced in a client-rendered tree. We upgrade the id and retry.
+        'App',
+
+        // Second render. This time innerId needs to be upgraded, so we
+        // retry again.
+        'App',
+
+        // Finally everything has upgraded.
+        'App',
+      ]);
+      const [div1, div2] = container.getElementsByTagName('div');
+      expect(typeof div1.getAttribute('id')).toBe('string');
+      expect(typeof div2.getAttribute('id')).toBe('string');
+    });
   });
 });

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

@@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes;
 // includes unprocessed updates, not work in bailed out children.
 let workInProgressRootSkippedLanes: Lanes = NoLanes;
 // Lanes that were updated (in an interleaved event) during this render.
-let workInProgressRootUpdatedLanes: Lanes = NoLanes;
+let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
+// Lanes that were updated during the render phase (*not* an interleaved event).
+let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
 // Lanes that were pinged (in an interleaved event) during this render.
 let workInProgressRootPingedLanes: Lanes = NoLanes;
 
@@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber(
     // an implementation detail, like selective hydration
     // and useOpaqueIdentifier.
     warnAboutRenderPhaseUpdatesInDEV(fiber);
+
+    // Track lanes that were updated during the render phase
+    workInProgressRootRenderPhaseUpdatedLanes = mergeLanes(
+      workInProgressRootRenderPhaseUpdatedLanes,
+      lane,
+    );
   } else {
     // This is a normal update, scheduled from outside the render phase. For
     // example, during an input event.
@@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber(
         deferRenderPhaseUpdateToNextBatch ||
         (executionContext & RenderContext) === NoContext
       ) {
-        workInProgressRootUpdatedLanes = mergeLanes(
-          workInProgressRootUpdatedLanes,
+        workInProgressRootInterleavedUpdatedLanes = mergeLanes(
+          workInProgressRootInterleavedUpdatedLanes,
           lane,
         );
       }
@@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
     clearContainer(root.containerInfo);
   }
 
-  const exitStatus = renderRootSync(root, errorRetryLanes);
+  let exitStatus;
+
+  const MAX_ERROR_RETRY_ATTEMPTS = 50;
+  for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) {
+    exitStatus = renderRootSync(root, errorRetryLanes);
+    if (
+      exitStatus === RootErrored &&
+      workInProgressRootRenderPhaseUpdatedLanes !== NoLanes
+    ) {
+      // There was a render phase update during this render. This was likely a
+      // useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering
+      // again. This time, the component will use a client ID and will proceed
+      // without throwing. If multiple IDs upgrade as a result of the same
+      // update, we will have to do multiple render passes. To protect against
+      // an inifinite loop, eventually we'll give up.
+      continue;
+    }
+    break;
+  }
 
   executionContext = prevExecutionContext;
 
@@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) {
   // TODO: Lol maybe there's a better way to factor this besides this
   // obnoxiously named function :)
   suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
-  suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes);
+  suspendedLanes = removeLanes(
+    suspendedLanes,
+    workInProgressRootInterleavedUpdatedLanes,
+  );
   markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
 }
 
@@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
   workInProgressRootExitStatus = RootIncomplete;
   workInProgressRootFatalError = null;
   workInProgressRootSkippedLanes = NoLanes;
-  workInProgressRootUpdatedLanes = NoLanes;
+  workInProgressRootInterleavedUpdatedLanes = NoLanes;
+  workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
   workInProgressRootPingedLanes = NoLanes;
 
   enqueueInterleavedUpdates();
@@ -1456,7 +1486,7 @@ export function renderDidSuspendDelayIfPossible(): void {
   if (
     workInProgressRoot !== null &&
     (includesNonIdleWork(workInProgressRootSkippedLanes) ||
-      includesNonIdleWork(workInProgressRootUpdatedLanes))
+      includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes))
   ) {
     // Mark the current render as suspended so that we switch to working on
     // the updates that were skipped. Usually we only suspend at the end of

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

@@ -294,7 +294,9 @@ let workInProgressRootIncludedLanes: Lanes = NoLanes;
 // includes unprocessed updates, not work in bailed out children.
 let workInProgressRootSkippedLanes: Lanes = NoLanes;
 // Lanes that were updated (in an interleaved event) during this render.
-let workInProgressRootUpdatedLanes: Lanes = NoLanes;
+let workInProgressRootInterleavedUpdatedLanes: Lanes = NoLanes;
+// Lanes that were updated during the render phase (*not* an interleaved event).
+let workInProgressRootRenderPhaseUpdatedLanes: Lanes = NoLanes;
 // Lanes that were pinged (in an interleaved event) during this render.
 let workInProgressRootPingedLanes: Lanes = NoLanes;
 
@@ -474,6 +476,12 @@ export function scheduleUpdateOnFiber(
     // an implementation detail, like selective hydration
     // and useOpaqueIdentifier.
     warnAboutRenderPhaseUpdatesInDEV(fiber);
+
+    // Track lanes that were updated during the render phase
+    workInProgressRootRenderPhaseUpdatedLanes = mergeLanes(
+      workInProgressRootRenderPhaseUpdatedLanes,
+      lane,
+    );
   } else {
     // This is a normal update, scheduled from outside the render phase. For
     // example, during an input event.
@@ -514,8 +522,8 @@ export function scheduleUpdateOnFiber(
         deferRenderPhaseUpdateToNextBatch ||
         (executionContext & RenderContext) === NoContext
       ) {
-        workInProgressRootUpdatedLanes = mergeLanes(
-          workInProgressRootUpdatedLanes,
+        workInProgressRootInterleavedUpdatedLanes = mergeLanes(
+          workInProgressRootInterleavedUpdatedLanes,
           lane,
         );
       }
@@ -878,7 +886,25 @@ function recoverFromConcurrentError(root, errorRetryLanes) {
     clearContainer(root.containerInfo);
   }
 
-  const exitStatus = renderRootSync(root, errorRetryLanes);
+  let exitStatus;
+
+  const MAX_ERROR_RETRY_ATTEMPTS = 50;
+  for (let i = 0; i < MAX_ERROR_RETRY_ATTEMPTS; i++) {
+    exitStatus = renderRootSync(root, errorRetryLanes);
+    if (
+      exitStatus === RootErrored &&
+      workInProgressRootRenderPhaseUpdatedLanes !== NoLanes
+    ) {
+      // There was a render phase update during this render. This was likely a
+      // useOpaqueIdentifier hook upgrading itself to a client ID. Try rendering
+      // again. This time, the component will use a client ID and will proceed
+      // without throwing. If multiple IDs upgrade as a result of the same
+      // update, we will have to do multiple render passes. To protect against
+      // an inifinite loop, eventually we'll give up.
+      continue;
+    }
+    break;
+  }
 
   executionContext = prevExecutionContext;
 
@@ -1055,7 +1081,10 @@ function markRootSuspended(root, suspendedLanes) {
   // TODO: Lol maybe there's a better way to factor this besides this
   // obnoxiously named function :)
   suspendedLanes = removeLanes(suspendedLanes, workInProgressRootPingedLanes);
-  suspendedLanes = removeLanes(suspendedLanes, workInProgressRootUpdatedLanes);
+  suspendedLanes = removeLanes(
+    suspendedLanes,
+    workInProgressRootInterleavedUpdatedLanes,
+  );
   markRootSuspended_dontCallThisOneDirectly(root, suspendedLanes);
 }
 
@@ -1313,7 +1342,8 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
   workInProgressRootExitStatus = RootIncomplete;
   workInProgressRootFatalError = null;
   workInProgressRootSkippedLanes = NoLanes;
-  workInProgressRootUpdatedLanes = NoLanes;
+  workInProgressRootInterleavedUpdatedLanes = NoLanes;
+  workInProgressRootRenderPhaseUpdatedLanes = NoLanes;
   workInProgressRootPingedLanes = NoLanes;
 
   enqueueInterleavedUpdates();
@@ -1456,7 +1486,7 @@ export function renderDidSuspendDelayIfPossible(): void {
   if (
     workInProgressRoot !== null &&
     (includesNonIdleWork(workInProgressRootSkippedLanes) ||
-      includesNonIdleWork(workInProgressRootUpdatedLanes))
+      includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes))
   ) {
     // Mark the current render as suspended so that we switch to working on
     // the updates that were skipped. Usually we only suspend at the end of

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

@@ -1915,6 +1915,65 @@ describe('ReactIncrementalErrorHandling', () => {
     expect(root).toMatchRenderedOutput('Everything is fine.');
   });
 
+  it("does not infinite loop if there's a render phase update in the same render as an error", async () => {
+    // useOpaqueIdentifier uses an render phase update as an implementation
+    // detail. When an error is accompanied by a render phase update, we assume
+    // that it comes from useOpaqueIdentifier, because render phase updates
+    // triggered from userspace are not allowed (we log a warning). So we keep
+    // attempting to recover until no more opaque identifiers need to be
+    // upgraded. However, we should give up after some point to prevent an
+    // infinite loop in the case where there is (by accident) a render phase
+    // triggered from userspace.
+
+    spyOnDev(console, 'error');
+
+    let numberOfThrows = 0;
+
+    let setStateInRenderPhase;
+    function Child() {
+      const [, setState] = React.useState(0);
+      setStateInRenderPhase = setState;
+      return 'All good';
+    }
+
+    function App({shouldThrow}) {
+      if (shouldThrow) {
+        setStateInRenderPhase();
+        numberOfThrows++;
+        throw new Error('Oops!');
+      }
+      return <Child />;
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(<App shouldThrow={false} />);
+    });
+    expect(root).toMatchRenderedOutput('All good');
+
+    let error;
+    try {
+      await act(async () => {
+        root.render(<App shouldThrow={true} />);
+      });
+    } catch (e) {
+      error = e;
+    }
+
+    expect(error.message).toBe('Oops!');
+    expect(numberOfThrows < 100).toBe(true);
+
+    if (__DEV__) {
+      expect(console.error).toHaveBeenCalledTimes(2);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'Cannot update a component (`%s`) while rendering a different component',
+      );
+      expect(console.error.calls.argsFor(1)[0]).toContain(
+        'The above error occurred in the <App> component',
+      );
+    }
+  });
+
   if (global.__PERSISTENT__) {
     it('regression test: should fatal if error is thrown at the root', () => {
       const root = ReactNoop.createRoot();