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

Dialog memory leak (detached Dialog.Content) #3202

Open
mdubourg001 opened this issue Oct 31, 2024 · 11 comments · May be fixed by #3234
Open

Dialog memory leak (detached Dialog.Content) #3202

mdubourg001 opened this issue Oct 31, 2024 · 11 comments · May be fixed by #3234
Assignees
Labels
Resolution: Needs Investigation This issue needs more investigation Type: Bug Confirmed bug

Comments

@mdubourg001
Copy link

Bug report

Current Behavior

Every time a Dialog gets opened and then closed, a Detached <div> element (the Dialog.Content) gets detached but is never garbage collected, as if a reference was kept, preventing it. This represents a memory leak because opening and closing the dialog 20 times means 20 Detached Dialog.Content elements as long as their whole tree of children, which makes the memory usage grow.

Expected behavior

No reference should be held with the Dialog.Content element upon closing to allow proper garbage collection.

Reproducible example

  • Go to the following CodeSandbox: https://codesandbox.io/p/sandbox/cool-meadow-h6vftg and click the "Open in a new tab" button at the top right of the Preview tab (or directly open the preview link: https://h6vftg.csb.app/)
  • Open the Chrome Devtools, go to the "Memory" tab, and click the "Detached elements" checkbox
  • Click the blue "Start" button at the bottom of the panel
  • Notice that no element is detached yet (or at least none related to Radix's Dialog)
  • Click the "Edit profile" button of the app and close the dialog 4 of 5 times in a row
  • Run a new "Memory"/"Detached elements" analysis in the devtools
  • Notice multiple <div> elements with the role=dialog attribute get leaked (this is actually the Dialog.Content element)

Suggested solution

Find which reference might not get removed when the dialog gets closed. I tried to search in the sources but am not able to notice anything.

Your environment

Software Name(s) Version
Radix Package(s) @radix-ui/react-dialog 1.1.2
React n/a ^18.0.0
Browser Google Chrome 130.0.6723.92
Assistive tech
Node n/a
npm/yarn
Operating System Apple M1 Pro Sonoma 14.7
@atwhiteley
Copy link

This has a pretty big impact in our usage of radix-ui. We have slowly been migrating from MUI to Radix, and we've recently noticed severe performance degradation. We could easily reproduce this memory leak, and we use dialog workflows quite extensively, this is therefore an urgent issue for us.

If anyone has a workaround we could patch or apply that'd be highly appreciated. We will look for one in the meantime.

We may need to revert to alternative dialog solutions in the short term if this issue is not picked up.

@Lexachoc
Copy link

Hi, I have tried to set the Dailog with modal={false}, I keep opening it several times, the memory is not going higher anymore.

This is indeed a serious problem, as I got hundreds of MB of memory used when opening the Dialog (modal={true} by default) many times later.

@Lexachoc
Copy link

Lexachoc commented Nov 14, 2024

This is how I use the current Dialog (for shadcn/ui):

https://codesandbox.io/p/sandbox/radix-ui-primitives-issues-3202-jzd42z

use modal={false} and custom Overlay to simulate the effect.

It seems that this does not cause the memory leak by doing so.

Any feedback would be greatly appreciated! As this is just quick workaround.

@Kjetiljv
Copy link

I can't get the garbage collector to remove all of the dialog nodes in your example either @Lexachoc.

It is easy to reproduce if I type something in one of the input fields before closing the dialog.

@chaance chaance added Type: Bug Confirmed bug Resolution: Needs Investigation This issue needs more investigation labels Nov 14, 2024
@chaance chaance self-assigned this Nov 14, 2024
@Lexachoc
Copy link

Lexachoc commented Nov 14, 2024

@Kjetiljv Thank you for pointing out! I only looked at the Permance monitor for the JS heap size and DOM nodes and taking snapshots for comparasion.

First time using it. Not sure how badly is the memory leak of my example. Could you please enlighten me if my example's memory leak situation better?The input fields are already there. Would retyping do anything about the memory leak?

This is from the original example:
image
image

And this is from my example:
image

image

both are reopened multiple time.

@Kjetiljv
Copy link

@Lexachoc, sorry I should have formulated myself better.

Your example seems to produce fewer dom nodes when I open and close the dialogg without making a change to its content. But if I type something in one of the input fields it will continue to grow. I made a video that I hope will explain it better.

ScreenShot-000274.mp4

@atwhiteley
Copy link

FYI - this behavior is reproducable with Radix Popover as well, but not as consistently.

@lauri865
Copy link

lauri865 commented Nov 19, 2024

And with Tooltips – every hover leaves around a detached element that doesn't get garbage collected.

Easily reproducible here: https://www.radix-ui.com/primitives/docs/components/tooltip

Screenshot 2024-11-19 at 21 57 18

@lauri865 lauri865 linked a pull request Nov 20, 2024 that will close this issue
@atwhiteley
Copy link

@lauri865 fantastic with a fix pending 👏

Is there any chance for us to patch this sooner rather than later? Any ETA on when we'll see this in a released version?

@lauri865
Copy link

lauri865 commented Dec 6, 2024

That's up to the maintainers when they have time to review and release the next version.

For the time being, since it's a simple one-line fix, you can patch @radix-ui/react-presence locally – we have done so for now. Either with pnpm if you use that or alternatively with https://www.npmjs.com/package/patch-package

@atwhiteley
Copy link

@lauri865 thanks - that makes sense. This is quite urgent for us, will have a look at your suggestions 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Needs Investigation This issue needs more investigation Type: Bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants