932880 - Developer tools leak many windows until shutdown in browser-chrome tests

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: browser-chrome shutdown log

from IRC:
04:07	khuey	and a ton of devtools stuff
04:08	khuey	https://khuey.pastebin.mozilla.org/3378934
04:08	khuey	that's a successful bc run
04:08	khuey	notice all the devtools stuff not getting GCd until the end
04:08	njn	is the devtools pane being opened and then not closed?
04:09	khuey	well the number of windows seems to suggest no
04:09	khuey	since that would be 50 or more devtools pages
04:09	khuey	*panes

(I've attached the shutdown log in question for when the pastebin goes away)

Comment 1

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

Attachment: Patch

This is where I got today but we're still leaking through toolsMap in ways I don't understand.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Victor, any ideas?

Comment 3

[Dave Camp (:dcamp)]

Joe, can you find an owner for this please?

Comment 4

[Ed Morley [:emorley]]

(This bug is one of three holding the trees closed - bug 932781 comment 11)

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 824754 [details] [diff] [review]
Patch

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

::: browser/devtools/framework/toolbox.js
@@ +912,1 @@
>      });

Let's add a .then(null, console.error); here to see if there is an error in the teardown path.

Comment 6

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 824754 [details] [diff] [review]
> Patch
> 
> Review of attachment 824754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +912,1 @@
> >      });
> 
> Let's add a .then(null, console.error); here to see if there is an error in
> the teardown path.

This is from our conversation in the day. I think he did the .then(null, console.error) and got this : (not sure though)

INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/inspector/t
est/browser_inspector_bug_665880.js | finished in 8658ms
TEST-INFO | checking window state
DBG-SERVER: Received packet 33: {
  "from": "conn0.domwalker23",
  "error": "unknownError",
  "message": "[Exception... \"Component returned failure code: 0x80004002 (NS_NO
INTERFACE) [nsIInterfaceRequestor.getInterface]\"  nsresult: \"0x80004002 (NS_NO
INTERFACE)\"  location: \"JS frame :: resource://gre/modules/devtools/LayoutHelp
ers.jsm :: LayoutHelpers :: line 20\"  data: no]"
}
DBG-SERVER: Received packet 34: {
  "from": "conn0.pagestyle24",
  "error": "unknownError",
  "message": "[Exception... \"Component returned failure code: 0x80004002 (NS_NO
INTERFACE) [nsIInterfaceRequestor.getInterface]\"  nsresult: \"0x80004002 (NS_NO
INTERFACE)\"  location: \"JS frame :: resource://gre/modules/devtools/LayoutHelp
ers.jsm :: LayoutHelpers :: line 20\"  data: no]"
}
DBG-SERVER: Received packet 35: {
  "from": "conn0.pagestyle24",
  "error": "unknownError",
  "message": "TypeError: win is null"
}
DBG-SERVER: Received packet 36: {
  "registered": [
    "console-api-profiler"
  ],
  "from": "conn0.profiler6"
}
DBG-SERVER: Received packet 37: {
  "from": "conn0.domwalker23"
}
DBG-SERVER: Received packet 38: {
  "error": "wrongState",
  "from": "conn0.tab2"
}
DBG-SERVER: Cleaning up connection.

Comment 7

[Girish Sharma [:Optimizer]]

(our conversation earlier in the morning*)

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

No, that was something different. Abruptly removing the tab isn't properly handled by the inspector backend.

On a related note we could try adding this to the inspector's head.js:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/head.js#32

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 824754 [details] [diff] [review]
> Patch
> 
> Review of attachment 824754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox.js
> @@ +912,1 @@
> >      });
> 
> Let's add a .then(null, console.error); here to see if there is an error in
> the teardown path.

Pushed the patch along with the additional logging to try: https://tbpl.mozilla.org/?tree=Try&rev=104b7f5a0cec.

I'm trying to get my environment set up to provide the same information that I see at the end of the test here: https://bugzilla.mozilla.org/attachment.cgi?id=824741.  Right now I've added --enable-logrefcnt, which gives plenty of information, but it isn't the same as what I see in the attachment.  For reference, here is what I am seeing at the end of a devtools/inspector tests with logrefcnt on: https://gist.github.com/bgrins/ee04e1cf20633f07586c.  Is there a particular configuration setting that will allow the end of the test to appear as it does in the log attached to this bug?

