Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error boundary handling for unmounted subtrees #19542

Merged
merged 1 commit into from
Aug 14, 2020
Merged

Improve error boundary handling for unmounted subtrees #19542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 40 additions & 16 deletions packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1667,16 +1667,28 @@ describe('DOMPluginEventSystem', () => {

function Test() {
React.useEffect(() => {
setClick1(buttonRef.current, targetListener1);
setClick2(buttonRef.current, targetListener2);
setClick3(buttonRef.current, targetListener3);
setClick4(buttonRef.current, targetListener4);
const clearClick1 = setClick1(
buttonRef.current,
targetListener1,
);
const clearClick2 = setClick2(
buttonRef.current,
targetListener2,
);
const clearClick3 = setClick3(
buttonRef.current,
targetListener3,
);
const clearClick4 = setClick4(
buttonRef.current,
targetListener4,
);

return () => {
setClick1();
setClick2();
setClick3();
setClick4();
clearClick1();
clearClick2();
clearClick3();
clearClick4();
};
});

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

function Test2() {
React.useEffect(() => {
setClick1(buttonRef.current, targetListener1);
setClick2(buttonRef.current, targetListener2);
setClick3(buttonRef.current, targetListener3);
setClick4(buttonRef.current, targetListener4);
const clearClick1 = setClick1(
buttonRef.current,
targetListener1,
);
const clearClick2 = setClick2(
buttonRef.current,
targetListener2,
);
const clearClick3 = setClick3(
buttonRef.current,
targetListener3,
);
const clearClick4 = setClick4(
buttonRef.current,
targetListener4,
);

return () => {
setClick1();
setClick2();
setClick3();
setClick4();
clearClick1();
clearClick2();
clearClick3();
clearClick4();
};
});

Expand Down
51 changes: 33 additions & 18 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,13 @@ function safelyCallComponentWillUnmount(current, instance) {
);
if (hasCaughtError()) {
const unmountError = clearCaughtError();
captureCommitPhaseError(current, unmountError);
captureCommitPhaseError(current, current.return, unmountError);
}
} else {
try {
callComponentWillUnmountWithTimer(current, instance);
} catch (unmountError) {
captureCommitPhaseError(current, unmountError);
captureCommitPhaseError(current, current.return, unmountError);
}
}
}
Expand All @@ -192,13 +192,13 @@ function safelyDetachRef(current: Fiber) {
invokeGuardedCallback(null, ref, null, null);
if (hasCaughtError()) {
const refError = clearCaughtError();
captureCommitPhaseError(current, refError);
captureCommitPhaseError(current, current.return, refError);
}
} else {
try {
ref(null);
} catch (refError) {
captureCommitPhaseError(current, refError);
captureCommitPhaseError(current, current.return, refError);
}
}
} else {
Expand All @@ -207,18 +207,22 @@ function safelyDetachRef(current: Fiber) {
}
}

export function safelyCallDestroy(current: Fiber, destroy: () => void) {
export function safelyCallDestroy(
current: Fiber,
nearestMountedAncestor: Fiber | null,
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
destroy: () => void,
) {
if (__DEV__) {
invokeGuardedCallback(null, destroy, null);
if (hasCaughtError()) {
const error = clearCaughtError();
captureCommitPhaseError(current, error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
} else {
try {
destroy();
} catch (error) {
captureCommitPhaseError(current, error);
captureCommitPhaseError(current, nearestMountedAncestor, error);
}
}
}
Expand Down Expand Up @@ -337,10 +341,10 @@ function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) {

// TODO: Remove this duplication.
function commitHookEffectListUnmount2(
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
// Tags to check for when deciding whether to unmount. e.g. to skip over
// layout effects
// Tags to check for when deciding whether to unmount. e.g. to skip over layout effects
hookEffectTag: HookEffectTag,
fiber: Fiber,
nearestMountedAncestor: Fiber | null,
): void {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
Expand All @@ -359,10 +363,10 @@ function commitHookEffectListUnmount2(
fiber.mode & ProfileMode
) {
startPassiveEffectTimer();
safelyCallDestroy(fiber, destroy);
safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
recordPassiveEffectDuration(fiber);
} else {
safelyCallDestroy(fiber, destroy);
safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
}
}
}
Expand Down Expand Up @@ -465,7 +469,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
} else {
try {
Expand All @@ -488,7 +492,7 @@ function commitHookEffectListMount2(fiber: Fiber): void {
// The warning refers to useEffect but only applies to useLayoutEffect.
} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
}
}
Expand Down Expand Up @@ -997,10 +1001,10 @@ function commitUnmount(
current.mode & ProfileMode
) {
startLayoutEffectTimer();
safelyCallDestroy(current, destroy);
safelyCallDestroy(current, current.return, destroy);
recordLayoutEffectDuration(current);
} else {
safelyCallDestroy(current, destroy);
safelyCallDestroy(current, current.return, destroy);
}
}
}
Expand Down Expand Up @@ -1842,18 +1846,29 @@ function commitPassiveWork(finishedWork: Fiber): void {
case ForwardRef:
case SimpleMemoComponent:
case Block: {
commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork);
commitHookEffectListUnmount2(
HookPassive | HookHasEffect,
finishedWork,
finishedWork.return,
);
}
}
}

