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.

Sign up for GitHub

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
Merged

Fix bugs related to unmounting error boundaries #10403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7fc149
Don't warn about setState on unmounted when scheduling error recovery
acdlite Aug 7, 2017
e07542b
Should be able to unmount an error boundary before it is handled
acdlite Aug 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

@sophiebits sophiebits Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if (
capturedErrors !== null &&
capturedErrors.size > 0 &&
nextPriorityLevel === TaskPriority
) {
while (nextUnitOfWork !== null) {
if (hasCapturedError(nextUnitOfWork)) {
// Use a forked version of performUnitOfWork
Expand All @@ -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.',
);
}
}
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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();
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the warnAboutInvalidUpdates above cause an error?

Copy link
Collaborator Author

@acdlite acdlite Aug 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

warnAboutUpdateOnUnmounted(fiber.stateNode);
}
}
Expand Down Expand Up @@ -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) {
Expand Down
110 changes: 99 additions & 11 deletions src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 />
Expand Down Expand Up @@ -327,9 +323,7 @@ describe('ReactIncrementalErrorHandling', () => {
expect(() => {
ReactNoop.flushSync(() => {
ReactNoop.render(
<RethrowErrorBoundary>
Before the storm.
</RethrowErrorBoundary>,
<RethrowErrorBoundary>Before the storm.</RethrowErrorBoundary>,
);
ReactNoop.render(
<RethrowErrorBoundary>
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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) {}

Expand Down Expand Up @@ -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) {}

Expand Down Expand Up @@ -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) {}

Expand Down