Closing browser window with Developer Toolbar open leaks an everything

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: jwalker)

Tracking

(Blocks: 1 bug, {mlk})

20 Branch
Firefox 20
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1][fixed-in-fxteam])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.)
Whiteboard: [MemShrink]
Duplicate of this bug: 793653
This sounds bad!  Can someone who knows this code take a look?
Whiteboard: [MemShrink] → [MemShrink:P1]

Updated

5 years ago
Component: Developer Tools: Console → Developer Tools
Has anybody looked at this yet? It seems like this could easily cause serious problems for developers...
Target Milestone: --- → Firefox 19
Version: Trunk → 20 Branch

Comment 4

5 years ago
Joe - any idea about what is going on here? Can I help?
Flags: needinfo?(jwalker)
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.
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?
Flags: needinfo?(jwalker)
Assignee: nobody → mratcliffe
Blocks: 818910
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.
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
Assignee: mratcliffe → jwalker
Status: NEW → ASSIGNED
Created attachment 694819 [details] [diff] [review]
v1

Clean on try (https://tbpl.mozilla.org/?tree=Try&rev=df1e1af376ab) but want to tidy up patch.
Attachment #694510 - Attachment is obsolete: true
Blocks: 821732
Blocks: 818432
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.
Attachment #694819 - Flags: review?(paul)
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?
Attachment #694819 - Flags: review?(mratcliffe)
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

5 years ago
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 :)
(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

4 years ago
Comment on attachment 694819 [details] [diff] [review]
v1

r+ with

> var environment = { value: 42 };

explained.
Attachment #694819 - Flags: review?(paul) → review+
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.
Attachment #694819 - Attachment is obsolete: true
Attachment #694819 - Flags: review?(mratcliffe)
Attachment #697441 - Flags: review?(ttaubert)
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.
(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.
Created attachment 697516 [details] [diff] [review]
v3
Attachment #697441 - Attachment is obsolete: true
Attachment #697441 - Flags: review?(ttaubert)
Attachment #697516 - Flags: review?(ttaubert)
https://tbpl.mozilla.org/?tree=Try&rev=63032d5e2c40
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 :)
Attachment #697516 - Flags: review?(ttaubert) → review+
(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.
(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)
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)
Try is open now.
https://tbpl.mozilla.org/?tree=Try&rev=bbc9a937c4cb
Whiteboard: [MemShrink:P1] → [MemShrink:P1][fixed-in-fxteam]
Attachment #697516 - Attachment is obsolete: true
Attachment #694832 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Fx-Team&rev=a2754cd8e66b
https://hg.mozilla.org/integration/fx-team/rev/8e0815207999
https://hg.mozilla.org/mozilla-central/rev/8e0815207999
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 19 → Firefox 20
(Reporter)

Updated

4 years ago
Blocks: 894118
You need to log in before you can comment on or make changes to this bug.