Bugfix: "Captured" updates on legacy queue

acdlite · acdlite · commit 287932cc1e7e · 2020-03-10T00:13:02.000-07:00
This fixes a bug with error boundaries. Error boundaries have a notion
of "captured" updates that represent errors that are thrown in its
subtree during the render phase. These updates are meant to be dropped
if the render is aborted.

The bug happens when there's a concurrent update (an update from an
interleaved event) in between when the error is thrown and when the
error boundary does its second pass. The concurrent update is
transferred from the pending queue onto the base queue. Usually, at this
point the base queue is the same as the current queue. So when we
append the pending updates to the work-in-progress queue, it also
appends to the current queue.

However, in the case of an error boundary's second pass, the base queue
has already forked from the current queue; it includes both the
"captured" updates and any concurrent updates. In that case, what we
need to do is append separately to both queues. Which we weren't doing.

That isn't the full story, though. You would expect that this mistake
would manifest as dropping the interleaved updates. But instead what
was happening is that the "captured" updates, the ones that are meant
to be dropped if the render is aborted, were being added to the
current queue.

The reason is that the `baseQueue` structure is a circular linked list.
The motivation for this was to save memory; instead of separate `first`
and `last` pointers, you only need to point to `last`.

But this approach does not work with structural sharing. So what was
happening is that the captured updates were accidentally being added
to the current queue because of the circular link.

To fix this, I changed the `baseQueue` from a circular linked list to a
singly-linked list so that we can take advantage of structural sharing.

The "pending" queue, however, remains a circular list because it doesn't
need to be persistent.

This bug also affects the root fiber, which uses the same update queue
implementation and also acts like an error boundary.

It does not affect the hook update queue because they do not have any
notion of "captured" updates. So I've left it alone for now. However,
when we implement resuming, we will have to account for the same issue.
diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -984,19 +984,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
 
       function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) {
         log('  '.repeat(depth + 1) + 'QUEUED UPDATES');
-        const last = updateQueue.baseQueue;
+        const first = updateQueue.firstBaseUpdate;
+        const last = updateQueue.lastBaseUpdate;
         if (last === null) {
           return;
         }
-        const first = last.next;
         let update = first;
         if (update !== null) {
           do {
             log(
               '  '.repeat(depth + 1) + '~',
               '[' + update.expirationTime + ']',
             );
-          } while (update !== null && update !== first);
+          } while (update !== null);
         }
 
         const lastPending = updateQueue.shared.pending;
