Skip to content

Commit 3bfe779

Browse files
author
Brian Vaughn
committed
Improve error boundary handling for unmounted subtrees
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 dab0854 commit 3bfe779

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
@@ -2406,14 +2406,14 @@ function commitBeforeMutationEffects(firstChild: Fiber) {
24062406
invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber);
24072407
if (hasCaughtError()) {
24082408
const error = clearCaughtError();
2409-
captureCommitPhaseError(fiber, error);
2409+
captureCommitPhaseError(fiber, fiber.return, error);
24102410
}
24112411
resetCurrentDebugFiberInDEV();
24122412
} else {
24132413
try {
24142414
commitBeforeMutationEffectsImpl(fiber);
24152415
} catch (error) {
2416-
captureCommitPhaseError(fiber, error);
2416+
captureCommitPhaseError(fiber, fiber.return, error);
24172417
}
24182418
}
24192419
fiber = fiber.sibling;
@@ -2503,14 +2503,14 @@ function commitMutationEffects(
25032503
);
25042504
if (hasCaughtError()) {
25052505
const error = clearCaughtError();
2506-
captureCommitPhaseError(fiber, error);
2506+
captureCommitPhaseError(fiber, fiber.return, error);
25072507
}
25082508
resetCurrentDebugFiberInDEV();
25092509
} else {
25102510
try {
25112511
commitMutationEffectsImpl(fiber, root, renderPriorityLevel);
25122512
} catch (error) {
2513-
captureCommitPhaseError(fiber, error);
2513+
captureCommitPhaseError(fiber, fiber.return, error);
25142514
}
25152515
}
25162516
fiber = fiber.sibling;
@@ -2606,13 +2606,13 @@ function commitMutationEffectsDeletions(
26062606
);
26072607
if (hasCaughtError()) {
26082608
const error = clearCaughtError();
2609-
captureCommitPhaseError(childToDelete, error);
2609+
captureCommitPhaseError(childToDelete, childToDelete.return, error);
26102610
}
26112611
} else {
26122612
try {
26132613
commitDeletion(root, childToDelete, renderPriorityLevel);
26142614
} catch (error) {
2615-
captureCommitPhaseError(childToDelete, error);
2615+
captureCommitPhaseError(childToDelete, childToDelete.return, error);
26162616
}
26172617
}
26182618
}
@@ -2654,14 +2654,14 @@ function commitLayoutEffects(
26542654
);
26552655
if (hasCaughtError()) {
26562656
const error = clearCaughtError();
2657-
captureCommitPhaseError(fiber, error);
2657+
captureCommitPhaseError(fiber, fiber.return, error);
26582658
}
26592659
resetCurrentDebugFiberInDEV();
26602660
} else {
26612661
try {
26622662
commitLayoutEffectsImpl(fiber, root, committedLanes);
26632663
} catch (error) {
2664-
captureCommitPhaseError(fiber, error);
2664+
captureCommitPhaseError(fiber, fiber.return, error);
26652665
}
26662666
}
26672667
fiber = fiber.sibling;
@@ -2761,7 +2761,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
27612761
if (deletions !== null) {
27622762
for (let i = 0; i < deletions.length; i++) {
27632763
const fiberToDelete = deletions[i];
2764-
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
2764+
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete, fiber);
27652765

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

27942794
function flushPassiveUnmountEffectsInsideOfDeletedTree(
27952795
fiberToDelete: Fiber,
2796+
nearestMountedAncestor: Fiber,
27962797
): void {
27972798
if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
27982799
// If any children have passive effects then traverse the subtree.
@@ -2801,14 +2802,17 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
28012802
// since that would not cover passive effects in siblings.
28022803
let child = fiberToDelete.child;
28032804
while (child !== null) {
2804-
flushPassiveUnmountEffectsInsideOfDeletedTree(child);
2805+
flushPassiveUnmountEffectsInsideOfDeletedTree(
2806+
child,
2807+
nearestMountedAncestor,
2808+
);
28052809
child = child.sibling;
28062810
}
28072811
}
28082812

28092813
if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
28102814
setCurrentDebugFiberInDEV(fiberToDelete);
2811-
commitPassiveUnmount(fiberToDelete);
2815+
commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
28122816
resetCurrentDebugFiberInDEV();
28132817
}
28142818
}
@@ -2935,15 +2939,19 @@ function captureCommitPhaseErrorOnRoot(
29352939
}
29362940
}
29372941

2938-
export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
2942+
export function captureCommitPhaseError(
2943+
sourceFiber: Fiber,
2944+
nearestMountedAncestor: Fiber | null,
2945+
error: mixed,
2946+
) {
29392947
if (sourceFiber.tag === HostRoot) {
29402948
// Error was thrown at the root. There is no parent, so the root
29412949
// itself should capture it.
29422950
captureCommitPhaseErrorOnRoot(sourceFiber, sourceFiber, error);
29432951
return;
29442952
}
29452953

2946-
let fiber = sourceFiber.return;
2954+
let fiber = nearestMountedAncestor;
29472955
while (fiber !== null) {
29482956
if (fiber.tag === HostRoot) {
29492957
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
@@ -2863,6 +2863,24 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
28632863
markRootUpdated(root, SyncLane, eventTime);
28642864
ensureRootIsScheduled(root, eventTime);
28652865
schedulePendingInteractions(root, SyncLane);
2866+
} else {
2867+
// This component has already been unmounted.
2868+
// We can't schedule any follow up work for the root because the fiber is already unmounted,
2869+
// but we can still call the log-only boundary so the error isn't swallowed.
2870+
//
2871+
// TODO This is only a temporary bandaid for the old reconciler fork.
2872+
// We can delete this special case once the new fork is merged.
2873+
if (
2874+
typeof instance.componentDidCatch === 'function' &&
2875+
!isAlreadyFailedLegacyErrorBoundary(instance)
2876+
) {
2877+
try {
2878+
instance.componentDidCatch(error, errorInfo);
2879+
} catch (errorToIgnore) {
2880+
// TODO Ignore this error? Rethrow it?
2881+
// This is kind of an edge case.
2882+
}
2883+
}
28662884
}
28672885
return;
28682886
}

0 commit comments

Comments
 (0)