Dialog memory leak (detached Dialog.Content)

mdubourg001 commented 2 weeks ago

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)

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 commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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.

Lexachoc commented 3 days ago

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

And this is from my example:

both are reopened multiple time.

Kjetiljv commented 3 days ago

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

https://github.com/user-attachments/assets/18a6a083-8545-4546-9002-c2a4e492bd48

atwhiteley commented 3 days ago

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

radix-ui / primitives