From 0aaf5e8dd9b1d6e0e0b92dec60fd49afc83e6ce0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 5 Nov 2018 17:38:58 +0000 Subject: [PATCH 1/7] Recover from errors with a boundary in completion phase --- .../ReactErrorBoundaries-test.internal.js | 15 +++++++++++++++ .../react-reconciler/src/ReactFiberScheduler.js | 3 +++ 2 files changed, 18 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index f85fc1fa13eac..4b7f5f3f6c5fd 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -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( + + +
+ + , + container, + ); + expect(container.textContent).toContain( + 'Caught an error: input is a void element tag', + ); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 866fa9cdcc655..238bf25dd12d0 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -947,6 +947,9 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { const siblingFiber = workInProgress.sibling; if ((workInProgress.effectTag & Incomplete) === NoEffect) { + // Prepare this field so we can find an error boundary in case completing throws. + nextUnitOfWork = workInProgress; + // This fiber completed. if (enableProfilerTimer) { if (workInProgress.mode & ProfileMode) { From 9b82c9fec8a077734fe71e993caffdb18a43003e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 5 Nov 2018 18:04:17 +0000 Subject: [PATCH 2/7] Use a separate field for completing unit of work --- .../src/ReactFiberScheduler.js | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 238bf25dd12d0..c7ee32875374f 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -254,6 +254,7 @@ let isWorking: boolean = false; // The next work in progress fiber that we're currently working on. let nextUnitOfWork: Fiber | null = null; let nextRoot: FiberRoot | null = null; +let nextCompletingUnitOfWork: Fiber | null = null; // The time at which we're currently rendering work. let nextRenderExpirationTime: ExpirationTime = NoWork; let nextLatestAbsoluteTimeoutMs: number = -1; @@ -947,10 +948,8 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { const siblingFiber = workInProgress.sibling; if ((workInProgress.effectTag & Incomplete) === NoEffect) { - // Prepare this field so we can find an error boundary in case completing throws. - nextUnitOfWork = workInProgress; - // This fiber completed. + nextCompletingUnitOfWork = workInProgress; if (enableProfilerTimer) { if (workInProgress.mode & ProfileMode) { startProfilerTimer(workInProgress); @@ -973,6 +972,8 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { nextRenderExpirationTime, ); } + nextCompletingUnitOfWork = null; + stopWorkTimer(workInProgress); resetChildExpirationTime(workInProgress, nextRenderExpirationTime); if (__DEV__) { @@ -1280,7 +1281,7 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { resetContextDependences(); resetHooks(); - if (nextUnitOfWork === null) { + if (nextUnitOfWork === null && nextCompletingUnitOfWork === null) { // This is a fatal error. didFatal = true; onUncaughtError(thrownValue); @@ -1291,22 +1292,33 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { (resetCurrentlyProcessingQueue: any)(); } - const failedUnitOfWork: Fiber = nextUnitOfWork; - if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { - replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy); - } + let sourceFiber: Fiber; + if (nextUnitOfWork !== null) { + const failedUnitOfWork: Fiber = nextUnitOfWork; + if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { + replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy); + } - // TODO: we already know this isn't true in some cases. - // At least this shows a nicer error message until we figure out the cause. - // https://github.com/facebook/react/issues/12449#issuecomment-386727431 - invariant( - nextUnitOfWork !== null, - 'Failed to replay rendering after an error. This ' + - 'is likely caused by a bug in React. Please file an issue ' + - 'with a reproducing case to help us find it.', - ); + // TODO: we already know this isn't true in some cases. + // At least this shows a nicer error message until we figure out the cause. + // https://github.com/facebook/react/issues/12449#issuecomment-386727431 + invariant( + nextUnitOfWork !== null, + 'Failed to replay rendering after an error. This ' + + 'is likely caused by a bug in React. Please file an issue ' + + 'with a reproducing case to help us find it.', + ); + sourceFiber = nextUnitOfWork; + } else { + invariant( + nextCompletingUnitOfWork !== null, + 'Expected to be completing a unit of work. This ' + + 'is likely caused by a bug in React. Please file an issue ' + + 'with a reproducing case to help us find it.', + ); + sourceFiber = nextCompletingUnitOfWork; + } - const sourceFiber: Fiber = nextUnitOfWork; let returnFiber = sourceFiber.return; if (returnFiber === null) { // This is the root. The root could capture its own errors. However, From f644cb250f87bfff4302776b978199827e5eacb9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 5 Nov 2018 18:10:07 +0000 Subject: [PATCH 3/7] Use a simpler fix with one boolean --- .../src/ReactFiberScheduler.js | 50 +++++++++---------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index c7ee32875374f..9d2a2e2dd9d16 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -254,7 +254,6 @@ let isWorking: boolean = false; // The next work in progress fiber that we're currently working on. let nextUnitOfWork: Fiber | null = null; let nextRoot: FiberRoot | null = null; -let nextCompletingUnitOfWork: Fiber | null = null; // The time at which we're currently rendering work. let nextRenderExpirationTime: ExpirationTime = NoWork; let nextLatestAbsoluteTimeoutMs: number = -1; @@ -264,6 +263,7 @@ let nextRenderDidError: boolean = false; let nextEffect: Fiber | null = null; let isCommitting: boolean = false; +let isCompleting: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; let passiveEffectCallbackHandle: * = null; let passiveEffectCallback: * = null; @@ -949,31 +949,36 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { if ((workInProgress.effectTag & Incomplete) === NoEffect) { // This fiber completed. - nextCompletingUnitOfWork = workInProgress; if (enableProfilerTimer) { if (workInProgress.mode & ProfileMode) { startProfilerTimer(workInProgress); } + // Remember we're completing this unit so we can find a boundary if it fails. + isCompleting = true; + nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, nextRenderExpirationTime, ); + isCompleting = false; if (workInProgress.mode & ProfileMode) { // Update render duration assuming we didn't error. stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false); } } else { + // Remember we're completing this unit so we can find a boundary if it fails. + isCompleting = true; + nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, nextRenderExpirationTime, ); + isCompleting = false; } - nextCompletingUnitOfWork = null; - stopWorkTimer(workInProgress); resetChildExpirationTime(workInProgress, nextRenderExpirationTime); if (__DEV__) { @@ -1281,7 +1286,9 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { resetContextDependences(); resetHooks(); - if (nextUnitOfWork === null && nextCompletingUnitOfWork === null) { + const wasCompleting = isCompleting; + isCompleting = false; + if (nextUnitOfWork === null) { // This is a fatal error. didFatal = true; onUncaughtError(thrownValue); @@ -1292,33 +1299,24 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { (resetCurrentlyProcessingQueue: any)(); } - let sourceFiber: Fiber; - if (nextUnitOfWork !== null) { + if (!wasCompleting) { const failedUnitOfWork: Fiber = nextUnitOfWork; if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy); } - - // TODO: we already know this isn't true in some cases. - // At least this shows a nicer error message until we figure out the cause. - // https://github.com/facebook/react/issues/12449#issuecomment-386727431 - invariant( - nextUnitOfWork !== null, - 'Failed to replay rendering after an error. This ' + - 'is likely caused by a bug in React. Please file an issue ' + - 'with a reproducing case to help us find it.', - ); - sourceFiber = nextUnitOfWork; - } else { - invariant( - nextCompletingUnitOfWork !== null, - 'Expected to be completing a unit of work. This ' + - 'is likely caused by a bug in React. Please file an issue ' + - 'with a reproducing case to help us find it.', - ); - sourceFiber = nextCompletingUnitOfWork; } + // TODO: we already know this isn't true in some cases. + // At least this shows a nicer error message until we figure out the cause. + // https://github.com/facebook/react/issues/12449#issuecomment-386727431 + invariant( + nextUnitOfWork !== null, + 'Failed to replay rendering after an error. This ' + + 'is likely caused by a bug in React. Please file an issue ' + + 'with a reproducing case to help us find it.', + ); + + const sourceFiber: Fiber = nextUnitOfWork; let returnFiber = sourceFiber.return; if (returnFiber === null) { // This is the root. The root could capture its own errors. However, From 0fb84406e62522652054c7a45d11593a83dc262c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 6 Nov 2018 13:04:16 +0000 Subject: [PATCH 4/7] Reoder conditions --- packages/react-reconciler/src/ReactFiberScheduler.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 9d2a2e2dd9d16..5008e3707b939 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1299,9 +1299,9 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { (resetCurrentlyProcessingQueue: any)(); } - if (!wasCompleting) { - const failedUnitOfWork: Fiber = nextUnitOfWork; - if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { + if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { + if (!wasCompleting) { + const failedUnitOfWork: Fiber = nextUnitOfWork; replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy); } } From ac7c3abc6e8267918b53401706909893fcd48dac Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 6 Nov 2018 13:12:37 +0000 Subject: [PATCH 5/7] Clarify which paths are DEV-only --- .../src/ReactFiberScheduler.js | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 5008e3707b939..841c0bc3a7055 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -263,7 +263,6 @@ let nextRenderDidError: boolean = false; let nextEffect: Fiber | null = null; let isCommitting: boolean = false; -let isCompleting: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; let passiveEffectCallbackHandle: * = null; let passiveEffectCallback: * = null; @@ -275,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 = ( @@ -948,36 +949,38 @@ 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. if (enableProfilerTimer) { if (workInProgress.mode & ProfileMode) { startProfilerTimer(workInProgress); } - // Remember we're completing this unit so we can find a boundary if it fails. - isCompleting = true; nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, nextRenderExpirationTime, ); - isCompleting = false; - if (workInProgress.mode & ProfileMode) { // Update render duration assuming we didn't error. stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false); } } else { // Remember we're completing this unit so we can find a boundary if it fails. - isCompleting = true; nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, nextRenderExpirationTime, ); - isCompleting = false; + } + if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { + // We're out of completion phase so replaying is fine now. + mayReplayFailedUnitOfWork = true; } stopWorkTimer(workInProgress); resetChildExpirationTime(workInProgress, nextRenderExpirationTime); @@ -1286,8 +1289,11 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { resetContextDependences(); resetHooks(); - const wasCompleting = isCompleting; - isCompleting = false; + // Reset in case completion throws. + // This is only used in DEV and when replaying is on. + const mayReplay = mayReplayFailedUnitOfWork; + mayReplayFailedUnitOfWork = true; + if (nextUnitOfWork === null) { // This is a fatal error. didFatal = true; @@ -1300,7 +1306,7 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { } if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { - if (!wasCompleting) { + if (mayReplay) { const failedUnitOfWork: Fiber = nextUnitOfWork; replayUnitOfWork(failedUnitOfWork, thrownValue, isYieldy); } From 3b93b2e8b127491282ac298cbbd525acfa5e11a1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 6 Nov 2018 13:13:57 +0000 Subject: [PATCH 6/7] Move duplicated line out --- packages/react-reconciler/src/ReactFiberScheduler.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 841c0bc3a7055..102d4fcd67937 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -954,12 +954,12 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { 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); } - // Remember we're completing this unit so we can find a boundary if it fails. - nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, @@ -970,8 +970,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { stopProfilerTimerIfRunningAndRecordDelta(workInProgress, false); } } else { - // Remember we're completing this unit so we can find a boundary if it fails. - nextUnitOfWork = workInProgress; nextUnitOfWork = completeWork( current, workInProgress, From 22158a509e77a8d6ab417c9f6911071c1a60237d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 6 Nov 2018 21:49:58 +0000 Subject: [PATCH 7/7] Make it clearer this code is DEV-only --- packages/react-reconciler/src/ReactFiberScheduler.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 102d4fcd67937..bccffa0bf6444 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1289,8 +1289,11 @@ function renderRoot(root: FiberRoot, isYieldy: boolean): void { // Reset in case completion throws. // This is only used in DEV and when replaying is on. - const mayReplay = mayReplayFailedUnitOfWork; - mayReplayFailedUnitOfWork = true; + let mayReplay; + if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { + mayReplay = mayReplayFailedUnitOfWork; + mayReplayFailedUnitOfWork = true; + } if (nextUnitOfWork === null) { // This is a fatal error.