Skip to content

Commit ffb749c

Browse files
author
Brian Vaughn
authored
Improve error boundary handling for unmounted subtrees (#19542)
A passive effect's cleanup function may throw after an unmount. Prior to this commit, such an error would be ignored. (React would not notify any error boundaries.) After this commit, React's behavior varies depending on which reconciler fork is being used. For the old reconciler, React will call componentDidCatch for the nearest unmounted error boundary (if there is one). If there are no unmounted error boundaries, React will still swallow the error because the return pointer has been disconnected, so the normal error handling logic does not know how to traverse the tree to find the nearest still-mounted ancestor. For the new reconciler, React will skip any unmounted boundaries and look for a still-mounted boundary. If one is found, it will call getDerivedStateFromError and/or componentDidCatch (depending on the type of boundary). Tests have been added for both reconciler variants for now.
1 parent 9b35dd2 commit ffb749c

File tree

5 files changed

+400
-53
lines changed

5 files changed

+400
-53
lines changed

‎packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1667,16 +1667,28 @@ describe('DOMPluginEventSystem', () => {
16671667

16681668
function Test() {
16691669
React.useEffect(() => {
1670-
setClick1(buttonRef.current, targetListener1);
1671-
setClick2(buttonRef.current, targetListener2);
1672-
setClick3(buttonRef.current, targetListener3);
1673-
setClick4(buttonRef.current, targetListener4);
1670+
const clearClick1 = setClick1(
1671+
buttonRef.current,
1672+
targetListener1,
1673+
);
1674+
const clearClick2 = setClick2(
1675+
buttonRef.current,
1676+
targetListener2,
1677+
);
1678+
const clearClick3 = setClick3(
1679+
buttonRef.current,
1680+
targetListener3,
1681+
);
1682+
const clearClick4 = setClick4(
1683+
buttonRef.current,
1684+
targetListener4,
1685+
);
16741686

16751687
return () => {
1676-
setClick1();
1677-
setClick2();
1678-
setClick3();
1679-
setClick4();
1688+
clearClick1();
1689+
clearClick2();
1690+
clearClick3();
1691+
clearClick4();
16801692
};
16811693
});
16821694

@@ -1701,16 +1713,28 @@ describe('DOMPluginEventSystem', () => {
17011713

17021714
function Test2() {
17031715
React.useEffect(() => {
1704-
setClick1(buttonRef.current, targetListener1);
1705-
setClick2(buttonRef.current, targetListener2);
1706-
setClick3(buttonRef.current, targetListener3);
1707-
setClick4(buttonRef.current, targetListener4);
1716+
const clearClick1 = setClick1(
1717+
buttonRef.current,
1718+
targetListener1,
1719+
);
1720+
const clearClick2 = setClick2(
1721+
buttonRef.current,
1722+
targetListener2,
1723+
);
1724+
const clearClick3 = setClick3(
1725+
buttonRef.current,
1726+
targetListener3,
1727+
);
1728+
const clearClick4 = setClick4(
1729+
buttonRef.current,
1730+
targetListener4,
1731+
);
17081732

17091733
return () => {
1710-
setClick1();
1711-
setClick2();
1712-
setClick3();
1713-
setClick4();
1734+
clearClick1();
1735+
clearClick2();
1736+
clearClick3();
1737+
clearClick4();
17141738
};
17151739
});
17161740

‎packages/react-reconciler/src/ReactFiberCommitWork.new.js‎

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -173,13 +173,13 @@ function safelyCallComponentWillUnmount(current, instance) {
173173
);
174174
if (hasCaughtError()) {
175175
const unmountError = clearCaughtError();
176-
captureCommitPhaseError(current, unmountError);
176+
captureCommitPhaseError(current, current.return, unmountError);
177177
}
178178
} else {
179179
try {
180180
callComponentWillUnmountWithTimer(current, instance);
181181
} catch (unmountError) {
182-
captureCommitPhaseError(current, unmountError);
182+
captureCommitPhaseError(current, current.return, unmountError);
183183
}
184184
}
185185
}
@@ -192,13 +192,13 @@ function safelyDetachRef(current: Fiber) {
192192
invokeGuardedCallback(null, ref, null, null);
193193
if (hasCaughtError()) {
194194
const refError = clearCaughtError();
195-
captureCommitPhaseError(current, refError);
195+
captureCommitPhaseError(current, current.return, refError);
196196
}
197197
} else {
198198
try {
199199
ref(null);
200200
} catch (refError) {
201-
captureCommitPhaseError(current, refError);
201+
captureCommitPhaseError(current, current.return, refError);
202202
}
203203
}
204204
} else {
@@ -207,18 +207,22 @@ function safelyDetachRef(current: Fiber) {
207207
}
208208
}
209209

210-
export function safelyCallDestroy(current: Fiber, destroy: () => void) {
210+
export function safelyCallDestroy(
211+
current: Fiber,
212+
nearestMountedAncestor: Fiber | null,
213+
destroy: () => void,
214+
) {
211215
if (__DEV__) {
212216
invokeGuardedCallback(null, destroy, null);
213217
if (hasCaughtError()) {
214218
const error = clearCaughtError();
215-
captureCommitPhaseError(current, error);
219+
captureCommitPhaseError(current, nearestMountedAncestor, error);
216220
}
217221
} else {
218222
try {
219223
destroy();
220224
} catch (error) {
221-
captureCommitPhaseError(current, error);
225+
captureCommitPhaseError(current, nearestMountedAncestor, error);
222226
}
223227
}
224228
}
@@ -337,10 +341,10 @@ function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) {
337341

338342
// TODO: Remove this duplication.
339343
function commitHookEffectListUnmount2(
340-
// Tags to check for when deciding whether to unmount. e.g. to skip over
341-
// layout effects
344+
// Tags to check for when deciding whether to unmount. e.g. to skip over layout effects
342345
hookEffectTag: HookEffectTag,
343346
fiber: Fiber,
347+
nearestMountedAncestor: Fiber | null,
344348
): void {
345349
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
346350
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
@@ -359,10 +363,10 @@ function commitHookEffectListUnmount2(
359363
fiber.mode & ProfileMode
360364
) {
361365
startPassiveEffectTimer();
362-
safelyCallDestroy(fiber, destroy);
366+
safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
363367
recordPassiveEffectDuration(fiber);
364368
} else {
365-
safelyCallDestroy(fiber, destroy);
369+
safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
366370
}
367371
}
368372
}
@@ -465,7 +469,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
465469
if (hasCaughtError()) {
466470
invariant(fiber !== null, 'Should be working on an effect.');
467471
const error = clearCaughtError();
468-
captureCommitPhaseError(fiber, error);
472+
captureCommitPhaseError(fiber, fiber.return, error);
469473
}
470474
} else {
471475
try {
@@ -488,7 +492,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
488492
// The warning refers to useEffect but only applies to useLayoutEffect.
489493
} catch (error) {
490494
invariant(fiber !== null, 'Should be working on an effect.');
491-
captureCommitPhaseError(fiber, error);
495+
captureCommitPhaseError(fiber, fiber.return, error);
492496
}
493497
}
494498
}
@@ -997,10 +1001,10 @@ function commitUnmount(
9971001
current.mode & ProfileMode
9981002
) {
9991003
startLayoutEffectTimer();
1000-
safelyCallDestroy(current, destroy);
1004+
safelyCallDestroy(current, current.return, destroy);
10011005
recordLayoutEffectDuration(current);
10021006
} else {
1003-
safelyCallDestroy(current, destroy);
1007+
safelyCallDestroy(current, current.return, destroy);
10041008
}
10051009
}
10061010
}
@@ -1842,18 +1846,29 @@ function commitPassiveWork(finishedWork: Fiber): void {
18421846
case ForwardRef:
18431847
case SimpleMemoComponent:
18441848
case Block: {
1845-
commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork);
1849+
commitHookEffectListUnmount2(
1850+
HookPassive | HookHasEffect,
1851+
finishedWork,
1852+
finishedWork.return,
1853+
);
18461854
}
18471855
}
18481856
}
18491857

