React DevTools might retain references to unmounted DOM elements (and their Fibers)

[bgirard]

There's seems to be circumstances where unmounted DOM/Fibers are kept alive by React DevTools. They're kept alive in primaryFibers:

react/packages/react-devtools-shared/src/backend/renderer.js

Line 772 in 3452706

const primaryFibers: Set<Fiber> = new Set();

It seems like a WeakSet would be appropriate and would remove the leak. Otherwise we'd need to understand why recordUnmount isn't called.

CC @bvaughn

[bvaughn]

I'm not necessarily convinced that the above screenshot is showing what this issue suggests. I believe it's possible that this indicates that component rendered null where it previously rendered a DOM element, and the "alternate" Fiber still has a reference to the (now unmounted) DOM element. In this case both DevTools and React itself would have references to both Fibers as well as the unmounted DOM element.

We do clear pointers for both "current" and "alternate" in detachFiber but this does not include the stateNode property:

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

Lines 889 to 909 in 0cf22a5

	function detachFiber(current: Fiber) {
	const alternate = current.alternate;
	// Cut off the return pointers to disconnect it from the tree. Ideally, we
	// should clear the child pointer of the parent alternate to let this
	// get GC:ed but we don't know which for sure which parent is the current
	// one so we'll settle for GC:ing the subtree of this child. This child
	// itself will be GC:ed when the parent updates the next time.
	current.return = null;
	current.child = null;
	current.memoizedState = null;
	current.updateQueue = null;
	current.dependencies = null;
	current.alternate = null;
	current.firstEffect = null;
	current.lastEffect = null;
	current.pendingProps = null;
	current.memoizedProps = null;
	if (alternate !== null) {
	detachFiber(alternate);
	}
	}

The retain behavior I describe above can be verified with the following example app code:

class App extends Component {
  render() {
    return this.props.mount ? <div/> : null;
  }
}

const container = document.getElementById('root');

let root = ReactDOM.render(<App mount={true} />, container);
root._reactInternalFiber.child.stateNode // div
root._reactInternalFiber.child.stateNode.parentElement // div#root

root = ReactDOM.render(<App mount={false} />, container);
root._reactInternalFiber.child.stateNode // div
root._reactInternalFiber.child.stateNode.parentElement // null

I'm not sure I would call this a "leak", since the retained DOM elements would go away once the leaf Fiber itself went away, but it could at least exhibit the behavior your Memory profile shows in the picture above. If one was to argue this was a leak, then I believe it would be considered a React leak rather than a DevTools leak.

Edit I think we also leak Fibers in this way, since the stateNode may have a reference to its previous children - which themselves have references to their Fibers via the "__reactInternalInstance" field.

[bvaughn]

Quick update about this issue and PR #17666

The thing that caused the above temporarily "leak" is the alternate pointer- which will be cleared during the next render. That being said, the reason the temporary "leak" retains so much (the whole subtree) is because we weren't nulling out the stateNode field. PR #17666 changes this behavior so that we now will null that field. This should make our memory snapshot comparisons less noisy in the future.

In other words, hopefully this will show that DevTools isn't actually leaking DOM nodes (and Fibers) but if it is at least we should be able to identify them less ambiguously now.

[piecyk]

Looks like there is a memory leak in DevTools, was noticed in TanStack/virtual#196

Created small demo, it can be observer while scrolling a list, memory increase because of detached fiber and dom elements

https://github.com/piecyk/react-devtools-memory-leak-17092021

It happens also in production build, noticed that elements aren't removed from fiberToIDMap after they are unmounted, something fishy there 🤔

[bvaughn]

Thanks for sharing a repro, @piecyk. I'll take a look.

[bvaughn]

It does look like DevTools isn't recording unmounts for list-item Fibers in this repro, which causes it to retain them after they've been unmounted.

I thinks a React bug though. (React is supposed to explicitly notify DevTools of unmounts and it doesn't seem to be.) I wonder what it is about react-virtual that triggers this behavior that doesn't repro with a library like react-window. This might have been a bad lead here. I think the reason I'm initially not seeing calls to commit unmount are because react-virtual grows a pool of items initially. Still digging. It's definitely the list items that are leaking though. Just not sure why yet.

---

Update I'm not convinced this is the entire problem, but it does seem like DevTools isn't properly cleaning out all of the alternate entries in the fiberToId Map. I think this is because the alternate is being cleared out synchronously by React when a Fiber gets unmounted (as part of our memory cleanup flag) but DevTools is asynchronously processing the untracking in order to support fast Refresh:

react/packages/react-devtools-shared/src/backend/renderer.js

Lines 1126 to 1139 in bc9bb87

	// Untrack Fibers after a slight delay in order to support a Fast Refresh edge case:
	// 1. Component type is updated and Fast Refresh schedules an update+remount.
	// 2. flushPendingErrorsAndWarningsAfterDelay() runs, sees the old Fiber is no longer mounted
	// (it's been disconnected by Fast Refresh), and calls untrackFiberID() to clear it from the Map.
	// 3. React flushes pending passive effects before it runs the next render,
	// which logs an error or warning, which causes a new ID to be generated for this Fiber.
	// 4. DevTools now tries to unmount the old Component with the new ID.
	//
	// The underlying problem here is the premature clearing of the Fiber ID,
	// but DevTools has no way to detect that a given Fiber has been scheduled for Fast Refresh.
	// (The "_debugNeedsRemount" flag won't necessarily be set.)
	//
	// The best we can do is to delay untracking by a small amount,
	// and give React time to process the Fast Refresh delay.

---

The previous update is the problem. I think I have a fix. Will be pushing shortly.

[piecyk]

Also as side note, i would expect to see similar behaviour in react-window, as in windowing nature there is lot of mounting/unmounting, only difference to react-virtual is, that it's hook based and react-window on classes.

[bvaughn]

This fix will go out with the next DevTools release.