I seem to have one or more browser.xul windows leaked in my 46.0.1 browser instance: │ │ ├───6.32 MB (00.61%) -- top(none)/detached │ │ │ ├──6.32 MB (00.61%) -- window(chrome://browser/content/browser.xul) │ │ │ │ ├──4.76 MB (00.46%) ++ js-compartment([System Principal], about:blank) │ │ │ │ ├──1.55 MB (00.15%) ++ dom │ │ │ │ ├──0.01 MB (00.00%) ── property-tables [2] │ │ │ │ └──0.00 MB (00.00%) ── style-sheets [2] │ │ │ └──0.00 MB (00.00%) ── window([system])/dom/other [2] I often open articles and blog posts in private browsing windows, so this is likely a PB window. I'm not sure, though. I have CC logs if anyone wants to take a look.

This is on a win10 machine.

In case it matters, I'm also running with flash enabled and tracking protection disabled since this is how most of our users work.

Ok, looking at the logs I managed to trace this back to devtools responsive design tool. Steps to reproduce: 1) Open fresh browser instance 2) Open about:memory and measure. Observe no top(none)/detached browser.xul windows. 3) Open a new window and navigate to a site (I used mozilla.org) 4) Open responsive design tools in this new window and drag the width around a bit 5) Close the new window with responsive design tools still open 6) Click minimize memory in about:memory tab 7) Measure memory again and observe there is a top(none)/detached browser.xul entry.

I think I see the problem.

I am happy to review and / or take a look!

Comment on attachment 8757486 [details] [diff] [review] P1 Don't leak windows when response design tool is closed while active. r=jryans Review of attachment 8757486 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/responsivedesign/responsivedesign.jsm @@ +362,5 @@ > this.closebutton.removeEventListener("command", this.bound_close, true); > this.addbutton.removeEventListener("command", this.bound_addPreset, true); > this.removebutton.removeEventListener("command", this.bound_removePreset, true); > this.touchbutton.removeEventListener("command", this.bound_touch, true); > + this.userAgentInput.removeEventListener("blur", this.bound_changeUA, true); This is just an event listener I noticed was not cleared. Its somewhat unrelated to the main window unload thing.

[Tracking Requested - why for this release]: I realize we are late in the beta cycle, but this is a very small change and it would be nice to fix a memory leak before going to release.

Comment on attachment 8757486 [details] [diff] [review] P1 Don't leak windows when response design tool is closed while active. r=jryans Review of attachment 8757486 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking a look at this, I had just a few cleanup notes, but otherwise looks good to me. A test would be even better! Still not quite sure how to best measure the leak though... ::: devtools/client/responsivedesign/responsivedesign.jsm @@ +186,5 @@ > > this.mm.addMessageListener("ResponsiveMode:OnContentResize", > this.bound_onContentResize); > > + this.mainWindow.addEventListener("unload", this); Let's add this when we add the TabClose / TabSelect listeners in `init` if possible. @@ +381,5 @@ > this.container.removeAttribute("responsivemode"); > this.stack.removeAttribute("responsivemode"); > > ActiveTabs.delete(this.tab); > + this.mainWindow.removeEventListener("unload", this); Can we move this up to the block above where all the others are removed? @@ +395,5 @@ > > this._telemetry.toolClosed("responsive"); > > let stopped = this.waitForMessage("ResponsiveMode:Stop:Done"); > + if (this.tab.linkedBrowser.messageManager) { waitForMessage also uses the messageManager, though it saved it to `this.mm`. If the error is that `linkedBrowser` now fails, does `this.mm` work here instead?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9) > A test would be even better! Still not quite sure how to best measure the > leak though... Browser chrome tests should already be automatically checking for windows that aren't cleaned up by the end of the tests, so anything that does the STR should work. Presumably there just weren't any tests that opened a new window.

Updated for review feedback. Moved the TabClose and TabSelect event handlers to the constructor as discussed in IRC. I'll see if I can get a test working soon.

This test will timeout and leak the window without P1. It almost works with the P1 patch, but the ResponsiveUI.close() method does not actually complete with the current P1. We can't rely on receiving messages after this.tab.linkedBrowser.messageManager is set to null. I'll post a new P1 with those fixes.

