1261869 - DevTools are leaking windows (and other objects) across refreshes, until DevTools are closed

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P1)

Description

[Greg Tatum [:gregtatum] (REO Fx132)]

I'm not sure if this is intended or not, but the memory tool is retaining references to things between refreshes. I did a few tests to explore the behavior, and it's really clear when using arrays.

I've noticed several different types of retained memory between refreshes, and this is the first one I've isolated. It seemed the simplest. There is an open bug on a game retaining memory with Bug 1255568. I don't think this particular one is related.

See the attached video for the steps to reproduce.

https://github.com/TatumCreative/memory-leak-examples

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Which video?

Comment 2

[Greg Tatum [:gregtatum] (REO Fx132)]

Ah I guess it failed to upload as it was too big.

http://gregtatum.com/tmp/bugzilla/memory-leak-array.mov

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Given those shortest paths in the video and the fact that it is Window objects that keep leaking, my guess is that we are keeping old windows alive as debuggees somehow (but the Debugger *should* be weakly holding debuggees).

Either that, or the devtools in general (not the Debugger itself) are holding the old windows alive, and thus they are still debuggees because they haven't been collected yet. I think this is actually more plausible.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: WIP add test against leaking Window objects

This adds a test that we don't leak Window objects when devtools are open and we
keep refreshing. When it fails, it prints out the leaking window's retaining
paths. It currently fails.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: failing test output: retaining paths of leaking window

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We can safely ignore any paths going through a `Debugger` object in that test output: `Debugger`s hold debuggees weakly.

Every other path starts with "Preserved wrapper" edges, which seems to involve `nsWrapperCache`: https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/dom/base/nsWrapperCache.h#191

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think we aren't seeing the whole picture in these logs.

IIUC, `nsWrapperCache` caches JSObject reflectors of C++ DOM things, so to understand why the JSObject reflector is still alive, we have to understand why the DOM thing is still alive, but that is a question for the cycle collector, which JS::ubi::Node does not talk to yet.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Need to dig into the CC logs and post-processing tools for them to find out more (or extend JS::ubi::Node).

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Does this still reproduce after bug 1276271?

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Does this still reproduce after bug 1276271?

Yes this does.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, so I'm digging in with the CC logs and find_roots.py. The results are confusing to me.

The GC sees the window is retained in a few different ways (the test added in this patch dumps the first 20 of them). For example:

> MEMORY-TEST: Shortest retaining paths for leaking Window 0x12f5cc1a0 =========================
> MEMORY-TEST:     Path #1: --------------------------------------------------------------------
> MEMORY-TEST:         0x7fff5681bc20 (other > JS::ubi::RootList)
> MEMORY-TEST:                |
> MEMORY-TEST:               DOM expando object
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5db160 (objects > Location)
> MEMORY-TEST:                |
> MEMORY-TEST:               ProxyObject_shape
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f856718 (other > js::Shape)
> MEMORY-TEST:                |
> MEMORY-TEST:               base
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f84c448 (other > js::BaseShape)
> MEMORY-TEST:                |
> MEMORY-TEST:               global
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5cc1a0 (objects > Window)

Ok, so why is the CC rooting this DOM expando object, which ends up holding the window alive?

> $ python ~/bin/heapgraph/find_roots.py ~/scratch/cc-logs/cc-edges.67348.log 0x12f5db160
> Parsing /Users/fitzgen/scratch/cc-logs/cc-edges.67348.log. Done loading graph. Reversing graph. Done.
> 
> No roots found for 0x12f5db160
>     known edges:
>        0x12fa76830 [nsLocation] --[Preserved wrapper]--> 0x12f5db160

No roots in the CC heap found? That doesn't sound good... Let's try some other GC roots that end up keeping the window alive.

> MEMORY-TEST:     Path #2: --------------------------------------------------------------------
> MEMORY-TEST:         0x7fff5681bc20 (other > JS::ubi::RootList)
> MEMORY-TEST:                |
> MEMORY-TEST:               mExpandoAndGeneration.expando
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5ce480 (objects > Object)
> MEMORY-TEST:                |
> MEMORY-TEST:               shape
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f854350 (other > js::Shape)
> MEMORY-TEST:                |
> MEMORY-TEST:               base
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5cac40 (other > js::BaseShape)
> MEMORY-TEST:                |
> MEMORY-TEST:               global
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5cc1a0 (objects > Window)

