Bugfix: "Captured" updates on legacy queue (#18265)
* 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. * Ensure base queue is a clone When an error boundary captures an error, we append the error update to the work-in-progress queue only so that if the render is aborted, the error update is dropped. Before appending to the queue, we need to make sure the queue is a work-in-progress copy. Usually we clone the queue during `processUpdateQueue`; however, if the base queue has lower priority than the current render, we may have bailed out on the boundary fiber without ever entering `processUpdateQueue`. So we need to lazily clone the queue. * Add warning to protect against refactor hazard The hook queue does not have resuming or "captured" updates, but if we ever add them in the future, we'll need to make sure we check if the queue is forked before transfering the pending updates to them.
Andrew Clark committed
Mar 11, 2020 at 11:53 UTC
8fe066fdacff554d1c623c71472cf6ecc1cd8e08
4 files changed
+276
-129
packages/react-noop-renderer/src/createReactNoop.js
+2
-6
@@ -984,11 +984,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
984
985
function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) {
986
log(' '.repeat(depth + 1) + 'QUEUED UPDATES');
987
- const last = updateQueue.baseQueue;
988
- if (last === null) {
989
- return;
990
- }
991
- const first = last.next;
987
+ const first = updateQueue.firstBaseUpdate;
988
let update = first;
989
if (update !== null) {
990
do {
@@ -996,7 +992,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
992
' '.repeat(depth + 1) + '~',
993
'[' + update.expirationTime + ']',
994
);
999
- } while (update !== null && update !== first);
995
+ } while (update !== null);
996
}
997
998
const lastPending = updateQueue.shared.pending;
packages/react-reconciler/src/ReactFiberHooks.js
+10
@@ -691,6 +691,16 @@ function updateReducer<S, I, A>(
691
baseQueue.next = pendingFirst;
692
pendingQueue.next = baseFirst;
693
}
694
+ if (__DEV__) {
695
+ if (current.baseQueue !== baseQueue) {
696
+ // Internal invariant that should never happen, but feasibly could in
697
+ // the future if we implement resuming, or some form of that.
698
+ console.error(
699
+ 'Internal error: Expected work-in-progress queue to be a clone. ' +
700
+ 'This is a bug in React.',
701
+ );
702
+ }
703
+ }
704
current.baseQueue = baseQueue = pendingQueue;
705
queue.pending = null;
706
}
packages/react-reconciler/src/ReactUpdateQueue.js
+194
-123
@@ -115,7 +115,7 @@ export type Update<State> = {|
115
payload: any,
116
callback: (() => mixed) | null,
117
118
- next: Update<State>,
118
+ next: Update<State> | null,
119
120
// DEV only
121
priority?: ReactPriorityLevel,
@@ -125,7 +125,8 @@ type SharedQueue<State> = {|pending: Update<State> | null|};
125
126
export type UpdateQueue<State> = {|
127
baseState: State,
128
- baseQueue: Update<State> | null,
128
+ firstBaseUpdate: Update<State> | null,
129
+ lastBaseUpdate: Update<State> | null,
130
shared: SharedQueue<State>,
131
effects: Array<Update<State>> | null,
132
|};
@@ -154,7 +155,8 @@ if (__DEV__) {
155
export function initializeUpdateQueue<State>(fiber: Fiber): void {
156
const queue: UpdateQueue<State> = {
157
baseState: fiber.memoizedState,
157
- baseQueue: null,
158
+ firstBaseUpdate: null,
159
+ lastBaseUpdate: null,
160
shared: {
161
pending: null,
162
},
@@ -173,7 +175,8 @@ export function cloneUpdateQueue<State>(
175
if (queue === currentQueue) {
176
const clone: UpdateQueue<State> = {
177
baseState: currentQueue.baseState,
176
- baseQueue: currentQueue.baseQueue,
178
+ firstBaseUpdate: currentQueue.firstBaseUpdate,
179
+ lastBaseUpdate: currentQueue.lastBaseUpdate,
180
shared: currentQueue.shared,
181
effects: currentQueue.effects,
182
};
@@ -193,9 +196,8 @@ export function createUpdate(
196
payload: null,
197
callback: null,
198
196
- next: (null: any),
199
+ next: null,
200
};
198
- update.next = update;
201
if (__DEV__) {
202
update.priority = getCurrentPriorityLevel();
203
}
@@ -238,25 +240,81 @@ export function enqueueUpdate<State>(fiber: Fiber, update: Update<State>) {
240
241
export function enqueueCapturedUpdate<State>(
242
workInProgress: Fiber,
241
- update: Update<State>,
243
+ capturedUpdate: Update<State>,
244
) {
245
+ // Captured updates are updates that are thrown by a child during the render
246
+ // phase. They should be discarded if the render is aborted. Therefore,
247
+ // we should only put them on the work-in-progress queue, not the current one.
248
+ let queue: UpdateQueue<State> = (workInProgress.updateQueue: any);
249
+
250
+ // Check if the work-in-progress queue is a clone.
251
const current = workInProgress.alternate;
252
if (current !== null) {
245
- // Ensure the work-in-progress queue is a clone
246
- cloneUpdateQueue(current, workInProgress);
253
+ const currentQueue: UpdateQueue<State> = (current.updateQueue: any);
254
+ if (queue === currentQueue) {
255
+ // The work-in-progress queue is the same as current. This happens when
256
+ // we bail out on a parent fiber that then captures an error thrown by
257
+ // a child. Since we want to append the update only to the work-in
258
+ // -progress queue, we need to clone the updates. We usually clone during
259
+ // processUpdateQueue, but that didn't happen in this case because we
260
+ // skipped over the parent when we bailed out.
261
+ let newFirst = null;
262
+ let newLast = null;
263
+ const firstBaseUpdate = queue.firstBaseUpdate;
264
+ if (firstBaseUpdate !== null) {
265
+ // Loop through the updates and clone them.
266
+ let update = firstBaseUpdate;
267
+ do {
268
+ const clone: Update<State> = {
269
+ expirationTime: update.expirationTime,
270
+ suspenseConfig: update.suspenseConfig,
271
+
272
+ tag: update.tag,
273
+ payload: update.payload,
274
+ callback: update.callback,
275
+
276
+ next: null,
277
+ };
278
+ if (newLast === null) {
279
+ newFirst = newLast = clone;
280
+ } else {
281
+ newLast.next = clone;
282
+ newLast = clone;
283
+ }
284
+ update = update.next;
285
+ } while (update !== null);
286
+
287
+ // Append the captured update the end of the cloned list.
288
+ if (newLast === null) {
289
+ newFirst = newLast = capturedUpdate;
290
+ } else {
291
+ newLast.next = capturedUpdate;
292
+ newLast = capturedUpdate;
293
+ }
294
+ } else {
295
+ // There are no base updates.
296
+ newFirst = newLast = capturedUpdate;
297
+ }
298
+ queue = {
299
+ baseState: currentQueue.baseState,
300
+ firstBaseUpdate: newFirst,
301
+ lastBaseUpdate: newLast,
302
+ shared: currentQueue.shared,
303
+ effects: currentQueue.effects,
304
+ };
305
+ workInProgress.updateQueue = queue;
306
+ return;
307
+ }
308
}
309
249
- // Captured updates go only on the work-in-progress queue.
250
- const queue: UpdateQueue<State> = (workInProgress.updateQueue: any);
310
// Append the update to the end of the list.
252
- const last = queue.baseQueue;
253
- if (last === null) {
254
- queue.baseQueue = update.next = update;
255
- update.next = update;
311
+ const lastBaseUpdate = queue.lastBaseUpdate;
312
+ if (lastBaseUpdate === null) {
313
+ queue.firstBaseUpdate = capturedUpdate;
314
} else {
257
- update.next = last.next;
258
- last.next = update;
315
+ lastBaseUpdate.next = capturedUpdate;
316
}
317
+ queue.lastBaseUpdate = capturedUpdate;
318
}
319
320
function getStateFromUpdate<State>(
@@ -347,147 +405,160 @@ export function processUpdateQueue<State>(
405
currentlyProcessingQueue = queue.shared;
406
}
407
350
- // The last rebase update that is NOT part of the base state.
351
- let baseQueue = queue.baseQueue;
408
+ let firstBaseUpdate = queue.firstBaseUpdate;
409
+ let lastBaseUpdate = queue.lastBaseUpdate;
410
353
- // The last pending update that hasn't been processed yet.
411
+ // Check if there are pending updates. If so, transfer them to the base queue.
412
let pendingQueue = queue.shared.pending;
413
if (pendingQueue !== null) {
356
- // We have new updates that haven't been processed yet.
357
- // We'll add them to the base queue.
358
- if (baseQueue !== null) {
359
- // Merge the pending queue and the base queue.
360
- let baseFirst = baseQueue.next;
361
- let pendingFirst = pendingQueue.next;
362
- baseQueue.next = pendingFirst;
363
- pendingQueue.next = baseFirst;
364
- }
414
+ queue.shared.pending = null;
415
366
- baseQueue = pendingQueue;
416
+ // The pending queue is circular. Disconnect the pointer between first
417
+ // and last so that it's non-circular.
418
+ const lastPendingUpdate = pendingQueue;
419
+ const firstPendingUpdate = lastPendingUpdate.next;
420
+ lastPendingUpdate.next = null;
421
+ // Append pending updates to base queue
422
+ if (lastBaseUpdate === null) {
423
+ firstBaseUpdate = firstPendingUpdate;
424
+ } else {
425
+ lastBaseUpdate.next = firstPendingUpdate;
426
+ }
427
+ lastBaseUpdate = lastPendingUpdate;
428
368
- queue.shared.pending = null;
429
+ // If there's a current queue, and it's different from the base queue, then
430
+ // we need to transfer the updates to that queue, too. Because the base
431
+ // queue is a singly-linked list with no cycles, we can append to both
432
+ // lists and take advantage of structural sharing.
433
// TODO: Pass `current` as argument
434
const current = workInProgress.alternate;
435
if (current !== null) {
372
- const currentQueue = current.updateQueue;
373
- if (currentQueue !== null) {
374
- currentQueue.baseQueue = pendingQueue;
436
+ // This is always non-null on a ClassComponent or HostRoot
437
+ const currentQueue: UpdateQueue<State> = (current.updateQueue: any);
438
+ const currentLastBaseUpdate = currentQueue.lastBaseUpdate;
439
+ if (currentLastBaseUpdate !== lastBaseUpdate) {
440
+ if (currentLastBaseUpdate === null) {
441
+ currentQueue.firstBaseUpdate = firstPendingUpdate;
442
+ } else {
443
+ currentLastBaseUpdate.next = firstPendingUpdate;
444
+ }
445
+ currentQueue.lastBaseUpdate = lastPendingUpdate;
446
}
447
}
448
}
449
450
// These values may change as we process the queue.
380
- if (baseQueue !== null) {
381
- let first = baseQueue.next;
451
+ if (firstBaseUpdate !== null) {
452
// Iterate through the list of updates to compute the result.
453
let newState = queue.baseState;
454
let newExpirationTime = NoWork;
455
456
let newBaseState = null;
387
- let newBaseQueueFirst = null;
388
- let newBaseQueueLast = null;
389
-
390
- if (first !== null) {
391
- let update = first;
392
- do {
393
- const updateExpirationTime = update.expirationTime;
394
- if (updateExpirationTime < renderExpirationTime) {
395
- // Priority is insufficient. Skip this update. If this is the first
396
- // skipped update, the previous update/state is the new base
397
- // update/state.
457
+ let newFirstBaseUpdate = null;
458
+ let newLastBaseUpdate = null;
459
+
460
+ let update = firstBaseUpdate;
461
+ do {
462
+ const updateExpirationTime = update.expirationTime;
463
+ if (updateExpirationTime < renderExpirationTime) {
464
+ // Priority is insufficient. Skip this update. If this is the first
465
+ // skipped update, the previous update/state is the new base
466
+ // update/state.
467
+ const clone: Update<State> = {
468
+ expirationTime: update.expirationTime,
469
+ suspenseConfig: update.suspenseConfig,
470
+
471
+ tag: update.tag,
472
+ payload: update.payload,
473
+ callback: update.callback,
474
+
475
+ next: null,
476
+ };
477
+ if (newLastBaseUpdate === null) {
478
+ newFirstBaseUpdate = newLastBaseUpdate = clone;
479
+ newBaseState = newState;
480
+ } else {
481
+ newLastBaseUpdate = newLastBaseUpdate.next = clone;
482
+ }
483
+ // Update the remaining priority in the queue.
484
+ if (updateExpirationTime > newExpirationTime) {
485
+ newExpirationTime = updateExpirationTime;
486
+ }
487
+ } else {
488
+ // This update does have sufficient priority.
489
+
490
+ if (newLastBaseUpdate !== null) {
491
const clone: Update<State> = {
399
- expirationTime: update.expirationTime,
492
+ expirationTime: Sync, // This update is going to be committed so we never want uncommit it.
493
suspenseConfig: update.suspenseConfig,
494
495
tag: update.tag,
496
payload: update.payload,
497
callback: update.callback,
498
406
- next: (null: any),
499
+ next: null,
500
};
408
- if (newBaseQueueLast === null) {
409
- newBaseQueueFirst = newBaseQueueLast = clone;
410
- newBaseState = newState;
411
- } else {
412
- newBaseQueueLast = newBaseQueueLast.next = clone;
413
- }
414
- // Update the remaining priority in the queue.
415
- if (updateExpirationTime > newExpirationTime) {
416
- newExpirationTime = updateExpirationTime;
417
- }
418
- } else {
419
- // This update does have sufficient priority.
420
-
421
- if (newBaseQueueLast !== null) {
422
- const clone: Update<State> = {
423
- expirationTime: Sync, // This update is going to be committed so we never want uncommit it.
424
- suspenseConfig: update.suspenseConfig,
425
-
426
- tag: update.tag,
427
- payload: update.payload,
428
- callback: update.callback,
429
-
430
- next: (null: any),
431
- };
432
- newBaseQueueLast = newBaseQueueLast.next = clone;
433
- }
434
-
435
- // Mark the event time of this update as relevant to this render pass.
436
- // TODO: This should ideally use the true event time of this update rather than
437
- // its priority which is a derived and not reverseable value.
438
- // TODO: We should skip this update if it was already committed but currently
439
- // we have no way of detecting the difference between a committed and suspended
440
- // update here.
441
- markRenderEventTimeAndConfig(
442
- updateExpirationTime,
443
- update.suspenseConfig,
444
- );
445
-
446
- // Process this update.
447
- newState = getStateFromUpdate(
448
- workInProgress,
449
- queue,
450
- update,
451
- newState,
452
- props,
453
- instance,
454
- );
455
- const callback = update.callback;
456
- if (callback !== null) {
457
- workInProgress.effectTag |= Callback;
458
- let effects = queue.effects;
459
- if (effects === null) {
460
- queue.effects = [update];
461
- } else {
462
- effects.push(update);
463
- }
464
- }
501
+ newLastBaseUpdate = newLastBaseUpdate.next = clone;
502
}
466
- update = update.next;
467
- if (update === null || update === first) {
468
- pendingQueue = queue.shared.pending;
469
- if (pendingQueue === null) {
470
- break;
503
+
504
+ // Mark the event time of this update as relevant to this render pass.
505
+ // TODO: This should ideally use the true event time of this update rather than
506
+ // its priority which is a derived and not reverseable value.
507
+ // TODO: We should skip this update if it was already committed but currently
508
+ // we have no way of detecting the difference between a committed and suspended
509
+ // update here.
510
+ markRenderEventTimeAndConfig(
511
+ updateExpirationTime,
512
+ update.suspenseConfig,
513
+ );
514
+
515
+ // Process this update.
516
+ newState = getStateFromUpdate(
517
+ workInProgress,
518
+ queue,
519
+ update,
520
+ newState,
521
+ props,
522
+ instance,
523
+ );
524
+ const callback = update.callback;
525
+ if (callback !== null) {
526
+ workInProgress.effectTag |= Callback;
527
+ let effects = queue.effects;
528
+ if (effects === null) {
529
+ queue.effects = [update];
530
} else {
472
- // An update was scheduled from inside a reducer. Add the new
473
- // pending updates to the end of the list and keep processing.
474
- update = baseQueue.next = pendingQueue.next;
475
- pendingQueue.next = first;
476
- queue.baseQueue = baseQueue = pendingQueue;
477
- queue.shared.pending = null;
531
+ effects.push(update);
532
}
533
}
480
- } while (true);
481
- }
534
+ }
535
+ update = update.next;
536
+ if (update === null) {
537
+ pendingQueue = queue.shared.pending;
538
+ if (pendingQueue === null) {
539
+ break;
540
+ } else {
541
+ // An update was scheduled from inside a reducer. Add the new
542
+ // pending updates to the end of the list and keep processing.
543
+ const lastPendingUpdate = pendingQueue;
544
+ // Intentionally unsound. Pending updates form a circular list, but we
545
+ // unravel them when transferring them to the base queue.
546
+ const firstPendingUpdate = ((lastPendingUpdate.next: any): Update<State>);
547
+ lastPendingUpdate.next = null;
548
+ update = firstPendingUpdate;
549
+ queue.lastBaseUpdate = lastPendingUpdate;
550
+ queue.shared.pending = null;
551
+ }
552
+ }
553
+ } while (true);
554
483
- if (newBaseQueueLast === null) {
555
+ if (newLastBaseUpdate === null) {
556
newBaseState = newState;
485
- } else {
486
- newBaseQueueLast.next = (newBaseQueueFirst: any);
557
}
558
559
queue.baseState = ((newBaseState: any): State);
490
- queue.baseQueue = newBaseQueueLast;
560
+ queue.firstBaseUpdate = newFirstBaseUpdate;
561
+ queue.lastBaseUpdate = newLastBaseUpdate;
562
563
// Set the remaining expiration time to be whatever is remaining in the queue.
564
// This should be fine because the only two other things that contribute to
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
+70
@@ -1729,6 +1729,76 @@ describe('ReactIncrementalErrorHandling', () => {
1729
]);
1730
});
1731
1732
+ it('uncaught errors should be discarded if the render is aborted', async () => {
1733
+ const root = ReactNoop.createRoot();
1734
+
1735
+ function Oops() {
1736
+ Scheduler.unstable_yieldValue('Oops');
1737
+ throw Error('Oops');
1738
+ }
1739
+
1740
+ await ReactNoop.act(async () => {
1741
+ ReactNoop.discreteUpdates(() => {
1742
+ root.render(<Oops />);
1743
+ });
1744
+ // Render past the component that throws, then yield.
1745
+ expect(Scheduler).toFlushAndYieldThrough(['Oops']);
1746
+ expect(root).toMatchRenderedOutput(null);
1747
+ // Interleaved update. When the root completes, instead of throwing the
1748
+ // error, it should try rendering again. This update will cause it to
1749
+ // recover gracefully.
1750
+ root.render('Everything is fine.');
1751
+ });
1752
+
1753
+ // Should finish without throwing.
1754
+ expect(root).toMatchRenderedOutput('Everything is fine.');
1755
+ });
1756
+
1757
+ it('uncaught errors are discarded if the render is aborted, case 2', async () => {
1758
+ const {useState} = React;
1759
+ const root = ReactNoop.createRoot();
1760
+
1761
+ let setShouldThrow;
1762
+ function Oops() {
1763
+ const [shouldThrow, _setShouldThrow] = useState(false);
1764
+ setShouldThrow = _setShouldThrow;
1765
+ if (shouldThrow) {
1766
+ throw Error('Oops');
1767
+ }
1768
+ return null;
1769
+ }
1770
+
1771
+ function AllGood() {
1772
+ Scheduler.unstable_yieldValue('Everything is fine.');
1773
+ return 'Everything is fine.';
1774
+ }
1775
+
1776
+ await ReactNoop.act(async () => {
1777
+ root.render(<Oops />);
1778
+ });
1779
+
1780
+ await ReactNoop.act(async () => {
1781
+ // Schedule a high pri and a low pri update on the root.
1782
+ ReactNoop.discreteUpdates(() => {
1783
+ root.render(<Oops />);
1784
+ });
1785
+ root.render(<AllGood />);
1786
+ // Render through just the high pri update. The low pri update remains on
1787
+ // the queue.
1788
+ expect(Scheduler).toFlushAndYieldThrough(['Everything is fine.']);
1789
+
1790
+ // Schedule a high pri update on a child that triggers an error.
1791
+ // The root should capture this error. But since there's still a pending
1792
+ // update on the root, the error should be suppressed.
1793
+ ReactNoop.discreteUpdates(() => {
1794
+ setShouldThrow(true);
1795
+ });
1796
+ });
1797
+ // Should render the final state without throwing the error.
1798
+ expect(Scheduler).toHaveYielded(['Everything is fine.']);
1799
+ expect(root).toMatchRenderedOutput('Everything is fine.');
1800
+ });
1801
+
1802
if (global.__PERSISTENT__) {
1803
it('regression test: should fatal if error is thrown at the root', () => {
1804
const root = ReactNoop.createRoot();