Last Comment Bug 795988 - Closing browser window with Developer Toolbar open leaks an everything
: Closing browser window with Developer Toolbar open leaks an everything
Status: RESOLVED FIXED
[MemShrink:P1][fixed-in-fxteam]
: mlk
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 20 Branch
: x86_64 Mac OS X
: -- normal (vote)
: Firefox 20
Assigned To: Joe Walker [:jwalker] (needinfo me or ping on irc)
:
Mentors:
: 793653 (view as bug list)
Depends on:
Blocks: fuzz-keys 793653 818432 818910 821732
  Show dependency treegraph
 
Reported: 2012-10-01 10:23 PDT by Jesse Ruderman
Modified: 2013-07-15 16:29 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
leak log (67.21 KB, text/plain)
2012-10-01 10:23 PDT, Jesse Ruderman
no flags Details
WIP (14.53 KB, patch)
2012-12-20 11:53 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
v1 (14.77 KB, patch)
2012-12-21 05:57 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
paul: review+
Details | Diff | Splinter Review
For posterity only. In case we need to use unload after all. (14.65 KB, patch)
2012-12-21 06:45 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
v2 (14.79 KB, patch)
2013-01-03 06:56 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review
v3 (14.40 KB, patch)
2013-01-03 09:55 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
ttaubert: review+
Details | Diff | Splinter Review
with getOwnPropertyDescriptor (14.45 KB, patch)
2013-01-04 06:04 PST, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2012-10-01 10:23:35 PDT
Created attachment 666602 [details]
leak log

1. Run a debug build of Firefox with XPCOM_MEM_LEAK_LOG=2
2. Press Shift+F2 to open the Developer Toolbar.
3. Close the browser window (while the Developer Toolbar is still open).
4. Quit Firefox.

Result: trace-refcnt reports leak of 6 nsGlobalWindow, 24 nsDocument, etc

(cf bug 793653, which I was unable to reproduce.)
Comment 1 Nicholas Nethercote [:njn] 2012-10-02 16:16:22 PDT
*** Bug 793653 has been marked as a duplicate of this bug. ***
Comment 2 Nicholas Nethercote [:njn] 2012-10-02 16:17:29 PDT
This sounds bad!  Can someone who knows this code take a look?
Comment 3 Kris Maglione [:kmag] 2012-11-29 11:45:19 PST
Has anybody looked at this yet? It seems like this could easily cause serious problems for developers...
Comment 4 Paul Rouget [:paul] 2012-12-03 03:42:47 PST
Joe - any idea about what is going on here? Can I help?
Comment 5 Michael Ratcliffe [:miker] [:mratcliffe] 2012-12-04 03:28:10 PST
I looked into this a while back. Closing the toolbar when the browser is closed is the easy fix (as long as the open/closed status is preserved).

Joe probably knows the specifics though.
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-05 02:33:02 PST
I think Mike is right, that we need to close the toolbar on window close, which is a simple fix. What do you think about fixing it Mike?
Comment 7 Michael Ratcliffe [:miker] [:mratcliffe] 2012-12-06 08:15:24 PST
PR in https://github.com/joewalker/devtools-window/pull/322 ... this patch fixes the case where a single browser window is opened and closed with the developer toolbar open. I have created a new bug for separating command line instances so that they can be destroyed individually.
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-20 11:53:21 PST
Created attachment 694510 [details] [diff] [review]
WIP

Will probably want to cut the patch up. We're solving several problems here.
Also the browser.js hack is vile.

https://tbpl.mozilla.org/?tree=Try&rev=5e1a77ef9322
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 05:57:20 PST
Created attachment 694819 [details] [diff] [review]
v1

