react icon indicating copy to clipboard operation
react copied to clipboard

Bug: Detached DOM nodes exist when component is unmounted

Open snakepoongmail opened this issue 3 years ago • 13 comments

I've written a simple React APP which contains 2 buttons, one is to increase the count number trigger the child component to create a table element with some random numbers, another is to toggle mount/unmount status of the child component.

I found that just a simply mount the component then unmount it which leaves detached DOM nodes in memory profiling. Following below steps:

  1. Click regen button
  2. Click toggle button

image

but if I mount and unmount the component again by adding 3, 4 steps below:

  1. Click regen button
  2. Click toggle button
  3. Click toggle button
  4. Click toggle button

In such orders, those detached DOM nodes will be cleaned. Is there any reason why those detached DOM nodes are not clear for the first time but distinguished by the second time the component is unmounted? image

React version: 17.0.2

Steps To Reproduce

  1. Click regen button
  2. Click toggle button

Link to code example: https://v6dhm.csb.app/

code attached:

import React, { useCallback, useMemo, useState } from "react";

const App = () => {
  const [count, setCount] = useState(0);
  const [display, setDisplay] = useState(true);

  const regen = useCallback(() => {
    setCount((e) => ++e);
  }, []);
  const toggle = useCallback(() => {
    setDisplay((e) => !e);
  }, []);

  return (
    <div>
      <button onClick={regen}>regen</button>
      <button onClick={toggle}>toggle</button>
      {display && <FTable count={count}></FTable>}
    </div>
  );
};
const Cell = () => {
  const num = Math.floor(Math.random() * 100);
  return <td>{num}</td>;
};

const FTable = (props) => {
  const { count } = props;
  const rows = useMemo(() => {
    const r = [];
    if (count == 0) {
      return r;
    }
    for (let i = 0; i < 1000; i++) {
      r.push(
        <tr key={`${i}-${count}`}>
          <Cell></Cell>
          <Cell></Cell>
          <Cell></Cell>
        </tr>
      );
    }
    return r;
  }, [count]);

  return (
    <div>
      <table>
        <tbody>{rows}</tbody>
      </table>
    </div>
  );
};

export default App;

The current behavior

Detached HTMLTable* are found in the devTool memory timeline

The expected behavior

No Detached DOMs are found in the devTool memory timeline

snakepoongmail avatar Jan 30 '22 09:01 snakepoongmail

Is there anyone taking a look or what else do I need to provide for further investigation?

snakepoongmail avatar Feb 07 '22 23:02 snakepoongmail

Hello @snakepoongmail,

is this causing you any troubles? I think this is a side effect of how React Fiber holds onto pieces of data and is harmless. The data will eventually be cleaned up.

sammy-SC avatar Feb 08 '22 15:02 sammy-SC

Hello @snakepoongmail,

is this causing you any troubles? I think this is a side effect of how React Fiber holds onto pieces of data and is harmless. The data will eventually be cleaned up.

Hello @sammy-SC , thx for your responding.

The issue now is I'm not quite sure about the timing when this data will be clean up. During the test, I've tried waiting for about 5 mins, clicking GC button in devTool, but it won't help freeing those occupied memory.

Those Detached DOM elements should be considered as memory leak, in my application there are other detached DOM elements found held by React Fiber as well which means they are not able to be collected by GC, the size of the leak could be incrementing over time may end up crashing the application by consuming too many memory. Especially for building big and long running applications, memory leak is fatal for not being tackle well.

snakepoongmail avatar Feb 09 '22 01:02 snakepoongmail

@snakepoongmail in DEV tools you can see exactly what is retaining the object. In another issue it was DevTools. Please check what is retaining the object.

salazarm avatar Mar 03 '22 15:03 salazarm

It seems like we are experiencing this issue with React 17 in our app. The number of nodes just grows and never they never get clean up.

I've tried switching to React 18 without success.

Is there anything we can do? Are there any known workarounds or ways to investigate?

gyzerok avatar Oct 10 '22 09:10 gyzerok

@gyzerok

could it be that you have retain cycles in your app? Are resources correctly cleaned up when component is unmounted?

Alternatively, could you try to use React past this commit: https://github.com/facebook/react/commit/d1bb1c586117df11123859d1ef59228bbf7c750a to see if it resolves it?

sammy-SC avatar Oct 11 '22 15:10 sammy-SC

Hello @sammy-SC!

Thank you for getting back to me. Not sure what you mean by retain cycles, can you elaborate?

By investigating deeper we discovered that the leak is caused by a contenteditable ref. We have it deep down our tree and after unmount it and all of it parents are kept in memory. Due to the nature of how people use our app it quicly leads to gigabytes of leaked memory.

Looking at the code we see nothing that could be causing it. Whatever needs to be clean up (like addEventListener) gets cleaned up. Commenting things for search puprposes does not show any specific place (unless you comment everything).

One specific thing about this code is that ref is used in multiple function wrapped into useCallback. Not sure when exactly React cleans this memory, but I can imagine how it can lead to the leaks.

And by looking at the devtools I can see that FiberNode seems to be memory holder here.

Alternatively, could you try to use React past this commit: https://github.com/facebook/react/commit/d1bb1c586117df11123859d1ef59228bbf7c750a to see if it resolves it? Are there any instructions available on how to install React from specific commit?

Also are these fixes going to be available for React 17? We are using it and it's unlikely we will be able to migrate soon.

gyzerok avatar Oct 13 '22 03:10 gyzerok

Not sure what you mean by retain cycles, can you elaborate?

By retain cycles I meant things like missing removeEventListener calls or similar. Something that would prevent component form getting garbage collected. So you found contenteditable, were you able to fix it?

Also are these fixes going to be available for React 17? We are using it and it's unlikely we will be able to migrate soon.

The fix is for problem specific to React 18, so that probably won't be the issue you're running into.

sammy-SC avatar Oct 20 '22 14:10 sammy-SC

+1

whatwg6 avatar Nov 28 '22 13:11 whatwg6

This seems to be a related issue to the one I posted recently: #25772 Maybe this will help to understand what's going on.

TUTOR03 avatar Dec 02 '22 12:12 TUTOR03

is there solution for this bug? :'(

bb7bb avatar Jan 29 '24 09:01 bb7bb

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Apr 28 '24 10:04 github-actions[bot]

bump

gyzerok avatar Apr 28 '24 10:04 gyzerok

bump on this one please!

Genesys-AlexW avatar Jul 18 '24 12:07 Genesys-AlexW