From 8527595a12d0cfba6acd6632b0a3cdc7a8aaba27 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 6 Aug 2020 10:35:09 -0400 Subject: [PATCH] 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. --- .../src/ReactFiberCommitWork.new.js | 29 ++ .../src/ReactFiberWorkLoop.new.js | 104 +++++- .../src/ReactFiberWorkLoop.old.js | 18 ++ .../ReactHooksWithNoopRenderer-test.js | 302 +++++++++++++++++- 4 files changed, 443 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6fda9c97cd1f0..9047a83fc95f0 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -112,6 +112,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, + captureCommitPhaseErrorForUnmountedFiber, resolveRetryWakeable, markCommitTimeOfFallback, enqueuePendingPassiveProfilerEffect, @@ -216,6 +217,34 @@ export function safelyCallDestroy(current: Fiber, destroy: () => void) { } } +export function safelyCallDestroyForUnmountedFiber( + current: Fiber, + nearestMountedAncestor: Fiber, + destroy: () => void, +) { + if (__DEV__) { + invokeGuardedCallback(null, destroy, null); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseErrorForUnmountedFiber( + current, + nearestMountedAncestor, + error, + ); + } + } else { + try { + destroy(); + } catch (error) { + captureCommitPhaseErrorForUnmountedFiber( + current, + nearestMountedAncestor, + error, + ); + } + } +} + function commitBeforeMutationLifeCycles( current: Fiber | null, finishedWork: Fiber, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 63ffe2d356aae..b8d3eb69f175d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -212,6 +212,7 @@ import { commitResetTextContent, isSuspenseBoundaryBeingHidden, safelyCallDestroy, + safelyCallDestroyForUnmountedFiber, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -2790,7 +2791,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; - flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete); + flushPassiveUnmountEffectsForUnmountedFiber(fiberToDelete, fiber); // Now that passive effects have been processed, it's safe to detach lingering pointers. detachFiberAfterEffects(fiberToDelete); @@ -2825,8 +2826,9 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { } } -function flushPassiveUnmountEffectsInsideOfDeletedTree( +function flushPassiveUnmountEffectsForUnmountedFiber( fiberToDelete: Fiber, + nearestMountedAncestor: Fiber, ): void { if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) { // If any children have passive effects then traverse the subtree. @@ -2835,7 +2837,10 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( // since that would not cover passive effects in siblings. let child = fiberToDelete.child; while (child !== null) { - flushPassiveUnmountEffectsInsideOfDeletedTree(child); + flushPassiveUnmountEffectsForUnmountedFiber( + child, + nearestMountedAncestor, + ); child = child.sibling; } } @@ -2846,16 +2851,64 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( case ForwardRef: case SimpleMemoComponent: case Block: { - flushPassiveUnmountEffectsImpl(fiberToDelete, HookPassive); + flushPassiveUnmountEffectsForUnmountedFiberImpl( + fiberToDelete, + nearestMountedAncestor, + ); } } } } +function flushPassiveUnmountEffectsForUnmountedFiberImpl( + fiber: Fiber, + nearestMountedAncestor: Fiber, +): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + setCurrentDebugFiberInDEV(fiber); + + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + if ((tag & HookPassive) === HookPassive) { + const destroy = effect.destroy; + if (destroy !== undefined) { + effect.destroy = undefined; + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + safelyCallDestroyForUnmountedFiber( + fiber, + nearestMountedAncestor, + destroy, + ); + recordPassiveEffectDuration(fiber); + } else { + safelyCallDestroyForUnmountedFiber( + fiber, + nearestMountedAncestor, + destroy, + ); + } + } + } + effect = next; + } while (effect !== firstEffect); + + resetCurrentDebugFiberInDEV(); + } +} + function flushPassiveUnmountEffectsImpl( fiber: Fiber, - // 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, ): void { const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); @@ -3055,6 +3108,45 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { } } +export function captureCommitPhaseErrorForUnmountedFiber( + sourceFiber: Fiber, + nearestMountedAncestor: Fiber, + error: mixed, +) { + let fiber = nearestMountedAncestor.return; + while (fiber !== null) { + if (fiber.tag === HostRoot) { + captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); + return; + } else if (fiber.tag === ClassComponent) { + const ctor = fiber.type; + const instance = fiber.stateNode; + if ( + typeof ctor.getDerivedStateFromError === 'function' || + (typeof instance.componentDidCatch === 'function' && + !isAlreadyFailedLegacyErrorBoundary(instance)) + ) { + const errorInfo = createCapturedValue(error, sourceFiber); + const update = createClassErrorUpdate( + fiber, + errorInfo, + (SyncLane: Lane), + ); + enqueueUpdate(fiber, update); + const eventTime = requestEventTime(); + const root = markUpdateLaneFromFiberToRoot(fiber, (SyncLane: Lane)); + if (root !== null) { + markRootUpdated(root, SyncLane, eventTime); + ensureRootIsScheduled(root, eventTime); + schedulePendingInteractions(root, SyncLane); + } + return; + } + } + fiber = fiber.return; + } +} + export function pingSuspendedRoot( root: FiberRoot, wakeable: Wakeable, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 36670a257f0ec..0c38f42bc18c4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2806,6 +2806,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; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index eaa222456fc66..258995b77628f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -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,13 +2352,35 @@ describe('ReactHooksWithNoopRenderer', () => { render() { if (this.state.error) { Scheduler.unstable_yieldValue('ErrorBoundary render error'); - return 'ErrorBoundary fallback'; + return ; } Scheduler.unstable_yieldValue('ErrorBoundary render success'); return this.props.children; } }; + 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 ; + } + Scheduler.unstable_yieldValue( + 'DerivedStateOnlyErrorBoundary render success', + ); + return this.props.children; + } + }; + LogOnlyErrorBoundary = class extends React.Component { componentDidCatch(error, info) { Scheduler.unstable_yieldValue( @@ -2371,7 +2394,8 @@ describe('ReactHooksWithNoopRenderer', () => { }; }); - 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,280 @@ 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 ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + 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 ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render(); + }); + + 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 ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'DerivedStateOnlyErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'BrokenUseEffectCleanup useEffect destroy', + ]); + }); + + // @gate new + it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ; + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + 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 ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + 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 ; + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + 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 ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + expect(() => { + act(() => { + ReactNoop.render(); + }); + }).toThrow('Expected error'); + + expect(Scheduler).toHaveYielded([ + 'BrokenUseEffectCleanup useEffect destroy', + ]); + + expect(ReactNoop.getChildren()).toEqual([]); + }); }); });