Replace console.error() with a throw in setTimeout() as last resort exception logging (#13310)

* Add a regression test for #13188

* Replace console.error() with a throw in setTimeout() as last resort

* Fix lint and comment

* Fix tests to check we throw after all

* Fix build tests

Author: gaearon

packages/react-dom/src/__tests__/ReactErrorLoggingRecovery-test.js

@@ -0,0 +1,79 @@
+/**
+ * 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
+ */
+
+// This is a regression test for https://github.com/facebook/react/issues/13188.
+// It reproduces a combination of conditions that led to a problem.
+
+if (global.window) {
+  throw new Error('This test must run in a Node environment.');
+}
+
+// The issue only reproduced when React was loaded before JSDOM.
+const React = require('react');
+const ReactDOM = require('react-dom');
+
+// Unlike other tests, we want to enable error logging.
+// Note this is not a real Error prototype property,
+// it's only set in our Jest environment.
+// eslint-disable-next-line no-extend-native
+Error.prototype.suppressReactErrorLogging = false;
+
+// Initialize JSDOM separately.
+// We don't use our normal JSDOM setup because we want to load React first.
+const {JSDOM} = require('jsdom');
+global.requestAnimationFrame = setTimeout;
+global.cancelAnimationFrame = clearTimeout;
+const jsdom = new JSDOM(`<div id="app-root"></div>`);
+global.window = jsdom.window;
+global.document = jsdom.window.document;
+global.navigator = jsdom.window.navigator;
+
+class Bad extends React.Component {
+  componentDidUpdate() {
+    throw new Error('no');
+  }
+  render() {
+    return null;
+  }
+}
+
+describe('ReactErrorLoggingRecovery', () => {
+  let originalConsoleError = console.error;
+
+  beforeEach(() => {
+    console.error = error => {
+      throw new Error('Buggy console.error');
+    };
+  });
+
+  afterEach(() => {
+    console.error = originalConsoleError;
+  });
+
+  it('should recover from errors in console.error', function() {
+    const div = document.createElement('div');
+    let didCatch = false;
+    try {
+      ReactDOM.render(<Bad />, div);
+      ReactDOM.render(<Bad />, div);
+    } catch (e) {
+      expect(e.message).toBe('no');
+      didCatch = true;
+    }
+    expect(didCatch).toBe(true);
+    ReactDOM.render(<span>Hello</span>, div);
+    expect(div.firstChild.textContent).toBe('Hello');
+
+    // Verify the console.error bug is surfaced
+    expect(() => {
+      jest.runAllTimers();
+    }).toThrow('Buggy console.error');
+  });
+});

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -116,7 +116,12 @@ export function logError(boundary: Fiber, errorInfo: CapturedValue<mixed>) {
     // A cycle may still occur if logCapturedError renders a component that throws.
     const suppressLogging = e && e.suppressReactErrorLogging;
     if (!suppressLogging) {
-      console.error(e);
+      // 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 overriden.
+      // https://github.com/facebook/react/issues/13188
+      setTimeout(() => {
+        throw e;
+      });
     }
   }
 }

packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js

@@ -144,11 +144,10 @@ describe('ReactIncrementalErrorLogging', () => {
 
       expect(logCapturedErrorCalls.length).toBe(1);
 
-      // The error thrown in logCapturedError should also be logged
-      expect(console.error).toHaveBeenCalledTimes(1);
-      expect(console.error.calls.argsFor(0)[0].message).toContain(
-        'logCapturedError error',
-      );
+      // The error thrown in logCapturedError should be rethrown with a clean stack
+      expect(() => {
+        jest.runAllTimers();
+      }).toThrow('logCapturedError error');
     } finally {
       jest.unmock('../ReactFiberErrorLogger');
     }

scripts/jest/config.build.js

@@ -10,6 +10,11 @@ const packages = readdirSync(packagesRoot).filter(dir => {
   if (dir.charAt(0) === '.') {
     return false;
   }
+  if (dir === 'events') {
+    // There's an actual Node package called "events"
+    // that's used by jsdom so we don't want to alias that.
+    return false;
+  }
   const packagePath = join(packagesRoot, dir, 'package.json');
   return statSync(packagePath).isFile();
 });