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 bugs related to unmounting error boundaries #10403

Merged
merged 2 commits into from Aug 8, 2017

Conversation

Projects
None yet
4 participants
@acdlite
Member

acdlite commented Aug 7, 2017

Fixes two bugs

  1. 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.

  2. 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. A non-null capturedErrors does not imply that we have unhandled errors.

@@ -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) {

This comment has been minimized.

@sophiebits

sophiebits Aug 7, 2017

Member

Can the warnAboutInvalidUpdates above cause an error?

@sophiebits

sophiebits Aug 7, 2017

Member

Can the warnAboutInvalidUpdates above cause an error?

This comment has been minimized.

@acdlite

acdlite Aug 7, 2017

Member

Good call, I'll add a guard there as well.

@acdlite

acdlite Aug 7, 2017

Member

Good call, I'll add a guard there as well.

@@ -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) {

This comment has been minimized.

@sophiebits

sophiebits Aug 7, 2017

Member

Why don't we want to throw here any more? My suggestion:

diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -753,7 +753,7 @@
 
   function handleCommitPhaseErrors() {
     // This is a special work loop for handling commit phase errors. It's
-    // similar to the syncrhonous work loop, but does an additional check on
+    // similar to the synchronous work loop, but does an additional check on
     // each fiber to see if it's an error boundary with an unhandled error. If
     // so, it uses a forked version of performUnitOfWork that unmounts the
     // failed subtree.
@@ -761,41 +761,40 @@
     // 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) {
-      while (nextUnitOfWork !== null) {
-        if (hasCapturedError(nextUnitOfWork)) {
-          // Use a forked version of performUnitOfWork
-          nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
-        } else {
-          nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
-        }
-        if (nextUnitOfWork === null) {
-          invariant(
-            pendingCommit !== null,
-            'Should have a pending commit. This error is likely caused by ' +
-              'a bug in React. Please file an issue.',
-          );
-          // We just completed a root. Commit it now.
-          priorityContext = TaskPriority;
-          commitAllWork(pendingCommit);
-          priorityContext = nextPriorityLevel;
-
-          if (capturedErrors === null || capturedErrors.size === 0) {
-            // 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.',
-          );
-        }
+    while (capturedErrors !== null && capturedErrors.size > 0) {
+      invariant(
+        nextUnitOfWork !== null,
+        'Expected work to have been scheduled for commit phase error ' +
+          'recovery. This error is likely caused by a bug in React. Please ' +
+          'file an issue.',
+      );
+      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.',
+      );
+      if (hasCapturedError(nextUnitOfWork)) {
+        // Use a forked version of performUnitOfWork
+        nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
+      } else {
+        nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
+      }
+      if (nextUnitOfWork === null) {
+        invariant(
+          pendingCommit !== null,
+          'Should have a pending commit. This error is likely caused by ' +
+            'a bug in React. Please file an issue.',
+        );
+        // We just completed a root. Commit it now.
+        priorityContext = TaskPriority;
+        commitAllWork(pendingCommit);
+        priorityContext = nextPriorityLevel;
       }
     }
+    // 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.
   }
 
   function workLoop(

seemed clearer about intent to me.

@sophiebits

sophiebits Aug 7, 2017

Member

Why don't we want to throw here any more? My suggestion:

diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js
--- a/src/renderers/shared/fiber/ReactFiberScheduler.js
+++ b/src/renderers/shared/fiber/ReactFiberScheduler.js
@@ -753,7 +753,7 @@
 
   function handleCommitPhaseErrors() {
     // This is a special work loop for handling commit phase errors. It's
-    // similar to the syncrhonous work loop, but does an additional check on
+    // similar to the synchronous work loop, but does an additional check on
     // each fiber to see if it's an error boundary with an unhandled error. If
     // so, it uses a forked version of performUnitOfWork that unmounts the
     // failed subtree.
@@ -761,41 +761,40 @@
     // 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) {
-      while (nextUnitOfWork !== null) {
-        if (hasCapturedError(nextUnitOfWork)) {
-          // Use a forked version of performUnitOfWork
-          nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
-        } else {
-          nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
-        }
-        if (nextUnitOfWork === null) {
-          invariant(
-            pendingCommit !== null,
-            'Should have a pending commit. This error is likely caused by ' +
-              'a bug in React. Please file an issue.',
-          );
-          // We just completed a root. Commit it now.
-          priorityContext = TaskPriority;
-          commitAllWork(pendingCommit);
-          priorityContext = nextPriorityLevel;
-
-          if (capturedErrors === null || capturedErrors.size === 0) {
-            // 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.',
-          );
-        }
+    while (capturedErrors !== null && capturedErrors.size > 0) {
+      invariant(
+        nextUnitOfWork !== null,
+        'Expected work to have been scheduled for commit phase error ' +
+          'recovery. This error is likely caused by a bug in React. Please ' +
+          'file an issue.',
+      );
+      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.',
+      );
+      if (hasCapturedError(nextUnitOfWork)) {
+        // Use a forked version of performUnitOfWork
+        nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork);
+      } else {
+        nextUnitOfWork = performUnitOfWork(nextUnitOfWork);
+      }
+      if (nextUnitOfWork === null) {
+        invariant(
+          pendingCommit !== null,
+          'Should have a pending commit. This error is likely caused by ' +
+            'a bug in React. Please file an issue.',
+        );
+        // We just completed a root. Commit it now.
+        priorityContext = TaskPriority;
+        commitAllWork(pendingCommit);
+        priorityContext = nextPriorityLevel;
       }
     }
+    // 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.
   }
 
   function workLoop(

seemed clearer about intent to me.

This comment has been minimized.

@acdlite

acdlite Aug 7, 2017

Member

We decided that cleaning up capturedErrors when an error boundary's parent is unmounted (see test added in 894656c) wasn't worth it, and so capturedErrors !== null is not a reliable indicator that there are unhandled errors.

If you're referring to moving the check to a while loop, that's how it used to work, but we recently refactored the work loops to do as few checks as possible on each iteration, and only do certain checks (like whether the priority level is correct) right after committing, which is the only time they change. Requires a bit more duplicated code but fewer runtime checks.

@acdlite

acdlite Aug 7, 2017

Member

We decided that cleaning up capturedErrors when an error boundary's parent is unmounted (see test added in 894656c) wasn't worth it, and so capturedErrors !== null is not a reliable indicator that there are unhandled errors.

If you're referring to moving the check to a while loop, that's how it used to work, but we recently refactored the work loops to do as few checks as possible on each iteration, and only do certain checks (like whether the priority level is correct) right after committing, which is the only time they change. Requires a bit more duplicated code but fewer runtime checks.

acdlite added some commits Aug 7, 2017

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.

@acdlite acdlite merged commit 5cdd744 into facebook:master Aug 8, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@bvaughn bvaughn referenced this pull request Aug 8, 2017

Closed

React 16 RC #10294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment