unmount an empty component is breaking with ReactDOM portals

[KhodorAmmar]

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

What is the current behavior? When unmounting a component that has a child being rendered under a different parent (with portals), react is throwing an error

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem:

https://codesandbox.io/s/73n31lwpjx

What is the expected behavior?

Component should unmount normally

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
16.8.1
Issue also happens with 16.7.0 (https://codesandbox.io/s/oxmpxmllvy)

The issue is only happening under very strict conditions:

• The component being rendered with ReactDOM Portals (Modal) should not render any HTML
• The parent component (Panel) should render Modal as the first component under <React.Fragment>

Avoiding this is as simple as moving Modal under some other HTML. I'm not entirely sure this is an issue or I'm just doing something wrong with Fragment and portals.

The actual error being thrown is:
react-dom.development.js:9254 Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

[gaearon]

Seems like the same as #14434.

Want to look into it?

[KhodorAmmar]

Sure, I might need guidance into where exactly to start looking though

[gaearon]

I'm guessing it's somewhere here

react/packages/react-reconciler/src/ReactFiberCommitWork.js

Lines 741 to 775 in c11015f

	function commitNestedUnmounts(root: Fiber): void {
	// While we're inside a removed host node we don't want to call
	// removeChild on the inner nodes because they're removed by the top
	// call anyway. We also want to call componentWillUnmount on all
	// composites before this host node is removed from the tree. Therefore
	// we do an inner loop while we're still inside the host node.
	let node: Fiber = root;
	while (true) {
	commitUnmount(node);
	// Visit children because they may contain more composite or host nodes.
	// Skip portals because commitUnmount() currently visits them recursively.
	if (
	node.child !== null &&
	// If we use mutation we drill down into portals using commitUnmount above.
	// If we don't use mutation we drill down into portals here instead.
	(!supportsMutation || node.tag !== HostPortal)
	) {
	node.child.return = node;
	node = node.child;
	continue;
	}
	if (node === root) {
	return;
	}
	while (node.sibling === null) {
	if (node.return === null || node.return === root) {
	return;
	}
	node = node.return;
	}
	node.sibling.return = node.return;
	node = node.sibling;
	}
	}

Or here:

react/packages/react-reconciler/src/ReactFiberCommitWork.js

Lines 989 to 1077 in c11015f

	function unmountHostComponents(current): void {
	// We only have the top Fiber that was deleted but we need to recurse down its
	// children to find all the terminal nodes.
	let node: Fiber = current;
	// Each iteration, currentParent is populated with node's host parent if not
	// currentParentIsValid.
	let currentParentIsValid = false;
	// Note: these two variables *must* always be updated together.
	let currentParent;
	let currentParentIsContainer;
	while (true) {
	if (!currentParentIsValid) {
	let parent = node.return;
	findParent: while (true) {
	invariant(
	parent !== null,
	'Expected to find a host parent. This error is likely caused by ' +
	'a bug in React. Please file an issue.',
	);
	switch (parent.tag) {
	case HostComponent:
	currentParent = parent.stateNode;
	currentParentIsContainer = false;
	break findParent;
	case HostRoot:
	currentParent = parent.stateNode.containerInfo;
	currentParentIsContainer = true;
	break findParent;
	case HostPortal:
	currentParent = parent.stateNode.containerInfo;
	currentParentIsContainer = true;
	break findParent;
	}
	parent = parent.return;
	}
	currentParentIsValid = true;
	}
	if (node.tag === HostComponent || node.tag === HostText) {
	commitNestedUnmounts(node);
	// After all the children have unmounted, it is now safe to remove the
	// node from the tree.
	if (currentParentIsContainer) {
	removeChildFromContainer((currentParent: any), node.stateNode);
	} else {
	removeChild((currentParent: any), node.stateNode);
	}
	// Don't visit children because we already visited them.
	} else if (node.tag === HostPortal) {
	// When we go into a portal, it becomes the parent to remove from.
	// We will reassign it back when we pop the portal on the way up.
	currentParent = node.stateNode.containerInfo;
	currentParentIsContainer = true;
	// Visit children because portals might contain host components.
	if (node.child !== null) {
	node.child.return = node;
	node = node.child;
	continue;
	}
	} else {
	commitUnmount(node);
	// Visit children because we may find more host components below.
	if (node.child !== null) {
	node.child.return = node;
	node = node.child;
	continue;
	}
	}
	if (node === current) {
	return;
	}
	while (node.sibling === null) {
	if (node.return === null || node.return === current) {
	return;
	}
	node = node.return;
	if (node.tag === HostPortal) {
	// When we go out of the portal, we need to restore the parent.
	// Since we don't keep a stack of them, we will search for it.
	currentParentIsValid = false;
	}
	}
	node.sibling.return = node.return;
	node = node.sibling;
	}
	}

[KhodorAmmar]

Okay then I’ll take a look

[gaearon]

You might also want to start by making a failing test. Here's some existing tests for portals.

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

Lines 418 to 462 in c11015f

	it('should reconcile portal children', () => {
	const portalContainer = document.createElement('div');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(<div>portal:1</div>, portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('<div>portal:1</div>');
	expect(container.innerHTML).toBe('<div></div>');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(<div>portal:2</div>, portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('<div>portal:2</div>');
	expect(container.innerHTML).toBe('<div></div>');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(<p>portal:3</p>, portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('<p>portal:3</p>');
	expect(container.innerHTML).toBe('<div></div>');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(['Hi', 'Bye'], portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('HiBye');
	expect(container.innerHTML).toBe('<div></div>');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(['Bye', 'Hi'], portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('ByeHi');
	expect(container.innerHTML).toBe('<div></div>');
	ReactDOM.render(
	<div>{ReactDOM.createPortal(null, portalContainer)}</div>,
	container,
	);
	expect(portalContainer.innerHTML).toBe('');
	expect(container.innerHTML).toBe('<div></div>');
	});

[KhodorAmmar]

Would this work? the test creates a component with the same criteria as the demo above and removes the child component via a state update. It currently breaks

https://github.com/KhodorAmmar/react/blob/1aa3167b15b2eaa74f9afeac16d498a781b8c09b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js#L464-L496

Also is ReactDOMFiber-test the correct place for the test?

[KhodorAmmar]

Ok I was debugging the code and I believe I can pinpoint where is the issue coming from:

react/packages/react-reconciler/src/ReactFiberCommitWork.js

Lines 1040 to 1073 in c11015f

	} else if (node.tag === HostPortal) {
	// When we go into a portal, it becomes the parent to remove from.
	// We will reassign it back when we pop the portal on the way up.
	currentParent = node.stateNode.containerInfo;
	currentParentIsContainer = true;
	// Visit children because portals might contain host components.
	if (node.child !== null) {
	node.child.return = node;
	node = node.child;
	continue;
	}
	} else {
	commitUnmount(node);
	// Visit children because we may find more host components below.
	if (node.child !== null) {
	node.child.return = node;
	node = node.child;
	continue;
	}
	}
	if (node === current) {
	return;
	}
	while (node.sibling === null) {
	if (node.return === null || node.return === current) {
	return;
	}
	node = node.return;
	if (node.tag === HostPortal) {
	// When we go out of the portal, we need to restore the parent.
	// Since we don't keep a stack of them, we will search for it.
	currentParentIsValid = false;
	}
	}

When the node is a HostPortal, and it is rendering nothing, and it is the first child under a Fragment, the currentParentIsValid is not being reset to false correctly in line 1071

Adding currentParentIsValid = false; in line 1044 is fixing the issue, all other tests are still passing.

I just feel that it's a too general solution? Am I on the right path at least?

[gaearon]

Thanks, this was very helpful. I sent a fix in #14820.