If console.error throws while reporting an error, React can enter an invalid internal state

[brokentone]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When ReactFiberScheduler exceeds the update count and throws the Maximum update depth exceeded. invariant, it did not update nestedUpdateCount (leaving it in a "dirty" state). This has been fixed in this PR yet to be released: https://github.com/facebook/react/pull/13160/files#diff-24152ba0b2ac251decb6a12f41bdf116R1478

But out of interest + further concern, I began to explore some concerns around whether ReactDOM (Fiber really) maintaining some "state" and in fact, being left dirty in at least some error cases was indeed a concern or had been considered in the community. While nestedUpdateCount might be fixed here, I can't imagine it was the only state value which could have issues.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

I created a repo with create-react-app which demonstrates my concern: https://github.com/brokentone/react-dom-enzyme

Observe that you will have 2 test failures. ComponentOne is indeed poorly written, but ComponentTwo is quite simple and should be "ok." Skipping ComponentOne should should ComponentTwo passing.

What is the expected behavior?

Only ComponentOne should fail in my example.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Unclear

[aweary]

@brokentone can you please try to reproduce without Enzyme? The latest stable Enzyme release doesn't yet have full support for React 16. It's also harder to isolate the issue to React itself.

[gaearon]

Here's a minimal repro.

const React = require('react');
const ReactDOM = require('react-dom')

const {JSDOM} = require('jsdom');
require('raf/polyfill');
const jsdom = new JSDOM(`<div id="app-root"></div>`);
global.window = jsdom.window;
global.document = jsdom.window.document;
global.navigator = jsdom.window.navigator;
console.error = (error) => {
  throw new Error(error);
};

class Bad extends React.Component {
  componentDidUpdate() {
    throw new Error('noo');
  }
  render() {
    return null;
  }
}

it('should mount', function () {
  const div = document.createElement('div')
  try {
    ReactDOM.render(React.createElement(Bad), div);
    ReactDOM.render(React.createElement(Bad), div);
  } catch (e) {}
  ReactDOM.render(React.createElement('div'), div);
});

It crashes (at least with 16.4.1).

Interestingly it doesn't crash if I move react-dom import below JSDOM initialization.

[gaearon]

Going to continue the investigation in #13310.

Part of this was indeed fixed on master, but another part wasn't.

[gaearon]

(Thanks to @brokentone, your repro case was very helpful)

[brokentone]

Thank you very much for the minimal repro case @gaearon! I actually spent hours stripping down enzyme to figure out what it was doing in these cases... I learned a lot but never got to a useful conclusion. Building up was a far better idea.

I'll use your case to further elaborate my initial concern!

[gaearon]

My methodology was to remove imports one by one. I noticed it's the adapter that was causing the issue. Then I noticed that the only statement causing the issue inside the adapter is the react-dom import. That told me that the order of imports matters. Gradually I got it all in one file and removed everything that didn't have an effect.

[gaearon]

The reason for the bug is because the reconciler assumed the function that reports errors never throws. But it throws in your setup because console.error is overridden to throw.

[gaearon]

Fixed by #13310