Fix runtime error that happens if a passive destroy function throws within an unmounted tree (#19543)
A passive effect's cleanup function may throw after an unmount. In that event, React sometimes threw an uncaught runtime error trying to access a property on a null stateNode field. This commit fixes that (and adds a regression test).
Brian Vaughn committed
Aug 5, 2020 at 18:22 UTC
e67a6b16030ebc30257a69a7fb36a9ed67f29b39
3 files changed
+105
-1
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+4
-1
@@ -1011,6 +1011,10 @@ function detachFiberMutation(fiber: Fiber) {
1011
// with findDOMNode and how it requires the sibling field to carry out
1012
// traversal in a later effect. See PR #16820. We now clear the sibling
1013
// field after effects, see: detachFiberAfterEffects.
1014
+ //
1015
+ // Don't disconnect stateNode now; it will be detached in detachFiberAfterEffects.
1016
+ // It may be required if the current component is an error boundary,
1017
+ // and one of its descendants throws while unmounting a passive effect.
1018
fiber.alternate = null;
1019
fiber.child = null;
1020
fiber.dependencies = null;
@@ -1020,7 +1024,6 @@ function detachFiberMutation(fiber: Fiber) {
1024
fiber.memoizedState = null;
1025
fiber.pendingProps = null;
1026
fiber.return = null;
1023
- fiber.stateNode = null;
1027
fiber.updateQueue = null;
1028
if (__DEV__) {
1029
fiber._debugOwner = null;
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+1
@@ -3793,4 +3793,5 @@ export function act(callback: () => Thenable<mixed>): Thenable<void> {
3793
3794
function detachFiberAfterEffects(fiber: Fiber): void {
3795
fiber.sibling = null;
3796
+ fiber.stateNode = null;
3797
}
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+100
@@ -2316,6 +2316,106 @@ describe('ReactHooksWithNoopRenderer', () => {
2316
expect(Scheduler).toFlushAndYieldThrough(['Unmount: 1']);
2317
expect(ReactNoop.getChildren()).toEqual([]);
2318
});
2319
+
2320
+ describe('errors thrown in passive destroy function within unmounted trees', () => {
2321
+ let BrokenUseEffectCleanup;
2322
+ let ErrorBoundary;
2323
+ let LogOnlyErrorBoundary;
2324
+
2325
+ beforeEach(() => {
2326
+ BrokenUseEffectCleanup = function() {
2327
+ useEffect(() => {
2328
+ Scheduler.unstable_yieldValue('BrokenUseEffectCleanup useEffect');
2329
+ return () => {
2330
+ Scheduler.unstable_yieldValue(
2331
+ 'BrokenUseEffectCleanup useEffect destroy',
2332
+ );
2333
+ throw new Error('Expected error');
2334
+ };
2335
+ }, []);
2336
+
2337
+ return 'inner child';
2338
+ };
2339
+
2340
+ ErrorBoundary = class extends React.Component {
2341
+ state = {error: null};
2342
+ static getDerivedStateFromError(error) {
2343
+ Scheduler.unstable_yieldValue(
2344
+ `ErrorBoundary static getDerivedStateFromError`,
2345
+ );
2346
+ return {error};
2347
+ }
2348
+ componentDidCatch(error, info) {
2349
+ Scheduler.unstable_yieldValue(`ErrorBoundary componentDidCatch`);
2350
+ }
2351
+ render() {
2352
+ if (this.state.error) {
2353
+ Scheduler.unstable_yieldValue('ErrorBoundary render error');
2354
+ return 'ErrorBoundary fallback';
2355
+ }
2356
+ Scheduler.unstable_yieldValue('ErrorBoundary render success');
2357
+ return this.props.children;
2358
+ }
2359
+ };
2360
+
2361
+ LogOnlyErrorBoundary = class extends React.Component {
2362
+ componentDidCatch(error, info) {
2363
+ Scheduler.unstable_yieldValue(
2364
+ `LogOnlyErrorBoundary componentDidCatch`,
2365
+ );
2366
+ }
2367
+ render() {
2368
+ Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`);
2369
+ return this.props.children;
2370
+ }
2371
+ };
2372
+ });
2373
+
2374
+ it('should not error if the nearest unmounted boundary is log-only', () => {
2375
+ function Conditional({showChildren}) {
2376
+ if (showChildren) {
2377
+ return (
2378
+ <LogOnlyErrorBoundary>
2379
+ <BrokenUseEffectCleanup />
2380
+ </LogOnlyErrorBoundary>
2381
+ );
2382
+ } else {
2383
+ return null;
2384
+ }
2385
+ }
2386
+
2387
+ act(() => {
2388
+ ReactNoop.render(
2389
+ <ErrorBoundary>
2390
+ <Conditional showChildren={true} />
2391
+ </ErrorBoundary>,
2392
+ );
2393
+ });
2394
+
2395
+ expect(Scheduler).toHaveYielded([
2396
+ 'ErrorBoundary render success',
2397
+ 'LogOnlyErrorBoundary render',
2398
+ 'BrokenUseEffectCleanup useEffect',
2399
+ ]);
2400
+
2401
+ act(() => {
2402
+ ReactNoop.render(
2403
+ <ErrorBoundary>
2404
+ <Conditional showChildren={false} />
2405
+ </ErrorBoundary>,
2406
+ );
2407
+ expect(Scheduler).toFlushAndYieldThrough([
2408
+ 'ErrorBoundary render success',
2409
+ ]);
2410
+ });
2411
+
2412
+ expect(Scheduler).toHaveYielded([
2413
+ 'BrokenUseEffectCleanup useEffect destroy',
2414
+ // This should call componentDidCatch too, but we'll address that in a follow up.
2415
+ // 'LogOnlyErrorBoundary componentDidCatch',
2416
+ ]);
2417
+ });
2418
+ });
2419
});
2420
2421
describe('useLayoutEffect', () => {