Merge pull request #8961 from acdlite/fiberbreakonuncaught

[Fiber] Preserve "Break on all/uncaught exceptions" behavior in DEV mode

scripts/fiber/tests-passing.txt

@@ -1667,11 +1667,20 @@ src/renderers/shared/stack/reconciler/__tests__/Transaction-test.js
 * should allow nesting of transactions
 
 src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js
-* should call the callback with only the passed argument
-* should catch errors
-* should rethrow caught errors
-* should call the callback with only the passed argument
-* should use invokeGuardedCallbackWithCatch in production
+* it should rethrow errors caught by invokeGuardedCallbackAndCatchFirstError (development)
+* should call the callback the passed arguments (development)
+* should call the callback with the provided context (development)
+* should return a caught error (development)
+* should return null if no error is thrown (development)
+* can nest with same debug name (development)
+* does not return nested errors (development)
+* it should rethrow errors caught by invokeGuardedCallbackAndCatchFirstError (production)
+* should call the callback the passed arguments (production)
+* should call the callback with the provided context (production)
+* should return a caught error (production)
+* should return null if no error is thrown (production)
+* can nest with same debug name (production)
+* does not return nested errors (production)
 
 src/renderers/shared/utils/__tests__/accumulateInto-test.js
 * throws if the second item is null

src/renderers/native/__tests__/ReactNativeEvents-test.js

@@ -13,7 +13,6 @@
 
 var RCTEventEmitter;
 var React;
-var ReactErrorUtils;
 var ReactNative;
 var ResponderEventPlugin;
 var UIManager;
