839280 - devtools increase CC graph size

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

Devtools seem to leak a bit.

STR:
Install about:cc from https://bugzilla.mozilla.org/show_bug.cgi?id=726346
Restart
Load about:cc in a tab and run cycle collector, memorize the number of objects in the graph
Open a new tab and load a web page
Press ctrl+shift+i
Activate Style Editor
Close devtools (x on the right side)
Close the tab which had devtools activated
Run cycle collector again and compare the number of objects in the graph
The latter CC graph is larger than the original one (~200 -> ~350)

(note, you may need to have to run CC few times to get more stable CC graph sizes)

I see JS Object (nsXULPrototypeScript compilation scope) in the graph which is a bit odd.

There are also
0x7fa6c1aba700 [rc=2] root FragmentOrElement (XUL) image class='tab-throbber' chrome://browser/content/browser.xul
0x7fa6c1abab00 [rc=2] root FragmentOrElement (XUL) label class='tab-text tab-label' chrome://browser/content/browser.xul
0x7fa6c1abaa80 [rc=2] root FragmentOrElement (XUL) image class='tab-icon-image' chrome://browser/content/browser.xul

Comment 1

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

Looks like the debugger tool leaks badly, since the
nsDocument normal (XUL) chrome://browser/content/debugger.xul gets to the graph
if it is used.
Graph size increases about 10000 objects.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

It should be interesting to make the same test against beta, in order to figure out if this is a toolbox regression or not.

Comment 3

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

I haven't managed to reproduce the leak using devtools available in beta.

(Note, the baseline CC graph size in beta is about 200 objects more than in nightlies.)

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Looks like the debugger tool leaks badly, since the
> nsDocument normal (XUL) chrome://browser/content/debugger.xul gets to the
> graph
> if it is used.
> Graph size increases about 10000 objects.

From what I tested, the debugger increases graph size with about 50 objects (241 -> 295). Interestingly enough, if I close the debugger and the owner tab, open a new tab and start the debugger again on the same page, the graph size goes down to 256 objects).

Comment 5

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

And for some reason I can't reproduce the 10000 objects leak today, but only the smaller leak.

Comment 6

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

Leaks causing larger CC graphs are bad. They cause higher CC times and reduce responsiveness.

Comment 7

[Alex Keybl [:akeybl]]

