From 5cdd744e0610c914aa34ec1eb5a566006d693cae Mon Sep 17 00:00:00 2001
From: Andrew Clark <acdlite@me.com>
Date: Tue, 8 Aug 2017 10:09:43 -0700
Subject: [PATCH] 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.
---
 .../shared/fiber/ReactFiberScheduler.js       |  34 ++++--
 .../ReactIncrementalErrorHandling-test.js     | 110 ++++++++++++++++--
 2 files changed, 121 insertions(+), 23 deletions(-)

diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
index df1a13cfd4..be8545fcac 100644
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -761,7 +761,11 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
     // The loop stops once the children have unmounted and error lifecycles are
     // called. Then we return to the regular flow.
 
-    if (capturedErrors !== null && capturedErrors.size > 0) {
+    if (
+      capturedErrors !== null &&
+      capturedErrors.size > 0 &&
+      nextPriorityLevel === TaskPriority
+    ) {
       while (nextUnitOfWork !== null) {
         if (hasCapturedError(nextUnitOfWork)) {
           // Use a forked version of performUnitOfWork
@@ -780,19 +784,17 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
           commitAllWork(pendingCommit);
           priorityContext = nextPriorityLevel;
 
-          if (capturedErrors === null || capturedErrors.size === 0) {
+          if (
+            capturedErrors === null ||
+            capturedErrors.size === 0 ||
+            nextPriorityLevel !== TaskPriority
+          ) {
             // There are no more unhandled errors. We can exit this special
             // work loop. If there's still additional work, we'll perform it
             // using one of the normal work loops.
             break;
           }
           // The commit phase produced additional errors. Continue working.
-          invariant(
-            nextPriorityLevel === TaskPriority,
-            'Commit phase errors should be scheduled to recover with task ' +
-              'priority. This error is likely caused by a bug in React. ' +
-              'Please file an issue.',
-          );
         }
       }
     }
@@ -1129,7 +1131,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
             willRetry = true;
           }
         } else if (node.tag === HostRoot) {
-          // Treat the root like a no-op error boundary.
+          // Treat the root like a no-op error boundary
           boundary = node;
         }
 
