Fix error recovery for partially rendered subtrees (#5120)

When a component throws after `diffChildren()` has already started, its
`newVNode._children` may contain partially mounted children. The error recovery
path always replaced those children with `oldVNode._children`, which can be
`undefined` during initial mount.

That loses the partially rendered subtree, so children that queued updates are
not unmounted and keep their _parentDom. A later queued update can then call
`getDomSibling()` through an ancestor whose `_children` is undefined and crash.

Only restore old children when the new vnode has no children yet, and discard
commit/ref queue entries produced by the failed partial render so lifecycles from
the aborted subtree are not committed.

Author: JoviDeCroock

src/diff/index.js

@@ -77,6 +77,7 @@ export function diff(
 	if ((tmp = options._diff)) tmp(newVNode);
 
 	outer: if (typeof newType == 'function') {
+		let oldCommitQueueLength = commitQueue.length;
 		try {
 			let c, isNew, oldProps, oldState, snapshot, clearProcessingException;
 			let newProps = newVNode.props;
@@ -282,6 +283,10 @@ export function diff(
 				c._pendingError = c._processingException = NULL;
 			}
 		} catch (e) {
+			// We remove any componentDidMount, ...
+			// that have been invalidated by us
+			// intercepting the error.
+			commitQueue.length = oldCommitQueueLength;
 			newVNode._original = NULL;
 			// if hydrating or creating initial tree, bailout preserves DOM:
 			if (isHydrating || excessDomChildren != NULL) {
@@ -308,7 +313,9 @@ export function diff(
 				}
 			} else {
 				newVNode._dom = oldVNode._dom;
-				newVNode._children = oldVNode._children;
+				if (!newVNode._children && oldVNode._children) {
+					newVNode._children = oldVNode._children;
+				}
 				if (!e.then) markAsForce(newVNode);
 			}
 			options._catchError(e, newVNode, oldVNode);

test/browser/lifecycles/componentDidCatch.test.jsx

@@ -155,6 +155,108 @@ describe('Lifecycle methods', () => {
 			);
 		});
 
+		it('should discard commit callbacks from a partially rendered subtree', () => {
+			let componentDidMount = sinon.spy();
+
+			class NullBoundary extends Component {
+				componentDidCatch(error) {
+					this.setState({ error });
+				}
+
+				render() {
+					return this.state.error ? null : this.props.children;
+				}
+			}
+
+			class MountingChild extends Component {
+				componentDidMount() {
+					componentDidMount();
+				}
+
+				render() {
+					return null;
+				}
+			}
+
+			function Parent() {
+				return [
+					<MountingChild />,
+					// Throw from diffing after MountingChild has queued its mount
+					// callback, but before Parent has completed rendering.
+					createElement('div', { 'bad name': 'x' })
+				];
+			}
+
+			render(
+				<NullBoundary>
+					<Parent />
+				</NullBoundary>,
+				scratch
+			);
+
+			expect(componentDidMount).not.to.have.been.called;
+			rerender();
+			expect(componentDidMount).not.to.have.been.called;
+			expect(scratch.innerHTML).to.equal('');
+		});
+
+		it('should clean up pending updates in a partially rendered subtree when rendering null', () => {
+			let child;
+			let childRenders = 0;
+			let componentDidMount = sinon.spy();
+
+			class NullBoundary extends Component {
+				componentDidCatch(error) {
+					this.setState({ error });
+				}
+
+				render() {
+					return this.state.error ? null : this.props.children;
+				}
+			}
+
+			class NullChild extends Component {
+				constructor(props) {
+					super(props);
+					child = this;
+				}
+
+				componentDidMount() {
+					componentDidMount();
+				}
+
+				render() {
+					childRenders++;
+					if (!this.state.tick) this.setState({ tick: 1 });
+					return null;
+				}
+			}
+
+			function Parent() {
+				return [
+					<NullChild />,
+					// Throw from diffing after NullChild has been diffed, but before
+					// Parent has completed rendering its children.
+					createElement('div', { 'bad name': 'x' })
+				];
+			}
+
+			render(
+				<NullBoundary>
+					<Parent />
+				</NullBoundary>,
+				scratch
+			);
+
+			expect(childRenders).to.equal(1);
+			expect(scratch.innerHTML).to.equal('');
+			expect(() => rerender()).not.to.throw();
+			expect(childRenders).to.equal(1);
+			expect(componentDidMount).not.to.have.been.called;
+			expect(child._parentDom).to.equal(null);
+			expect(scratch.innerHTML).to.equal('');
+		});
+
 		it('should be called when child fails in componentDidMount', () => {
 			ThrowErr.prototype.componentDidMount = throwExpectedError;