From f9950346ad81e2f91156782e68e93eccfc44602d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sun, 1 Apr 2018 01:49:13 +0100 Subject: [PATCH 1/5] Add regression tests for error boundary replay bugs --- .../ReactIncrementalErrorReplay-test.js | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.js diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.js new file mode 100644 index 0000000000000..a14c6a9b3e393 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorReplay-test.js @@ -0,0 +1,59 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +'use strict'; + +let React; +let ReactNoop; + +describe('ReactIncrementalErrorReplay', () => { + beforeEach(() => { + jest.resetModules(); + React = require('react'); + ReactNoop = require('react-noop-renderer'); + }); + + function div(...children) { + children = children.map(c => (typeof c === 'string' ? {text: c} : c)); + return {type: 'div', children, prop: undefined}; + } + + function span(prop) { + return {type: 'span', children: [], prop}; + } + + it('should fail gracefully on error in the host environment', () => { + ReactNoop.simulateErrorInHostConfig(() => { + ReactNoop.render(); + expect(() => ReactNoop.flush()).toThrow('Error in host config.'); + }); + }); + + it('should fail gracefully on error that does not reproduce on replay', () => { + let didInit = false; + + function badLazyInit() { + const needsInit = !didInit; + didInit = true; + if (needsInit) { + throw new Error('Hi'); + } + } + + class App extends React.Component { + render() { + badLazyInit(); + return
; + } + } + ReactNoop.render(); + expect(() => ReactNoop.flush()).toThrow('Hi'); + }); +}); From 1130f89fa7a0237d3573910ad0a8f6d59ae64a6c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 2 Apr 2018 23:24:22 +0100 Subject: [PATCH 2/5] Ensure the context stack is aligned if renderer throws --- .../react-reconciler/src/ReactFiberHostContext.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHostContext.js b/packages/react-reconciler/src/ReactFiberHostContext.js index f58651381a28c..7caa1671a7787 100644 --- a/packages/react-reconciler/src/ReactFiberHostContext.js +++ b/packages/react-reconciler/src/ReactFiberHostContext.js @@ -60,12 +60,19 @@ export default function( // Push current root instance onto the stack; // This allows us to reset root when portals are popped. push(rootInstanceStackCursor, nextRootInstance, fiber); - - const nextRootContext = getRootHostContext(nextRootInstance); - // Track the context and the Fiber that provided it. // This enables us to pop only Fibers that provide unique contexts. push(contextFiberStackCursor, fiber, fiber); + + // Finally, we need to push the host context to the stack. + // However, we can't just call getRootHostContext() and push it because + // we'd have a different number of entries on the stack depending on + // whether getRootHostContext() throws somewhere in renderer code or not. + // So we push an empty value first. This lets us safely unwind on errors. + push(contextStackCursor, NO_CONTEXT, fiber); + const nextRootContext = getRootHostContext(nextRootInstance); + // Now that we know this function doesn't throw, replace it. + pop(contextStackCursor, fiber); push(contextStackCursor, nextRootContext, fiber); } From 220af1eaaf8637d7b3a756ad38e23a0ff33c48d6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 2 Apr 2018 15:38:28 -0700 Subject: [PATCH 3/5] Always throw when replaying a failed unit of work Replaying a failed unit of work should always throw, because the render phase is meant to be idempotent, If it doesn't throw, rethrow the original error, so React's internal stack is not misaligned. --- .../src/ReactFiberScheduler.js | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 3ec6af40bcdc8..ade0e2de010f0 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -263,9 +263,16 @@ export default function( let stashedWorkInProgressProperties; let replayUnitOfWork; + let isReplayingFailedUnitOfWork; + let originalReplayError; if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { stashedWorkInProgressProperties = null; - replayUnitOfWork = (failedUnitOfWork: Fiber, isAsync: boolean) => { + isReplayingFailedUnitOfWork = false; + replayUnitOfWork = ( + failedUnitOfWork: Fiber, + error: mixed, + isAsync: boolean, + ) => { // Retore the original state of the work-in-progress assignFiberPropertiesInDEV( failedUnitOfWork, @@ -290,12 +297,16 @@ export default function( break; } // Replay the begin phase. + isReplayingFailedUnitOfWork = true; + originalReplayError = error; invokeGuardedCallback(null, workLoop, null, isAsync); + isReplayingFailedUnitOfWork = false; if (hasCaughtError()) { clearCaughtError(); } else { - // This should be unreachable because the render phase is - // idempotent + // If the begin phase did not fail the second time, set this pointer + // back to the original value. + nextUnitOfWork = failedUnitOfWork; } }; } @@ -855,9 +866,15 @@ export default function( ); } let next = beginWork(current, workInProgress, nextRenderExpirationTime); - if (__DEV__) { ReactDebugCurrentFiber.resetCurrentFiber(); + if (isReplayingFailedUnitOfWork) { + // Currently replaying a failed unit of work. This should be unreachable, + // because the render phase is meant to be idempotent, and it should + // have thrown again. Since it didn't, rethrow the original error, so + // React's internal stack is not misaligned. + throw originalReplayError; + } } if (__DEV__ && ReactFiberInstrumentation.debugTool) { ReactFiberInstrumentation.debugTool.onBeginWork(workInProgress); @@ -935,7 +952,7 @@ export default function( if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { const failedUnitOfWork = nextUnitOfWork; - replayUnitOfWork(failedUnitOfWork, isAsync); + replayUnitOfWork(failedUnitOfWork, thrownValue, isAsync); } const sourceFiber: Fiber = nextUnitOfWork; From 602a2069f40b204d87cf6e890f30abf3ff0c64fc Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 2 Apr 2018 15:55:06 -0700 Subject: [PATCH 4/5] Reset originalReplayError after replaying --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index ade0e2de010f0..981d0b23a2027 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -268,6 +268,7 @@ export default function( if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { stashedWorkInProgressProperties = null; isReplayingFailedUnitOfWork = false; + originalReplayError = null; replayUnitOfWork = ( failedUnitOfWork: Fiber, error: mixed, @@ -301,6 +302,7 @@ export default function( originalReplayError = error; invokeGuardedCallback(null, workLoop, null, isAsync); isReplayingFailedUnitOfWork = false; + originalReplayError = null; if (hasCaughtError()) { clearCaughtError(); } else { From 03cc7c4d1e11c9ea9cc2dfefa73ee3f7f6ecd5e0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 2 Apr 2018 23:58:09 +0100 Subject: [PATCH 5/5] Typo fix --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 981d0b23a2027..538738f8bf80a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -274,7 +274,7 @@ export default function( error: mixed, isAsync: boolean, ) => { - // Retore the original state of the work-in-progress + // Restore the original state of the work-in-progress assignFiberPropertiesInDEV( failedUnitOfWork, stashedWorkInProgressProperties,