function commitPassiveUnmount(current: Fiber): void {
function commitPassiveUnmount(
current: Fiber,
nearestMountedAncestor: Fiber | null,
): void {
switch (current.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block:
commitHookEffectListUnmount2(HookPassive, current);
commitHookEffectListUnmount2(
HookPassive,
current,
nearestMountedAncestor,
);
}
}

Expand Down
34 changes: 21 additions & 13 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2406,14 +2406,14 @@ function commitBeforeMutationEffects(firstChild: Fiber) {
invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber);
if (hasCaughtError()) {
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitBeforeMutationEffectsImpl(fiber);
} catch (error) {
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
Expand Down Expand Up @@ -2503,14 +2503,14 @@ function commitMutationEffects(
);
if (hasCaughtError()) {
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitMutationEffectsImpl(fiber, root, renderPriorityLevel);
} catch (error) {
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
Expand Down Expand Up @@ -2606,13 +2606,13 @@ function commitMutationEffectsDeletions(
);
if (hasCaughtError()) {
const error = clearCaughtError();
captureCommitPhaseError(childToDelete, error);
captureCommitPhaseError(childToDelete, childToDelete.return, error);
}
} else {
try {
commitDeletion(root, childToDelete, renderPriorityLevel);
} catch (error) {
captureCommitPhaseError(childToDelete, error);
captureCommitPhaseError(childToDelete, childToDelete.return, error);
}
}
}
Expand Down Expand Up @@ -2654,14 +2654,14 @@ function commitLayoutEffects(
);
if (hasCaughtError()) {
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitLayoutEffectsImpl(fiber, root, committedLanes);
} catch (error) {
captureCommitPhaseError(fiber, error);
captureCommitPhaseError(fiber, fiber.return, error);
}
}
fiber = fiber.sibling;
Expand Down Expand Up @@ -2761,7 +2761,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
if (deletions !== null) {
for (let i = 0; i < deletions.length; i++) {
const fiberToDelete = deletions[i];
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete);
flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete, fiber);

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

function flushPassiveUnmountEffectsInsideOfDeletedTree(
fiberToDelete: Fiber,
nearestMountedAncestor: Fiber,
): void {
if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) {
// If any children have passive effects then traverse the subtree.
Expand All @@ -2801,14 +2802,17 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
// since that would not cover passive effects in siblings.
let child = fiberToDelete.child;
while (child !== null) {
flushPassiveUnmountEffectsInsideOfDeletedTree(child);
flushPassiveUnmountEffectsInsideOfDeletedTree(
child,
nearestMountedAncestor,
);
child = child.sibling;
}
}

if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) {
setCurrentDebugFiberInDEV(fiberToDelete);
commitPassiveUnmount(fiberToDelete);
commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
resetCurrentDebugFiberInDEV();
}
}
Expand Down Expand Up @@ -2935,15 +2939,19 @@ function captureCommitPhaseErrorOnRoot(
}
}

export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
export function captureCommitPhaseError(
sourceFiber: Fiber,
nearestMountedAncestor: Fiber | null,
error: mixed,
) {
if (sourceFiber.tag === HostRoot) {
// Error was thrown at the root. There is no parent, so the root
// itself should capture it.
captureCommitPhaseErrorOnRoot(sourceFiber, sourceFiber, error);
return;
}

let fiber = sourceFiber.return;
let fiber = nearestMountedAncestor;
Copy link
Contributor Author

@bvaughn bvaughn Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main fix for the new fork. The rest of the changes were just to funnel this fiber through.

while (fiber !== null) {
if (fiber.tag === HostRoot) {
captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error);
Expand Down
18 changes: 18 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2863,6 +2863,24 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
markRootUpdated(root, SyncLane, eventTime);
ensureRootIsScheduled(root, eventTime);
schedulePendingInteractions(root, SyncLane);
} else {
// This component has already been unmounted.
// We can't schedule any follow up work for the root because the fiber is already unmounted,
// but we can still call the log-only boundary so the error isn't swallowed.
//
// TODO This is only a temporary bandaid for the old reconciler fork.
// We can delete this special case once the new fork is merged.
if (
typeof instance.componentDidCatch === 'function' &&
!isAlreadyFailedLegacyErrorBoundary(instance)
) {
try {
instance.componentDidCatch(error, errorInfo);
} catch (errorToIgnore) {
// TODO Ignore this error? Rethrow it?
// This is kind of an edge case.
}
}
}
return;
}
Expand Down
Loading