From 4bcc6c5a0208234c2b7312e81b1628636f7621c0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 10 Mar 2020 00:13:02 -0700 Subject: [PATCH] Bugfix: "Captured" updates on legacy queue 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. --- .../src/createReactNoop.js | 6 +- .../react-reconciler/src/ReactUpdateQueue.js | 246 +++++++++--------- ...tIncrementalErrorHandling-test.internal.js | 25 ++ 3 files changed, 158 insertions(+), 119 deletions(-) diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 7233f7976409a..461c8a6bd0be6 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -984,11 +984,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { function logUpdateQueue(updateQueue: UpdateQueue, depth) { log(' '.repeat(depth + 1) + 'QUEUED UPDATES'); - const last = updateQueue.baseQueue; + const first = updateQueue.lastBaseUpdate; + const last = updateQueue.lastBaseUpdate; if (last === null) { return; } - const first = last.next; let update = first; if (update !== null) { do { @@ -996,7 +996,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { ' '.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 index 479f92dac9bbd..d8965347259db 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.js @@ -115,7 +115,7 @@ export type Update = {| payload: any, callback: (() => mixed) | null, - next: Update, + next: Update | null, // DEV only priority?: ReactPriorityLevel, @@ -125,7 +125,8 @@ type SharedQueue = {|pending: Update | null|}; export type UpdateQueue = {| baseState: State, - baseQueue: Update | null, + firstBaseUpdate: Update | null, + lastBaseUpdate: Update | null, shared: SharedQueue, effects: Array> | null, |}; @@ -154,7 +155,8 @@ if (__DEV__) { export function initializeUpdateQueue(fiber: Fiber): void { const queue: UpdateQueue = { baseState: fiber.memoizedState, - baseQueue: null, + firstBaseUpdate: null, + lastBaseUpdate: null, shared: { pending: null, }, @@ -173,7 +175,8 @@ export function cloneUpdateQueue( if (queue === currentQueue) { const clone: UpdateQueue = { 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( // Captured updates go only on the work-in-progress queue. const queue: UpdateQueue = (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( @@ -347,147 +348,160 @@ export function processUpdateQueue( 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 = (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 = { + 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 = { - 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 = { - 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); + 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 index 795488d38d19b..a94fe3c857c49 100644 --- 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(); + }); + // 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();