diff --git a/packages/react-reconciler/src/ReactUpdateQueue.js b/packages/react-reconciler/src/ReactUpdateQueue.js
@@ -115,7 +115,7 @@ export type Update<State> = {|
   payload: any,
   callback: (() => mixed) | null,
 
-  next: Update<State>,
+  next: Update<State> | null,
 
   // DEV only
   priority?: ReactPriorityLevel,
@@ -125,7 +125,8 @@ type SharedQueue<State> = {|pending: Update<State> | null|};
 
 export type UpdateQueue<State> = {|
   baseState: State,
-  baseQueue: Update<State> | null,
+  firstBaseUpdate: Update<State> | null,
+  lastBaseUpdate: Update<State> | null,
   shared: SharedQueue<State>,
   effects: Array<Update<State>> | null,
 |};
@@ -154,7 +155,8 @@ if (__DEV__) {
 export function initializeUpdateQueue<State>(fiber: Fiber): void {
   const queue: UpdateQueue<State> = {
     baseState: fiber.memoizedState,
-    baseQueue: null,
+    firstBaseUpdate: null,
+    lastBaseUpdate: null,
     shared: {
       pending: null,
     },
@@ -173,7 +175,8 @@ export function cloneUpdateQueue<State>(
   if (queue === currentQueue) {
     const clone: UpdateQueue<State> = {
       baseState: currentQueue.baseState,
-      baseQueue: currentQueue.baseQueue,
+      firstBaseUpdate: currentQueue.firstBaseUpdate,
+      lastBaseUpdate: currentQueue.lastBaseUpdate,
       shared: currentQueue.shared,
       effects: currentQueue.effects,
     };
@@ -193,9 +196,8 @@ export function createUpdate(
     payload: null,
     callback: null,
 
-    next: (null: any),
+    next: null,
   };
-  update.next = update;
   if (__DEV__) {
     update.priority = getCurrentPriorityLevel();
   }
@@ -249,14 +251,13 @@ export function enqueueCapturedUpdate<State>(
   // Captured updates go only on the work-in-progress queue.
   const queue: UpdateQueue<State> = (workInProgress.updateQueue: any);
   // Append the update to the end of the list.
-  const last = queue.baseQueue;
+  const last = queue.lastBaseUpdate;
   if (last === null) {
-    queue.baseQueue = update.next = update;
-    update.next = update;
+    queue.firstBaseUpdate = update;
   } else {
-    update.next = last.next;
     last.next = update;
   }
+  queue.lastBaseUpdate = update;
 }
 
 function getStateFromUpdate<State>(
@@ -347,147 +348,160 @@ export function processUpdateQueue<State>(
     currentlyProcessingQueue = queue.shared;
   }
 
-  // The last rebase update that is NOT part of the base state.
-  let baseQueue = queue.baseQueue;
+  let firstBaseUpdate = queue.firstBaseUpdate;
+  let lastBaseUpdate = queue.lastBaseUpdate;
 
-  // The last pending update that hasn't been processed yet.
+  // Check if there are pending updates. If so, transfer them to the base queue.
   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;
-    }
+    queue.shared.pending = null;
 
-    baseQueue = pendingQueue;
+    // The pending queue is circular. Disconnect the pointer between first
+    // and last so that it's non-circular.
+    const lastPendingUpdate = pendingQueue;
+    const firstPendingUpdate = lastPendingUpdate.next;
+    lastPendingUpdate.next = null;
+    // Append pending updates to base queue
+    if (lastBaseUpdate === null) {
+      firstBaseUpdate = firstPendingUpdate;
+    } else {
+      lastBaseUpdate.next = firstPendingUpdate;
+    }
+    lastBaseUpdate = lastPendingUpdate;
 
-    queue.shared.pending = null;
+    // If there's a current queue, and it's different from the base queue, then
+    // we need to transfer the updates to that queue, too. Because the base
+    // queue is a singly-linked list with no cycles, we can append to both
+    // lists and take advantage of structural sharing.
     // TODO: Pass `current` as argument
     const current = workInProgress.alternate;
     if (current !== null) {
-      const currentQueue = current.updateQueue;
-      if (currentQueue !== null) {
-        currentQueue.baseQueue = pendingQueue;
+      // This is always non-null on a ClassComponent or HostRoot
+      const currentQueue: UpdateQueue<State> = (current.updateQueue: any);
+      const currentLastBaseUpdate = currentQueue.lastBaseUpdate;
+      if (currentLastBaseUpdate !== lastBaseUpdate) {
+        if (currentLastBaseUpdate === null) {
+          currentQueue.firstBaseUpdate = firstPendingUpdate;
+        } else {
+          currentLastBaseUpdate.next = firstPendingUpdate;
+        }
+        currentQueue.lastBaseUpdate = lastPendingUpdate;
       }
     }
   }
 
   // These values may change as we process the queue.
-  if (baseQueue !== null) {
-    let first = baseQueue.next;
+  if (firstBaseUpdate !== null) {
     // Iterate through the list of updates to compute the result.
     let newState = queue.baseState;
     let newExpirationTime = NoWork;
 
     let newBaseState = null;
-    let newBaseQueueFirst = null;
-    let newBaseQueueLast = null;
-
-    if (first !== null) {
-      let update = first;
-      do {
-        const updateExpirationTime = update.expirationTime;
-        if (updateExpirationTime < renderExpirationTime) {
-          // Priority is insufficient. Skip this update. If this is the first
-          // skipped update, the previous update/state is the new base
-          // update/state.
+    let newFirstBaseUpdate = null;
+    let newLastBaseUpdate = null;
+
+    let update = firstBaseUpdate;
+    do {
+      const updateExpirationTime = update.expirationTime;
+      if (updateExpirationTime < renderExpirationTime) {
+        // Priority is insufficient. Skip this update. If this is the first
+        // skipped update, the previous update/state is the new base
+        // update/state.
+        const clone: Update<State> = {
+          expirationTime: update.expirationTime,
+          suspenseConfig: update.suspenseConfig,
+
+          tag: update.tag,
+          payload: update.payload,
+          callback: update.callback,
+
+          next: null,
+        };
+        if (newLastBaseUpdate === null) {
+          newFirstBaseUpdate = newLastBaseUpdate = clone;
+          newBaseState = newState;
+        } else {
+          newLastBaseUpdate = newLastBaseUpdate.next = clone;
+        }
+        // Update the remaining priority in the queue.
+        if (updateExpirationTime > newExpirationTime) {
+          newExpirationTime = updateExpirationTime;
+        }
+      } else {
+        // This update does have sufficient priority.
+
+        if (newLastBaseUpdate !== null) {
           const clone: Update<State> = {
-            expirationTime: update.expirationTime,
+            expirationTime: Sync, // This update is going to be committed so we never want uncommit it.
             suspenseConfig: update.suspenseConfig,
 
             tag: update.tag,
             payload: update.payload,
             callback: update.callback,
 
-            next: (null: any),
+            next: null,
           };
-          if (newBaseQueueLast === null) {
-            newBaseQueueFirst = newBaseQueueLast = clone;
-            newBaseState = newState;
-          } else {
-            newBaseQueueLast = newBaseQueueLast.next = clone;
-          }
-          // Update the remaining priority in the queue.
-          if (updateExpirationTime > newExpirationTime) {
-            newExpirationTime = updateExpirationTime;
-          }
-        } else {
-          // This update does have sufficient priority.
-
-          if (newBaseQueueLast !== null) {
-            const clone: Update<State> = {
-              expirationTime: Sync, // This update is going to be committed so we never want uncommit it.
-              suspenseConfig: update.suspenseConfig,
-
-              tag: update.tag,
-              payload: update.payload,
-              callback: update.callback,
-
-              next: (null: any),
-            };
-            newBaseQueueLast = newBaseQueueLast.next = clone;
-          }
-
-          // Mark the event time of this update as relevant to this render pass.
-          // TODO: This should ideally use the true event time of this update rather than
-          // its priority which is a derived and not reverseable value.
-          // TODO: We should skip this update if it was already committed but currently
-          // we have no way of detecting the difference between a committed and suspended
-          // update here.
-          markRenderEventTimeAndConfig(
-            updateExpirationTime,
-            update.suspenseConfig,
-          );
-
-          // Process this update.
-          newState = getStateFromUpdate(
-            workInProgress,
-            queue,
-            update,
-            newState,
-            props,
-            instance,
-          );
-          const callback = update.callback;
-          if (callback !== null) {
-            workInProgress.effectTag |= Callback;
-            let effects = queue.effects;
-            if (effects === null) {
-              queue.effects = [update];
-            } else {
-              effects.push(update);
-            }
-          }
+          newLastBaseUpdate = newLastBaseUpdate.next = clone;
         }
-        update = update.next;
-        if (update === null || update === first) {
-          pendingQueue = queue.shared.pending;
-          if (pendingQueue === null) {
-            break;
+
+        // Mark the event time of this update as relevant to this render pass.
+        // TODO: This should ideally use the true event time of this update rather than
+        // its priority which is a derived and not reverseable value.
+        // TODO: We should skip this update if it was already committed but currently
+        // we have no way of detecting the difference between a committed and suspended
+        // update here.
+        markRenderEventTimeAndConfig(
+          updateExpirationTime,
+          update.suspenseConfig,
+        );
+
+        // Process this update.
+        newState = getStateFromUpdate(
+          workInProgress,
+          queue,
+          update,
+          newState,
+          props,
+          instance,
+        );
+        const callback = update.callback;
+        if (callback !== null) {
+          workInProgress.effectTag |= Callback;
+          let effects = queue.effects;
+          if (effects === null) {
+            queue.effects = [update];
           } else {
-            // An update was scheduled from inside a reducer. Add the new
-            // pending updates to the end of the list and keep processing.
-            update = baseQueue.next = pendingQueue.next;
-            pendingQueue.next = first;
-            queue.baseQueue = baseQueue = pendingQueue;
-            queue.shared.pending = null;
+            effects.push(update);
           }
         }
-      } while (true);
-    }
+      }
+      update = update.next;
+      if (update === null) {
+        pendingQueue = queue.shared.pending;
+        if (pendingQueue === null) {
+          break;
+        } else {
+          // An update was scheduled from inside a reducer. Add the new
+          // pending updates to the end of the list and keep processing.
+          const lastPendingUpdate = pendingQueue;
+          // Intentionally unsound. Pending updates form a circular list, but we
+          // unravel them when transferring them to the base queue.
+          const firstPendingUpdate = ((lastPendingUpdate.next: any): Update<State>);
+          lastPendingUpdate.next = null;
+          update = firstPendingUpdate;
+          queue.lastBaseUpdate = lastPendingUpdate;
+          queue.shared.pending = null;
+        }
+      }
+    } while (true);
 
-    if (newBaseQueueLast === null) {
+    if (newLastBaseUpdate === null) {
       newBaseState = newState;
-    } else {
-      newBaseQueueLast.next = (newBaseQueueFirst: any);
     }
 
     queue.baseState = ((newBaseState: any): State);
-    queue.baseQueue = newBaseQueueLast;
+    queue.firstBaseUpdate = newFirstBaseUpdate;
+    queue.lastBaseUpdate = newLastBaseUpdate;
 
     // Set the remaining expiration time to be whatever is remaining in the queue.
     // This should be fine because the only two other things that contribute to
diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/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();