@@ -24,16 +23,10 @@ beforeEach(() => {
 
   RCTEventEmitter = require('RCTEventEmitter');
   React = require('React');
-  ReactErrorUtils = require('ReactErrorUtils');
   ReactNative = require('ReactNative');
   ResponderEventPlugin = require('ResponderEventPlugin');
   UIManager = require('UIManager');
   createReactNativeComponentClass = require('createReactNativeComponentClass');
-
-  // Ensure errors from event callbacks are properly surfaced (otherwise,
-  // jest/jsdom swallows them when we do the .dispatchEvent call)
-  ReactErrorUtils.invokeGuardedCallback =
-    ReactErrorUtils.invokeGuardedCallbackWithCatch;
 });
 
 it('handles events', () => {

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -26,6 +26,7 @@ var {
 } = ReactTypeOfWork;
 var { commitCallbacks } = require('ReactFiberUpdateQueue');
 var { onCommitUnmount } = require('ReactFiberDevToolsHook');
+var { invokeGuardedCallback } = require('ReactErrorUtils');
 
 var {
   Placement,
@@ -54,22 +55,35 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
 
   // Capture errors so they don't interrupt unmounting.
   function safelyCallComponentWillUnmount(current, instance) {
-    try {
-      instance.componentWillUnmount();
-    } catch (error) {
-      captureError(current, error);
+    if (__DEV__) {
+      const unmountError = invokeGuardedCallback(null, instance.componentWillUnmount, instance);
+      if (unmountError) {
+        captureError(current, unmountError);
+      }
+    } else {
+      try {
+        instance.componentWillUnmount();
+      } catch (unmountError) {
+        captureError(current, unmountError);
+      }
     }
   }
 
-  // Capture errors so they don't interrupt unmounting.
   function safelyDetachRef(current : Fiber) {
-    try {
-      const ref = current.ref;
-      if (ref !== null) {
-        ref(null);
+    const ref = current.ref;
+    if (ref !== null) {
+      if (__DEV__) {
+        const refError = invokeGuardedCallback(null, ref, null, null);
+        if (refError !== null) {
+          captureError(current, refError);
+        }
+      } else {
+        try {
+          ref(null);
+        } catch (refError) {
+          captureError(current, refError);
+        }
       }
-    } catch (error) {
-      captureError(current, error);
     }
   }
 

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -35,6 +35,7 @@ var {
   getStackAddendumByWorkInProgressFiber,
 } = require('ReactComponentTreeHook');
 var { logCapturedError } = require('ReactFiberErrorLogger');
+var { invokeGuardedCallback } = require('ReactErrorUtils');
 
 var ReactFiberBeginWork = require('ReactFiberBeginWork');
 var ReactFiberCompleteWork = require('ReactFiberCompleteWork');
@@ -164,6 +165,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
   // Keeps track of whether we're currently in a work loop.
   let isPerformingWork : boolean = false;
 
+  // Keeps track of whether the current deadline has expired.
+  let deadlineHasExpired : boolean = false;
+
   // Keeps track of whether we should should batch sync updates.
   let isBatchingUpdates : boolean = false;
 
@@ -414,9 +418,17 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
     // ref unmounts.
     nextEffect = firstEffect;
     while (nextEffect !== null) {
-      try {
-        commitAllHostEffects(finishedWork);
-      } catch (error) {
+      let error = null;
+      if (__DEV__) {
+        error = invokeGuardedCallback(null, commitAllHostEffects, null, finishedWork);
+      } else {
+        try {
+          commitAllHostEffects(finishedWork);
+        } catch (e) {
+          error = e;
+        }
+      }
+      if (error !== null) {
         invariant(
           nextEffect !== null,
           'Should have next effect. This error is likely caused by a bug ' +
@@ -444,9 +456,17 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
     // This pass also triggers any renderer-specific initial effects.
     nextEffect = firstEffect;
     while (nextEffect !== null) {
-      try {
-        commitAllLifeCycles(finishedWork, nextEffect);
-      } catch (error) {
+      let error = null;
+      if (__DEV__) {
+        error = invokeGuardedCallback(null, commitAllLifeCycles, null, finishedWork);
+      } else {
+        try {
+          commitAllLifeCycles(finishedWork);
+        } catch (e) {
+          error = e;
+        }
+      }
+      if (error !== null) {
         invariant(
           nextEffect !== null,
           'Should have next effect. This error is likely caused by a bug ' +
@@ -675,7 +695,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
     }
   }
 
-  function workLoop(priorityLevel, deadline : Deadline | null, deadlineHasExpired : boolean) : boolean {
+  function workLoop(priorityLevel, deadline : Deadline | null) {
     // Clear any errors.
     clearErrors();
 
@@ -743,8 +763,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
     if (hostRootTimeMarker) {
       console.timeEnd(hostRootTimeMarker);
     }
-
-    return deadlineHasExpired;
   }
 
   function performWork(priorityLevel : PriorityLevel, deadline : Deadline | null) {
@@ -755,7 +773,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
     );
     isPerformingWork = true;
     const isPerformingDeferredWork = Boolean(deadline);
-    let deadlineHasExpired = false;
 
     // This outer loop exists so that we can restart the work loop after
     // catching an error. It also lets us flush Task work at the end of a
@@ -776,18 +793,25 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
       // Nothing in performWork should be allowed to throw. All unsafe
       // operations must happen within workLoop, which is extracted to a
       // separate function so that it can be optimized by the JS engine.
-      try {
-        priorityContextBeforeReconciliation = priorityContext;
-        priorityContext = nextPriorityLevel;
-        deadlineHasExpired = workLoop(priorityLevel, deadline, deadlineHasExpired);
-      } catch (error) {
+      priorityContextBeforeReconciliation = priorityContext;
+      let error = null;
+      if (__DEV__) {
+        error = invokeGuardedCallback(null, workLoop, null, priorityLevel, deadline);
+      } else {
+        try {
+          workLoop(priorityLevel, deadline);
+        } catch (e) {
+          error = e;
+        }
+      }
+      // Reset the priority context to its value before reconcilation.
+      priorityContext = priorityContextBeforeReconciliation;
+
+      if (error !== null) {
         // We caught an error during either the begin or complete phases.
         const failedWork = nextUnitOfWork;
 
         if (failedWork !== null) {
-          // Reset the priority context to its value before reconciliation.
-          priorityContext = priorityContextBeforeReconciliation;
-
           // "Capture" the error by finding the nearest boundary. If there is no
           // error boundary, the nearest host container acts as one. If
           // captureError returns null, the error was intentionally ignored.
@@ -818,8 +842,6 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
           // inside resetAfterCommit.
           fatalError = error;
         }
-      } finally {
-        priorityContext = priorityContextBeforeReconciliation;
       }
 
       // Stop performing work
@@ -862,6 +884,7 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(config : HostConfig<T, P,
 
     // We're done performing work. Time to clean up.
     isPerformingWork = false;
+    deadlineHasExpired = false;
     fatalError = null;
     firstUncaughtError = null;
     capturedErrors = null;

src/renderers/shared/shared/event/EventPluginUtils.js

@@ -89,15 +89,7 @@ if (__DEV__) {
 function executeDispatch(event, simulated, listener, inst) {
   var type = event.type || 'unknown-event';
   event.currentTarget = EventPluginUtils.getNodeFromInstance(inst);
-  if (simulated) {
-    ReactErrorUtils.invokeGuardedCallbackWithCatch(
-      type,
-      listener,
-      event
-    );
-  } else {
-    ReactErrorUtils.invokeGuardedCallback(type, listener, event);
-  }
+  ReactErrorUtils.invokeGuardedCallbackAndCatchFirstError(type, listener, undefined, event);
   event.currentTarget = null;
 }
 

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

@@ -557,7 +557,7 @@ var ReactCompositeComponent = {
       if (safely) {
         if (!skipLifecycle) {
           var name = this.getName() + '.componentWillUnmount()';
-          ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst));
+          ReactErrorUtils.invokeGuardedCallbackAndCatchFirstError(name, inst.componentWillUnmount, inst);
         }
       } else {
         if (__DEV__) {

src/renderers/shared/utils/ReactErrorUtils.js

@@ -1,4 +1,4 @@
-/**
+  /**
  * Copyright 2013-present, Facebook, Inc.
  * All rights reserved.
  *
@@ -12,37 +12,63 @@
 
 'use strict';
 
-var caughtError = null;
+let caughtError = null;
 
 /**
  * Call a function while guarding against errors that happens within it.
+ * Returns an error if it throws, otherwise null.
  *
  * @param {String} name of the guard to use for logging or debugging
  * @param {Function} func The function to invoke
- * @param {*} a Argument
+ * @param {*} context The context to use when calling the function
+ * @param {...*} args Arguments for function
  */
-function invokeGuardedCallback<A>(
-  name: string,
-  func: (a: A) => void,
-  a: A,
-): void {
-  try {
-    func(a);
-  } catch (x) {
-    if (caughtError === null) {
-      caughtError = x;
+const ReactErrorUtils = {
+  invokeGuardedCallback: function<A, B, C, D, E, F, Context>(
+    name: string | null,
+    func: (A, B, C, D, E, F) => void,
+    context: Context,
+    a: A,
+    b: B,
+    c: C,
+    d: D,
+    e: E,
+    f: F,
+  ): Error | null {
+    const funcArgs = Array.prototype.slice.call(arguments, 3);
+    try {
+      func.apply(context, funcArgs);
+    } catch (error) {
+      return error;
     }
-  }
-}
-
-var ReactErrorUtils = {
-  invokeGuardedCallback: invokeGuardedCallback,
+    return null;
+  },
 
   /**
-   * Invoked by ReactTestUtils.Simulate so that any errors thrown by the event
-   * handler are sure to be rethrown by rethrowCaughtError.
+   * Same as invokeGuardedCallback, but instead of returning an error, it stores
+   * it in a global so it can be rethrown by `rethrowCaughtError` later.
+   *
+   * @param {String} name of the guard to use for logging or debugging
+   * @param {Function} func The function to invoke
+   * @param {*} context The context to use when calling the function
+   * @param {...*} args Arguments for function
    */
-  invokeGuardedCallbackWithCatch: invokeGuardedCallback,
+  invokeGuardedCallbackAndCatchFirstError: function<A, B, C, D, E, F, Context>(
+    name: string | null,
+    func: (A, B, C, D, E, F) => void,
+    context: Context,
+    a: A,
+    b: B,
+    c: C,
+    d: D,
+    e: E,
+    f: F,
+  ): void {
+    const error = ReactErrorUtils.invokeGuardedCallback.apply(this, arguments);
+    if (error !== null && caughtError === null) {
+      caughtError = error;
+    }
+  },
 
   /**
    * During execution of guarded functions we will capture the first error which
@@ -50,7 +76,7 @@ var ReactErrorUtils = {
    */
   rethrowCaughtError: function() {
     if (caughtError) {
-      var error = caughtError;
+      const error = caughtError;
       caughtError = null;
       throw error;
     }
@@ -66,21 +92,44 @@ if (__DEV__) {
       typeof window.dispatchEvent === 'function' &&
       typeof document !== 'undefined' &&
       typeof document.createEvent === 'function') {
-    var fakeNode = document.createElement('react');
-    ReactErrorUtils.invokeGuardedCallback = function<A>(
-      name: string,
-      func: (a: A) => void,
-      a: A,
-    ): void {
-      var boundFunc = function() {
-        func(a);
+
+    const fakeNode = document.createElement('react');
+    let depth = 0;
+
+    ReactErrorUtils.invokeGuardedCallback = function(
+      name,
+      func,
+      context,
+      a,
+      b,
+      c,
+      d,
+      e,
+      f
+    ) {
+      depth++;
+      const thisDepth = depth;
+      const funcArgs = Array.prototype.slice.call(arguments, 3);
+      const boundFunc = function() {
+        func.apply(context, funcArgs);
       };
-      var evtType = `react-${name}`;
+      let fakeEventError = null;
+      const onFakeEventError = function(event) {
+        // Don't capture nested errors
+        if (depth === thisDepth) {
+          fakeEventError = event.error;
+        }
+      };
+      const evtType = `react-${name ? name : 'invokeguardedcallback'}-${depth}`;
+      window.addEventListener('error', onFakeEventError);
       fakeNode.addEventListener(evtType, boundFunc, false);
-      var evt = document.createEvent('Event');
+      const evt = document.createEvent('Event');
       evt.initEvent(evtType, false, false);
       fakeNode.dispatchEvent(evt);
       fakeNode.removeEventListener(evtType, boundFunc, false);
+      window.removeEventListener('error', onFakeEventError);
+      depth--;
+      return fakeEventError;
     };
   }
 }

src/renderers/shared/utils/__tests__/ReactErrorUtils-test.js

@@ -19,57 +19,92 @@ describe('ReactErrorUtils', () => {
     ReactErrorUtils = require('ReactErrorUtils');
   });
 
-  describe('invokeGuardedCallbackWithCatch', () => {
-    it('should call the callback with only the passed argument', () => {
-      var callback = jest.fn();
-      ReactErrorUtils.invokeGuardedCallbackWithCatch('foo', callback, 'arg');
-      expect(callback).toBeCalledWith('arg');
-    });
-
-    it('should catch errors', () => {
-      var callback = function() {
-        throw new Error('foo');
-      };
-      expect(
-        () => ReactErrorUtils.invokeGuardedCallbackWithCatch('foo', callback)
-      ).not.toThrow();
-    });
-  });
-
-  describe('rethrowCaughtError', () => {
-    it('should rethrow caught errors', () => {
-      var err = new Error('foo');
-      var callback = function() {
-        throw err;
-      };
-      ReactErrorUtils.invokeGuardedCallbackWithCatch('foo', callback);
-      expect(() => ReactErrorUtils.rethrowCaughtError()).toThrow(err);
-    });
-  });
-
-  describe('invokeGuardedCallback', () => {
-    it('should call the callback with only the passed argument', () => {
-      var callback = jest.fn();
-      ReactErrorUtils.invokeGuardedCallback('foo', callback, 'arg');
-      expect(callback).toBeCalledWith('arg');
-    });
-
-    it('should use invokeGuardedCallbackWithCatch in production', () => {
-      expect(ReactErrorUtils.invokeGuardedCallback).not.toEqual(
-        ReactErrorUtils.invokeGuardedCallbackWithCatch
-      );
+  // Run tests in both DEV and production
+  describe('invokeGuardedCallback (development)', invokeGuardedCallbackTests.bind(null, 'development'));
+  describe('invokeGuardedCallback (production)', () => {
+    let oldProcess;
+    beforeEach(() => {
       __DEV__ = false;
-      var oldProcess = process;
+      oldProcess = process;
       global.process = {
         env: Object.assign({}, process.env, {NODE_ENV: 'production'}),
       };
       jest.resetModules();
       ReactErrorUtils = require('ReactErrorUtils');
-      expect(ReactErrorUtils.invokeGuardedCallback).toEqual(
-        ReactErrorUtils.invokeGuardedCallbackWithCatch
-      );
+    });
+
+    afterEach(() => {
       __DEV__ = true;
       global.process = oldProcess;
     });
+
+    invokeGuardedCallbackTests('production');
   });
+
+  function invokeGuardedCallbackTests(environment) {
+    it(`it should rethrow errors caught by invokeGuardedCallbackAndCatchFirstError (${environment})`, () => {
+      var err = new Error('foo');
+      var callback = function() {
+        throw err;
+      };
+      ReactErrorUtils.invokeGuardedCallbackAndCatchFirstError('foo', callback, null);
+      expect(() => ReactErrorUtils.rethrowCaughtError()).toThrow(err);
+    });
+
+    it(`should call the callback the passed arguments (${environment})`, () => {
+      var callback = jest.fn();
+      ReactErrorUtils.invokeGuardedCallback('foo', callback, null, 'arg1', 'arg2');
+      expect(callback).toBeCalledWith('arg1', 'arg2');
+    });
+
+    it(`should call the callback with the provided context (${environment})`, () => {
+      var context = { didCall: false };
+      ReactErrorUtils.invokeGuardedCallback('foo', function() {
+        this.didCall = true;
+      }, context);
+      expect(context.didCall).toBe(true);
+    });
+
+    it(`should return a caught error (${environment})`, () => {
+      const error = new Error();
+      const returnValue = ReactErrorUtils.invokeGuardedCallback('foo', function() {
+        throw error;
+      }, null, 'arg1', 'arg2');
+      expect(returnValue).toBe(error);
+    });
+
+    it(`should return null if no error is thrown (${environment})`, () => {
+      var callback = jest.fn();
+      const returnValue = ReactErrorUtils.invokeGuardedCallback('foo', callback, null);
+      expect(returnValue).toBe(null);
+    });
+
+    it(`can nest with same debug name (${environment})`, () => {
+      const err1 = new Error();
+      let err2;
+      const err3 = new Error();
+      const err4 = ReactErrorUtils.invokeGuardedCallback('foo', function() {
+        err2 = ReactErrorUtils.invokeGuardedCallback('foo', function() {
+          throw err1;
+        }, null);
+        throw err3;
+      }, null);
+
+      expect(err2).toBe(err1);
+      expect(err4).toBe(err3);
+    });
+
+    it(`does not return nested errors (${environment})`, () => {
+      const err1 = new Error();
+      let err2;
+      const err3 = ReactErrorUtils.invokeGuardedCallback('foo', function() {
+        err2 = ReactErrorUtils.invokeGuardedCallback('foo', function() {
+          throw err1;
+        }, null);
+      }, null);
+
+      expect(err3).toBe(null); // Returns null because inner error was already captured
+      expect(err2).toBe(err1);
+    });
+  }
 });