From 812cf1c6a3dc8dd62f4d663b0e179ae831718e6e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 25 Jul 2017 09:44:43 -0700 Subject: [PATCH] [invokeGuardedCallback] Handle nested errors across separate renderers (#10270) invokeGuardedCallback is a function we use in place of try-catch statement. It accepts a function, and if the function throws, it captures the error. In production, the implementation is a normal try- catch. In development, we swap out the prod implementation for a special version designed to preserve "Pause on all exceptions" behavior of the browser DevTools. invokeGuardedCallbackDev works by dispatching an event to a dummy DOM node and calling the provided function inside a handler for that event. We also attach an error event handler to the window object. If the function throws, the global event handler is called and we can access the error. The global event handler is added and removed right before and after the fake event is dispatched. But if invokeGuardedCallbackDev is nested -- that is, if it's invoked inside the body of another invokeGuardedCallbackDev -- multiple error event handlers will attached simultaneously. We only want the handler that corresponds to the deepest level to handle the error. So we keep track of a depth counter, and within the event handler, we only handle the error if the current depth matches the depth at the time the function was invoked. The problem that we discovered, and that this PR fixes, is that the depth counter is local to each renderer. So if you nest separate copies of invokeGuardedCallback from separate renderers, each renderer will have its own depth counter, and multiple error handlers will fire for a single, nested error. --- scripts/fiber/tests-passing.txt | 6 ++-- .../shared/fiber/ReactFiberCommitWork.js | 2 +- src/renderers/shared/utils/ReactErrorUtils.js | 33 +++++++++++-------- .../utils/__tests__/ReactErrorUtils-test.js | 33 ++++++++++++++++++- 4 files changed, 57 insertions(+), 17 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 9269029426..31f59bcf08 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -2334,7 +2334,8 @@ src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js * should catch errors (development) * should return false from clearCaughtError if no error was thrown (development) * can nest with same debug name (development) -* does not return nested errors (development) +* handles nested errors (development) +* handles nested errors in separate renderers * can be shimmed (development) * it should rethrow caught errors (production) * should call the callback the passed arguments (production) @@ -2342,7 +2343,8 @@ src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js * should catch errors (production) * should return false from clearCaughtError if no error was thrown (production) * can nest with same debug name (production) -* does not return nested errors (production) +* handles nested errors (production) +* handles nested errors in separate renderers * catches null values * can be shimmed (production) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index d8c3beb6c8..d6ed708926 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -47,7 +47,7 @@ if (__DEV__) { module.exports = function( config: HostConfig, - captureError: (failedFiber: Fiber, error: Error) => Fiber | null, + captureError: (failedFiber: Fiber, error: mixed) => Fiber | null, ) { const { commitMount, diff --git a/src/renderers/shared/utils/ReactErrorUtils.js b/src/renderers/shared/utils/ReactErrorUtils.js index afea4a7882..0f6c507910 100644 --- a/src/renderers/shared/utils/ReactErrorUtils.js +++ b/src/renderers/shared/utils/ReactErrorUtils.js @@ -16,12 +16,12 @@ const invariant = require('fbjs/lib/invariant'); const ReactErrorUtils = { // Used by Fiber to simulate a try-catch. - _caughtError: null, - _hasCaughtError: false, + _caughtError: (null: mixed), + _hasCaughtError: (false: boolean), // Used by event system to capture/rethrow the first error. - _rethrowError: null, - _hasRethrowError: false, + _rethrowError: (null: mixed), + _hasRethrowError: (false: boolean), injection: { injectErrorUtils(injectedErrorUtils: Object) { @@ -155,33 +155,40 @@ if (__DEV__) { e, f, ) { - ReactErrorUtils._hasCaughtError = false; - ReactErrorUtils._caughtError = null; - depth++; - const thisDepth = depth; + + let error; + let didError = true; const funcArgs = Array.prototype.slice.call(arguments, 3); const boundFunc = function() { func.apply(context, funcArgs); + didError = false; }; const onFakeEventError = function(event) { - // Don't capture nested errors - if (depth === thisDepth) { - ReactErrorUtils._caughtError = event.error; - ReactErrorUtils._hasCaughtError = true; - } + error = event.error; if (preventDefault) { event.preventDefault(); } }; + const evtType = `react-${name ? name : 'invokeguardedcallback'}-${depth}`; window.addEventListener('error', onFakeEventError); fakeNode.addEventListener(evtType, boundFunc, false); const evt = document.createEvent('Event'); + evt.initEvent(evtType, false, false); fakeNode.dispatchEvent(evt); + if (didError) { + ReactErrorUtils._hasCaughtError = true; + ReactErrorUtils._caughtError = error; + } else { + ReactErrorUtils._hasCaughtError = false; + ReactErrorUtils._caughtError = null; + } + fakeNode.removeEventListener(evtType, boundFunc, false); window.removeEventListener('error', onFakeEventError); + depth--; }; diff --git a/src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js b/src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js index 7384e5c84f..a0a0b808b7 100644 --- a/src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js +++ b/src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js @@ -132,7 +132,7 @@ describe('ReactErrorUtils', () => { expect(err4).toBe(err3); }); - it(`does not return nested errors (${environment})`, () => { + it(`handles nested errors (${environment})`, () => { const err1 = new Error(); let err2; ReactErrorUtils.invokeGuardedCallback( @@ -155,6 +155,37 @@ describe('ReactErrorUtils', () => { expect(err2).toBe(err1); }); + it('handles nested errors in separate renderers', () => { + const ReactErrorUtils1 = require('ReactErrorUtils'); + jest.resetModules(); + const ReactErrorUtils2 = require('ReactErrorUtils'); + expect(ReactErrorUtils1).not.toEqual(ReactErrorUtils2); + + let ops = []; + + ReactErrorUtils1.invokeGuardedCallback( + null, + () => { + ReactErrorUtils2.invokeGuardedCallback( + null, + () => { + throw new Error('nested error'); + }, + null, + ); + // ReactErrorUtils2 should catch the error + ops.push(ReactErrorUtils2.hasCaughtError()); + ops.push(ReactErrorUtils2.clearCaughtError().message); + }, + null, + ); + + // ReactErrorUtils1 should not catch the error + ops.push(ReactErrorUtils1.hasCaughtError()); + + expect(ops).toEqual([true, 'nested error', false]); + }); + if (environment === 'production') { // jsdom doesn't handle this properly, but Chrome and Firefox should. Test // this with a fixture.