829300 - Library window seems to cause some runtime leaks

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I noticed some stuff from Library window in the CC graph.
No documents, but elements and js stuff.
Elements are from document chrome://browser/content/places/places.xul
and there are various DPV_* prefixed functions.

To analyze leaks one can create cc and gc logs
https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump
but it might be enough to just look at the cc graph in the browser.
Ugly, yet rather effective addon can be installed from Bug 726346.
Restart after installation. Load about:cc and press 'Run cycle collector'.
You may need to run CC few times to get more stable graph.
Nowadays the base graph in FF which is just started and has only 1 tab is close
to 200 objects. After downloading something and opening and closing library window
I see some extra objects in the graph.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

IIRC Tim and Felipe have done some FF memleak hunting.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(some of the leaks I see are coming from about:downloads. )

Comment 3

[Marco Bonardo [:mak]]

Actually we set some expando on elements that change binding in between, we plan to remove that mess as soon as possible, to go with a nicer WeakMap. Not sure if related but worth to mention.

Comment 4

[Asaf Romano (gone)]

Attachment: break shell->element->shell cycle

Comment 5

[Asaf Romano (gone)]

I'm not sure if it fixes the leak, but we probably want to take this fix anyway. So, if it doesn't help, I'll file a new bug and attach it there.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 702989 [details] [diff] [review]
break shell->element->shell cycle

Review of attachment 702989 [details] [diff] [review]:
-----------------------------------------------------------------

I still have to verify whether this fixes the leak or not, doing after these comments.
f+, the approach looks sane and less complicated than I thought.

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +1085,5 @@
>        // Also activate the first invisible nodes in both boundaries (that is,
>        // above and below the visible area) to ensure proper keyboard navigation
>        // in both directions.
>        let nodeBelowVisibleArea = lastVisibleNode && lastVisibleNode.nextSibling;
> +      if (nodeBelowVisibleArea && nodeBelowVisibleArea)

the second check is useless

@@ +1091,4 @@
>  
>        let nodeABoveVisibleArea =
>          firstVisibleNode && firstVisibleNode.previousSibling;
> +      if (nodeABoveVisibleArea && nodeABoveVisibleArea)

the second check is useless

and please, while here, fix my typo ABove instead of Above

Comment 7

[Marco Bonardo [:mak]]

So a couple things from my investigation.

The downloads panel initialization is async, and once happens it will add objects to the windows in a permanent way.  When you start the download it's likely this happens and some object may be added. same clicking on the button in the toolbar.

About the library, I see that opening it and closing it increases the objects from 200 to about 250.  Most of Places is initialized in an async way, due to the nature of the test the Library does part of this initialization, but I can't tell how much of those 50 objects are expected, likely just a part.

I couldn't find any DPV_ objects in the graph, before or after Mano's patch, that doesn't seem to make any difference regarding the objects count. I could see some promise/task that went away after a couple CC.

Opening Show All History, or Show all Bookmarks causes a similar increase in the number of objects (still about 50).

My understanding of this is that the Downloads view doesn't seem to make things worse than they were already, this bug is more about understanding why the Library causes this 50 objects increase and reduce that, than figuring if the downloads view leaks.

Thus, the patch should be moves elsewhere and unless there is any more data demonstrating the problem is the Downloads view, the bug should be moved to Bookmarks & History (more generic Places bug, not downloads related)

Comment 8

[Nicholas Nethercote [inactive]]

Olli, how often does this happen, and how much gets leaked when it does?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

After ff startup CC graph size is 185-195 (I have about:cc installed in the test profile).
After opening and closing Library, graph is ~435 objects (still no chrome elements in the graph).

After downloading some file and opening and closing Library, the graph is about 500 objects
and contains 
[rc=2] root FragmentOrElement (XUL) image class='tab-icon-image' chrome://browser/content/browser.xul
0xffffffffa34869c0 [rc=2] root FragmentOrElement (XUL) label class='tab-text tab-label' chrome://browser/content/browser.xul
0xffffffffa3486920 [rc=2] root FragmentOrElement (XUL) image class='tab-throbber' chrome://browser/content/browser.xul
0xffffffffa38bb5b0 [rc=3] root FragmentOrElement (XUL) commandset id='editMenuCommandSetAll' chrome://browser/content/places/places.xul
0xffffffffa3b2bb50 [rc=10] root FragmentOrElement (XUL) commandset id='downloadCommands' chrome://browser/content/places/places.xul
0xffffffff9f0bd6a0 [rc=21] root FragmentOrElement (XUL) commandset id='placesCommands' chrome://browser/content/places/places.xul
0xffffffff9f0bd650 [rc=13] root FragmentOrElement (XUL) commandset id='editMenuCommands' chrome://browser/content/places/places.xul

which means also the document chrome://browser/content/places/places.xul is kept alive, but
just optimized out from cycle collection graph. (perhaps the document is kept alive on purpose?)

Also, every Library window open/close seems to increase the graph size and adding new FragmentOrElement objects to it.

Comment 10

[Marco Bonardo [:mak]]

I couldn't reproduce this, usually after closing the library and running CC, it went down to about 250 objects (as I said, I measured an increse of 50 objects) and multiple openings didn't change a thing. Looks like my testing method may be wrong :(
Should I not run CC? all of our results and nodes partecipate in CC.
btw no, places.xul is not supposed to stay around on purpose.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

After closing Library I run CC several times until the graph size stabilizes.

Did you try to download something and then opening the window from "Show All Downloads"?

I'm using Linux, if it matters.

Comment 12

[Marco Bonardo [:mak]]

yes, I tried, though on Win7.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(I think DPV_* functions were coming from about:downloads which isn't enabled by default, IIRC)

Comment 14

[Marco Bonardo [:mak]]

(In reply to Olli Pettay [:smaug] from comment #13)
> (I think DPV_* functions were coming from about:downloads which isn't
> enabled by default, IIRC)

it's used only in PB mode, I didn't try that.

Comment 15

[Nicholas Nethercote [inactive]]

Stupid question:  what is the Library?  How do I enable/view it?

Comment 16

[Marco Bonardo [:mak]]

either show all bookmarks, show all history or show all downloads.

Comment 17

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.