@samitouri / QOS-React / commits / 5cdd744e06

Fix bugs related to unmounting error boundaries (#10403)

* Don't warn about setState on unmounted when scheduling error recovery We shouldn't schedule an update on unmounted error boundaries, but we don't know if a boundary is unmounted until we traverse its parents. Added an additional argument to scheduleUpdate so we know not to warn about setState on unmounted components. * Should be able to unmount an error boundary before it is handled Fixes the case where an error boundary captures an error, but its parent is unmounted before we can re-render it. componentDidCatch is never called, and we don't remove the boundary from our set of unhandled error boundaries. We should not assume that if capturedErrors is non-null that we still have unhandled errors.

Andrew Clark committed Aug 8, 2017 at 10:09 UTC 5cdd744e0610c914aa34ec1eb5a566006d693cae

2 files changed +121 -23

src/renderers/shared/fiber/ReactFiberScheduler.js

+22 -12

@@ -761,7 +761,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

761 // The loop stops once the children have unmounted and error lifecycles are

762 // called. Then we return to the regular flow.

763

764 - if (capturedErrors !== null && capturedErrors.size > 0) {

764 + if (

765 + capturedErrors !== null &&

766 + capturedErrors.size > 0 &&

767 + nextPriorityLevel === TaskPriority

768 + ) {

769 while (nextUnitOfWork !== null) {

770 if (hasCapturedError(nextUnitOfWork)) {

771 // Use a forked version of performUnitOfWork

@@ -780,19 +784,17 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

784 commitAllWork(pendingCommit);

785 priorityContext = nextPriorityLevel;

786

783 - if (capturedErrors === null || capturedErrors.size === 0) {

787 + if (

788 + capturedErrors === null ||

789 + capturedErrors.size === 0 ||

790 + nextPriorityLevel !== TaskPriority

791 + ) {

792 // There are no more unhandled errors. We can exit this special

793 // work loop. If there's still additional work, we'll perform it

794 // using one of the normal work loops.

795 break;

796 }

797 // The commit phase produced additional errors. Continue working.

790 - invariant(

791 - nextPriorityLevel === TaskPriority,

792 - 'Commit phase errors should be scheduled to recover with task ' +

793 - 'priority. This error is likely caused by a bug in React. ' +

794 - 'Please file an issue.',

795 - );

798 }

799 }

800 }

@@ -1129,7 +1131,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

1131 willRetry = true;

1132 }

1133 } else if (node.tag === HostRoot) {

1132 - // Treat the root like a no-op error boundary.

1134 + // Treat the root like a no-op error boundary

1135 boundary = node;

1136 }

1137

@@ -1349,6 +1351,14 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

1351 }

1352

1353 function scheduleUpdate(fiber: Fiber, priorityLevel: PriorityLevel) {

1354 + return scheduleUpdateImpl(fiber, priorityLevel, false);

1355 + }

1356 +

1357 + function scheduleUpdateImpl(

1358 + fiber: Fiber,

1359 + priorityLevel: PriorityLevel,

1360 + isErrorRecovery: boolean,

1361 + ) {

1362 if (__DEV__) {

1363 recordScheduleUpdate();

1364 }

@@ -1372,7 +1382,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

1382 }

1383

