1986144 - Memory leak for #colorSwatches in output-parser.js

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[Artem Manushenkov]

steps to reproduce:
go to example.com, open devtools
repeatedly focus on body and then on div, causing rules panel to re-render
put breakpoint here https://searchfox.org/firefox-main/source/devtools/client/shared/output-parser.js#1904, look at #colorSwatches WeakMap
go to google.com, click on something in inspector and check the #colorSwatches again, it still holds references to the old swatches

this weakmap can get very big if you repeatedly inspect elements with a lot of big rules, something is still referencing elements in the WeakMap

Comment 1

[Julian Descottes [:jdescottes]]

Normally weakmap should stop referencing the values when the keys are garbage collected.

How are you checking this leak? Can you share your process more precisely? Is there any chance you are holding the keys alive by keeping a reference to them in the debugger, the console, anything else?

Comment 2

[Artem Manushenkov]

@jdescottes https://youtu.be/EZsU-uZIXZI
Here's a video of it, starting with a fresh firefox instance, I switch between html elements to cause many rules panel re-renders, every time we re-render the swatches, they're added to that WeakMap. Then i go to another website keeping the console open and the weakmap is still huge

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the details. Indeed you might have surfaced an actual leak here. I just added

        console.log(
          "@@@@@@ colorSwatches length",
          ChromeUtils.nondeterministicGetWeakMapKeys(this.#colorSwatches).length
        );

in order to log the size of the weakmap, with minimal interference. It keeps growing forever as long as the toolbox is open. If I stop adding the swatch to the DOM then the growth becomes normal again (ie it is periodically emptied as elements are GC'd).

Putting this back to triage.

Comment 4

[Julian Descottes [:jdescottes]]

We should check if the nodes are also still connected to the document. Use isConnected to check this.
Also check the memory usage.

Comment 5

[Artem Manushenkov]

I've looked at it via parentElement chain and it returns null around the time you get to ruleveiw element

Comment 6

[Artem Manushenkov]

I'd like to take a few days to try to fix it myself

Comment 7

[Julian Descottes [:jdescottes]]

Ok sounds good. So the element still has some ancestors in the chain, but is no longer connected to the initial document. Meaning we probably leak the ruleview elements. I will let you take a look when you have time.

I feel like this is quite an old issue, so for now we can set P3/S3.

Comment 8

[Artem Manushenkov]

The before video: https://youtu.be/h2U0ajAVymA for 1 minute I switch between html elements with arrow keys on youtube.com, they have a LOT of rules and properties, then I go to example.com.
Only closing and reopening devtools starts the CG process, freezing the browser for about 15 seconds (from 1:45 to 2:03) Note that my machine is a performant modern macbook
Ram usage goes from about 700mb to 5.5gb, and down to 2gb after GC. during GC it peaks to 8gb

The after video: https://youtu.be/F-oPZwQAvXU 1 minute of the same thing, then i'm going to example.com, and after some delay CG starts and it's a much much shorter freeze
Ram usage goes from 600mb to 2.5gb, and down to 1.2gb after GC

Comment 9

[Artem Manushenkov]

Attachment: Bug 1986144 - fix rule, property, swatch memory leak in inspector. r=#devtools,jdescottes

Comment 10

[Artem Manushenkov]

The patch fixes 95% to 100% of the colorswatch leaks. In the after video you see full CG for the swatches, but in some circumstances about 5% of the leaks remain, I don't know yet why. I'll keep digging.

Comment 11

[Artem Manushenkov]

Attachment: Bug 1986144 - breakup circular references when destroying entities in inspector to reduce memory leak. r=#devtools,jdescottes

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1986144 - [devtools] Add test to cover inspector node selection leaks. r=#devtools

Comment 13

[Julian Descottes [:jdescottes]]

Bumping this to P2 based on comment 8

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/602a0d7ecc7c
https://hg.mozilla.org/integration/autoland/rev/09a77e9139f3
[devtools] Add test to cover inspector node selection leaks. r=devtools-reviewers,nchevobbe

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/09a77e9139f3

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/42c622fc99c3
https://hg.mozilla.org/integration/autoland/rev/6b05e725e2ed
fix rule, property, swatch memory leak in inspector. r=devtools-reviewers,ochameau

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6b05e725e2ed