The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: smaug, Assigned: ttaubert)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 17
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 wontfix, firefox15 verified, firefox16 fixed, firefox17 verified)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Whiteboard: [MemShrink]
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.
Whiteboard: [MemShrink] → [MemShrink:P2]

Updated

5 years ago
Blocks: 658738

Updated

5 years ago
Blocks: 705705
Duplicate of this bug: 705705
(Assignee)

Updated

5 years ago
Blocks: 759709
(Assignee)

Comment 6

5 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

5 years ago
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #643798 - Flags: review?(dao)
(Assignee)

Comment 8

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

5 years ago
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.
Attachment #643798 - Attachment is obsolete: true
Attachment #643798 - Flags: review?(dao)
Attachment #643808 - Flags: review?(dao)

Updated

5 years ago
Attachment #643808 - Flags: review?(dao) → review+
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

5 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

5 years ago
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
Attachment #643808 - Attachment is obsolete: true
Attachment #643820 - Flags: review?(dao)

Updated

5 years ago
Attachment #643820 - Flags: review?(dao) → review+
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
(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 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

5 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

5 years ago
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.
Attachment #643820 - Attachment is obsolete: true
Attachment #644625 - Flags: review?(gavin.sharp)
Attachment #644625 - Flags: review?(dao)

Updated

5 years ago
Attachment #644625 - Flags: review?(dao) → review+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/a42fbbc49fbb
Whiteboard: [MemShrink:P2] → [MemShrink:P2][fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Attachment #644625 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Target Milestone: --- → Firefox 17
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a42fbbc49fbb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-fx-team] → [MemShrink:P2]
Blocks: 776370
removing compat since we went the first approach.
Keywords: addon-compat
(Assignee)

Comment 22

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

5 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

5 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.
status-firefox15: fixed → verified
Depends on: 811089
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.
status-firefox17: fixed → verified
You need to log in before you can comment on or make changes to this bug.