Closed Bug 728426 Opened 10 years ago Closed 9 years ago
Opening and then closing bookmarks sidebar keeps the bookmarks
Panel .xul and/or history-panel .xul document alive
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.
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
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.
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.
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.
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!
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: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #643798 - Flags: review?(dao)
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 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?
(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.
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-
(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.
Corrected a couple of tests and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ef417851a515
I suppose this may cause some add-on compatibility problem, where they check/set hidden on the sidebar, so adding compat keyword.
(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?
(In reply to :Gavin Sharp (use firstname.lastname@example.org 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)
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?
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.
Whiteboard: [MemShrink:P2] → [MemShrink:P2][fixed-in-fx-team]
Target Milestone: --- → Firefox 17
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-fx-team] → [MemShrink:P2]
removing compat since we went the first approach.
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.
Comment on attachment 644625 [details] [diff] [review] patch v1b low risk, approving.
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.
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.