(In reply to Olli Pettay [:smaug] from comment #5)
> And for some reason I can't reproduce the 10000 objects leak today, but only
> the smaller leak.

Are you requesting tracking for the smaller leak, or the larger leak?

Comment 8

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

I'm not sure how to reproduce the large CC graph size increase, but even the smaller leak may
mean significant memory usage leak - just not so bad responsiveness regression.
So, someone familiar with this code should investigate this some more and perhaps backout some
leaky features or fix them.

Comment 9

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

But we certainly should make sure the larger leak won't happen in release builds.

Comment 10

[Alexandre Poirot [:ochameau]]

Based on my preliminary script output, some references to chrome nodes are kept alive from MarkupView class. But I was testing against gecko-18... 

I won't have time to dig that futher until tuesday. But if it can wait I'll continue digging into this leak.

Comment 11

[Alex Keybl [:akeybl]]

We'll track for now, but unless we reproduce a significant memory leak or find a low risk fix for the smaller leak, this may be untracked later in the Beta 20 cycle.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 839280: Fix document leak in nsLoginManager.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Bug 839280: Fix frame document leak on Toolbox destroy.

Comment 14

[Alexandre Poirot [:ochameau]]

So, I finally found two leaks. One in nsLoginManager.js, an obvious one where we never remove the DOMContentLoaded listener so that we end up leaking the related document...

The second one was harder to spot but seems to involve the _host thing in Toolbox.jsm devtool module. 

To be extremelly precise about toolbox.jsm, the cross compartment wrapper that was keeping these fragment alive was this click anonymous function listener:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/DeveloperToolbar.jsm#102
Then I made the assumption that this `button` should have been destroyed on inspector close, and I was lucky enough to give a try to nullify _host which seems to be the main link to keep alive button's document.

Please, can someone pick reviewers for these patches?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Alexandre Poirot (:ochameau) from comment #14)
> So, I finally found two leaks. One in nsLoginManager.js, an obvious one
> where we never remove the DOMContentLoaded listener so that we end up
> leaking the related document...

When was this causing leaks, exactly? And for how long? We don't keep a reference to the documents, AFAIK, and so having an event listener attached to it shouldn't cause any leaks (and if it were, we'd be leaking documents all over the place, which I imagine would have been noticed before now).

Comment 16

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

(In reply to Alexandre Poirot (:ochameau) from comment #14)
> So, I finally found two leaks. One in nsLoginManager.js, an obvious one
> where we never remove the DOMContentLoaded listener so that we end up
> leaking the related document...
Why is this obvious one? document has a reference to the listener, but do you know how chrome ends up
keeping the document alive? Also, what 1if content calls event.stopPropagation() ?
(based on the patch event listener is added to bubble phase.)
Or is this not about content document, but some chrome document?

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 839280: Fix xul button leaks on Toolbox destroy

Comment 18

[Alexandre Poirot [:ochameau]]

Comment on attachment 720918 [details] [diff] [review]
Bug 839280: Fix xul button leaks on Toolbox destroy

While trying to compute some memory number before/after patches, I figured out that my previous patch wasn't actually fixing the leak. This one does!

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 720890 [details] [diff] [review]
Bug 839280: Fix document leak in nsLoginManager.

I'm also curious how this is actually causing a leak, but it's certainly harmless to fix.

Also, mbrubeck saw something similar over in bug 838948, so that may be a dupe of this now.

Comment 20

[Alexandre Poirot [:ochameau]]

let me rephrase what I just said. Modifying these two pieces of code fix the leak (fragments do not appear in CC after we close the inspector).

The explaination about why, how, and since when is another question that I'm afraid, I'm not able to respond right now. Why not? Because that the first time I see this code. And then we don't have necessary tool to understand what is going on. So it would be helpfull to get insight from people used with this code.

Here is what I did: I searched for cross compartment references targeting objects from the suspicious fragment's compartment. The __domEventListener object from nsLoginManager was one of those. That why I assumed that the addEventListener was keeping the document alive, but it may keep something else alive that keeps the fragments alive:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#272

And for the toolbox.jsm leak, the click anynomous listener was the equivalent cross compartment reference.

I'm planning to release the script I used in order to automatically spot these fragment leaks. BTW Olli, I was wondering if we can always make the assumption that something is wrong/leaking when we see FragmentOrElement nodes in CC graph?

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like this was accidentally untracked, returning the flags to + and fwiw we should get this approval requested asap so that, if low risk, the patch can be uplifted to aurora/beta asap for wider coverage and bake time.

Comment 23

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

(In reply to Alexandre Poirot (:ochameau) from comment #20)
> BTW Olli, I was wondering if we can always make the
> assumption that something is wrong/leaking when we see FragmentOrElement
> nodes in CC graph?
Unfortunately the answer is vague. Usually we end up optimizing FragmentOrElement objects out from the cycle collector graph, but
there can be still some valid optimizations missing.
But if there is a simple way to increase CC graph size (something like: open tab, do X, close tab), that usually means some kind of leak.

Comment 24

[Alexandre Poirot [:ochameau]]

Attachment: CC graph for nsLoginManager leak

Arrows are CC edges with their names.
Light blue is for the devtool document being leaked, which contains the Fragments. Red is for nsLoginManager compartment and yellow for others.

The cross compartment link I identified and allowed me to fix this leak is the blue JSObject (Proxy) linked the the red JSObject (Object).

I hope this graph will allow you to explain why it leaks...

Comment 25

[Joe Walker [:jwalker]]

Comment on attachment 720918 [details] [diff] [review]
Bug 839280: Fix xul button leaks on Toolbox destroy

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

Thanks for the fix.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7952fadd1732
https://hg.mozilla.org/integration/fx-team/rev/6deb17bc6c56

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing crashes and leaks.
https://hg.mozilla.org/integration/fx-team/rev/1156d4f8ec2c

https://tbpl.mozilla.org/php/getParsedLog.php?id=20437557&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=20439639&tree=Fx-Team

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

Tested locally with just the Toolbox.jsm patch and I could reproduce the leaks (Mountain Lion, debug build). Went over all the devtools tests, and it looks like there are 2 GCLI tests that are leaking with this patch: browser_dbg_cmd_break.js and browser_cmd_commands.js.

I didn't figure out how to fix the leaks, but I remembered that bug 685526 disables the former, and lo and behold, applying that patch on top of this one makes the leaks disappear!

Comment 29

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #28)
> Tested locally with just the Toolbox.jsm patch and I could reproduce the
> leaks (Mountain Lion, debug build).

Both patches are required to fix the leak. If you just apply one, you will still leak fragments.
And it looks like there is some additional leak being introduced on nightly, as my tool reports leaks again with patch applied on current head. Whereas leaks were fixed on m-c:5e6c0a2a2ffa41bbcab7febba7713005a3f2fcff

Comment 30

[Florian Bender]

This is probably not platform specific, so please update the platform field. Thanks!

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: Fixes new leaks

Previous browser/devtools/ patched isn't enough on current HEAD,
here is an updated patch to fix this leak.

Comment 32

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

Comment on attachment 724636 [details] [diff] [review]
Fixes new leaks


>+    let container = this.doc.getElementById("toolbox-buttons");
>+    while(container.firstChild) {
>+      container.removeChild(container.firstChild);
>+    }
Why not container.textContent = null;

Comment 33

[Alexandre Poirot [:ochameau]]

(In reply to Olli Pettay [:smaug] from comment #32)
> Comment on attachment 724636 [details] [diff] [review]
> Fixes new leaks
> 
> 
> >+    let container = this.doc.getElementById("toolbox-buttons");
> >+    while(container.firstChild) {
> >+      container.removeChild(container.firstChild);
> >+    }
> Why not container.textContent = null;

I can try this. Otherwise would you happen to have an opinion on attachment 720971 [details]?
(Do you have an explanation on why my patch against nsLoginManager actually fix the leak?)

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Since we have not met the criteria in comment 11, this is being untracked for FF20.

Comment 35

[Alexandre Poirot [:ochameau]]

I pushed updated patches to try:
  https://tbpl.mozilla.org/?tree=Try&rev=1e4c72c82948

Various intermittents, but still a suspicious leak report:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20929922&tree=Try
07:17:47 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | 6348484 bytes leaked (AsyncStatement, AtomImpl, BackstagePass, BodyRule, CalculateFrecencyFunction, ...)

Can we easily execute this test?

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Alexandre Poirot (:ochameau) from comment #35)
> Can we easily execute this test?

To reproduce the same results you could do: mach mochitest-browser

If you want to limit it to the devtools tests only, do: mach mochitest-browser browser/devtools

If my findings in comment 28 are still valid, you could run each test in isolation by supplying the relative paths of either of those two test files instead.

Comment 37

[Alexandre Poirot [:ochameau]]

Attachment: Bug 839280 - devtools increase CC graph size

Comment 38

[Alexandre Poirot [:ochameau]]

Comment on attachment 728932 [details] [diff] [review]
Bug 839280 - devtools increase CC graph size

My previous patch was introducing an exception in the following test:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
That's because, now, _host is nullified and various properties of toolbox, like frame can be null if we try to interact with toolbox after it has been destroyed.

It might happen in other locations, but here it was throwing in gDevTools.forgetBrowserWindow.

Comment 39

[Alexandre Poirot [:ochameau]]

The two patches are ready to be checked-in again.

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eab3530d9ce7
https://hg.mozilla.org/mozilla-central/rev/e0ef49d585ad

Comment 42

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Panos Astithas [:past] from comment #40)
> https://hg.mozilla.org/integration/fx-team/rev/eab3530d9ce7

This changeset is empty.

Comment 43

[Panos Astithas (he/him) [:past] (please ni?)]

Grrr, I must have missed an error message when applying the patch. Relanding.

Comment 44

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bcc54a65d95

Comment 46

[bhavana bajaj [:bajaj]]

(In reply to Panos Astithas [:past] from comment #44)
> https://hg.mozilla.org/integration/fx-team/rev/2bcc54a65d95

Please request approval for beta asap with risk analysis if this is ready to be uplifted.We should try taking this earlier in Beta cycle than later to get wider coverage and bake time.

Comment 47

[Panos Astithas (he/him) [:past] (please ni?)]

I tried to generate a patch for beta, but after fixing the conflicts I'm getting leaks. Alex, since you can do a far better job at testing the fix than me, can you give it a shot?

Comment 48

[Alexandre Poirot [:ochameau]]

Attachment: Attachment 728932 rebased for beta

Here is the rebased patch. Panos, you are right, there is still various leaks happening on beta.
First, we immediatly see FragmentOrElement on startup...
And for some reason, it still leaks with empty default profile opening about:home.
But with these patches applied, it doesn't leak with the empty profile generated by cfx, that tweak various prefs and use about:blank as first tab document.
I'm not sure it is really worth trying to search why it leaks on beta while it is fixed on nightly? (leak hunting is a time consuming task...)

Comment 49

[bhavana bajaj [:bajaj]]

(In reply to Alexandre Poirot (:ochameau) from comment #48)
> Created attachment 734068 [details] [diff] [review]
> Attachment 728932 [details] [diff] rebased for beta
> 
> Here is the rebased patch. Panos, you are right, there is still various
> leaks happening on beta.
> First, we immediatly see FragmentOrElement on startup...
> And for some reason, it still leaks with empty default profile opening
> about:home.
> But with these patches applied, it doesn't leak with the empty profile
> generated by cfx, that tweak various prefs and use about:blank as first tab
> document.
> I'm not sure it is really worth trying to search why it leaks on beta while
> it is fixed on nightly? (leak hunting is a time consuming task...)

Given comment #11 & considering the rebased patch does not completely fix the leaks here and as we have fixed the issue completely resolved in Fx22, I am going to wontfix this for Fx21 and let it ride the trains.

Also note, we are past Beta 3 for Fx 21 and this will induce more risk to the product due to late uplift in the cycle.

Comment 50

[Cornel Ionce [:noni] [Hubs QA]]

Is there anything QA can do here?