From c74790fd1ed2b7a654e87cc20195758bf892d849 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 11:18:37 -0400 Subject: [PATCH 1/3] Always skip unmounted/unmounting error boundaries The behavior of error boundaries for passive effects that throw during cleanup was recently changed so that React ignores boundaries which are also unmounting in favor of still-mounted boundaries. This commit implements that same behavior for layout effects (useLayoutEffect and componentWillUnmount). --- .../ReactErrorBoundaries-test.internal.js | 84 +++++++++++++++++ .../src/ReactFiberCommitWork.new.js | 79 ++++++++++++---- .../src/ReactFiberCommitWork.old.js | 93 ++++++++++++++----- .../src/ReactFiberWorkLoop.new.js | 22 ++++- .../src/ReactFiberWorkLoop.old.js | 40 +++++--- .../ReactHooksWithNoopRenderer-test.js | 78 ++++++++++++++++ ...tIncrementalErrorHandling-test.internal.js | 17 ++-- 7 files changed, 351 insertions(+), 62 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 06f51b0f397..bf8fd72b730 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2473,4 +2473,88 @@ describe('ReactErrorBoundaries', () => { 'Caught an error: gotta catch em all.', ); }); + + it('catches errors thrown in componentWillUnmount', () => { + class LocalErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + class Component extends React.Component { + render() { + const {id} = this.props; + Scheduler.unstable_yieldValue('Component render ' + id); + return id; + } + } + + class LocalBrokenComponentWillUnmount extends React.Component { + componentWillUnmount() { + Scheduler.unstable_yieldValue( + 'BrokenComponentWillUnmount componentWillUnmount', + ); + throw Error('Expected'); + } + + render() { + Scheduler.unstable_yieldValue('BrokenComponentWillUnmount render'); + return 'broken'; + } + } + + const container = document.createElement('div'); + + ReactDOM.render( + + + + + + , + container, + ); + + expect(container.firstChild.textContent).toBe('sibling'); + expect(container.lastChild.textContent).toBe('broken'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'BrokenComponentWillUnmount render', + ]); + + ReactDOM.render( + + + , + container, + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(container.firstChild.textContent).toBe('OuterFallback'); + expect(container.lastChild.textContent).toBe('OuterFallback'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'BrokenComponentWillUnmount componentWillUnmount', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index f086e47cf93..c451585a074 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -162,7 +162,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current, instance) { +function safelyCallComponentWillUnmount( + current: Fiber, + instance: any, + nearestMountedAncestor: Fiber, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -173,13 +177,13 @@ function safelyCallComponentWillUnmount(current, instance) { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(current, current.return, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } else { try { callComponentWillUnmountWithTimer(current, instance); } catch (unmountError) { - captureCommitPhaseError(current, current.return, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } } @@ -974,6 +978,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -1001,10 +1006,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, current.return, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, current.return, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -1018,7 +1023,11 @@ function commitUnmount( safelyDetachRef(current); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + instance, + nearestMountedAncestor, + ); } return; } @@ -1031,7 +1040,12 @@ function commitUnmount( // We are also not using this parent because // the portal will get pushed immediately. if (supportsMutation) { - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else if (supportsPersistence) { emptyPortalContainer(current); } @@ -1071,6 +1085,7 @@ function commitUnmount( function commitNestedUnmounts( finishedRoot: FiberRoot, root: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { // While we're inside a removed host node we don't want to call @@ -1080,7 +1095,12 @@ function commitNestedUnmounts( // we do an inner loop while we're still inside the host node. let node: Fiber = root; while (true) { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if ( @@ -1361,9 +1381,10 @@ function insertOrAppendPlacementNode( } function unmountHostComponents( - finishedRoot, - current, - renderPriorityLevel, + finishedRoot: FiberRoot, + current: Fiber, + nearestMountedAncestor: Fiber, + renderPriorityLevel: ReactPriorityLevel, ): void { // We only have the top Fiber that was deleted but we need to recurse down its // children to find all the terminal nodes. @@ -1412,7 +1433,12 @@ function unmountHostComponents( } if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1429,7 +1455,12 @@ function unmountHostComponents( // Don't visit children because we already visited them. } else if (enableFundamentalAPI && node.tag === FundamentalComponent) { const fundamentalNode = node.stateNode.instance; - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1481,7 +1512,12 @@ function unmountHostComponents( continue; } } else { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because we may find more host components below. if (node.child !== null) { node.child.return = node; @@ -1511,15 +1547,26 @@ function unmountHostComponents( function commitDeletion( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { if (supportsMutation) { // Recursively delete all host nodes from the parent. // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else { // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } const alternate = current.alternate; detachFiberMutation(current); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 8e280e16005..81b226670f1 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -153,7 +153,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current, instance) { +function safelyCallComponentWillUnmount( + current: Fiber, + instance: any, + nearestMountedAncestor: Fiber | null, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -164,13 +168,13 @@ function safelyCallComponentWillUnmount(current, instance) { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } else { try { callComponentWillUnmountWithTimer(current, instance); } catch (unmountError) { - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } } @@ -183,13 +187,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 { @@ -198,18 +202,22 @@ function safelyDetachRef(current: Fiber) { } } -function safelyCallDestroy(current, destroy) { +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); } } } @@ -866,6 +874,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -895,10 +904,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -912,7 +921,11 @@ function commitUnmount( safelyDetachRef(current); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + instance, + nearestMountedAncestor, + ); } return; } @@ -925,7 +938,12 @@ function commitUnmount( // We are also not using this parent because // the portal will get pushed immediately. if (supportsMutation) { - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else if (supportsPersistence) { emptyPortalContainer(current); } @@ -965,6 +983,7 @@ function commitUnmount( function commitNestedUnmounts( finishedRoot: FiberRoot, root: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { // While we're inside a removed host node we don't want to call @@ -974,7 +993,12 @@ function commitNestedUnmounts( // we do an inner loop while we're still inside the host node. let node: Fiber = root; while (true) { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if ( @@ -1263,9 +1287,10 @@ function insertOrAppendPlacementNode( } function unmountHostComponents( - finishedRoot, - current, - renderPriorityLevel, + finishedRoot: FiberRoot, + current: Fiber, + nearestMountedAncestor: Fiber | null, + renderPriorityLevel: ReactPriorityLevel, ): void { // We only have the top Fiber that was deleted but we need to recurse down its // children to find all the terminal nodes. @@ -1314,7 +1339,12 @@ function unmountHostComponents( } if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1331,7 +1361,12 @@ function unmountHostComponents( // Don't visit children because we already visited them. } else if (enableFundamentalAPI && node.tag === FundamentalComponent) { const fundamentalNode = node.stateNode.instance; - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1383,7 +1418,12 @@ function unmountHostComponents( continue; } } else { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because we may find more host components below. if (node.child !== null) { node.child.return = node; @@ -1413,15 +1453,26 @@ function unmountHostComponents( function commitDeletion( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { if (supportsMutation) { // Recursively delete all host nodes from the parent. // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else { // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } const alternate = current.alternate; detachFiberMutation(current); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 87da91d58c5..578ffd427c7 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2462,13 +2462,18 @@ function commitBeforeMutationEffectsDeletions(deletions: Array) { function commitMutationEffects( firstChild: Fiber, root: FiberRoot, - renderPriorityLevel, + renderPriorityLevel: ReactPriorityLevel, ) { let fiber = firstChild; while (fiber !== null) { const deletions = fiber.deletions; if (deletions !== null) { - commitMutationEffectsDeletions(deletions, root, renderPriorityLevel); + commitMutationEffectsDeletions( + deletions, + fiber, + root, + renderPriorityLevel, + ); } if (fiber.child !== null) { @@ -2577,6 +2582,7 @@ function commitMutationEffectsImpl( function commitMutationEffectsDeletions( deletions: Array, + nearestMountedAncestor: Fiber, root: FiberRoot, renderPriorityLevel, ) { @@ -2589,17 +2595,23 @@ function commitMutationEffectsDeletions( null, root, childToDelete, + nearestMountedAncestor, renderPriorityLevel, ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(childToDelete, childToDelete.return, error); + captureCommitPhaseError(childToDelete, nearestMountedAncestor, error); } } else { try { - commitDeletion(root, childToDelete, renderPriorityLevel); + commitDeletion( + root, + childToDelete, + nearestMountedAncestor, + renderPriorityLevel, + ); } catch (error) { - captureCommitPhaseError(childToDelete, childToDelete.return, error); + captureCommitPhaseError(childToDelete, nearestMountedAncestor, error); } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 84188fea374..d40a164b94a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2084,7 +2084,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2092,7 +2092,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitBeforeMutationEffects(); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2121,7 +2121,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2129,7 +2129,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitMutationEffects(root, renderPriorityLevel); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2156,7 +2156,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2164,7 +2164,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitLayoutEffects(root, lanes); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2365,7 +2365,10 @@ function commitBeforeMutationEffects() { } } -function commitMutationEffects(root: FiberRoot, renderPriorityLevel) { +function commitMutationEffects( + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, +) { // TODO: Should probably move the bulk of this function to commitWork. while (nextEffect !== null) { setCurrentDebugFiberInDEV(nextEffect); @@ -2436,7 +2439,12 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) { break; } case Deletion: { - commitDeletion(root, nextEffect, renderPriorityLevel); + commitDeletion( + root, + nextEffect, + nextEffect.return, + renderPriorityLevel, + ); break; } } @@ -2647,7 +2655,7 @@ function flushPassiveEffectsImpl() { if (hasCaughtError()) { invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { @@ -2668,7 +2676,7 @@ function flushPassiveEffectsImpl() { } } catch (error) { invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2695,7 +2703,7 @@ function flushPassiveEffectsImpl() { if (hasCaughtError()) { invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { @@ -2717,7 +2725,7 @@ function flushPassiveEffectsImpl() { } } catch (error) { invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2816,7 +2824,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. @@ -2824,7 +2836,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); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 7ff61126f64..75e52dd951b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2797,6 +2797,84 @@ describe('ReactHooksWithNoopRenderer', () => { 'Mount normal [current: 1]', ]); }); + + it('catches errors thrown in useLayoutEffect', () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + function Component({id}) { + Scheduler.unstable_yieldValue('Component render ' + id); + return ; + } + + function BrokenLayoutEffectDestroy() { + useLayoutEffect(() => { + return () => { + Scheduler.unstable_yieldValue( + 'BrokenLayoutEffectDestroy useLayoutEffect destroy', + ); + throw Error('Expected'); + }; + }, []); + + Scheduler.unstable_yieldValue('BrokenLayoutEffectDestroy render'); + return ; + } + + ReactNoop.render( + + + + + + , + ); + + expect(Scheduler).toFlushAndYield([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'BrokenLayoutEffectDestroy render', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('sibling'), + span('broken'), + ]); + + ReactNoop.render( + + + , + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(Scheduler).toFlushAndYield([ + 'OuterBoundary render success', + 'Component render sibling', + 'BrokenLayoutEffectDestroy useLayoutEffect destroy', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + expect(ReactNoop.getChildren()).toEqual([span('OuterFallback')]); + }); }); describe('useCallback', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 503dc98f0de..f41c9f661a1 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -992,12 +992,17 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.render(); expect(Scheduler).toFlushWithoutYielding(); - ReactNoop.render(null); - expect(Scheduler).toFlushAndYield([ - // Parent unmounts before the error is thrown. - 'Parent componentWillUnmount', - 'ThrowsOnUnmount componentWillUnmount', - ]); + + // Because the error boundary is also unmounting, + // an error in ThrowsOnUnmount should be rethrown. + expect(() => { + ReactNoop.render(null); + expect(Scheduler).toFlushAndYield([ + 'Parent componentWillUnmount', + 'ThrowsOnUnmount componentWillUnmount', + ]); + }).toThrow('unmount error'); + ReactNoop.render(); }); From a994a537cfd0aff8bc38668d15af5eebc551b61f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 12:10:09 -0400 Subject: [PATCH 2/3] Move skip-unmounting boundaries behavior behind a feature flag --- .../src/__tests__/ReactErrorBoundaries-test.internal.js | 1 + packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 9 ++++++++- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 9 ++++++++- .../src/__tests__/ReactHooksWithNoopRenderer-test.js | 1 + .../ReactIncrementalErrorHandling-test.internal.js | 1 + packages/shared/ReactFeatureFlags.js | 6 ++++++ packages/shared/forks/ReactFeatureFlags.native-fb.js | 1 + packages/shared/forks/ReactFeatureFlags.native-oss.js | 1 + packages/shared/forks/ReactFeatureFlags.test-renderer.js | 1 + .../forks/ReactFeatureFlags.test-renderer.native.js | 1 + .../shared/forks/ReactFeatureFlags.test-renderer.www.js | 1 + packages/shared/forks/ReactFeatureFlags.testing.js | 1 + packages/shared/forks/ReactFeatureFlags.testing.www.js | 1 + packages/shared/forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 1 + 15 files changed, 34 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index bf8fd72b730..9799f0f0453 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2474,6 +2474,7 @@ describe('ReactErrorBoundaries', () => { ); }); + // @gate skipUnmountedBoundaries it('catches errors thrown in componentWillUnmount', () => { class LocalErrorBoundary extends React.Component { state = {error: null}; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 578ffd427c7..c7d1faef4cf 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -30,6 +30,7 @@ import { enableDebugTracing, enableSchedulingProfiler, enableScopeAPI, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2950,7 +2951,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index d40a164b94a..ff3fd6a9d0e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -30,6 +30,7 @@ import { enableDebugTracing, enableSchedulingProfiler, enableScopeAPI, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2836,7 +2837,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 75e52dd951b..7fa56978e74 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2798,6 +2798,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('catches errors thrown in useLayoutEffect', () => { class ErrorBoundary extends React.Component { state = {error: null}; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index f41c9f661a1..06668b4eb38 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -961,6 +961,7 @@ describe('ReactIncrementalErrorHandling', () => { expect(Scheduler).toFlushAndYield(['Foo']); }); + // @gate skipUnmountedBoundaries it('should not attempt to recover an unmounting error boundary', () => { class Parent extends React.Component { componentWillUnmount() { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 634c19b14ea..2ae1805af95 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -92,6 +92,12 @@ export const enableComponentStackLocations = true; export const enableNewReconciler = false; +// Errors that are thrown while unmounting (or after in the case of passive effects) +// should bypass any error boundaries that are also unmounting (or have unmounted) +// and be handled by the nearest still-mounted boundary. +// If there are no still-mounted boundaries, the errors should be rethrown. +export const skipUnmountedBoundaries = false; + // -------------------------- // Future APIs to be deprecated // -------------------------- diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 664a11a43b9..6b61818c6d0 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -43,6 +43,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 9632b8b7da0..c8598c23cd9 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 6f348ed149b..47a729c1d0a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index ea8f7d4e0a9..4af72e9d901 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 3e5046f0fcc..7c25d50d07d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 8d4fe939dc0..5e530afbd9d 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index d3796937a30..e11f9f9d133 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = __EXPERIMENTAL__; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 9b23645a3d8..c7df70d1929 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -18,6 +18,7 @@ export const disableInputAttributeSyncing = __VARIANT__; export const enableFilterEmptyStringAttributesDOM = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; export const decoupleUpdatePriorityFromScheduler = __VARIANT__; +export const skipUnmountedBoundaries = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 1c637762fe6..35f5767413f 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -26,6 +26,7 @@ export const { deferRenderPhaseUpdateToNextBatch, decoupleUpdatePriorityFromScheduler, enableDebugTracing, + skipUnmountedBoundaries, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From a048c8536b9213b4283ac8f74242cf4a150e0bc1 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 14:49:41 -0400 Subject: [PATCH 3/3] Skip unmounted error boundaries for errors thrown during ref cleanup --- .../ReactErrorBoundaries-test.internal.js | 86 +++++++++++++++++++ .../src/ReactFiberCommitWork.new.js | 12 +-- .../src/ReactFiberCommitWork.old.js | 12 +-- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 9799f0f0453..204c67ba528 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2558,4 +2558,90 @@ describe('ReactErrorBoundaries', () => { 'Component render OuterFallback', ]); }); + + // @gate skipUnmountedBoundaries + it('catches errors thrown while detaching refs', () => { + class LocalErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + class Component extends React.Component { + render() { + const {id} = this.props; + Scheduler.unstable_yieldValue('Component render ' + id); + return id; + } + } + + class LocalBrokenCallbackRef extends React.Component { + _ref = ref => { + Scheduler.unstable_yieldValue('LocalBrokenCallbackRef ref ' + !!ref); + if (ref === null) { + throw Error('Expected'); + } + }; + + render() { + Scheduler.unstable_yieldValue('LocalBrokenCallbackRef render'); + return
ref
; + } + } + + const container = document.createElement('div'); + + ReactDOM.render( + + + + + + , + container, + ); + + expect(container.firstChild.textContent).toBe('sibling'); + expect(container.lastChild.textContent).toBe('ref'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'LocalBrokenCallbackRef render', + 'LocalBrokenCallbackRef ref true', + ]); + + ReactDOM.render( + + + , + container, + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(container.firstChild.textContent).toBe('OuterFallback'); + expect(container.lastChild.textContent).toBe('OuterFallback'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'LocalBrokenCallbackRef ref false', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index c451585a074..727df9a08ff 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -188,7 +188,7 @@ function safelyCallComponentWillUnmount( } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -196,13 +196,13 @@ function safelyDetachRef(current: Fiber) { invokeGuardedCallback(null, ref, null, null); if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { ref(null); } catch (refError) { - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -1020,7 +1020,7 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { safelyCallComponentWillUnmount( @@ -1032,7 +1032,7 @@ function commitUnmount( return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -1075,7 +1075,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 81b226670f1..3265edd9cf5 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -179,7 +179,7 @@ function safelyCallComponentWillUnmount( } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -187,13 +187,13 @@ function safelyDetachRef(current: Fiber) { invokeGuardedCallback(null, ref, null, null); if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { ref(null); } catch (refError) { - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -918,7 +918,7 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { safelyCallComponentWillUnmount( @@ -930,7 +930,7 @@ function commitUnmount( return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -973,7 +973,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; }