> $ python ~/bin/heapgraph/find_roots.py ~/scratch/cc-logs/cc-edges.67348.log 0x12f5ce480
> Parsing /Users/fitzgen/scratch/cc-logs/cc-edges.67348.log. Done loading graph. Reversing graph. Done.
> 
> No roots found for 0x12f5ce480
>     known edges:
>        0x12ef3e000 [nsDocument normal (xhtml) http://example.com/browser/devtools/client/memory/test/browser/doc_empty.html] > --[mExpandoAndGeneration.expando]--> 0x12f5ce480

No luck again... Let's try a GC root further down.

> MEMORY-TEST:     Path #14: --------------------------------------------------------------------
> MEMORY-TEST:         0x7fff5681bc20 (other > JS::ubi::RootList)
> MEMORY-TEST:                |
> MEMORY-TEST:               Preserved wrapper
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5db120 (objects > HTMLDocument)
> MEMORY-TEST:                |
> MEMORY-TEST:               group
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f842790 (other > js::ObjectGroup)
> MEMORY-TEST:                |
> MEMORY-TEST:               group_proto
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5cd2e0 (objects > HTMLDocumentPrototype)
> MEMORY-TEST:                |
> MEMORY-TEST:               constructor
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5ce4a0 (objects > Function)
> MEMORY-TEST:                |
> MEMORY-TEST:               shape
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f84eab0 (other > js::Shape)
> MEMORY-TEST:                |
> MEMORY-TEST:               base
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f84c218 (other > js::BaseShape)
> MEMORY-TEST:                |
> MEMORY-TEST:               global
> MEMORY-TEST:                |
> MEMORY-TEST:                V
> MEMORY-TEST:         0x12f5cc1a0 (objects > Window)

> $ python ~/bin/heapgraph/find_roots.py ~/scratch/cc-logs/cc-edges.67348.log 0x12f5db120
> Parsing /Users/fitzgen/scratch/cc-logs/cc-edges.67348.log. Done loading graph. Reversing graph. Done.
> 
> No roots found for 0x12f5db120
>     known edges:
>        0x12f5cc1a0 [JS Object (Window)] --[CLASS_OBJECT(Array)]--> 0x12f5db120
>        0x12ef3e000 [nsDocument normal (xhtml) http://example.com/browser/devtools/client/memory/test/browser/doc_empty.html] --[Preserved wrapper]--> 0x12f5db120

... etc ...

All of the GC's retaining paths to the window follow the same pattern. Either an expando or preserved wrapper object is rooted by the CC, which ends up holding the window alive in various ways. But then the find_roots.py script says that the rooted objects are not rooted in the CC heap. IIUC, I think that means they should be garbage, and shouldn't end up as roots in the GC heap. Not one hundred percent sure what exactly "no roots found for <x>" in the find_roots.py script implies.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

:mccr8, can you tell me precisely what find_scripts.py means by "no roots found for <x>"? Does the scenario in comment 11 (where the CC is rooting objects in the GC heap that do not have any roots in the CC heap) make sense to you? Thanks!

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Full test output + dumped retaining paths to window in GC heap

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #13)
> Created attachment 8761387 [details]
> Full test output + dumped retaining paths to window in GC heap

Here are the corresponding CC logs: http://fitzgeraldnick.com/media/dumping-grounds/bug-1261869-cc-logs.zip

Comment 15

[Andrew McCreight [:mccr8]]

If these aren't shutdown logs, CC optimizations might be getting in the way. You could try running with MOZ_CC_ALL_TRACES=all. That might or might not reveal something.

For a CC log, "No roots found" means that the script does not know why 0x12f5ce480 is held alive in the CC graph. This means that every object that can reach 0x12f5ce480 is either a non-black JS object or has all of its references accounted for in the graph. I assume there's no entry like 0x12f5ce480 [garbage] in the log? That would mean the CC decided it is garbage. So, that seems odd. Also double check that you have an up to date find_roots.py as I added weak map support to it in the last few months.

The debugger weak maps are actually strong references, as long as the key is alive. One possibility is that there's a cycle through C++ and debugger weak maps, and somehow the CC doesn't know about the latter. I don't remember if debugger weak maps are reported to the CC.

I filed bug 1084605 about a similar problem with windows leaking, though that was with the browser console open. I also filed bug 1084626 about a "hueyfix for debugger references", which shu wrote a patch for, but it somehow horribly broke devtools in a way that needed a devtools person to fix, so it never landed.

Comment 16

[Andrew McCreight [:mccr8]]

I looked at the logs you uploaded, and 0x12f5cc1a0 only shows up in cc-edges.67348.log, where it is marked as garbage: 0x12f5cc1a0 [garbage]

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> If these aren't shutdown logs, CC optimizations might be getting in the way.
> You could try running with MOZ_CC_ALL_TRACES=all. That might or might not
> reveal something.

I was running like this:

> MOZ_CC_LOG_ALL=1 MOZ_CC_LOG_DIRECTORY=~/scratch/cc-logs ./mach test devtools/client/memory/test/browser/browser_memory_refresh_does_not_leak.js

Is there a difference between MOZ_CC_LOG_ALL=all and MOZ_CC_LOG_ALL=1 ?

> For a CC log, "No roots found" means that the script does not know why
> 0x12f5ce480 is held alive in the CC graph. This means that every object that
> can reach 0x12f5ce480 is either a non-black JS object or has all of its
> references accounted for in the graph. I assume there's no entry like
> 0x12f5ce480 [garbage] in the log? That would mean the CC decided it is
> garbage. So, that seems odd.

(In reply to Andrew McCreight [:mccr8] from comment #16)
> I looked at the logs you uploaded, and 0x12f5cc1a0 only shows up in
> cc-edges.67348.log, where it is marked as garbage: 0x12f5cc1a0 [garbage]

So we have something that the CC knows is garbage, but is getting rooted in the GC heap anyways? That sounds bad...

> Also double check that you have an up to date
> find_roots.py as I added weak map support to it in the last few months.

Just updated, will report back.

> The debugger weak maps are actually strong references, as long as the key is
> alive. One possibility is that there's a cycle through C++ and debugger weak
> maps, and somehow the CC doesn't know about the latter. I don't remember if
> debugger weak maps are reported to the CC.

The `Debugger.Object`s hold strong references to their referent, as they should: you don't want your debuggee to disappear out from under you (barring hueyfixes).

The `DebuggerWeakMap`s map from debuggee object referent -> Debugger.Object should definitely be weak, although it is hard for me to tell if they have ephemeron behavior or not (all this weakmap subclassing is a little hard to follow and everything is named WeakMap...).

> I filed bug 1084605 about a similar problem with windows leaking, though
> that was with the browser console open. I also filed bug 1084626 about a
> "hueyfix for debugger references", which shu wrote a patch for, but it
> somehow horribly broke devtools in a way that needed a devtools person to
> fix, so it never landed.

Looking...

Comment 18

[Andrew McCreight [:mccr8]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #17)
> > MOZ_CC_LOG_ALL=1 MOZ_CC_LOG_DIRECTORY=~/scratch/cc-logs ./mach test devtools/client/memory/test/browser/browser_memory_refresh_does_not_leak.js
> 
> Is there a difference between MOZ_CC_LOG_ALL=all and MOZ_CC_LOG_ALL=1 ?
I was suggesting that you add ALL_TRACES not LOG_ALL. :)
 
> So we have something that the CC knows is garbage, but is getting rooted in
> the GC heap anyways? That sounds bad...

That's the normal way this works. It is being held alive by a C++ root, and marked gray. The CC will cause the C++ object to drop the JS reference, then the next time the GC runs it will free the JS object.

> The `DebuggerWeakMap`s map from debuggee object referent -> Debugger.Object
> should definitely be weak, although it is hard for me to tell if they have
> ephemeron behavior or not (all this weakmap subclassing is a little hard to
> follow and everything is named WeakMap...).

Oh, okay, I didn't realize that some of it might not be like an ephmeron table.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Andrew McCreight [:mccr8] from comment #18)
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #17)
> > > MOZ_CC_LOG_ALL=1 MOZ_CC_LOG_DIRECTORY=~/scratch/cc-logs ./mach test devtools/client/memory/test/browser/browser_memory_refresh_does_not_leak.js
> > 
> > Is there a difference between MOZ_CC_LOG_ALL=all and MOZ_CC_LOG_ALL=1 ?
> I was suggesting that you add ALL_TRACES not LOG_ALL. :)

Woops, fixed now, thanks :)

> > So we have something that the CC knows is garbage, but is getting rooted in
> > the GC heap anyways? That sounds bad...
> 
> That's the normal way this works. It is being held alive by a C++ root, and
> marked gray. The CC will cause the C++ object to drop the JS reference, then
> the next time the GC runs it will free the JS object.

I always get confused about CC<-->GC cycle breaking.

So a GC --> CC --> GC should always find and break cycles across the two heaps, correct? I updated the test to do that after refreshing, and the old window is still retained (and still by expandos and preserved wrapper GC roots).

The updated find_roots.py, with MOZ_CC_ALL_TRACES=all when generating the logs, is still not finding any roots for those expandos, however.

Consider me stumped :-/

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

In the logs that are posted in comment 14 there is an Event holding alive a path through JS to the window.  The Event is being destroyed in that CC though, so everything is getting collected.  Perhaps it's just taking multiple iterations of cycle collection to clean everything up (possibly because there are missing edges somewhere)?

Anyways, the attached test doesn't apply anymore, and it's not immediately obvious to me how to fix it up :(

Comment 21

[Eric Rahm [:erahm]]

P1 as this is a memory tool leaking memory. Nick per comment 20 can you add an updated test so Kyle can repro?

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: WIP add test against leaking Window objects

Rebased!

STR:

  $ ./mach test devtools/client/memory/test/browser/browser_memory_refresh_does_not_leak.js

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Rebased patch is now attached.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think in the testcase provided the "leak" may just be an artifact of GC timing. The original doc_empty.html goes away immediately after printing out the paths, even if I comment out the code in makeMemoryTest that wants to close the memory panel and the tab.

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> I think in the testcase provided the "leak" may just be an artifact of GC
> timing. The original doc_empty.html goes away immediately after printing out
> the paths, even if I comment out the code in makeMemoryTest that wants to
> close the memory panel and the tab.

I previously added a GC -> CC -> GC after refreshing which didn't make it into the latest patch I attached, but it still reproduced the leaking window.

For a non-contrived example, follow the following STR:

* Load any web page (I used google.com this time)
* Open the memory tool
* Take snapshot 1
* Change the view: "tree map" -> "aggregate"
* Type "Window" in the filter search box to show only Window objects
* Refresh
* Take snapshot 2
* Refresh
* Take snapshot 3
* Close devtools, refresh, reopen memory tool to aggregate, filtered for Window
* Take snapshot 4

I see 4 windows in snapshot 1, 7 windows in snapshot 2, 10 in snapshot 3, and 2 in snapshot 4. I see this same behavior across many pages.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, that I can definitely reproduce.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Grabbing the navigator object of a couple different Navigator objects from those leaked Google windows, I see:

khuey@minbar:~/dev/gecko-dev (dev)$ ~/dev/heapgraph/find_roots.py /tmp/gc-edges.29510.1467408671.log 0x7f1ae6b7a380 -bro
Parsing /tmp/gc-edges.29510.1467408671.log. Done loading graph.

via persistent-Object :
0x7f1aee5ca6a0 [BackstagePass 7f1aecd79a60]
    --[loader, devtools]--> 0x7f1aece13f60 [Object <no private>]
    --[_provider]--> 0x7f1aecea1060 [Object <no private>]
    --[loader]--> 0x7f1ae6b5c360 [Proxy <no private>]
    --[private]--> 0x7f1ae81ec980 [Object <no private>]
    --[sandboxes]--> 0x7f1aee5f47c0 [Object <no private>]
    --[resource://devtools/server/actors/webbrowser.js]--> 0x7f1ae6af87c0 [Proxy <no private>]
    --[private]--> 0x7f1ae6774780 [Object <no private>]
    --[DebuggerServer]--> 0x7f1ae6af8600 [Object <no private>]
    --[_connections]--> 0x7f1ae6af8da0 [Object <no private>]
    --[server1.conn0.child1/]--> 0x7f1ae6789880 [Object <no private>]
    --[_extraPools]--> 0x7f1ae40fa1c0 [Array <no private>]
    --[objectElements[4]]--> 0x7f1ae67f0920 [Object <no private>]
    --[_actors]--> 0x7f1ae40d5c80 [Object <no private>]
    --[server1.conn0.child1/26]--> 0x7f1ae40b8f40 [Object <no private>]
    --[_debuggerSourcesSeen]--> 0x7f1aef4a3550 [Set 7f1aef2690d0]
    --[key]--> 0x7f1ae2cf00a0 [Proxy <no private>]
    --[private]--> 0x7f1ae0f14e00 [Source 7f1adff516c0]
    --[Debugger.Source referent]--> 0x7f1adff516c0 [ScriptSource <no private>]
    --[shape]--> 0x7f1ae2c00998 [shape]
    --[base]--> 0x7f1ae415a218 [base_shape]
    --[global]--> 0x7f1ae6b7a1a0 [Window <no private>]
    --[s_Saa]--> 0x7f1ae6b7a380 [Navigator <no private>]

khuey@minbar:~/dev/gecko-dev (dev)$ ~/dev/heapgraph/find_roots.py /tmp/gc-edges.29510.1467408671.log 0x7f1ae6b7a2e0 -bro
Parsing /tmp/gc-edges.29510.1467408671.log. Done loading graph.

via persistent-Object :
0x7f1aee5ca6a0 [BackstagePass 7f1aecd79a60]
    --[loader, devtools]--> 0x7f1aece13f60 [Object <no private>]
    --[_provider]--> 0x7f1aecea1060 [Object <no private>]
    --[loader]--> 0x7f1ae6b5c360 [Proxy <no private>]
    --[private]--> 0x7f1ae81ec980 [Object <no private>]
    --[sandboxes]--> 0x7f1aee5f47c0 [Object <no private>]
    --[resource://devtools/server/actors/webbrowser.js]--> 0x7f1ae6af87c0 [Proxy <no private>]
    --[private]--> 0x7f1ae6774780 [Object <no private>]
    --[DebuggerServer]--> 0x7f1ae6af8600 [Object <no private>]
    --[_connections]--> 0x7f1ae6af8da0 [Object <no private>]
    --[server1.conn0.child1/]--> 0x7f1ae6789880 [Object <no private>]
    --[_extraPools]--> 0x7f1ae40fa1c0 [Array <no private>]
    --[objectElements[4]]--> 0x7f1ae67f0920 [Object <no private>]
    --[_actors]--> 0x7f1ae40d5c80 [Object <no private>]
    --[server1.conn0.child1/26]--> 0x7f1ae40b8f40 [Object <no private>]
    --[_debuggerSourcesSeen]--> 0x7f1aef4a3550 [Set 7f1aef2690d0]
    --[key]--> 0x7f1ae33ab880 [Proxy <no private>]
    --[private]--> 0x7f1ae30d1500 [Source 7f1ae2db9380]
    --[Debugger.Source referent]--> 0x7f1ae2db9380 [ScriptSource <no private>]
    --[shape]--> 0x7f1ae35560d8 [shape]
    --[base]--> 0x7f1ae2da92b8 [base_shape]
    --[global]--> 0x7f1ae6b7a240 [Window <no private>]
    --[s_Saa]--> 0x7f1ae6b7a2e0 [Navigator <no private>]

Note that both paths go through the same _debuggerSourcesSeen Set. Trivial grepping for _debuggerSourcesSeen makes it look like that's a set that nothing is ever removed from (until the tools are closed and the Set itself is garbage, anyways).

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Kyle, thanks for helping track this down! I can whip up a fix from here. Again: very appreciated!

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Happy to help.

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: roots-of-navigators.txt

I can no longer get teh test to fail either, when I add a GC -> CC -> GC, but I still repro on actual pages + refreshing.

Making _debuggerSourcesSeen a WeakSet didn't fix the issue, nor did manually clearing it on navigations.

I found a CC log with lots of Navigators that was froma CC after I knew I was retaining the windows for too long. I ran find_roots.py on every Navigator, but the results aren't telling me much.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I made _debuggerSourcesSeen a WeakSet locally, refreshed Google with the memory panel open several times to get the Window count in the snapshot up to 15ish.  Using minimize memory usage in about:memory results in them getting collected and getting down to 3 Windows in the next snapshot.

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Fix leaks in devtools

There are two leaks addressed in this commit:

1. The thread actor's `_debuggerSourcesSeen` set was never cleared. This set
exists only as a performance optimization to speed up `_addSource` in cases
where we've already added the source. Unfortunately, this set wasn't getting
cleared when we cleared debuggees out and it ended up keeping the
`Debugger.Source`, its referent, and transitively its referent's global alive. I
figured it was simpler to make it a `WeakSet` than to add it as a special case
in `ThreadActor.prototype._clearDebuggees` and manage the lifetimes by hand. I
think this fits well with its intended use as an ephemeral performance
optimization.

2. Due to a logic error, we were not clearing debuggees in the memory actor's
`Debugger` instance on navigations. This isn't really a "proper" leak, in that
if you forced a GC, the old debuggees would go away as `Debugger` holds them
weakly, however if there was no GC between navigations, then you could still see
the old windows (and everything they "retained") as roots in the snapshot. This
issue is straightforward to fix once identified: ensure that `_clearDebuggees`
is actually called on navigation.

Finally, this commit adds a test that we don't leak Window objects when devtools
are open and we keep refreshing a tab. When it fails, it prints out the leaking
window's retaining paths.

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b222df4c394

Comment 34

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8768237 [details] [diff] [review]
Fix leaks in devtools

Review of attachment 8768237 [details] [diff] [review]:
-----------------------------------------------------------------

I see nothing wrong with this patch. However, it took me some time to figure out what the patch was doing, since I'm not familiar with the memory reporter. You might want to get a second review from someone who knows the memory reporter better.

That said, if there is no such person, and you feel comfortable with me reviewing patches for the memory reporter, I'd be happy to do so, and familiarise myself with the code in the process!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/df4803bab0bc
Fix leaks in devtools; r=ejpbruel

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df4803bab0bc

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8768237 [details] [diff] [review]
Fix leaks in devtools

Approval Request Comment
[Feature/regressing bug #]:
Since the memory panel initially shipped in 44, not sure which bug # exactly

[User impact if declined]:
The tool for debugging memory leaks has a memory leak itself and ends up giving users bad data for diagnosing their own memory leaks.

[Describe test coverage new/current, TreeHerder]:
New tests for this regression, many extant tests for the memory tool's other functionality.

[Risks and why]:
Very little risk because this only affects one (relatively lesser used) tool within the devtools, so not even a risk for most users who don't use devtools at all. However, for the folks who do use the memory tool, this fix is sorely needed. Additionally, the fix is simple (investigation to find what fix to make was much harder).

[String/UUID change made/needed]:
None.

Comment 38

[Gerry Chang [:gchang]]

Comment on attachment 8768237 [details] [diff] [review]
Fix leaks in devtools

Review of attachment 8768237 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes memory leaks. Take it in 48 beta 7 and aurora.

Comment 39

[Carsten Book [:Tomcat]]

has problems to apply to aurora:

merging devtools/client/framework/test/shared-head.js
merging devtools/server/actors/memory.js
merging devtools/server/actors/script.js
warning: conflicts while merging devtools/server/actors/memory.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 40

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: [Aurora] Fix leaks in devtools

Rebased on aurora.

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: [Beta] Fix leaks in devtools

Rebased on beta.

Comment 42

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Carsten Book [:Tomcat] from comment #39)
> has problems to apply to aurora:
> 
> merging devtools/client/framework/test/shared-head.js
> merging devtools/server/actors/memory.js
> merging devtools/server/actors/script.js
> warning: conflicts while merging devtools/server/actors/memory.js! (edit,
> then use 'hg resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

I've attached aurora and beta versions of the patch. Thanks!

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4d9e1ed33f

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a57ca2165419