[Umbrella] Memory Leaks

[sebmarkbage]

This issue is a summary of issues mentioned in #15157.

There are many different ways to create memory leaks with React since we give you access to the imperative power to do so. Most should be dealt with by clean up / unmount functions.

Some could be pure React bugs. Some could be related to the lack of clean up of render phase effects. Others could be related to leaks that exists but the way React works makes them larger than they otherwise would've.

Resolved

• Land Clean up nextEffect pointer #16115 What patterns are actually covered? It can cut down on a potentially larger leak but is that the whole leak? I could imagine some patterns where this is the complete solution but unclear if it's the complete solution for the patterns that people are actually hitting in practice.

Actionable

I think there are at least two actionable patterns to address from #15157:

• If a handle on a DOM node is leaked, it takes the React tree with it. This is a fairly easy mistake to make and the effect is pretty high. What we would do here is special case DOM nodes with refs on them, and always detach their back pointer to the React Fiber, if it was ever fully mounted. We currently traverse these trees anyway when they get deleted. We want to stop doing this for most things but for nodes with a ref it seems minor to special case since they typically need to be invoked with null anyway.
• Investigate the source of the leak in https://github.com/jonnycornwell/potential_react_leak and fix the source of the problem.

Unresolved

• Closing over setState/dispatch or class component instances to global state can leak too. Does this pattern warrant special casing too? Under what conditions?
• It appears Chrome (and maybe other browsers?) may retain inputs due to the Undo Stack (Input nodes leaked by the browser retain React fibers #17581)
• What other issues remain after solving the actionable above? Let's make another pass investigating if people's original issues remain.

Won't Fix

• Side-effects in class constructor, componentWillMount, componentWillReceiveProps, componentWillUpdate, getDerivedStateFrom... and render that store a reference to anything stateful outside React won't be able to clean up. This is documented in the 16.3 release and is a major design decision as part of the concurrent/suspense/error handling strategy.
• Effects/state retained temporarily in alternate fiber likely won't be fixed short term. This is due to how Fiber uses two trees and swaps between them. This can lead to additional values being retained until that tree gets some work on it to swap again. This was observed in the Hooks release and there are some confusing cases where a destroy function can hold onto more memory than expected in the closure. Typically this is solved by using a custom hook since that gets its own scope/closure.
• Props/child trees retained by alternate children. Similarly, children that was just removed can sometimes between retained by the alternate copy of that. That is until that node gets another update on it which clears out the old children. These cases are fairly unusual and fix themselves eventually as the app lives on.

[gaearon]

@matthewbryancurtis You are using CodeSandbox which runs React in development mode and includes a ton of other code. Can you please create a React-only demo in production mode? Thanks.

[gaearon]

If GC cleans it up, then it's not a memory leak, is it?

[handeyeco]

Sorry, my misunderstanding. I'll go ahead and delete my comments to reduce noise on this issue.

[albertxing]

Regarding

If a handle on a DOM node is leaked, it takes the React tree with it ... What we would do here is special case DOM nodes with refs on them, and always detach their back pointer to the React Fiber, if it was ever fully mounted

In #17581 I suggest that most if not all input nodes are retained by the browser; in which case React might want to detach fibers from all input nodes as well, not just nodes with refs. These leaks are occurring automatically and are not necessarily caused by developer error.

Do we think this should be prioritized? I'd be happy to contribute, though I'm not familiar at all with the unmounting/cleanup cycle and would appreciate some guidance 🙇

[pimterry]

@albertxing I think I'm seeing the same memory leak issue as you here. I have DOM input nodes being held by Chrome seemingly indefinitely after they're unmounted, and those are retaining huge amounts of memory via memoizedProps etc. It's causing me some big problems. Did you make any progress on this?