Open
Bug 1388583
Opened 7 years ago
Updated 2 years ago
browser.xul leak through downloads/content/indicator.js
Categories
(Firefox :: Downloads Panel, defect, P3)
Firefox
Downloads Panel
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox57 | --- | wontfix |
People
(Reporter: bkelly, Unassigned)
References
Details
(Whiteboard: [MemShrink:P2])
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>]
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.
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.
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [MemShrink] → [MemShrink:P2]
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bkelly)
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.
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?
Flags: needinfo?(bkelly) → needinfo?(continuation)
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.
Flags: needinfo?(continuation)
Comment 7•7 years ago
|
||
The way we load these scripts has changed in bug 1381853. Is this a related regression?
Priority: -- → P2
Updated•7 years ago
|
status-firefox57:
--- → wontfix
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...
Flags: needinfo?(bkelly)
Reporter | ||
Comment 9•7 years ago
|
||
I expect you are being bitten by bug 1296099. Maybe listen for tab close as well?
Flags: needinfo?(bkelly)
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.
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. :-)
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
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
Priority: P2 → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•