Comment 10

[Andrew McCreight [:mccr8]]

1. regular debug build
2. don't have MOZ_QUIET set in your environment

That should be enough to get it to show ++DOMWINDOW and --DOMWINDOW.

Comment 11

[Dave Camp (:dcamp)]

Attachment: updated patch

When the connection is closed suddenly, outstanding requests will never get an answer, so toolbox shutdown is being delayed indefinitely.  This rejects all outstanding request promises during front destruction.

I added this on top of khuey's patch, which seemed fine.

khuey, does this clean stuff up for you?

Comment 12

[Dave Camp (:dcamp)]

https://tbpl.mozilla.org/?tree=Try&rev=e9a2f6f416e7

Comment 13

[Anton Kovalyov (:anton)]

Comment on attachment 824976 [details] [diff] [review]
updated patch

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

LGTM as long as try is green.

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Dave Camp (:dcamp) from comment #11)
> Created attachment 824976 [details] [diff] [review]
> updated patch
> 
> When the connection is closed suddenly, outstanding requests will never get
> an answer, so toolbox shutdown is being delayed indefinitely.  This rejects
> all outstanding request promises during front destruction.
> 
> I added this on top of khuey's patch, which seemed fine.
> 
> khuey, does this clean stuff up for you?

I've run dcamp's patch locally and can confirm that this cleans up the shutdown log for the inspector tests.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Looks like there is one unknown testfailure
chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_835722_infobar_reappears.js | uncaught exception - TypeError: node is undefined at chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_835722_infobar_reappears.js:25

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #15)
> Looks like there is one unknown testfailure
> chrome://mochitests/content/browser/browser/devtools/inspector/test/
> browser_inspector_bug_835722_infobar_reappears.js | uncaught exception -
> TypeError: node is undefined at
> chrome://mochitests/content/browser/browser/devtools/inspector/test/
> browser_inspector_bug_835722_infobar_reappears.js:25

I'm looking into this error.

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 824976 [details] [diff] [review]
updated patch

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

::: browser/devtools/inspector/inspector-panel.js
@@ +652,5 @@
>      this._markupFrame.removeEventListener("load", this._boundMarkupFrameLoad, true);
>      delete this._boundMarkupFrameLoad;
>  
>      this._markupBox.removeAttribute("hidden");
> +    delete this._markupBox;

This line should be removed, it is causing a test failure on browser_inspector_bug_835722_infobar_reappears.js

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: updatedpatch2.patch

Resolves test failure from Attachment 824976 [details] [diff].  Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=9f808fe8ed10

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: 932880.patch

Rebuild patch after resolving bug with proper commit message.  Pushed to try, again: https://tbpl.mozilla.org/?tree=Try&rev=1ef13f4cdfa6

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/494d0cca0cb0

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

The new/old shutdown leak detector from bug 932898 reveals lots of leaks, only when running browser/devtools/inspector/test/:

http://pastebin.mozilla.org/3388252

With the patch applied that Panos just pushed it looks a lot better, but there's still some leaks left:

http://pastebin.mozilla.org/3388365

Please ignore the first two (maybe three) lines. They're caused by the BrowserNewTabPreloader that keeps two windows alive until shutdown.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #21)
> The new/old shutdown leak detector from bug 932898 reveals lots of leaks,
> only when running browser/devtools/inspector/test/:
> 
> http://pastebin.mozilla.org/3388252
> 
> With the patch applied that Panos just pushed it looks a lot better, but
> there's still some leaks left:
> 
> http://pastebin.mozilla.org/3388365
> 
> Please ignore the first two (maybe three) lines. They're caused by the
> BrowserNewTabPreloader that keeps two windows alive until shutdown.

I'm looking into the remaining leaks.  So we should expect that there will still be a few even after everything is resolved due to the BrowserNewTabPreloader?

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: leak-inspector.patch

This fixes the leaks in browser/devtools/inspector.  Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f8b2e831429e.  