Clean on try (https://tbpl.mozilla.org/?tree=Try&rev=df1e1af376ab) but want to tidy up patch.
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 06:36:18 PST
Comment on attachment 694819 [details] [diff] [review]
v1

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

The issue with using the chrome window unload event is the large number of unload events, and the danger of not actually cleaning up properly. This uses the same basic system as the Toolbox.
Comment 11 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 06:38:00 PST
Comment on attachment 694819 [details] [diff] [review]
v1

Mike, you understand the issues of the shared output in GCLI. Perhaps you could give r+ on the changes to GCLI?
Comment 12 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 06:45:32 PST
Created attachment 694832 [details] [diff] [review]
For posterity only. In case we need to use unload after all.

Don't feel the need to review this. If we need to use unload this where I was at. Still leaks
Comment 13 Paul Rouget [:paul] 2012-12-21 07:30:40 PST
Joe, can you explain a little more what you're doing in this patch? I understand the unload thing, and I'm ok with it, but all the other modifications about CommandOutputManager, I'm not sure to understand how it's related, and it's 80% of the patch :)
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-12-21 15:22:05 PST
(In reply to Paul Rouget [:paul] from comment #13)
> Joe, can you explain a little more what you're doing in this patch? I
> understand the unload thing, and I'm ok with it, but all the other
> modifications about CommandOutputManager, I'm not sure to understand how
> it's related, and it's 80% of the patch :)

Originally the assumption with GCLI was that if you had more than one command line on a web page, they would share the same output stream (I think there was some logic, but it's lost in the mists of time) In Firefox that means that by default 2 windows share the same output stream, which means that freeing stuff used by one window, and leaving other windows in tact was problematic.

This change makes the default be that there is a output stream per command line (with the ability to configure if differently if you want)

It might also be helpful to see the commits, which you can here:

https://github.com/joewalker/gcli/pull/6
Comment 15 Paul Rouget [:paul] 2013-01-03 05:07:39 PST
Comment on attachment 694819 [details] [diff] [review]
v1

r+ with

> var environment = { value: 42 };

explained.
Comment 16 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-03 06:56:27 PST
Created attachment 697441 [details] [diff] [review]
v2

Updated with review comments.
Try: https://tbpl.mozilla.org/?tree=Try&rev=0d0452ed39b1


Tim: This is a minor change to browser.js to shutdown the developer toolbar cleanly.
Comment 17 Tim Taubert [:ttaubert] 2013-01-03 08:00:19 PST
Comment on attachment 697441 [details] [diff] [review]
v2

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

::: browser/base/content/browser.js
@@ +1518,4 @@
>  
>      gDevToolsBrowser.forgetBrowserWindow(window);
>  
> +    if (window.DeveloperToolbarLoaded) {

How about:

> if (!__lookupGetter__("DeveloperToolbar"))
>   DeveloperToolbar.destroy();

So we don't need to track state with an extra property.
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-03 09:48:34 PST
(In reply to Tim Taubert [:ttaubert] from comment #17)
> Comment on attachment 697441 [details] [diff] [review]
> v2
> 
> Review of attachment 697441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +1518,4 @@
> >  
> >      gDevToolsBrowser.forgetBrowserWindow(window);
> >  
> > +    if (window.DeveloperToolbarLoaded) {
> 
> How about:
> 
> > if (!__lookupGetter__("DeveloperToolbar"))
> >   DeveloperToolbar.destroy();
> 
> So we don't need to track state with an extra property.

For a couple of reasons - __lookupGetter__ is evil (for some value of evil), but more, because using __lookupGetter__ assumes the implementation of defineLazyGetter, making it harder to alter defineLazyGetter in the future (although I'll admit that's unlikely)

But I'm not fussed, so I'll update the patch.
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-03 09:55:17 PST
Created attachment 697516 [details] [diff] [review]
v3
Comment 20 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-03 10:00:47 PST
https://tbpl.mozilla.org/?tree=Try&rev=63032d5e2c40
Comment 21 Tim Taubert [:ttaubert] 2013-01-03 10:41:38 PST
Comment on attachment 697516 [details] [diff] [review]
v3

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

(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #18)
> For a couple of reasons - __lookupGetter__ is evil (for some value of evil),
> but more, because using __lookupGetter__ assumes the implementation of
> defineLazyGetter, making it harder to alter defineLazyGetter in the future
> (although I'll admit that's unlikely)

That's true but it's a little less invasive... Also we use that in browser.js already so we could update all those places at once if that happens. You could of course also use Object.getOwnPropertyDescriptor() if you like that more but as we use that already just let it as is :)
Comment 22 Tim Taubert [:ttaubert] 2013-01-04 01:24:33 PST
(In reply to Tim Taubert [:ttaubert] from comment #21)
> You could of course also use Object.getOwnPropertyDescriptor()

Maybe we should rather do that because I just realized that people are working on bug 811857. Sorry for missing that.
Comment 23 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-04 05:58:20 PST
(In reply to Tim Taubert [:ttaubert] from comment #22)
> (In reply to Tim Taubert [:ttaubert] from comment #21)
> > You could of course also use Object.getOwnPropertyDescriptor()
> 
> Maybe we should rather do that because I just realized that people are
> working on bug 811857. Sorry for missing that.

I guess mixing __[define,lookup][GS]etter__ and PropertyDescriptors will work, but I'm mervous, this close to merge day.

Can we do this in a followup or contribute to the patch in bug 811857?

(Cc: Anton)
Comment 24 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-04 06:04:46 PST
Created attachment 697889 [details] [diff] [review]
with getOwnPropertyDescriptor

(Prev comment still holds - try is closed, this should work in theory, but I'd still like to be safe)
Comment 25 Victor Porof [:vporof][:vp] 2013-01-04 06:20:40 PST
Try is open now.
Comment 26 Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-01-04 08:52:37 PST
https://tbpl.mozilla.org/?tree=Try&rev=bbc9a937c4cb
Comment 28 Panos Astithas [:past] (away until 7/21) 2013-01-05 01:51:17 PST
https://hg.mozilla.org/mozilla-central/rev/8e0815207999

Note You need to log in before you can comment on or make changes to this bug.