amirho/react
1
0
Fork 0
mirror of https://github.com/facebook/react.git synced 2026-02-26 18:58:05 +00:00
Code Issues Packages Projects Releases Wiki Activity

Improve error boundary handling for unmounted subtrees (#19542)

Browse Source 
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.
This commit is contained in:
Brian Vaughn
2020-08-14 16:46:46 -04:00
committed by GitHub
parent 9b35dd2fcc
commit ffb749c95e
5 changed files with 400 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
View File

@@ -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();
};
});
@@ -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();
};
});

51
packages/react-reconciler/src/ReactFiberCommitWork.new.js
View File

@@ -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);
}
}
}
@@ -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 {
@@ -207,18 +207,22 @@ function safelyDetachRef(current: Fiber) {
}
}
export function safelyCallDestroy(current: Fiber, destroy: () => void) {
export function safelyCallDestroy(
current: Fiber,
nearestMountedAncestor: Fiber | null,
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);
}
}
}
@@ -337,10 +341,10 @@ function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) {
// TODO: Remove this duplication.
function commitHookEffectListUnmount2(
// 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;
@@ -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);
}
}
}
@@ -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 {
@@ -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);
}
}
}
@@ -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);
}
}
}
@@ -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,
);
}
}

34
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View File

@@ -2393,14 +2393,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;
@@ -2490,14 +2490,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;
@@ -2593,13 +2593,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);
}
}
}
@@ -2641,14 +2641,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;
@@ -2748,7 +2748,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);
@@ -2780,6 +2780,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.
@@ -2788,14 +2789,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();
}
}
@@ -2922,7 +2926,11 @@ 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.
@@ -2930,7 +2938,7 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) {
return;
}
let fiber = sourceFiber.return;
let fiber = nearestMountedAncestor;
while (fiber !== null) {
if (fiber.tag === HostRoot) {
captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error);

18
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View File

@@ -2850,6 +2850,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;
}

294
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js vendored
View File

@@ -2320,6 +2320,7 @@ describe('ReactHooksWithNoopRenderer', () => {
describe('errors thrown in passive destroy function within unmounted trees', () => {
let BrokenUseEffectCleanup;
let ErrorBoundary;
let DerivedStateOnlyErrorBoundary;
let LogOnlyErrorBoundary;
beforeEach(() => {
@@ -2351,10 +2352,32 @@ describe('ReactHooksWithNoopRenderer', () => {
render() {
if (this.state.error) {
Scheduler.unstable_yieldValue('ErrorBoundary render error');
return 'ErrorBoundary fallback';
return <span prop="ErrorBoundary fallback" />;
}
Scheduler.unstable_yieldValue('ErrorBoundary render success');
return this.props.children;
return this.props.children || null;
}
};
DerivedStateOnlyErrorBoundary = class extends React.Component {
state = {error: null};
static getDerivedStateFromError(error) {
Scheduler.unstable_yieldValue(
`DerivedStateOnlyErrorBoundary static getDerivedStateFromError`,
);
return {error};
}
render() {
if (this.state.error) {
Scheduler.unstable_yieldValue(
'DerivedStateOnlyErrorBoundary render error',
);
return <span prop="DerivedStateOnlyErrorBoundary fallback" />;
}
Scheduler.unstable_yieldValue(
'DerivedStateOnlyErrorBoundary render success',
);
return this.props.children || null;
}
};
@@ -2366,12 +2389,13 @@ describe('ReactHooksWithNoopRenderer', () => {
}
render() {
Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`);
return this.props.children;
return this.props.children || null;
}
};
});
it('should not error if the nearest unmounted boundary is log-only', () => {
// @gate old
it('should call componentDidCatch() for the nearest unmounted log-only boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
@@ -2411,10 +2435,268 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
// This should call componentDidCatch too, but we'll address that in a follow up.
// 'LogOnlyErrorBoundary componentDidCatch',
'LogOnlyErrorBoundary componentDidCatch',
]);
});
// @gate old
it('should call componentDidCatch() for the nearest unmounted logging-capable boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<ErrorBoundary>
<BrokenUseEffectCleanup />
</ErrorBoundary>
);
} else {
return null;
}
}
act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={true} />
</ErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={false} />
</ErrorBoundary>,
);
expect(Scheduler).toFlushAndYieldThrough([
'ErrorBoundary render success',
]);
});
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
'ErrorBoundary componentDidCatch',
]);
});
// @gate old
it('should not call getDerivedStateFromError for unmounted error boundaries', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<ErrorBoundary>
<BrokenUseEffectCleanup />
</ErrorBoundary>
);
} else {
return null;
}
}
act(() => {
ReactNoop.render(<Conditional showChildren={true} />);
});
expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(<Conditional showChildren={false} />);
});
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
'ErrorBoundary componentDidCatch',
]);
});
// @gate old
it('should not throw if there are no unmounted logging-capable boundaries to call', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<DerivedStateOnlyErrorBoundary>
<BrokenUseEffectCleanup />
</DerivedStateOnlyErrorBoundary>
);
} else {
return null;
}
}
act(() => {
ReactNoop.render(<Conditional showChildren={true} />);
});
expect(Scheduler).toHaveYielded([
'DerivedStateOnlyErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(<Conditional showChildren={false} />);
});
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
]);
});
// @gate new
it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => {
act(() => {
ReactNoop.render(
<LogOnlyErrorBoundary>
<BrokenUseEffectCleanup />
</LogOnlyErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'LogOnlyErrorBoundary render',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(<LogOnlyErrorBoundary />);
});
expect(Scheduler).toHaveYielded([
'LogOnlyErrorBoundary render',
'BrokenUseEffectCleanup useEffect destroy',
'LogOnlyErrorBoundary componentDidCatch',
]);
});
// @gate new
it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<ErrorBoundary>
<BrokenUseEffectCleanup />
</ErrorBoundary>
);
} else {
return null;
}
}
act(() => {
ReactNoop.render(
<LogOnlyErrorBoundary>
<Conditional showChildren={true} />
</LogOnlyErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'LogOnlyErrorBoundary render',
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(
<LogOnlyErrorBoundary>
<Conditional showChildren={false} />
</LogOnlyErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'LogOnlyErrorBoundary render',
'BrokenUseEffectCleanup useEffect destroy',
'LogOnlyErrorBoundary componentDidCatch',
]);
});
// @gate new
it('should call getDerivedStateFromError in the nearest still-mounted boundary', () => {
function Conditional({showChildren}) {
if (showChildren) {
return <BrokenUseEffectCleanup />;
} else {
return null;
}
}
act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={true} />
</ErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={false} />
</ErrorBoundary>,
);
});
expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect destroy',
'ErrorBoundary static getDerivedStateFromError',
'ErrorBoundary render error',
'ErrorBoundary componentDidCatch',
]);
expect(ReactNoop.getChildren()).toEqual([
span('ErrorBoundary fallback'),
]);
});
// @gate new
it('should rethrow error if there are no still-mounted boundaries', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<ErrorBoundary>
<BrokenUseEffectCleanup />
</ErrorBoundary>
);
} else {
return null;
}
}
act(() => {
ReactNoop.render(<Conditional showChildren={true} />);
});
expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'BrokenUseEffectCleanup useEffect',
]);
expect(() => {
act(() => {
ReactNoop.render(<Conditional showChildren={false} />);
});
}).toThrow('Expected error');
expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
]);
expect(ReactNoop.getChildren()).toEqual([]);
});
});
});