Last Comment Bug 728426 - Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul and/or history-panel.xul document alive
: Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul and/o...
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Firefox 17
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 705705 (view as bug list)
Depends on: 811089
Blocks: leakychrome 776370 bc-leaks 705705 759709
  Show dependency treegraph
 
Reported: 2012-02-17 14:43 PST by Olli Pettay [:smaug]
Modified: 2012-11-15 06:46 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
verified
fixed
verified


Attachments
patch v1 (1.44 KB, patch)
2012-07-19 04:50 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (6.40 KB, patch)
2012-07-19 05:24 PDT, Tim Taubert [:ttaubert]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3 (10.45 KB, patch)
2012-07-19 06:14 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
Details | Diff | Splinter Review
patch v1b (2.06 KB, patch)
2012-07-21 05:15 PDT, Tim Taubert [:ttaubert]
dao+bmo: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-02-17 14:43:24 PST
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.
Comment 1 Olli Pettay [:smaug] 2012-02-17 14:44:53 PST
Looks like same applies to history sidebar.
Comment 2 Olli Pettay [:smaug] 2012-02-17 14:54:04 PST
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.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-17 16:57:33 PST
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 Marco Bonardo [::mak] 2012-02-21 09:52:42 PST
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.
Comment 5 Andrew McCreight [:mccr8] 2012-04-17 23:25:09 PDT
*** Bug 705705 has been marked as a duplicate of this bug. ***
Comment 6 Tim Taubert [:ttaubert] 2012-07-18 09:40:19 PDT
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!
Comment 7 Tim Taubert [:ttaubert] 2012-07-19 04:50:34 PDT
Created attachment 643798 [details] [diff] [review]
patch v1

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.
Comment 8 Tim Taubert [:ttaubert] 2012-07-19 04:54:39 PDT
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 Dão Gottwald [:dao] 2012-07-19 05:00:26 PDT
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?
Comment 10 Tim Taubert [:ttaubert] 2012-07-19 05:24:39 PDT
Created attachment 643808 [details] [diff] [review]
patch v2

(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 11 Dão Gottwald [:dao] 2012-07-19 05:27:47 PDT
Comment on attachment 643808 [details] [diff] [review]
patch v2

Looks like you'll also need to update a couple of tests.
Comment 12 Tim Taubert [:ttaubert] 2012-07-19 05:28:57 PDT
(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.
Comment 13 Tim Taubert [:ttaubert] 2012-07-19 06:14:25 PDT
Created attachment 643820 [details] [diff] [review]
patch v3

Corrected a couple of tests and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=ef417851a515
Comment 14 Marco Bonardo [::mak] 2012-07-19 07:01:21 PDT
I suppose this may cause some add-on compatibility problem, where they check/set hidden on the sidebar, so adding compat keyword.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 10:49:54 PDT
(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 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-19 10:50:37 PDT
(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)
Comment 17 Tim Taubert [:ttaubert] 2012-07-19 14:01:07 PDT
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?
Comment 18 Tim Taubert [:ttaubert] 2012-07-21 05:15:12 PDT
Created attachment 644625 [details] [diff] [review]
patch v1b

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.
Comment 19 Tim Taubert [:ttaubert] 2012-07-21 10:51:29 PDT
https://hg.mozilla.org/integration/fx-team/rev/a42fbbc49fbb
Comment 20 Tim Taubert [:ttaubert] 2012-07-22 01:50:14 PDT
https://hg.mozilla.org/mozilla-central/rev/a42fbbc49fbb
Comment 21 Marco Bonardo [::mak] 2012-07-24 03:25:15 PDT
removing compat since we went the first approach.
Comment 22 Tim Taubert [:ttaubert] 2012-08-06 16:37:36 PDT
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 23 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-07 07:40:25 PDT
Comment on attachment 644625 [details] [diff] [review]
patch v1b

low risk, approving.
Comment 25 Ioana (away) 2012-08-09 07:08:54 PDT
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 Virgil Dicu [:virgil] [QA] 2012-11-15 06:46:13 PST
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.

Note You need to log in before you can comment on or make changes to this bug.