728426 - Opening and then closing bookmarks sidebar keeps the bookmarksPanel.xul and/or history-panel.xul document alive

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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][[e-mail]] (holiday -> Aug30)]

Looks like same applies to history sidebar.

Comment 2

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

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: [e-mail]]]

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]]

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 6

[Tim Taubert [:ttaubert] (inactive)]

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] (inactive)]

Attachment: 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] (inactive)]

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]]

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] (inactive)]

Attachment: 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]]

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] (inactive)]

(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] (inactive)]

Attachment: patch v3

Corrected a couple of tests and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=ef417851a515

Comment 14

[Marco Bonardo [:mak]]

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: [e-mail]]]

(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: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] 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] (inactive)]

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] (inactive)]

Attachment: 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] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a42fbbc49fbb

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a42fbbc49fbb

Comment 21

[Marco Bonardo [:mak]]

removing compat since we went the first approach.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

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]

Comment on attachment 644625 [details] [diff] [review]
patch v1b

low risk, approving.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6eccd9aabcfc
https://hg.mozilla.org/releases/mozilla-beta/rev/c5ebffc79bb4

Comment 25

[Ioana (away)]

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]]

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.