Skip to content

Recover from errors with a boundary in completion phase #14104

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

Merged
gaearon merged 7 commits into from
Nov 6, 2018
Merged

Recover from errors with a boundary in completion phase #14104

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0aaf5e8
Recover from errors with a boundary in completion phase
gaearon Nov 5, 2018
9b82c9f
Use a separate field for completing unit of work
gaearon Nov 5, 2018
f644cb2
Use a simpler fix with one boolean
gaearon Nov 5, 2018
0fb8440
Reoder conditions
gaearon Nov 6, 2018
ac7c3ab
Clarify which paths are DEV-only
gaearon Nov 6, 2018
3b93b2e
Move duplicated line out
gaearon Nov 6, 2018
22158a5
Make it clearer this code is DEV-only
gaearon Nov 6, 2018
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
15 changes: 15 additions & 0 deletions packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2149,4 +2149,19 @@ describe('ReactErrorBoundaries', () => {
expect(componentDidCatchError).toBe(thrownError);
expect(getDerivedStateFromErrorError).toBe(thrownError);
});

it('should catch errors from invariants in completion phase', () => {
const container = document.createElement('div');
ReactDOM.render(
<ErrorBoundary>
<input>
<div />
</input>
</ErrorBoundary>,
container,
);
expect(container.textContent).toContain(
'Caught an error: input is a void element tag',
);
});
});
25 changes: 21 additions & 4 deletions packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,13 @@ let interruptedBy: Fiber | null = null;

let stashedWorkInProgressProperties;
let replayUnitOfWork;
let mayReplayFailedUnitOfWork;
let isReplayingFailedUnitOfWork;
let originalReplayError;
let rethrowOriginalError;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
stashedWorkInProgressProperties = null;
mayReplayFailedUnitOfWork = true;
isReplayingFailedUnitOfWork = false;
originalReplayError = null;
replayUnitOfWork = (
Expand Down Expand Up @@ -947,18 +949,22 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
const siblingFiber = workInProgress.sibling;

if ((workInProgress.effectTag & Incomplete) === NoEffect) {
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
// Don't replay if it fails during completion phase.
mayReplayFailedUnitOfWork = false;
}
// This fiber completed.
// Remember we're completing this unit so we can find a boundary if it fails.
nextUnitOfWork = workInProgress;
if (enableProfilerTimer) {
if (workInProgress.mode & ProfileMode) {
startProfilerTimer(workInProgress);
}

nextUnitOfWork = completeWork(
current,
workInProgress,
nextRenderExpirationTime,
);

if (workInProgress.mode & ProfileMode) {
// Update render duration assuming we didn't error.
stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false);
Expand All @@ -970,6 +976,10 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
nextRenderExpirationTime,
);
}
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
// We're out of completion phase so replaying is fine now.
mayReplayFailedUnitOfWork = true;
}
stopWorkTimer(workInProgress);
resetChildExpirationTime(workInProgress, nextRenderExpirationTime);
if (__DEV__) {
Expand Down Expand Up @@ -1277,6 +1287,11 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
resetContextDependences();
resetHooks();

// Reset in case completion throws.
// This is only used in DEV and when replaying is on.
const mayReplay = mayReplayFailedUnitOfWork;
mayReplayFailedUnitOfWork = true;
Copy link
Copy Markdown
Collaborator

@acdlite acdlite Nov 6, 2018

Choose a reason for hiding this comment

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

I don't love this boolean, especially because it's not dev only. But I'm more concerned that this replay logic is getting really convoluted and it seems there must be a better way to structure it.

Maybe we can move where replayUnitOfWork is called instead. If replayUnitOfWork is only called after a failed begin phase, we could fork beginWork to beginWorkInDEV which has a local try/catch.

function beginWorkInDEV(current, workInProgress, renderExpirationTime) {
  try {
    beginWork(current, workInProgress, renderExpirationTime);
  } catch (e) {
    resetContextDependences();
    resetHooks();

    // Either call replayUnitOfWork or inline the whole thing here

    // Then throw to outer catch block
    throw e;
  }
}

There might be other implications that I'm forgetting, but I think we should give this a shot before piling more complexity onto this code path.

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Nov 6, 2018

Choose a reason for hiding this comment

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

especially because it's not dev only

It is though. I think it would get DCE’d because we never read it outside DEV. The only reason I write to it outside a flag is just because it seems awkward to do extra wrapping because of scoping.

Copy link
Copy Markdown
Collaborator

@acdlite acdlite Nov 6, 2018

Choose a reason for hiding this comment

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

Just checked — this is what I originally did, but @sebmarkbage asked that I move the extra try out of the hot path: #12201 (comment)

I feel we should reconsider given the additional complexity of distinguishing between the begin and complete phases.

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Nov 6, 2018

Choose a reason for hiding this comment

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

Pushed another commit that makes it clearer that part is DEV-only too. I'll look at your suggestion tomorrow/


if (nextUnitOfWork === null) {
// This is a fatal error.
didFatal = true;
Expand All @@ -1288,9 +1303,11 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void {
(resetCurrentlyProcessingQueue: any)();
}

const failedUnitOfWork: Fiber = nextUnitOfWork;
if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
if (mayReplay) {
const failedUnitOfWork: Fiber = nextUnitOfWork;
replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
}
Copy link
Copy Markdown

@blixt blixt Nov 6, 2018

Choose a reason for hiding this comment

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

Does this mean that the bug only happens when in DEV?

Also, couldn't this code be combined into the following block to avoid implying that the conditions/variables have any effect in production?

        if (__DEV__) {
          // Reset global debug state
          // We assume this is defined in DEV
          (resetCurrentlyProcessingQueue: any)();

          if (!wasCompleting && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
            const failedUnitOfWork: Fiber = nextUnitOfWork;
            replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy);
          }
        }

Copy link
Copy Markdown
Collaborator Author

@gaearon gaearon Nov 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I pushed another commit that clarifies which parts of the fix are DEV-only.

}

// TODO: we already know this isn't true in some cases.
Expand Down
⚡