1850-
function commitPassiveUnmount(current: Fiber): void {
1858+
function commitPassiveUnmount(
1859+
current: Fiber,
1860+
nearestMountedAncestor: Fiber | null,
1861+
): void {
18511862
switch (current.tag) {
18521863
case FunctionComponent:
18531864
case ForwardRef:
18541865
case SimpleMemoComponent:
18551866
case Block:
1856-
commitHookEffectListUnmount2(HookPassive, current);
1867+
commitHookEffectListUnmount2(
1868+
HookPassive,
1869+
current,
1870+
nearestMountedAncestor,
1871+
);
18571872
}
18581873
}
18591874

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

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2393,14 +2393,14 @@ function commitBeforeMutationEffects(firstChild: Fiber) {
23932393
invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber);
23942394
if (hasCaughtError()) {
23952395
const error = clearCaughtError();
2396-
captureCommitPhaseError(fiber, error);
2396+
captureCommitPhaseError(fiber, fiber.return, error);
23972397
}
23982398
resetCurrentDebugFiberInDEV();
23992399
} else {
24002400
try {
24012401
commitBeforeMutationEffectsImpl(fiber);
24022402
} catch (error) {
2403-
captureCommitPhaseError(fiber, error);
2403+
captureCommitPhaseError(fiber, fiber.return, error);
24042404
}
24052405
}
24062406
fiber = fiber.sibling;
@@ -2490,14 +2490,14 @@ function commitMutationEffects(
24902490
);
24912491
if (hasCaughtError()) {
24922492
const error = clearCaughtError();
2493-
captureCommitPhaseError(fiber, error);
2493+
captureCommitPhaseError(fiber, fiber.return, error);
24942494
}
24952495
resetCurrentDebugFiberInDEV();
24962496
} else {
24972497
try {
24982498
commitMutationEffectsImpl(fiber, root, renderPriorityLevel);
24992499
} catch (error) {
2500-
captureCommitPhaseError(fiber, error);
2500+
captureCommitPhaseError(fiber, fiber.return, error);
25012501
}
25022502
}
25032503
fiber = fiber.sibling;
@@ -2593,13 +2593,13 @@ function commitMutationEffectsDeletions(
25932593
);
25942594
if (hasCaughtError()) {
25952595
const error = clearCaughtError();
2596-
captureCommitPhaseError(childToDelete, error);
2596+
captureCommitPhaseError(childToDelete, childToDelete.return, error);
25972597
}
25982598
} else {
25992599
try {
26002600
commitDeletion(root, childToDelete, renderPriorityLevel);
26012601
} catch (error) {
2602-
captureCommitPhaseError(childToDelete, error);
2602+
captureCommitPhaseError(childToDelete, childToDelete.return, error);
26032603
}
26042604
}
26052605
}
@@ -2641,14 +2641,14 @@ function commitLayoutEffects(
26412641
);
26422642
if (hasCaughtError()) {
26432643
const error = clearCaughtError();
2644-
captureCommitPhaseError(fiber, error);
2644+
captureCommitPhaseError(fiber, fiber.return, error);
26452645
}
26462646
resetCurrentDebugFiberInDEV();
26472647
} else {
26482648
try {
26492649
commitLayoutEffectsImpl(fiber, root, committedLanes);
26502650
} catch (error) {
2651-
captureCommitPhaseError(fiber, error);
2651+
captureCommitPhaseError(fiber, fiber.return, error);
26522652
}
26532653
}
26542654
fiber = fiber.sibling;
@@ -2748,7 +2748,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
27482748
if (deletions !== null) {
27492749
for (let i = 0; i < deletions.length; i++) {
27502750
const fiberToDelete = deletions[i];
2751-
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
2751+
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete, fiber);
27522752

27532753
// Now that passive effects have been processed, it's safe to detach lingering pointers.
27542754
detachFiberAfterEffects(fiberToDelete);
@@ -2780,6 +2780,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
27802780

27812781
function flushPassiveUnmountEffectsInsideOfDeletedTree(
27822782
fiberToDelete: Fiber,
2783+
nearestMountedAncestor: Fiber,
27832784
): void {
27842785
if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
27852786
// If any children have passive effects then traverse the subtree.
@@ -2788,14 +2789,17 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
27882789
// since that would not cover passive effects in siblings.
27892790
let child = fiberToDelete.child;
27902791
while (child !== null) {
2791-
flushPassiveUnmountEffectsInsideOfDeletedTree(child);
2792+
flushPassiveUnmountEffectsInsideOfDeletedTree(
2793+
child,
2794+
nearestMountedAncestor,
2795+
);
27922796
child = child.sibling;
27932797
}
27942798
}
27952799

27962800
if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
27972801
setCurrentDebugFiberInDEV(fiberToDelete);
2798-
commitPassiveUnmount(fiberToDelete);
2802+
commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
27992803
resetCurrentDebugFiberInDEV();
28002804
}
28012805
}
@@ -2922,15 +2926,19 @@ function captureCommitPhaseErrorOnRoot(
29222926
}
29232927
}
29242928