1384 if (__DEV__) {

1375 - if (fiber.tag === ClassComponent) {

1385 + if (!isErrorRecovery && fiber.tag === ClassComponent) {

1386 const instance = fiber.stateNode;

1387 warnAboutInvalidUpdates(instance);

1388 }

@@ -1438,7 +1448,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

1448 }

1449 } else {

1450 if (__DEV__) {

1441 - if (fiber.tag === ClassComponent) {

1451 + if (!isErrorRecovery && fiber.tag === ClassComponent) {

1452 warnAboutUpdateOnUnmounted(fiber.stateNode);

1453 }

1454 }

@@ -1478,7 +1488,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(

1488 }

1489

1490 function scheduleErrorRecovery(fiber: Fiber) {

1481 - scheduleUpdate(fiber, TaskPriority);

1491 + scheduleUpdateImpl(fiber, TaskPriority, true);

1492 }

1493

1494 function performWithPriority(priorityLevel: PriorityLevel, fn: Function) {

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

+99 -11

@@ -177,11 +177,7 @@ describe('ReactIncrementalErrorHandling', () => {

177 }

178

179 ReactNoop.flushSync(() => {

180 - ReactNoop.render(

181 - <ErrorBoundary>

182 - Before the storm.

183 - </ErrorBoundary>,

184 - );

180 + ReactNoop.render(<ErrorBoundary>Before the storm.</ErrorBoundary>);

181 ReactNoop.render(

182 <ErrorBoundary>

183 <BrokenRender />

@@ -327,9 +323,7 @@ describe('ReactIncrementalErrorHandling', () => {

323 expect(() => {

324 ReactNoop.flushSync(() => {

325 ReactNoop.render(

330 - <RethrowErrorBoundary>

331 - Before the storm.

332 - </RethrowErrorBoundary>,

326 + <RethrowErrorBoundary>Before the storm.</RethrowErrorBoundary>,

327 );

328 ReactNoop.render(

329 <RethrowErrorBoundary>

@@ -478,6 +472,82 @@ describe('ReactIncrementalErrorHandling', () => {

472 expect(ops).toEqual(['Foo']);

473 });

474

475 + it('should not attempt to recover an unmounting error boundary', () => {

476 + class Parent extends React.Component {

477 + componentWillUnmount() {

478 + ReactNoop.yield('Parent componentWillUnmount');

479 + }

480 + render() {

481 + return <Boundary />;

482 + }

483 + }

484 +

485 + class Boundary extends React.Component {

486 + componentDidCatch(e) {

487 + ReactNoop.yield(`Caught error: ${e.message}`);

488 + }

489 + render() {

490 + return <ThrowsOnUnmount />;

491 + }

492 + }

493 +

494 + class ThrowsOnUnmount extends React.Component {

495 + componentWillUnmount() {

496 + ReactNoop.yield('ThrowsOnUnmount componentWillUnmount');

497 + throw new Error('unmount error');

498 + }

499 + render() {

500 + return null;

501 + }

502 + }

503 +

504 + ReactNoop.render(<Parent />);

505 + ReactNoop.flush();

506 + ReactNoop.render(null);

507 + expect(ReactNoop.flush()).toEqual([

508 + // Parent unmounts before the error is thrown.

509 + 'Parent componentWillUnmount',

510 + 'ThrowsOnUnmount componentWillUnmount',

511 + ]);

512 + ReactNoop.render(<Parent />);

513 + });

514 +

515 + it('can unmount an error boundary before it is handled', () => {

516 + let parent;

517 +

518 + class Parent extends React.Component {

519 + state = {step: 0};

520 + render() {

521 + parent = this;

522 + return this.state.step === 0 ? <Boundary /> : null;

523 + }

524 + }

525 +

526 + class Boundary extends React.Component {

527 + componentDidCatch() {}

528 + render() {

529 + return <Child />;

530 + }

531 + }

532 +

533 + class Child extends React.Component {

534 + componentDidUpdate() {

535 + parent.setState({step: 1});

536 + throw new Error('update error');

537 + }

538 + render() {

539 + return null;

540 + }

541 + }

542 +

543 + ReactNoop.render(<Parent />);

544 + ReactNoop.flush();

545 +

546 + ReactNoop.flushSync(() => {

547 + ReactNoop.render(<Parent />);

548 + });

549 + });

550 +

551 it('continues work on other roots despite caught errors', () => {

552 class ErrorBoundary extends React.Component {

553 state = {error: null};

@@ -896,7 +966,13 @@ describe('ReactIncrementalErrorHandling', () => {

966 }

967

968 try {

899 - ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);

969 + ReactNoop.render(

970 + <div>

971 + <span>

972 + <ErrorThrowingComponent />

973 + </span>

974 + </div>,

975 + );

976 ReactNoop.flushDeferredPri();

977 } catch (error) {}

978

@@ -927,7 +1003,13 @@ describe('ReactIncrementalErrorHandling', () => {

1003 }

1004

1005 try {

930 - ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);

1006 + ReactNoop.render(

1007 + <div>

1008 + <span>

1009 + <ErrorThrowingComponent />

1010 + </span>

1011 + </div>,

1012 + );

1013 ReactNoop.flushDeferredPri();

1014 } catch (error) {}

1015

@@ -965,7 +1047,13 @@ describe('ReactIncrementalErrorHandling', () => {

1047 );

1048

1049 try {

968 - ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);

1050 + ReactNoop.render(

1051 + <div>

1052 + <span>

1053 + <ErrorThrowingComponent />

1054 + </span>

1055 + </div>,

1056 + );

1057 ReactNoop.flushDeferredPri();

1058 } catch (error) {}

1059