Updating the patch not to rely on receiving messages in the close() method. Asking for re-review since you previously asked me to use this.mm even if the linkedBrowser.messageManager was gone. https://treeherder.mozilla.org/#/jobs?repo=try&revision=939c5d2ffcce

FWIW, I searched for other places TabClose might be used in devtools and the other cases do have an unload handler. So I don't see this exact problem anywhere else.

Comment on attachment 8757592 [details] [diff] [review] P2 Verify responsive design UI does not leak when window is closed. r=jryans Review of attachment 8757592 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this!

https://hg.mozilla.org/integration/mozilla-inbound/rev/55dc8f7c505c https://hg.mozilla.org/integration/mozilla-inbound/rev/17d8abb5b180

Comment on attachment 8757593 [details] [diff] [review] P1 Don't leak windows when response design tool is closed while active. r=jryans Approval Request Comment [Feature/regressing bug #]: Devtools responsive design tool [User impact if declined]: Memory leak if window is closed with response design tool open. [Describe test coverage new/current, TreeHerder]: New mochitest to verify leak is fixed when window is closed. Existing mochitests to check for functional regressions. [Risks and why]: Minimal. This patch basically just adds an extra listener for window close and executes a graceful shutdown for the tool. The shutdown code was already written. [String/UUID change made/needed]: None

Comment on attachment 8757592 [details] [diff] [review] P2 Verify responsive design UI does not leak when window is closed. r=jryans See comment 17.

https://hg.mozilla.org/mozilla-central/rev/55dc8f7c505c https://hg.mozilla.org/mozilla-central/rev/17d8abb5b180

Comment on attachment 8757593 [details] [diff] [review] P1 Don't leak windows when response design tool is closed while active. r=jryans Fix a leak, taking it

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fd4a2fd817c https://hg.mozilla.org/releases/mozilla-aurora/rev/3c7a33fe0b64

Comment on attachment 8757593 [details] [diff] [review] P1 Don't leak windows when response design tool is closed while active. r=jryans AFAIK, responsive design mode is pref'd off in Fx47 by default. We are in RC week and to me this is not a release blocker as this is not a new regression in FX47 nor a default scenario.

(In reply to Ritu Kothari (:ritu) from comment #22) > Comment on attachment 8757593 [details] [diff] [review] > P1 Don't leak windows when response design tool is closed while active. > r=jryans > > AFAIK, responsive design mode is pref'd off in Fx47 by default. We are in RC > week and to me this is not a release blocker as this is not a new regression > in FX47 nor a default scenario. Ritu, this is not quite right... This bug applies to the "legacy" RDM, which has been around many release cycles, and is enabled by default. You are correct that it is not a new regression though, I would guess this bug has been present for a long time. We are also working on a rewrite of RDM, and that is disabled like you say, but that's not what is being changed in this bug. Setting ni? in case this would change the decision.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23) > (In reply to Ritu Kothari (:ritu) from comment #22) > > Comment on attachment 8757593 [details] [diff] [review] > > P1 Don't leak windows when response design tool is closed while active. > > r=jryans > > > > AFAIK, responsive design mode is pref'd off in Fx47 by default. We are in RC > > week and to me this is not a release blocker as this is not a new regression > > in FX47 nor a default scenario. > > Ritu, this is not quite right... This bug applies to the "legacy" RDM, > which has been around many release cycles, and is enabled by default. You > are correct that it is not a new regression though, I would guess this bug > has been present for a long time. > > We are also working on a rewrite of RDM, and that is disabled like you say, > but that's not what is being changed in this bug. > > Setting ni? in case this would change the decision. Thanks JRyans for the additional context. I think the decision to not uplift still holds for the following reasons: 1. It is not a new regression. and 2. The size of the patches is too big for this close to go-live date. What kind of mem leak are we looking at? Is it progressively building over the browser active session? Or is it a fixed leak? That could potentially change how we are looking at the critical-ness of this bug. But again, if this did not cause a dot release in Fx46, I doubt if this should block release for Fx47. Right?

Belatedly adding tracking for 48/49 in case this reopens.