Closed
Bug 795988
Opened 12 years ago
Closed 12 years ago
Closing browser window with Developer Toolbar open leaks an everything
Categories
(DevTools :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 20
People
(Reporter: jruderman, Assigned: jwalker)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P1][fixed-in-fxteam])
Attachments
(2 files, 5 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.)
Updated•12 years ago
|
Whiteboard: [MemShrink]
![]() |
||
Comment 2•12 years ago
|
||
This sounds bad! Can someone who knows this code take a look?
Whiteboard: [MemShrink] → [MemShrink:P1]
Updated•12 years ago
|
Component: Developer Tools: Console → Developer Tools
Comment 3•12 years ago
|
||
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•12 years ago
|
||
Joe - any idea about what is going on here? Can I help?
Flags: needinfo?(jwalker)
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
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)
Updated•12 years ago
|
Assignee: nobody → mratcliffe
Comment 7•12 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
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
Assignee | ||
Comment 9•12 years ago
|
||
Clean on try (https://tbpl.mozilla.org/?tree=Try&rev=df1e1af376ab) but want to tidy up patch.
Attachment #694510 -
Attachment is obsolete: true
Assignee | ||
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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)
Assignee | ||
Comment 12•12 years ago
|
||
Don't feel the need to review this. If we need to use unload this where I was at. Still leaks
Comment 13•12 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 :)
Assignee | ||
Comment 14•12 years ago
|
||
(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•12 years ago
|
||
Comment on attachment 694819 [details] [diff] [review] v1 r+ with > var environment = { value: 42 }; explained.
Attachment #694819 -
Flags: review?(paul) → review+
Assignee | ||
Comment 16•12 years ago
|
||
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 17•12 years ago
|
||
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.
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #697441 -
Attachment is obsolete: true
Attachment #697441 -
Flags: review?(ttaubert)
Attachment #697516 -
Flags: review?(ttaubert)
Comment 21•12 years ago
|
||
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+
Comment 22•12 years ago
|
||
(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.
Assignee | ||
Comment 23•12 years ago
|
||
(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)
Assignee | ||
Comment 24•12 years ago
|
||
(Prev comment still holds - try is closed, this should work in theory, but I'd still like to be safe)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [MemShrink:P1] → [MemShrink:P1][fixed-in-fxteam]
Assignee | ||
Updated•12 years ago
|
Attachment #697516 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #694832 -
Attachment is obsolete: true
Assignee | ||
Comment 27•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Fx-Team&rev=a2754cd8e66b https://hg.mozilla.org/integration/fx-team/rev/8e0815207999
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e0815207999
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 19 → Firefox 20
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•