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.

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

Fix runtime error that happens if a passive destroy function throws within an unmounted tree #19543

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,10 @@ function detachFiberMutation(fiber: Fiber) {
// with findDOMNode and how it requires the sibling field to carry out
// traversal in a later effect. See PR #16820. We now clear the sibling
// field after effects, see: detachFiberAfterEffects.
//
// Don't disconnect stateNode now; it will be detached in detachFiberAfterEffects.
// It may be required if the current component is an error boundary,
// and one of its descendants throws while unmounting a passive effect.
fiber.alternate = null;
fiber.child = null;
fiber.dependencies = null;
Expand All @@ -1020,7 +1024,6 @@ function detachFiberMutation(fiber: Fiber) {
fiber.memoizedState = null;
fiber.pendingProps = null;
fiber.return = null;
fiber.stateNode = null;
fiber.updateQueue = null;
if (__DEV__) {
fiber._debugOwner = null;
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -3793,4 +3793,5 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {

function detachFiberAfterEffects(fiber: Fiber): void {
fiber.sibling = null;
fiber.stateNode = null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2316,6 +2316,106 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toFlushAndYieldThrough(['Unmount: 1']);
expect(ReactNoop.getChildren()).toEqual([]);
});

describe('errors thrown in passive destroy function within unmounted trees', () => {
let BrokenUseEffectCleanup;
let ErrorBoundary;
let LogOnlyErrorBoundary;

beforeEach(() => {
BrokenUseEffectCleanup = function() {
useEffect(() => {
Scheduler.unstable_yieldValue('BrokenUseEffectCleanup useEffect');
return () => {
Scheduler.unstable_yieldValue(
'BrokenUseEffectCleanup useEffect destroy',
);
throw new Error('Expected error');
};
}, []);

return 'inner child';
};

ErrorBoundary = class extends React.Component {
state = {error: null};
static getDerivedStateFromError(error) {
Scheduler.unstable_yieldValue(
`ErrorBoundary static getDerivedStateFromError`,
);
return {error};
}
componentDidCatch(error, info) {
Scheduler.unstable_yieldValue(`ErrorBoundary componentDidCatch`);
}
render() {
if (this.state.error) {
Scheduler.unstable_yieldValue('ErrorBoundary render error');
return 'ErrorBoundary fallback';
}
Scheduler.unstable_yieldValue('ErrorBoundary render success');
return this.props.children;
}
};

LogOnlyErrorBoundary = class extends React.Component {
componentDidCatch(error, info) {
Scheduler.unstable_yieldValue(
`LogOnlyErrorBoundary componentDidCatch`,
);
}
render() {
Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`);
return this.props.children;
}
};
});

it('should not error if the nearest unmounted boundary is log-only', () => {
function Conditional({showChildren}) {
if (showChildren) {
return (
<LogOnlyErrorBoundary>
<BrokenUseEffectCleanup />
</LogOnlyErrorBoundary>
);
} else {
return null;
}
}

act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={true} />
</ErrorBoundary>,
);
});

expect(Scheduler).toHaveYielded([
'ErrorBoundary render success',
'LogOnlyErrorBoundary render',
'BrokenUseEffectCleanup useEffect',
]);

act(() => {
ReactNoop.render(
<ErrorBoundary>
<Conditional showChildren={false} />
</ErrorBoundary>,
);
expect(Scheduler).toFlushAndYieldThrough([
'ErrorBoundary render success',
]);
});

expect(Scheduler).toHaveYielded([
'BrokenUseEffectCleanup useEffect destroy',
// This should call componentDidCatch too, but we'll address that in a follow up.
// 'LogOnlyErrorBoundary componentDidCatch',
]);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll add a few more tests to this new block in the follow up PR #19542

});

describe('useLayoutEffect', () => {
Expand Down