Bugfix: "Captured" updates on legacy queue

packages/react-reconciler/src/ReactUpdateQueue.js

@@ -347,32 +347,84 @@ export function processUpdateQueue<State>(
     currentlyProcessingQueue = queue.shared;
   }
 
-  // The last rebase update that is NOT part of the base state.
   let baseQueue = queue.baseQueue;
 
-  // The last pending update that hasn't been processed yet.
   let pendingQueue = queue.shared.pending;
   if (pendingQueue !== null) {
-    // We have new updates that haven't been processed yet.
-    // We'll add them to the base queue.
-    if (baseQueue !== null) {
-      // Merge the pending queue and the base queue.
-      let baseFirst = baseQueue.next;
-      let pendingFirst = pendingQueue.next;
-      baseQueue.next = pendingFirst;
-      pendingQueue.next = baseFirst;
-    }
-
-    baseQueue = pendingQueue;
-
     queue.shared.pending = null;
     // TODO: Pass `current` as argument
     const current = workInProgress.alternate;
+
+    // Check if the work-in-progress queue has forked from the current queue.
+    // If so, we need to clone the pending updates and append them separately
+    // to each queue. This currently only happens for error boundaries, when
+    // there's a concurrent update during the render phase and then we do a
+    // second pass.
+    // TODO: The reason we have to clone the updates is because the base queue
+    // is circular. The reason it's circular is to save an extra field. Also,
+    // we can do fewer type checks because `next` is always non-null. Might
+    // be simpler to switch to a non-circular queue, though.
     if (current !== null) {
-      const currentQueue = current.updateQueue;
-      if (currentQueue !== null) {
+      // This is always non-null on a ClassComponent or HostRoot
+      const currentQueue: UpdateQueue<State> = (current.updateQueue: any);
+      const currentBaseQueue = currentQueue.baseQueue;
+      if (currentBaseQueue !== baseQueue) {
+        // The queues are different. First, clone the pending updates and append
+        // the clones to the work-in-progress queue.
+        const pendingFirst = pendingQueue.next;
+        let pendingUpdate = pendingFirst;
+        do {
+          const clone: Update<State> = {
+            expirationTime: pendingUpdate.expirationTime,
+            suspenseConfig: pendingUpdate.suspenseConfig,
+
+            tag: pendingUpdate.tag,
+            payload: pendingUpdate.payload,
+            callback: pendingUpdate.callback,
+
+            next: (null: any),
+          };
+          if (baseQueue === null) {
+            baseQueue = clone;
+            clone.next = clone;
+          } else {
+            clone.next = baseQueue.next;
+          }
+          baseQueue = baseQueue.next = clone;
+          pendingUpdate = pendingUpdate.next;
+        } while (pendingUpdate !== pendingFirst);
+
+        // Next, append the pending updates to the current queue. We don't need
+        // to clone this time.
+        if (currentBaseQueue !== null) {
+          let currentBaseFirst = currentBaseQueue.next;
+          currentBaseQueue.next = pendingFirst;
+          pendingQueue.next = currentBaseFirst;
+        }
+        currentQueue.baseQueue = pendingQueue;
+      } else {
+        // The work-in-progress queue is the same as the current queue. We can
+        // take advantage of structural sharing.
+        if (baseQueue !== null) {
+          // Merge the pending queue and the base queue.
+          const baseFirst = baseQueue.next;
+          const pendingFirst = pendingQueue.next;
+          baseQueue.next = pendingFirst;
+          pendingQueue.next = baseFirst;
+        }
         currentQueue.baseQueue = pendingQueue;
+        baseQueue = pendingQueue;
+      }
+    } else {
+      // There is no current queue.
+      if (baseQueue !== null) {
+        // Merge the pending queue and the base queue.
+        const baseFirst = baseQueue.next;
+        const pendingFirst = pendingQueue.next;
+        baseQueue.next = pendingFirst;
+        pendingQueue.next = baseFirst;
       }
+      baseQueue = pendingQueue;
     }
   }
 
@@ -471,6 +523,10 @@ export function processUpdateQueue<State>(
           } else {
             // An update was scheduled from inside a reducer. Add the new
             // pending updates to the end of the list and keep processing.
+            // NOTE: `baseQueue` might be shared with current. This is not
+            // concurrent-safe; however, we also don't officially support
+            // calling setState from inside a reducer. In DEV, we double invoke
+            // them. We may want to consider warning, too.
             update = baseQueue.next = pendingQueue.next;
             pendingQueue.next = first;
             queue.baseQueue = baseQueue = pendingQueue;

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

@@ -1729,6 +1729,31 @@ describe('ReactIncrementalErrorHandling', () => {
     ]);
   });
 
+  it('uncaught errors should be discarded if the render is aborted', async () => {
+    const root = ReactNoop.createRoot();
+
+    function Oops() {
+      Scheduler.unstable_yieldValue('Oops');
+      throw Error('Oops');
+    }
+
+    await ReactNoop.act(async () => {
+      ReactNoop.discreteUpdates(() => {
+        root.render(<Oops />);
+      });
+      // Render past the component that throws, then yield.
+      expect(Scheduler).toFlushAndYieldThrough(['Oops']);
+      expect(root).toMatchRenderedOutput(null);
+      // Interleaved update. When the root completes, instead of throwing the
+      // error, it should try rendering again. This update will cause it to
+      // recover gracefully.
+      root.render('Everything is fine.');
+    });
+
+    // Should finish without throwing.
+    expect(root).toMatchRenderedOutput('Everything is fine.');
+  });
+
   if (global.__PERSISTENT__) {
     it('regression test: should fatal if error is thrown at the root', () => {
       const root = ReactNoop.createRoot();