Tim, I still see a few errors at the end of test, but if I remove these tests it just shows 4 errors inside of other tests which leads me to believe that it is a side effect of the leak checking.  I am running the latest patch on Bug 932898, should I not be seeing this?

 1:15.70 TEST-UNEXPECTED-FAIL | ShutdownLeaks | leaked 4 DOMWindow(s) and 1 DocShell(s) until shutdown
 1:15.70 TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_basic_highlighter.js | leaked 2 window(s) until shutdown [url = data:text/html,<html></html>]
 1:15.70 TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_sidebarstate.js | leaked 1 window(s) until shutdown [url = about:blank]
 1:15.70 TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_sidebarstate.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/markup-view.xhtml]
 1:15.70 TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_select_last_selected.js | leaked 1 docShell(s) until shutdown

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Brian Grinstead [:bgrins] from comment #23)
>  1:15.70 TEST-UNEXPECTED-FAIL |
> browser/devtools/inspector/test/browser_inspector_basic_highlighter.js |
> leaked 2 window(s) until shutdown [url = data:text/html,<html></html>]

This seems to be caused by browser/devtools/styleinspector/rule-view.js. It fills gDummyPromise but never clears it. What's the deal with that and what is it used for?

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: leak-inspector-2.patch

A few more updates to the leak-inspector patch.  Resolves some issues with ruleview and a couple style inspector fixes.  Have inspector failures down to one test, which I believe has to do with timing of the toolbox closing.  

Basically, the destroy() function is being called before selectTool() finishes in the open().  This callback https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#208 fires *after* the destroy: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#853.  Still investigating how to resolve this.

Comment 26

[Mihai Sucan [:msucan]]

With these two patches I see no more DOM window memleaks in the web console tests (local debug build).

Try push https://tbpl.mozilla.org/?tree=Try&rev=5cf4d69b2579

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make targets oblivious to inspectors

This patch doesn't seem to reduce the leaks, but it removes the inspector reference from the target object, which should at least make the code easier to follow. It also explicitly nullifies the inspector reference on destroy(), which might have made a small improvement.

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make preNest/postNest not throw when the browser is abruptly shut down

Another fix for an error that appears during tests, but doesn't cause the leak. This was a regression from bug 880511.

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: leak-devtools.patch

This patch includes fixes for scratchpad, notably dropping the number of leaked windows from 80 to 2.  It also lowers the number of leaked windows on the styleinspector to 8, inspector to 2, and I believe it clears up the webconsole as noted by msucan.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks bzexport, but Brian has been doing most of the work here.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 825533 [details] [diff] [review]
leak-devtools.patch

Pushed to try:

* Without leak detector applied: https://tbpl.mozilla.org/?tree=Try&rev=23e0f7182266.
* With leak detector applied (not sure if it will survive all the tests): https://tbpl.mozilla.org/?tree=Try&rev=b2535855adf6.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 825533 [details] [diff] [review]
leak-devtools.patch

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

::: browser/devtools/inspector/inspector-panel.js
@@ +675,5 @@
>        this._markupFrame.parentNode.removeChild(this._markupFrame);
>        delete this._markupFrame;
>      }
> +
> +    delete this._markupBox;

this._markupBox = null; would be better (avoids putting the object in dictionary mode), since this isn't just called from destroy().

::: browser/devtools/inspector/test/browser_inspector_select_last_selected.js
@@ +33,5 @@
> +    inspector.destroy();
> +    toolbox.destroy();
> +    toolbox = inspector = page1 = page2 = null;
> +    gBrowser.removeCurrentTab();
> +    finish();

Since the two destroy() calls return promises, let's chain them for maximum safety:

inspector.destroy().then(() => toolbox.destroy()).then(() => {...});

::: browser/devtools/scratchpad/test/browser.ini
@@ +27,5 @@
>  [browser_scratchpad_inspect.js]
>  [browser_scratchpad_long_string.js]
>  [browser_scratchpad_open.js]
>  [browser_scratchpad_open_error_console.js]
> +# Disabled, as escodegen is being replaced

Add the bug number so we won't forget please.

Comment 33

[Brian Grinstead [:bgrins]]

Running mochitest-browser on devtools locally with these patches applied, here are the remaining leaks:


25:01.21 *** End BrowserChrome Test Results ***
25:04.53 TEST-UNEXPECTED-FAIL | ShutdownLeaks | leaked 23 DOMWindow(s) and 1 DocShell(s) until shutdown
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/device.xhtml]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/connection-footer.xhtml]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = about:app-manager]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/projects.xhtml]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 1 window(s) until shutdown [url = about:blank]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/widgets/VariablesView.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | leaked 2 window(s) until shutdown [url = about:blank]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/framework/toolbox.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/markup-view.xhtml]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/inspector/inspector.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | leaked 1 docShell(s) until shutdown
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/scratchpad/test/browser_scratchpad_bug_679467_falsy.js | leaked 1 window(s) until shutdown [url = about:blank]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/scratchpad/test/browser_scratchpad_bug_679467_falsy.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/scratchpad.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/scratchpad/test/browser_scratchpad_contexts.js | leaked 1 window(s) until shutdown [url = about:blank]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/scratchpad/test/browser_scratchpad_contexts.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/scratchpad.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_bug722196_rule_view_media_queries.js | leaked 1 window(s) until shutdown [url = about:blank]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_bug722196_rule_view_media_queries.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/inspector/inspector.xul]
25:04.53 TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_clean-exit-window.js | leaked 1 window(s) until shutdown [url = http://example.com/browser/browser/devtools/debugger/test/doc_inline-debugger-statement.html]

Comment 34

[Brian Grinstead [:bgrins]]

Attachment: leak-devtools.patch

Updated with comments from Panos.  Waiting for try build to finish: https://tbpl.mozilla.org/?tree=Try&rev=e98f83b74355

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/494d0cca0cb0

Comment 36

[Nicholas Nethercote [inactive]]

Should this have been closed?  There are still two patches awaiting review.  Add "[leave open]" to the whiteboard?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 825590 [details] [diff] [review]
leak-devtools.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/885ec75e5600

Comment 38

[Brian Grinstead [:bgrins]]

Attachment: leak-devtools-2.patch

This patch will fix the remaining devtools bc leaks (WIP, will mark as obsolete with a final version).  See below for the logs of the remaining leaks after a full run with all three patches applied.

> 26:42.64 TEST-UNEXPECTED-FAIL | ShutdownLeaks | leaked 11 DOMWindow(s) and 0 DocShell(s) until shutdown
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/device.xhtml]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/connection-footer.xhtml]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = about:app-manager]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/app-manager/projects.xhtml]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 1 window(s) until shutdown [url = about:blank]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/app-manager/test/browser_manifest_editor.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/widgets/VariablesView.xul]
> 26:42.64 TEST-UNEXPECTED-FAIL | browser/devtools/debugger/test/browser_dbg_clean-exit-window.js | leaked 1 window(s) until shutdown [url = http://example.com/browser/browser/devtools/debugger/test/doc_inline-debugger-statement.html]

Comment 39

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Fix window leaks in App Manager

This patch fixes the 6 failures from App Manager window leaks.

Try: https://tbpl.mozilla.org/?tree=Try&rev=8f44cfab4fc8

Comment 40

[Ed Morley [:emorley]]

How far back do we need to backport each of these patches?

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 825512 [details] [diff] [review]
Make targets oblivious to inspectors

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

I just noticed that this patch breaks browser_inspector_basic_highlighter.js as it is. Let me think about it some more.

Comment 42

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Make targets oblivious to inspectors v2

I just had to make sure that the inspector panel doesn't pretend to be destroyed until its InspectorFront is destroyed, too.

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/885ec75e5600

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 825712 [details] [diff] [review]
Fix window leaks in App Manager

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

Looks good to me.

::: browser/devtools/app-manager/connection-store.js
@@ +33,5 @@
>  
>  ConnectionStore.prototype = {
> +  destroy: function() {
> +    if (this._connection) {
> +      this._connection.off(Connection.Events.DESTROYED, this.destroy);

Can you add a comment why this is necessary?

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Fix window leaks in App Manager v2

Carrying over past's r+ from attachment 825712 [details] [diff] [review].

(In reply to Panos Astithas [:past] from comment #44)
> ::: browser/devtools/app-manager/connection-store.js
> @@ +33,5 @@
> >  
> >  ConnectionStore.prototype = {
> > +  destroy: function() {
> > +    if (this._connection) {
> > +      this._connection.off(Connection.Events.DESTROYED, this.destroy);
> 
> Can you add a comment why this is necessary?

Sure!  I've added a comment in each place where the .once() handler is manually unbound like this.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

(In reply to Panos Astithas [:past] from comment #28)
> Created attachment 825532 [details] [diff] [review]
> Make preNest/postNest not throw when the browser is abruptly shut down
> 
> Another fix for an error that appears during tests, but doesn't cause the
> leak. This was a regression from bug 880511.

Does this need to land as well?

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 825532 [details] [diff] [review]
Make preNest/postNest not throw when the browser is abruptly shut down

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> (In reply to Panos Astithas [:past] from comment #28)
> > Created attachment 825532 [details] [diff] [review]
> > Make preNest/postNest not throw when the browser is abruptly shut down
> > 
> > Another fix for an error that appears during tests, but doesn't cause the
> > leak. This was a regression from bug 880511.
> 
> Does this need to land as well?

Yes please!

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/dd88e2d76be6
https://hg.mozilla.org/integration/fx-team/rev/271b1a2fde86

Comment 49

[Alex Keybl [:akeybl]]

Hasn't been an issue post-release of FF25. We'd take a low risk fix up to Beta, but no need to track.

Comment 50

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Small cleanups and fixes for browser_dbg_clean-exit-window.js

The last leaking test with ttaubert's patch from bug 932898 is browser_dbg_clean-exit-window.js. This patch contains some fixes and cleanups that unfortunately don't plug the leak. Unless someone can come up with any other ideas, I am of the opinion that once Tim's patch lands we should just disable this test.

The test was created to make sure we don't regress the small fix in bug 916458, but I think I have spent a disproportionate amount of time on it compared to its significance. I will try to track down someone more knowledgable with leaks than me to look into it, but if it comes to block the landing of Tim's patch, then let's just disable it for now.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dd88e2d76be6
https://hg.mozilla.org/mozilla-central/rev/271b1a2fde86

Comment 52

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

Comment on attachment 826022 [details] [diff] [review]
Small cleanups and fixes for browser_dbg_clean-exit-window.js

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

LGTM

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 826022 [details] [diff] [review]
Small cleanups and fixes for browser_dbg_clean-exit-window.js

https://hg.mozilla.org/integration/fx-team/rev/a9473ddc1cbf

Comment 54

[Brian Grinstead [:bgrins]]

Attachment: leak-devtools-2.patch

Resolved remaining leaks from Comment 38.  Have disabled the debugger test and filed Bug 933950 to resolve that.

Comment 55

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #54)
> Created attachment 826104 [details] [diff] [review]
> leak-devtools-2.patch
> 
> Resolved remaining leaks from Comment 38.  Have disabled the debugger test
> and filed Bug 933950 to resolve that.

Also have pushed this series to try:

* With shutdown leak inspector v3: https://tbpl.mozilla.org/?tree=Try&rev=5e320e17c1f4
* With shutdown leak inspector v4: https://tbpl.mozilla.org/?tree=Try&rev=85e32d15add2
* Without shutdown leak inspector: https://tbpl.mozilla.org/?tree=Try&rev=c03cbeb01cb1

Comment 56

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

Comment on attachment 826104 [details] [diff] [review]
leak-devtools-2.patch

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

For disabling the test only, I'll let someone else look at the other stuff (or someone already has?)

Comment 57

[Ed Morley [:emorley]]

Comment on attachment 826104 [details] [diff] [review]
leak-devtools-2.patch

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

::: browser/devtools/debugger/test/browser.ini
@@ +84,5 @@
>  [browser_dbg_breakpoints-new-script.js]
>  [browser_dbg_breakpoints-pane.js]
>  [browser_dbg_chrome-debugging.js]
> +# Disabled because of leaks - See Bug 933950.
> +# [browser_dbg_clean-exit-window.js]

I believe the preferred way to disable these is using:

[browser_dbg_clean-exit-window.js]
# Disabled because of leaks - See Bug 933950.
skip-if = true

(that way it appears in the skipped tests stats etc).

Comment 58

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

https://hg.mozilla.org/mozilla-central/rev/dd88e2d76be6
https://hg.mozilla.org/mozilla-central/rev/271b1a2fde86

Comment 59

[Brian Grinstead [:bgrins]]

Attachment: leak-devtools-2.patch

Updated test skipping syntax as suggested.  Panos, fitzgen r+ed the fact that we were skipping the tests but asked someone else take a look at the other couple of changes.

Last try push had unrelated failures, here is an updated one with the leak detector: https://tbpl.mozilla.org/?tree=Try&rev=1a4056821778.  Notice that the debugs are orange because of the leak detector, but if you take a look at the full logs you can see that there are no devtools leaks in the list: https://tbpl.mozilla.org/php/getParsedLog.php?id=30007598&tree=Try&full=1.

Comment 60

[Brian Grinstead [:bgrins]]

Comment on attachment 825836 [details] [diff] [review]
Make targets oblivious to inspectors v2

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

Looks good to me - ready for checkin pending green try: https://tbpl.mozilla.org/?tree=Try&rev=5851e5ce5d3c

Comment 61

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 826288 [details] [diff] [review]
leak-devtools-2.patch

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

::: browser/devtools/debugger/test/browser.ini
@@ +85,5 @@
>  [browser_dbg_breakpoints-pane.js]
>  [browser_dbg_chrome-debugging.js]
>  [browser_dbg_clean-exit-window.js]
> +# Disabled because of leaks - See Bug 933950.
> +skip-if = true

I'd rather make this a fail-if, so that we could tell if the leak is ever fixed by an unrelated code change, but I'm not sure if the browser.ini parser supports this syntax (others do). Can you give it a try?

Comment 62

[Ed Morley [:emorley]]

(In reply to Panos Astithas [:past] from comment #61)
> I'd rather make this a fail-if, so that we could tell if the leak is ever
> fixed by an unrelated code change, but I'm not sure if the browser.ini
> parser supports this syntax (others do). Can you give it a try?

The leak detector runs outside of the test pass/fail determination, so I don't think this would work - unless I'm misunderstanding something?

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 825836 [details] [diff] [review]
Make targets oblivious to inspectors v2

https://hg.mozilla.org/integration/fx-team/rev/a22b4507341d

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 826288 [details] [diff] [review]
leak-devtools-2.patch

Quite sure Ed is correct. With any luck, bug 933950 will ensure that it isn't disabled for long anyway :)

https://hg.mozilla.org/integration/fx-team/rev/facf28febe15

Comment 65

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Ed Morley [:edmorley UTC+1] from comment #62)
> The leak detector runs outside of the test pass/fail determination, so I
> don't think this would work - unless I'm misunderstanding something?

Ah, yes, that is a very good point! I spent too much time running this test in isolation :-)

Comment 66

[Ryan VanderMeulen [:RyanVM]]

Attachment: Aurora rollup patch

This is a rollup patch of the 6 patches that landed for Aurora uplift. Brian, if this is green, I'll ask you to formally request approval since you can better assess the risk :)

https://tbpl.mozilla.org/?tree=Try&rev=64266045fb53

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Attachment: Beta rollup patch

This one was a bit more complicated. Adding bug 915874 and bug 922125 to it made it apply for the most part.

https://tbpl.mozilla.org/?tree=Try&rev=25d348d03c0d

Comment 68

[Ryan VanderMeulen [:RyanVM]]

Miraculously, both of these look good on Try. Brian or Panos, can you please take a look for sanity's sake and request approval for uplift if you're happy with them? :)

Comment 69

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a22b4507341d
https://hg.mozilla.org/mozilla-central/rev/facf28febe15

Comment 70

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 826386 [details] [diff] [review]
Aurora rollup patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): a number of devtools-related leaks crept in after the previous leak detector was disabled in bug 728294
User impact if declined: memory usage will increase after opening and closing most (any?) developer tools
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): this is not a small patch, but we have good test coverage for the areas it touches (tool shutdown mostly), plus it is only developer-facing features that are affected
String or IDL/UUID changes made by this patch: none

Comment 71

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 826389 [details] [diff] [review]
Beta rollup patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): a number of devtools-related leaks crept in after the previous leak detector was disabled in bug 728294
User impact if declined: memory usage will increase after opening and closing most (any?) developer tools
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): this is not a small patch (and slightly bigger than the aurora patch), but we have good test coverage for the areas it touches (tool shutdown mostly), plus it is only developer-facing features that are affected
String or IDL/UUID changes made by this patch: none

Comment 72

[Panos Astithas (he/him) [:past] (please ni?)]

Thank you Ryan, my hat is off to you!

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2d77f338c43b
https://hg.mozilla.org/releases/mozilla-beta/rev/0d986a533e1c

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0d986a533e1c

Comment 75

[u279076]

If this needs extra QA testing please remove the [qa-] whiteboard tag and add the verifyme keyword.