@@ -1349,6 +1351,14 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
   }
 
   function scheduleUpdate(fiber: Fiber, priorityLevel: PriorityLevel) {
+    return scheduleUpdateImpl(fiber, priorityLevel, false);
+  }
+
+  function scheduleUpdateImpl(
+    fiber: Fiber,
+    priorityLevel: PriorityLevel,
+    isErrorRecovery: boolean,
+  ) {
     if (__DEV__) {
       recordScheduleUpdate();
     }
@@ -1372,7 +1382,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
     }
 
     if (__DEV__) {
-      if (fiber.tag === ClassComponent) {
+      if (!isErrorRecovery && fiber.tag === ClassComponent) {
         const instance = fiber.stateNode;
         warnAboutInvalidUpdates(instance);
       }
@@ -1438,7 +1448,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
           }
         } else {
           if (__DEV__) {
-            if (fiber.tag === ClassComponent) {
+            if (!isErrorRecovery && fiber.tag === ClassComponent) {
               warnAboutUpdateOnUnmounted(fiber.stateNode);
             }
           }
@@ -1478,7 +1488,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
   }
 
   function scheduleErrorRecovery(fiber: Fiber) {
-    scheduleUpdate(fiber, TaskPriority);
+    scheduleUpdateImpl(fiber, TaskPriority, true);
   }
 
   function performWithPriority(priorityLevel: PriorityLevel, fn: Function) {
diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
index 83b1ad07b1..95e8628670 100644
--- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
+++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
@@ -177,11 +177,7 @@ describe('ReactIncrementalErrorHandling', () => {
     }
 
     ReactNoop.flushSync(() => {
-      ReactNoop.render(
-        <ErrorBoundary>
-          Before the storm.
-        </ErrorBoundary>,
-      );
+      ReactNoop.render(<ErrorBoundary>Before the storm.</ErrorBoundary>);
       ReactNoop.render(
         <ErrorBoundary>
           <BrokenRender />
@@ -327,9 +323,7 @@ describe('ReactIncrementalErrorHandling', () => {
     expect(() => {
       ReactNoop.flushSync(() => {
         ReactNoop.render(
-          <RethrowErrorBoundary>
-            Before the storm.
-          </RethrowErrorBoundary>,
+          <RethrowErrorBoundary>Before the storm.</RethrowErrorBoundary>,
         );
         ReactNoop.render(
           <RethrowErrorBoundary>
@@ -478,6 +472,82 @@ describe('ReactIncrementalErrorHandling', () => {
     expect(ops).toEqual(['Foo']);
   });
 
+  it('should not attempt to recover an unmounting error boundary', () => {
+    class Parent extends React.Component {
+      componentWillUnmount() {
+        ReactNoop.yield('Parent componentWillUnmount');
+      }
+      render() {
+        return <Boundary />;
+      }
+    }
+
+    class Boundary extends React.Component {
+      componentDidCatch(e) {
+        ReactNoop.yield(`Caught error: ${e.message}`);
+      }
+      render() {
+        return <ThrowsOnUnmount />;
+      }
+    }
+
+    class ThrowsOnUnmount extends React.Component {
+      componentWillUnmount() {
+        ReactNoop.yield('ThrowsOnUnmount componentWillUnmount');
+        throw new Error('unmount error');
+      }
+      render() {
+        return null;
+      }
+    }
+
+    ReactNoop.render(<Parent />);
+    ReactNoop.flush();
+    ReactNoop.render(null);
+    expect(ReactNoop.flush()).toEqual([
+      // Parent unmounts before the error is thrown.
+      'Parent componentWillUnmount',
+      'ThrowsOnUnmount componentWillUnmount',
+    ]);
+    ReactNoop.render(<Parent />);
+  });
+
+  it('can unmount an error boundary before it is handled', () => {
+    let parent;
+
+    class Parent extends React.Component {
+      state = {step: 0};
+      render() {
+        parent = this;
+        return this.state.step === 0 ? <Boundary /> : null;
+      }
+    }
+
+    class Boundary extends React.Component {
+      componentDidCatch() {}
+      render() {
+        return <Child />;
+      }
+    }
+
+    class Child extends React.Component {
+      componentDidUpdate() {
+        parent.setState({step: 1});
+        throw new Error('update error');
+      }
+      render() {
+        return null;
+      }
+    }
+
+    ReactNoop.render(<Parent />);
+    ReactNoop.flush();
+
+    ReactNoop.flushSync(() => {
+      ReactNoop.render(<Parent />);
+    });
+  });
+
   it('continues work on other roots despite caught errors', () => {
     class ErrorBoundary extends React.Component {
       state = {error: null};
@@ -896,7 +966,13 @@ describe('ReactIncrementalErrorHandling', () => {
       }
 
       try {
-        ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);
+        ReactNoop.render(
+          <div>
+            <span>
+              <ErrorThrowingComponent />
+            </span>
+          </div>,
+        );
         ReactNoop.flushDeferredPri();
       } catch (error) {}
 
@@ -927,7 +1003,13 @@ describe('ReactIncrementalErrorHandling', () => {
       }
 
       try {
-        ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);
+        ReactNoop.render(
+          <div>
+            <span>
+              <ErrorThrowingComponent />
+            </span>
+          </div>,
+        );
         ReactNoop.flushDeferredPri();
       } catch (error) {}
 
@@ -965,7 +1047,13 @@ describe('ReactIncrementalErrorHandling', () => {
       );
 
       try {
-        ReactNoop.render(<div><span><ErrorThrowingComponent /></span></div>);
+        ReactNoop.render(
+          <div>
+            <span>
+              <ErrorThrowingComponent />
+            </span>
+          </div>,
+        );
         ReactNoop.flushDeferredPri();
       } catch (error) {}