Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Detached DOM nodes exist when component is unmounted #23214

Open
snakepoongmail opened this issue Jan 30, 2022 · 13 comments
Open

Bug: Detached DOM nodes exist when component is unmounted #23214

snakepoongmail opened this issue Jan 30, 2022 · 13 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@snakepoongmail
Copy link

snakepoongmail commented Jan 30, 2022

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 snakepoongmail added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 30, 2022
@snakepoongmail
Copy link
Author

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

@sammy-SC
Copy link
Contributor

sammy-SC commented Feb 8, 2022

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.

@snakepoongmail
Copy link
Author

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.

@salazarm
Copy link
Contributor

salazarm commented Mar 3, 2022

@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.

@gyzerok
Copy link

gyzerok commented Oct 10, 2022

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?

@sammy-SC
Copy link
Contributor

@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: d1bb1c5 to see if it resolves it?

@gyzerok
Copy link

gyzerok commented Oct 13, 2022

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: d1bb1c5 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.

@sammy-SC
Copy link
Contributor

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.

@whatwg6
Copy link

whatwg6 commented Nov 28, 2022

+1

@TUTOR03
Copy link

TUTOR03 commented Dec 2, 2022

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

@bb7bb
Copy link

bb7bb commented Jan 29, 2024

is there solution for this bug? :'(

Copy link

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 github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 28, 2024
@gyzerok
Copy link

gyzerok commented Apr 28, 2024

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

7 participants