Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 7 years ago

Earlier today I downloaded a new version of vcxsrv from sourceforge (what year is this?).  Tonight I noticed that the sourceforge window had leaked.  I closed all my windows and tabs except for one about:home in the affected content window.  The content ghost window went away, but I found I had a leaked browser.xul window.

bkelly@valen:/mnt/c/devel/tmp/cclogs$ grep nsGlobalWindow * | grep browser.xul
cc-edges.3724.1502241632.log:0000027067D46800 [rc=133] nsGlobalWindow # 13 inner chrome://browser/content/browser.xul
cc-edges.3724.1502241632.log:000002706A169000 [rc=51] nsGlobalWindow # 6 inner chrome://browser/content/browser.xul
cc-edges.3724.1502241632.log:0000027067D45000 [rc=9] nsGlobalWindow # 12 outer chrome://browser/content/browser.xul
bkelly@valen:/mnt/c/devel/tmp/cclogs$ /mnt/c/devel/heapgraph/find_roots.py cc-edges.3724.1502241632.log 000002706A169000
Parsing cc-edges.3724.1502241632.log. Done loading graph.

0000027069E13060 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 000002706A169000 [nsGlobalWindow # 6 inner chrome://browser/content/browser.xul]

    Root 0000027069E13060 is a marked GC object.

bkelly@valen:/mnt/c/devel/tmp/cclogs$ /mnt/c/devel/heapgraph/find_roots.py gc-edges.3724.1502241632.log -bro 0000027069E13060
Parsing gc-edges.3724.1502241632.log. Done loading graph.

via mCallback :
0000027077171FB0 [Function onWindowUnload]
    --[script]--> 000002707716A718 [script chrome://browser/content/downloads/indicato]
    --[sourceObject]--> 00000270730CFA40 [Proxy <no private>]
    --[private]--> 000002706E4158C0 [ScriptSource <no private>]
    --[group]--> 0000027069E101C0 [object_group]
    --[group_global]--> 0000027069E13060 [Window <no private>]

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 1 • 7 years ago

The thing where the content window leak morphed into a browser.xul leak seems similar to bug 1387583, but that seemed unrelated to downloads.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 2 • 7 years ago

Gijs, any ideas here?  It looks like a window unload handler is causing us to leak the whole browser.xul.  Not quite sure how to reproduce this, but its possible I had the downloads pane open when I closed the firefox window.

	:Gijs (he/him)
	Comment 3 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #2)
> Gijs, any ideas here?  It looks like a window unload handler is causing us
> to leak the whole browser.xul.  Not quite sure how to reproduce this, but
> its possible I had the downloads pane open when I closed the firefox window.

Not really... I don't know how to read the leak log here, so I'm not really sure what it's supposed to be telling me. But onWindowUnload is a thing in downloads.js and indicator.js, and both of those get loaded in a window. So it seems like the cycle collector should be able to deal with that - scripts in a window that add listeners to that window should just get wiped when the window is closed, right? Am I missing something obvious here?

	:Gijs (he/him)
	Comment 4 • 7 years ago

To be clear, normally when I've hit leaks in browser JS, the issue is that something that outlives the window (like a jsm) keeps references. But downloads.js and indicator.js aren't JSMs, they are per-window scripts, so I don't understand how they could be outliving the window in order to keep it alive after it's been closed.

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 5 • 7 years ago

> So it seems like the cycle collector should be able to deal with that - scripts in a window that
> add listeners to that window should just get wiped when the window is closed, right?

I believe this is only the case for content windows.  We would need to land bug 1276366 to for this to work in chrome windows.  Is that right Andrew?

	Andrew McCreight [:mccr8]
	Comment 6 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #5)
> I believe this is only the case for content windows.  We would need to land
> bug 1276366 to for this to work in chrome windows.  Is that right Andrew?

Yeah, we won't cut chrome-chrome wrappers when a window is closed. I don't know if there's some separate mechanism that should remove listeners for a particular window.

	:Paolo Amadini
	Comment 7 • 7 years ago

The way we load these scripts has changed in bug 1381853. Is this a related regression?

	:Gijs (he/him)
	Comment 8 • 7 years ago

So what's the appropriate fix here (removing event listeners onunload if we don't already?), and can we verify that this leak is 'real', ie do we have more explicit STR than comment #0? At this point I'm a bit confused about the state of this bug. If we leak the world (browser window) for every window in which we start a download then that's pretty serious...

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 9 • 7 years ago

I expect you are being bitten by bug 1296099.  Maybe listen for tab close as well?

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 10 • 7 years ago

Hmm, or not.  I can't reproduce by simple steps I would expect to break if that was the problem.

	:Gijs (he/him)
	Comment 11 • 7 years ago

(In reply to Ben Kelly [:bkelly] from comment #10)
> Hmm, or not.  I can't reproduce by simple steps I would expect to break if
> that was the problem.

So does this mean you can't easily reproduce the leak? I'd still like to address this, but I'm also still confused about how to reproduce. :-)

	Ben Kelly [:bkelly, not reviewing] Reporter
	Comment 12 • 7 years ago

Its a leak I found during my day-to-day browsing.  I don't have exact STR.  The GC/CC logs pointed at this event handler in indicator.js.  I don't have any more info, unfortunately.

	Sylvestre Ledru [:Sylvestre]
	Comment 13 • 6 years ago

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information