795988 - Closing browser window with Developer Toolbar open leaks an everything

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Jesse Ruderman]

Attachment: 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 2

[Nicholas Nethercote [inactive]]

This sounds bad!  Can someone who knows this code take a look?

Comment 3

[Kris Maglione [:kmag]]

Has anybody looked at this yet? It seems like this could easily cause serious problems for developers...

Comment 4

[Paul Rouget [:paul]]

Joe - any idea about what is going on here? Can I help?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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]]

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

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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]]

Attachment: 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]]

Attachment: v1

Clean on try (https://tbpl.mozilla.org/?tree=Try&rev=df1e1af376ab) but want to tidy up patch.

Comment 10

[Joe Walker [:jwalker]]

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]]

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]]

Attachment: 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]]

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]]

(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]]

Comment on attachment 694819 [details] [diff] [review]
v1

r+ with

> var environment = { value: 42 };

explained.

Comment 16

[Joe Walker [:jwalker]]

Attachment: 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] (inactive)]

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]]

(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]]

Attachment: v3

Comment 20

[Joe Walker [:jwalker]]

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

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

(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]]

(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]]

Attachment: 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]]

Try is open now.

Comment 26

[Joe Walker [:jwalker]]

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

Comment 27

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=a2754cd8e66b
https://hg.mozilla.org/integration/fx-team/rev/8e0815207999

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/8e0815207999