Closed
Bug 728426
Opened 13 years ago
Closed 12 years ago
Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul and/or history-panel.xul document alive
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
RESOLVED
FIXED
Firefox 17
People
(Reporter: smaug, Assigned: ttaubert)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
dao
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This doesn't seem to be a real leak, since when the sidebar is opened, the
document is again optimized out from the cycle collection.
Also, closing the window removes the document from cycle collection.
So, this could be either a missing cycle collection optimization or problem with
bookmarks sidebar.
Reporter | ||
Comment 1•13 years ago
|
||
Looks like same applies to history sidebar.
Summary: Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul document alive → Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul and/or history-panel.xul document alive
Reporter | ||
Comment 2•13 years ago
|
||
This is a valid runtime leak. re-opening the sidebar seems to just replace the document and the
old document is released. But while the sidebar is closed a large number objects are added
to each cycle collection and that increases CC time.
Updated•13 years ago
|
Whiteboard: [MemShrink]
Comment 3•13 years ago
|
||
Hmm, when closing the sidebar, we do navigate it to about:blank:
http://hg.mozilla.org/mozilla-central/annotate/2e34d3d7071f/browser/base/content/browser.js#l5601
(in fact you added that a few years ago in bug 424444 :)
So something must be keeping a reference to the old document somehow.
Comment 4•13 years ago
|
||
I replaced all the contents of bookmarksPanel.xul with an empty <page/> but I still see the nsDocument leak. Seems to be related to the way we handle the document, rather than a specific issue in one of these documents.
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 6•12 years ago
|
||
I can reproduce this leak locally when running the test from bug 759709. It goes away when I remove the following line:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4914
If anyone has an idea what's up with this, shoot!
Assignee | ||
Comment 7•12 years ago
|
||
It took me a litte while to find the actual because but in retrospect it's not that illogical. The bookmark panel's content viewer is kept alive as long as about:blank hasn't loaded - which does not happen as long as the <browser> is hidden. That's why it's discarded as soon as we open the bookmark panel again.
The patch simply replaces the current contentViewer with a blank one and fixes the leak on my machine.
Assignee | ||
Comment 8•12 years ago
|
||
Another option would be to wait until about:blank has loaded and then hide the sidebar. I didn't choose that because we'd need some safeguard for this asynchronous closing and we might run into a CC happening in between that could make this operation feel quite laggy.
Comment 9•12 years ago
|
||
Comment on attachment 643798 [details] [diff] [review]
patch v1
>+ // create a new content viewer because the old one doesn't get destroyed
>+ // until about:blank has loaded (which does not happen as long as the
>+ // element is hidden).
Should we just use collapsed instead of hidden?
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #9)
> Should we just use collapsed instead of hidden?
Good idea, that works as well but is a little more invasive. I wonder if we break some other things including add-ons by doing this?
Also, reduced the leak threshold by two. Bug 759709 is permanent.
Attachment #643798 -
Attachment is obsolete: true
Attachment #643798 -
Flags: review?(dao)
Attachment #643808 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #643808 -
Flags: review?(dao) → review+
Comment 11•12 years ago
|
||
Comment on attachment 643808 [details] [diff] [review]
patch v2
Looks like you'll also need to update a couple of tests.
Attachment #643808 -
Flags: review+ → review-
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #11)
> Looks like you'll also need to update a couple of tests.
Oh, yeah. I didn't really run tests so far.
Assignee | ||
Comment 13•12 years ago
|
||
Corrected a couple of tests and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=ef417851a515
Attachment #643808 -
Attachment is obsolete: true
Attachment #643820 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #643820 -
Flags: review?(dao) → review+
Comment 14•12 years ago
|
||
I suppose this may cause some add-on compatibility problem, where they check/set hidden on the sidebar, so adding compat keyword.
Keywords: addon-compat
Comment 15•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #9)
> Comment on attachment 643798 [details] [diff] [review]
> patch v1
>
> >+ // create a new content viewer because the old one doesn't get destroyed
> >+ // until about:blank has loaded (which does not happen as long as the
> >+ // element is hidden).
>
> Should we just use collapsed instead of hidden?
Using collapsed for the sidebar means that the sidebar will now have a greater cost (frames constructed, about:blank document loaded, etc.) when not open. What's the downside to the hack in this patch?
Comment 16•12 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> Using collapsed for the sidebar means that the sidebar will now have a
> greater cost (frames constructed, about:blank document loaded, etc.) when
> not open.
(not to mention potential add-on impact that Marco raises)
Assignee | ||
Comment 17•12 years ago
|
||
I'll probably also go for the first approach. Using "collapsed" feels too intrusive and may have side-effects like already mentioned. Can we reconsider the first patch?
Assignee | ||
Comment 18•12 years ago
|
||
Turns out we still have to set the "src" attribute to "about:blank" before we create the new content viewer or else the browser assumes the previous url is still loaded. createAboutBlankContentViewer() immediately stops the load for us.
Pushed to try and everything's green:
https://tbpl.mozilla.org/?tree=Try&rev=a4b74e0baaa3
The current leak threshold is now down to 3. Those left are false positives that will be fixed by bug 728294:
TEST-INFO | browser/base/content/test/browser_bug597218.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/tabview.html]
TEST-INFO | browser/base/content/test/browser_bug597218.js | leaked 1 docShell(s) until shutdown
TEST-INFO | browser/components/places/tests/browser/browser_views_liveupdate.js | leaked 1 window(s) until shutdown [url = about:blank]
We're "leaking" the outer and inner window of the tabview and of course the other window of the sidebar as that is created by browser_views_liveupdate.js - this bug is only about discarding the inner window.
Attachment #643820 -
Attachment is obsolete: true
Attachment #644625 -
Flags: review?(gavin.sharp)
Attachment #644625 -
Flags: review?(dao)
Updated•12 years ago
|
Attachment #644625 -
Flags: review?(dao) → review+
Assignee | ||
Comment 19•12 years ago
|
||
Whiteboard: [MemShrink:P2] → [MemShrink:P2][fixed-in-fx-team]
Assignee | ||
Updated•12 years ago
|
Attachment #644625 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → Firefox 17
Assignee | ||
Comment 20•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-fx-team] → [MemShrink:P2]
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 644625 [details] [diff] [review]
patch v1b
[Approval Request Comment]
Bug caused by (feature/regressing bug #): None.
User impact if declined: None.
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.
This is a permanent leak and we need this fix for bug 728294.
Attachment #644625 -
Flags: approval-mozilla-beta?
Attachment #644625 -
Flags: approval-mozilla-aurora?
Comment 23•12 years ago
|
||
Comment on attachment 644625 [details] [diff] [review]
patch v1b
low risk, approving.
Attachment #644625 -
Flags: approval-mozilla-beta?
Attachment #644625 -
Flags: approval-mozilla-beta+
Attachment #644625 -
Flags: approval-mozilla-aurora?
Attachment #644625 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6eccd9aabcfc
https://hg.mozilla.org/releases/mozilla-beta/rev/c5ebffc79bb4
status-firefox14:
--- → wontfix
status-firefox15:
--- → fixed
status-firefox16:
--- → fixed
status-firefox17:
--- → fixed
Comment 25•12 years ago
|
||
Verified as fixed with the guidelines from comment 6. The automated test mentioned there is now passing https://tbpl.mozilla.org/php/getParsedLog.php?id=14258029&full=1&branch=mozilla-central.
Please let me know if you need me to verify this fix any other way.
Comment 26•12 years ago
|
||
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Marking as verified in 17. Tested with about:cc add-on.
No leaked documents on beta 6, Ubuntu 12.04, after opening and closing all sidebars and toolbars from the view menu. Could previously reproduce the reported issue.
You need to log in
before you can comment on or make changes to this bug.
Description
•