From b1009e803268de50c8e6057c7317667779a1d373 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 9 Aug 2018 23:45:52 +0100 Subject: [PATCH 1/6] Remove e.suppressReactErrorLogging check before last resort throw It's unnecessary here. It was here because this method called console.error(). But we now rethrow with a clean stack, and that's worth doing regardless of whether the logging is silenced. --- .../src/ReactFiberCommitWork.js | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 42aa93a356857..f126796b0bf67 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -112,17 +112,13 @@ export function logError(boundary: Fiber, errorInfo: CapturedValue) { try { logCapturedError(capturedError); } catch (e) { - // Prevent cycle if logCapturedError() throws. - // A cycle may still occur if logCapturedError renders a component that throws. - const suppressLogging = e && e.suppressReactErrorLogging; - if (!suppressLogging) { - // Rethrow it from a clean stack because this function is assumed to never throw. - // We can't safely call console.error() here because it could *also* throw if overridden. - // https://github.com/facebook/react/issues/13188 - setTimeout(() => { - throw e; - }); - } + // This method must not throw, or React internal state will get messed up. + // If console.error is overridden, or logCapturedError() shows a dialog that throws, + // we want to report this error outside of the normal stack as a last resort. + // https://github.com/facebook/react/issues/13188 + setTimeout(() => { + throw e; + }); } } From dd9f09de8b661e9c712092565ef167e2101497fd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Aug 2018 19:51:21 +0100 Subject: [PATCH 2/6] Don't print error addendum if 'error' event got preventDefault() --- .../src/ReactFiberErrorLogger.js | 18 +++++++++++ .../src/ReactFiberScheduler.js | 12 +++++++- packages/shared/ReactErrorUtils.js | 30 +++++++++++++++++-- .../ReactErrorUtils-test.internal.js | 2 +- packages/shared/invokeGuardedCallback.js | 14 +++++++-- 5 files changed, 70 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index a5a4f756abc81..aad7042804191 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -9,6 +9,8 @@ import type {CapturedError} from './ReactCapturedValue'; +import ReactErrorUtils from 'shared/ReactErrorUtils'; + import {showErrorDialog} from './ReactFiberErrorDialog'; export function logCapturedError(capturedError: CapturedError): void { @@ -35,6 +37,22 @@ export function logCapturedError(capturedError: CapturedError): void { willRetry, } = capturedError; + // Browsers support silencing uncaught errors by calling + // `preventDefault()` in window `error` handler. + if ((ReactErrorUtils: any).isErrorSuppressedInDEV(error)) { + if (errorBoundaryFound && willRetry) { + // The error is recoverable and was silenced. + // Ignore it and print the stack addendum. + // This is handy for testing error boundaries without noise. + return; + } + // The error is fatal. Since the silencing might have + // been accidental, we'll surface it anyway. + // However, the browser would have silenced the original error + // so we'll print it first, and then print the stack addendum. + console.error(error); + } + const componentNameMessage = componentName ? `The above error occurred in the <${componentName}> component:` : 'The above error occurred in one of your React components:'; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index bca85e65a6f21..b77562dd7fa3c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -301,7 +301,17 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Replay the begin phase. isReplayingFailedUnitOfWork = true; originalReplayError = thrownValue; - invokeGuardedCallback(null, workLoop, null, isYieldy); + const didSuppressError = invokeGuardedCallback( + null, + workLoop, + null, + isYieldy, + ); + if (didSuppressError) { + // If logging of the replayed error was suppressed, + // we'll also remember that for the original error. + (ReactErrorUtils: any).markErrorAsSuppressedInDEV(thrownValue); + } isReplayingFailedUnitOfWork = false; originalReplayError = null; if (hasCaughtError()) { diff --git a/packages/shared/ReactErrorUtils.js b/packages/shared/ReactErrorUtils.js index ef3cb31a78370..40230188a73c9 100644 --- a/packages/shared/ReactErrorUtils.js +++ b/packages/shared/ReactErrorUtils.js @@ -42,8 +42,8 @@ const ReactErrorUtils = { d: D, e: E, f: F, - ): void { - invokeGuardedCallback.apply(ReactErrorUtils, arguments); + ): boolean | void { + return invokeGuardedCallback.apply(ReactErrorUtils, arguments); }, /** @@ -105,6 +105,32 @@ const ReactErrorUtils = { }, }; +if (__DEV__) { + (ReactErrorUtils: any)._suppressedErrorsInDEV = + typeof WeakSet === 'function' ? new WeakSet() : null; + + (ReactErrorUtils: any).markErrorAsSuppressedInDEV = function(error: any) { + const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV; + if ( + suppressedErrors != null && + error != null && + typeof error === 'object' + ) { + suppressedErrors.add(error); + } + }; + + (ReactErrorUtils: any).isErrorSuppressedInDEV = function( + error: any, + ): boolean { + const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV; + if (suppressedErrors != null) { + return suppressedErrors.has(error); + } + return false; + }; +} + let rethrowCaughtError = function() { if (ReactErrorUtils._hasRethrowError) { const error = ReactErrorUtils._rethrowError; diff --git a/packages/shared/__tests__/ReactErrorUtils-test.internal.js b/packages/shared/__tests__/ReactErrorUtils-test.internal.js index b0a6b58137584..421da6bcf7eaf 100644 --- a/packages/shared/__tests__/ReactErrorUtils-test.internal.js +++ b/packages/shared/__tests__/ReactErrorUtils-test.internal.js @@ -66,7 +66,7 @@ describe('ReactErrorUtils', () => { 'arg1', 'arg2', ); - expect(returnValue).toBe(undefined); + expect(returnValue).toBe(true); expect(ReactErrorUtils.hasCaughtError()).toBe(true); expect(ReactErrorUtils.clearCaughtError()).toBe(error); }); diff --git a/packages/shared/invokeGuardedCallback.js b/packages/shared/invokeGuardedCallback.js index fa6ee451c9193..d1dea469d7a2d 100644 --- a/packages/shared/invokeGuardedCallback.js +++ b/packages/shared/invokeGuardedCallback.js @@ -124,15 +124,23 @@ if (__DEV__) { let error; // Use this to track whether the error event is ever called. let didSetError = false; + let didSuppressError = false; let isCrossOriginError = false; - function onError(event) { + const onError = event => { error = event.error; didSetError = true; if (error === null && event.colno === 0 && event.lineno === 0) { isCrossOriginError = true; } - } + if (event.defaultPrevented) { + // Some other error handler has prevented default. + // Browsers silence the error report if this happens. + // We'll remember this to later decide whether to log it or not. + this.markErrorAsSuppressedInDEV(error); + didSuppressError = true; + } + }; // Create a fake event type. const evtType = `react-${name ? name : 'invokeguardedcallback'}`; @@ -175,6 +183,8 @@ if (__DEV__) { // Remove our event listeners window.removeEventListener('error', onError); + + return didSuppressError; }; invokeGuardedCallback = invokeGuardedCallbackDev; From 977ae186978f29b047c636e83e4e738d41db4401 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Aug 2018 19:51:29 +0100 Subject: [PATCH 3/6] Add fixtures --- .../fixtures/error-handling/index.js | 164 +++++++++++++++++- fixtures/dom/src/style.css | 2 +- 2 files changed, 163 insertions(+), 3 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/error-handling/index.js b/fixtures/dom/src/components/fixtures/error-handling/index.js index b25a56fbf78b4..e278dbfcffdfd 100644 --- a/fixtures/dom/src/components/fixtures/error-handling/index.js +++ b/fixtures/dom/src/components/fixtures/error-handling/index.js @@ -7,9 +7,21 @@ const ReactDOM = window.ReactDOM; function BadRender(props) { props.doThrow(); } + +class BadDidMount extends React.Component { + componentDidMount() { + this.props.doThrow(); + } + + render() { + return null; + } +} + class ErrorBoundary extends React.Component { static defaultProps = { buttonText: 'Trigger error', + badChildType: BadRender, }; state = { shouldThrow: false, @@ -33,7 +45,8 @@ class ErrorBoundary extends React.Component { } } if (this.state.shouldThrow) { - return ; + const BadChild = this.props.badChildType; + return ; } return ; } @@ -84,6 +97,112 @@ class TriggerErrorAndCatch extends React.Component { } } +function silenceWindowError(event) { + event.preventDefault(); +} + +class SilenceErrors extends React.Component { + state = { + silenceErrors: false, + }; + componentDidMount() { + if (this.state.silenceErrors) { + window.addEventListener('error', silenceWindowError); + } + } + componentDidUpdate(prevProps, prevState) { + if (!prevState.silenceErrors && this.state.silenceErrors) { + window.addEventListener('error', silenceWindowError); + } else if (prevState.silenceErrors && !this.state.silenceErrors) { + window.removeEventListener('error', silenceWindowError); + } + } + componentWillUnmount() { + if (this.state.silenceErrors) { + window.removeEventListener('error', silenceWindowError); + } + } + render() { + return ( +
+ + {this.state.silenceErrors && ( +
+ {this.props.children} +
+
+ + Don't forget to uncheck "Silence errors" when you're done with + this test! + +
+ )} +
+ ); + } +} + +class SilenceRecoverableError extends React.Component { + render() { + return ( + + { + throw new Error('Silenced error (render phase)'); + }} + /> + { + throw new Error('Silenced error (commit phase)'); + }} + /> + + ); + } +} + +class TrySilenceFatalError extends React.Component { + container = document.createElement('div'); + + triggerErrorAndCatch = () => { + try { + ReactDOM.flushSync(() => { + ReactDOM.render( + { + throw new Error('Caught error'); + }} + />, + this.container + ); + }); + } catch (e) {} + }; + + render() { + return ( + + + + ); + } +} + export default class ErrorHandlingTestCases extends React.Component { render() { return ( @@ -103,6 +222,12 @@ export default class ErrorHandlingTestCases extends React.Component { the BadRender component. After resuming, the "Trigger error" button should be replaced with "Captured an error: Oops!" Clicking reset should reset the test case. +
+
+ In the console, you should see two messages: the actual error + ("Oops") printed natively by the browser with its JavaScript stack, + and our addendum ("The above error occurred in BadRender component") + with a React component stack. { @@ -155,10 +280,45 @@ export default class ErrorHandlingTestCases extends React.Component { Open the console. "Uncaught Error: Caught error" should have been - logged by the browser. + logged by the browser. You should also see our addendum ("The above + error..."). + + +
  • Check the "Silence errors" checkbox below
  • +
  • Click the "Throw (render phase)" button
  • +
  • Click the "Throw (commit phase)" button
  • +
  • Uncheck the "Silence errors" checkbox
  • +
    + + Open the console. You shouldn't see any messages in the + console: neither the browser error, nor our "The above error" + addendum, from either of the buttons. The buttons themselves should + get replaced by two labels: "Captured an error: Silenced error + (render phase)" and "Captured an error: Silenced error (commit + phase)". + + +
    + + +
  • Check the "Silence errors" checkbox below
  • +
  • Click the "Throw fatal error" button
  • +
  • Uncheck the "Silence errors" checkbox
  • +
    + + Open the console. "Uncaught Error: Caught error" should have been + logged by React. You should also see our addendum ("The above + error..."). + + +
    ); } diff --git a/fixtures/dom/src/style.css b/fixtures/dom/src/style.css index c316bb3f37eb1..a8e1391e1d8e7 100644 --- a/fixtures/dom/src/style.css +++ b/fixtures/dom/src/style.css @@ -158,7 +158,7 @@ li { } .test-case__body { - padding: 0 15px; + padding: 10px; } .test-case__desc { From c4339328782ce3c903dabb961029712fae0d51fd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Aug 2018 21:06:14 +0100 Subject: [PATCH 4/6] Use an expando property instead of a WeakSet --- .../fixtures/error-handling/index.js | 5 ++-- .../src/ReactFiberErrorLogger.js | 5 ++-- .../src/ReactFiberScheduler.js | 26 ++++++++-------- packages/shared/ReactErrorUtils.js | 30 ++----------------- .../ReactErrorUtils-test.internal.js | 2 +- packages/shared/invokeGuardedCallback.js | 16 +++++----- 6 files changed, 30 insertions(+), 54 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/error-handling/index.js b/fixtures/dom/src/components/fixtures/error-handling/index.js index e278dbfcffdfd..8dcb8e1dc5e0a 100644 --- a/fixtures/dom/src/components/fixtures/error-handling/index.js +++ b/fixtures/dom/src/components/fixtures/error-handling/index.js @@ -313,9 +313,8 @@ export default class ErrorHandlingTestCases extends React.Component {
  • Uncheck the "Silence errors" checkbox
  • - Open the console. "Uncaught Error: Caught error" should have been - logged by React. You should also see our addendum ("The above - error..."). + Open the console. "Error: Caught error" should have been logged by + React. You should also see our addendum ("The above error..."). diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index aad7042804191..291dce9d3b017 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -9,8 +9,6 @@ import type {CapturedError} from './ReactCapturedValue'; -import ReactErrorUtils from 'shared/ReactErrorUtils'; - import {showErrorDialog} from './ReactFiberErrorDialog'; export function logCapturedError(capturedError: CapturedError): void { @@ -39,7 +37,8 @@ export function logCapturedError(capturedError: CapturedError): void { // Browsers support silencing uncaught errors by calling // `preventDefault()` in window `error` handler. - if ((ReactErrorUtils: any).isErrorSuppressedInDEV(error)) { + // We record this information as an expando on the error. + if (error != null && error._suppressLogging) { if (errorBoundaryFound && willRetry) { // The error is recoverable and was silenced. // Ignore it and print the stack addendum. diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index b77562dd7fa3c..f3e81f37cd6f8 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -301,21 +301,23 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { // Replay the begin phase. isReplayingFailedUnitOfWork = true; originalReplayError = thrownValue; - const didSuppressError = invokeGuardedCallback( - null, - workLoop, - null, - isYieldy, - ); - if (didSuppressError) { - // If logging of the replayed error was suppressed, - // we'll also remember that for the original error. - (ReactErrorUtils: any).markErrorAsSuppressedInDEV(thrownValue); - } + invokeGuardedCallback(null, workLoop, null, isYieldy); isReplayingFailedUnitOfWork = false; originalReplayError = null; if (hasCaughtError()) { - clearCaughtError(); + const replayError = clearCaughtError(); + if ( + replayError != null && + thrownValue != null && + replayError._suppressLogging + ) { + // Also suppress logging for the original error. + try { + (thrownValue: any)._suppressLogging = true; + } catch (inner) { + // Ignore. + } + } } else { // If the begin phase did not fail the second time, set this pointer // back to the original value. diff --git a/packages/shared/ReactErrorUtils.js b/packages/shared/ReactErrorUtils.js index 40230188a73c9..ef3cb31a78370 100644 --- a/packages/shared/ReactErrorUtils.js +++ b/packages/shared/ReactErrorUtils.js @@ -42,8 +42,8 @@ const ReactErrorUtils = { d: D, e: E, f: F, - ): boolean | void { - return invokeGuardedCallback.apply(ReactErrorUtils, arguments); + ): void { + invokeGuardedCallback.apply(ReactErrorUtils, arguments); }, /** @@ -105,32 +105,6 @@ const ReactErrorUtils = { }, }; -if (__DEV__) { - (ReactErrorUtils: any)._suppressedErrorsInDEV = - typeof WeakSet === 'function' ? new WeakSet() : null; - - (ReactErrorUtils: any).markErrorAsSuppressedInDEV = function(error: any) { - const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV; - if ( - suppressedErrors != null && - error != null && - typeof error === 'object' - ) { - suppressedErrors.add(error); - } - }; - - (ReactErrorUtils: any).isErrorSuppressedInDEV = function( - error: any, - ): boolean { - const suppressedErrors = (ReactErrorUtils: any)._suppressedErrorsInDEV; - if (suppressedErrors != null) { - return suppressedErrors.has(error); - } - return false; - }; -} - let rethrowCaughtError = function() { if (ReactErrorUtils._hasRethrowError) { const error = ReactErrorUtils._rethrowError; diff --git a/packages/shared/__tests__/ReactErrorUtils-test.internal.js b/packages/shared/__tests__/ReactErrorUtils-test.internal.js index 421da6bcf7eaf..b0a6b58137584 100644 --- a/packages/shared/__tests__/ReactErrorUtils-test.internal.js +++ b/packages/shared/__tests__/ReactErrorUtils-test.internal.js @@ -66,7 +66,7 @@ describe('ReactErrorUtils', () => { 'arg1', 'arg2', ); - expect(returnValue).toBe(true); + expect(returnValue).toBe(undefined); expect(ReactErrorUtils.hasCaughtError()).toBe(true); expect(ReactErrorUtils.clearCaughtError()).toBe(error); }); diff --git a/packages/shared/invokeGuardedCallback.js b/packages/shared/invokeGuardedCallback.js index d1dea469d7a2d..c0cce27cc4f1d 100644 --- a/packages/shared/invokeGuardedCallback.js +++ b/packages/shared/invokeGuardedCallback.js @@ -124,10 +124,9 @@ if (__DEV__) { let error; // Use this to track whether the error event is ever called. let didSetError = false; - let didSuppressError = false; let isCrossOriginError = false; - const onError = event => { + function onError(event) { error = event.error; didSetError = true; if (error === null && event.colno === 0 && event.lineno === 0) { @@ -137,10 +136,15 @@ if (__DEV__) { // Some other error handler has prevented default. // Browsers silence the error report if this happens. // We'll remember this to later decide whether to log it or not. - this.markErrorAsSuppressedInDEV(error); - didSuppressError = true; + if (error != null && typeof error === 'object') { + try { + error._suppressLogging = true; + } catch (inner) { + // Ignore. + } + } } - }; + } // Create a fake event type. const evtType = `react-${name ? name : 'invokeguardedcallback'}`; @@ -183,8 +187,6 @@ if (__DEV__) { // Remove our event listeners window.removeEventListener('error', onError); - - return didSuppressError; }; invokeGuardedCallback = invokeGuardedCallbackDev; From da9d6f6f64b89baeaf2d318bc1270c7a0a00f279 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Aug 2018 21:21:06 +0100 Subject: [PATCH 5/6] Make it a bit less fragile --- .../react-reconciler/src/ReactFiberScheduler.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f3e81f37cd6f8..1231e7073f21d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -306,14 +306,14 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { originalReplayError = null; if (hasCaughtError()) { const replayError = clearCaughtError(); - if ( - replayError != null && - thrownValue != null && - replayError._suppressLogging - ) { - // Also suppress logging for the original error. + if (replayError != null && thrownValue != null) { try { - (thrownValue: any)._suppressLogging = true; + // Reading the expando property is intentionally + // inside `try` because it might be a getter or Proxy. + if (replayError._suppressLogging) { + // Also suppress logging for the original error. + (thrownValue: any)._suppressLogging = true; + } } catch (inner) { // Ignore. } From 2b225247a15f2f5ad3358b2f7d8201747c423430 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Aug 2018 21:31:49 +0100 Subject: [PATCH 6/6] Clarify comments --- packages/react-reconciler/src/ReactFiberErrorLogger.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index 291dce9d3b017..81b1c7366d2ed 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -41,7 +41,7 @@ export function logCapturedError(capturedError: CapturedError): void { if (error != null && error._suppressLogging) { if (errorBoundaryFound && willRetry) { // The error is recoverable and was silenced. - // Ignore it and print the stack addendum. + // Ignore it and don't print the stack addendum. // This is handy for testing error boundaries without noise. return; } @@ -50,6 +50,8 @@ export function logCapturedError(capturedError: CapturedError): void { // However, the browser would have silenced the original error // so we'll print it first, and then print the stack addendum. console.error(error); + // For a more detailed description of this block, see: + // https://github.com/facebook/react/pull/13384 } const componentNameMessage = componentName