[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?

[greglittlefield-wf]

Is there an estimate for when #18814 or other workarounds to the leaked-DOM-leaking-React trees issue will be picked back up?

I'm also hitting the issue where DOM nodes are retained in Chrome, but I haven't been able to track down exactly why

[greglittlefield-wf]

Sorry to make the same comment again, but are there any updates on when the first "Actionable" item, around detaching backpointers on DOM nodes, might be implemented?

I know the root cause of memory leaks my team is seeing is DOM node memory leaks in Chrome, but unfortunately it's unclear when exactly those will get fixed (see also #17581 (comment)), and until then it seems like we don't have a lot of options to work around or mitigate React trees being leaked along with them.

We have some pretty large object graphs that are referenced either directly or indirectly (via callbacks, stores, etc.) in props, so we're pretty eager to find a solution or workaround.

We started testing out the potential fix from @PoByBolek's PR (#20290), and have found that it eliminates nearly all of the React-related memory leak fixes we've been seeing. Since there hasn't been much movement on this issue, we're starting to consider using a fork of React containing those fixes, which we'd really like to avoid if possible.

Does the solution in that PR seem like something the React team would want to move forward with?

If not, then @gaearon, I see that there was an update in February around double-invoking effects on @sebmarkbage's PR that also aimed to fix this issue, but was blocked by some work around hidden semantics. Is that now at a point where the fix in that PR could be picked back up? I see that @aweary has already asked you about this already a couple times, so sorry to pester you about it! 😓

[gaearon]

Heads-up that we've added slightly more aggressive cleanup in #21679. This doesn't mean we guarantee to keep it, but we've seen it help with some of the memory issues in our apps (so it might help with yours too). You can try building from main or wait for this change to hit the @next npm tag in a few days.

[PoByBolek]

Glad to see that something is happening here 😄
But does this mean that I'd have to check every new react release for whether you'd changed the deletedTreeCleanUpLevel from now on? (Your comment suggests that I do.) Would this be mentioned in the release notes? Are you planning on making this configurable from the client side at some point? All of the above? 😅

[gaearon]

All I'm saying is we can't make hard guarantees because there are several planned refactors of the internal data model. Some may create more memory pressure, some may relieve it. Yes, if this issue is very significant to you, it would make sense to check some particular scenario for regressions when upgrading versions. Note that the leaks we're aware of are not created by React, it's just that React isn't aggressive enough to prevent user code leaks though it can in theory.

[robbecker-wf]

FYI @gaearon Some folks on the Chrome team are currently working on memory issues if someone on the React team would like to help verify and fix these problems. There's activity on
https://bugs.chromium.org/p/chromium/issues/detail?id=1218275

and these 2 have seen activity and have been closed out
https://bugs.chromium.org/p/chromium/issues/detail?id=1144739
https://bugs.chromium.org/p/chromium/issues/detail?id=961494

[trentgrover-wf]

For posterity - We (Workiva) have verified that our memory leak problems are resolved by this fix in master: #21679. Is there any rough expectation for when this will receive an official release?

[gaearon]

It’s in the React 18 Alpha (next on npm) but a stable release is at minimum months away.

[exander77]

Memory leaking is the least of the problems here. This is a huge security issue as inputs provided by the user into the React input fields are held throughout the lifetime of the application and they are not cleared even when the application is refreshed in Chrome. This means all passwords, credentials inputted into those fields etc. can be recovered from browser memory even without a third-party tool as they can be browsed inside heap dump within Developer Tools within Browser and they can be recovered from memory or even hdd/ssd when the machine is suspended.

Simple example: User logs in to the web application and uses the application for hours. His password can be recovered:

1. Directly by using heap dump inside Developer Tools.
2. Memory dumping the browser.
3. Searching the whole RAM.
4. Searching the swap file.
5. Searching RAM suspended into hibernation file.

This should have been fixed two years ago when it was reported with the highest priority possible.

The fix should be released ASAP. This is no joke, I have caught the issue with persisting passwords in the wild.

[gaearon]

@exander77

First of all, the issue with inputs being retained is not related to React. It's an issue in Chrome which is being fixed in Chrome. #16087 (comment)

Next, I understand that you feel strongly about this, but I don't think your assessment is accurate from the security point of view. By definition, any information in JS heap is available if (for whatever reason) you have full access to RAM. A memory leak is a performance problem, not a security problem. Treating it as a security problem is misleading. For example, JS garbage collection is non-deterministic. Even when there is no leak, you don't have any guarantees about when the memory will be cleaned up. And even if the memory is cleaned up and the connection between the roots and the input node is destroyed, there is nothing in the specification that prevents the browsers from keeping the associated memory in the RAM. This may include the object itself, or even just the string values that have been used throughout the session. That would be true even without React on the page. If you don't want serialized RAM to be inspectable, you should encrypt it or use other general-purpose approaches.

One thing you could do is to null out input values yourself manually as soon as the input is no longer used (e.g. on unmount). However, like I mentioned earlier, this doesn't actually give you any guarantees about what will or will not be in your RAM since it's up to the JavaScript engine and the browser implementation details.

[exander77]

The issue may originate in Chrome, but it is delivered into production by React. I have not encountered the issue with plain HTML or plain JavaScript operated forms. Only with React.

Next, I understand that you feel strongly about this, but I don't think your assessment is accurate from the security point of view. By definition, any information in JS heap is available if (for whatever reason) you have full access to RAM. A memory leak is a performance problem, not a security problem. Treating it as a security problem is misleading. For example, JS garbage collection is non-deterministic. Even when there is no leak, you don't have any guarantees about when the memory will be cleaned up. And even if the memory is cleaned up and the connection between the roots and the input node is destroyed, there is nothing in the specification that prevents the browsers from keeping the associated memory in the RAM. This may include the object itself, or even just the string values that have been used throughout the session.

Sure, but there is no expectation of any developer or of any user that a password will be present in computer memory during the whole lifetime of the application. No standard says otherwise is just misdirection. If this is the case then it is a security issue and a very significant one. Developers of libraries should always prevent any security issues that may directly or indirectly arise from using the library. If a password is present in the memory after non-insignificant time has passed after the password was needed, it is a security issue.

One thing you could do is to null out input values yourself manually as soon as the input is no longer used (e.g. on unmount). However, like I mentioned earlier, this doesn't actually give you any guarantees about what will or will not be in your RAM since it's up to the JavaScript engine and the browser implementation details.

I certainly can make a workaround for my applications, but I cannot fix every web application I use that retains passwords in memory, can I? This needs to be solved within a library that is used by so many developers.

[gaearon]

I have not encountered the issue with plain HTML or plain JavaScript operated forms. Only with React.

None of the Chrome bug reports I linked to earlier include React.

Sure, but there is no expectation of any developer or of any user that a password will be present in computer memory during the whole lifetime of the application. No standard says otherwise is just misdirection.

If your security strategy depends on passwords not existing in RAM, your security strategy is open to being compromised because (1) most memory leaks in practice tend to happen in user code, (2) the browser provides no guarantees about the timing of garbage collection, (3) garbage collection does not imply or guarantee eviction of string data from RAM.

While I agree with you that React should do its best to remove unnecessary references even if user code is already leaky (and 18 alpha does this more aggressively than we did before), the underlying problem is still there: any userland leak in your product code (which tend to be very common), by your definition, is a security issue. The vast majority of leaks, in our experience, happens in product code. React tries to amortize those and make their impact less significant, but if you have to depend on this for security, I think you need a stronger strategy.