Null stateNode after unmount

[bvaughn]

For discussion purposes.

This change avoids a certain case where we over-retain Fibers after unmount:

In detachFiber we null out child and sibling pointers in an effort to clear references to the stale subtree, but-

1. We don't clear the stateNode pointer for host element fibers.
2. The stateNode has references to its previous host element children.
3. Each host element (the children) have references to their corresponding Fibers via the __reactInternalInstance* prop.

So we end up retaining the whole Fiber subtree through the single stateNode reference.

Although the cost of retaining these references is limited (e.g. it will go away the next time the parent fiber renders) it can still obscure investigations into other more serious, user space leaks.

An example of the previous behavior can be seen here:
https://codesandbox.io/s/sweet-leakey-rj2ii

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ded52f5:

Sandbox	Source
sharp-night-0qkrb	Configuration

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against ded52f5

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against ded52f5

[trueadm]

I don’t see any issues with this approach given we are clearing all the other properties. I’m glad you were able to find a fix for the issue I saw a while back too! Might be worth checking on www, as there might be another call-site that needs a guard. :)

Nice work!

[bvaughn]

Might be worth checking on www, as there might be another call-site that needs a guard. :)

I kicked off an early sync with this change to see if any FB tests failed, and they did not.

[bvaughn]

More context: this behavior is specific to class components.

The root of this leak is actually the _reactInternalFiber attribute, which is part of the ReactInstanceMap. This reference is needed in order for class component instances to schedule updates, but maybe another part of this PR should be to also clear this instance?

Although ReactInstanceMap exports a remove function, we never actually use it anywhere.

Looks like this instance map is also used to make an inference about whether the Fiber is mounted:

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

Lines 113 to 115 in 8979766

	const fiber: ?Fiber = getInstance(component);
	if (!fiber) {
	return false;

This would presumably return a false positive after an unmount?

[gaearon]

The root of this leak is actually the _reactInternalFiber attribute

I think this is a bit of a misunderstanding. In this example, _reactInternalFiber points to the App Fiber. We can't remove that reference because App itself is not unmounted. So would be too early to remove it.

We're seeing this problem because _reactInternalFiber points to the App alternate. And the alternate holds onto the deleted children until the next update. This is how it’s designed.

What if _reactInternalFiber pointed to the current App (which has child = null)? Would that help? Alas, no. It wouldn't help because then we’d still see our Fiber as _reactInternalFiber.alternate instead (and its child would be the deleted one).

So the root problem here is really the architectural choice of using alternates. The tradeoff this choice is making is that when children are deleted, the parent’s alternate will keep holding onto their Fibers until the next update (when they would get overwritten).

We've been gradually cutting down on the consequences of this choice by clearing more and more fields on those child Fibers eagerly on their deletion. So that retaining them isn’t as big of a deal. But we can’t get away from retaining them with the current architecture.

The fundamental problem isn’t that we have some leak we could easily remove. It’s the architectural choice of using alternates. It can be mitigated by... not using them. But that’s a big refactor.

This might still be good to get in regardless. If we’re clearing so many fields already, might as well do this one. This will reduce the effect of the leaks, but will likely make the real ones harder to spot because their effects would be less pronounced.

[bvaughn]

This might still be good to get in regardless. If we’re clearing so many fields already, might as well do this one. This will reduce the effect of the leaks

This is my feeling too after looking at, and thinking about, this a bit more. This would still be a nice change to get in because it would minimize the impact of the current architecture and improve the signal/noise ratio for other investigations.

[bvaughn]

I'm going to move forward with this change because I think it would help when investigating a reported potential DevTools leak, and because it does not seem to negatively impact any tests in this repo or in Facebook. Let's chat more about any other follow up items or concerns after the holidays!

[kelly-tock]

anyone seeing this in react 17 as well? seeing what seems like a similar issue in an app on react 17. trying the latest 16 to compare.

[eltonchan]

I can still encounter this problem in React 17 version. This does not necessarily happen, it is very random. Is this problem only with class components? Is it normal if use function component? @bvaughn

[shaikh-kamran]

I am still having this issue in React 17, how to avoid this?

[joankrastanov]

I am encountering this issue in React 17, does anyone know if this is fixed in future versions?