2925-
export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
2929+
export function captureCommitPhaseError(
2930+
sourceFiber: Fiber,
2931+
nearestMountedAncestor: Fiber | null,
2932+
error: mixed,
2933+
) {
29262934
if (sourceFiber.tag === HostRoot) {
29272935
// Error was thrown at the root. There is no parent, so the root
29282936
// itself should capture it.
29292937
captureCommitPhaseErrorOnRoot(sourceFiber, sourceFiber, error);
29302938
return;
29312939
}
29322940

2933-
let fiber = sourceFiber.return;
2941+
let fiber = nearestMountedAncestor;
29342942
while (fiber !== null) {
29352943
if (fiber.tag === HostRoot) {
29362944
captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error);

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2850,6 +2850,24 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
28502850
markRootUpdated(root, SyncLane, eventTime);
28512851
ensureRootIsScheduled(root, eventTime);
28522852
schedulePendingInteractions(root, SyncLane);
2853+
} else {
2854+
// This component has already been unmounted.
2855+
// We can't schedule any follow up work for the root because the fiber is already unmounted,
2856+
// but we can still call the log-only boundary so the error isn't swallowed.
2857+
//
2858+
// TODO This is only a temporary bandaid for the old reconciler fork.
2859+
// We can delete this special case once the new fork is merged.
2860+
if (
2861+
typeof instance.componentDidCatch === 'function' &&
2862+
!isAlreadyFailedLegacyErrorBoundary(instance)
2863+
) {
2864+
try {
2865+
instance.componentDidCatch(error, errorInfo);
2866+
} catch (errorToIgnore) {
2867+
// TODO Ignore this error? Rethrow it?
2868+
// This is kind of an edge case.
2869+
}
2870+
}
28532871
}
28542872
return;
28552873
}

0 commit comments

Comments
 (0)