Last Comment Bug 839280 - devtools increase CC graph size
: devtools increase CC graph size
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 22
Assigned To: Alexandre Poirot [:ochameau]
:
Mentors:
: 838948 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-07 15:21 PST by Olli Pettay [:smaug] (TPAC)
Modified: 2013-06-18 06:23 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
wontfix
fixed


Attachments
Bug 839280: Fix document leak in nsLoginManager. (1.01 KB, patch)
2013-03-04 14:16 PST, Alexandre Poirot [:ochameau]
dolske: review+
Details | Diff | Splinter Review
Bug 839280: Fix frame document leak on Toolbox destroy. (953 bytes, patch)
2013-03-04 14:18 PST, Alexandre Poirot [:ochameau]
no flags Details | Diff | Splinter Review
Bug 839280: Fix xul button leaks on Toolbox destroy (1010 bytes, patch)
2013-03-04 14:50 PST, Alexandre Poirot [:ochameau]
jwalker: review+
Details | Diff | Splinter Review
CC graph for nsLoginManager leak (37.87 KB, image/png)
2013-03-04 16:33 PST, Alexandre Poirot [:ochameau]
no flags Details
Fixes new leaks (1.32 KB, patch)
2013-03-13 15:19 PDT, Alexandre Poirot [:ochameau]
jwalker: review+
Details | Diff | Splinter Review
Bug 839280 - devtools increase CC graph size (2.34 KB, patch)
2013-03-25 05:39 PDT, Alexandre Poirot [:ochameau]
jwalker: review+
Details | Diff | Splinter Review
Attachment 728932 rebased for beta (1.53 KB, patch)
2013-04-05 14:42 PDT, Alexandre Poirot [:ochameau]
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (TPAC) 2013-02-07 15:21:38 PST
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] (TPAC) 2013-02-07 15:43:10 PST
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 [:past] 2013-02-07 23:21:50 PST
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] (TPAC) 2013-02-08 01:47:34 PST
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] 2013-02-08 01:49:51 PST
(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] (TPAC) 2013-02-08 01:53:04 PST
And for some reason I can't reproduce the 10000 objects leak today, but only the smaller leak.
Comment 6 Olli Pettay [:smaug] (TPAC) 2013-02-10 07:09:21 PST
Leaks causing larger CC graphs are bad. They cause higher CC times and reduce responsiveness.
Comment 7 Alex Keybl [:akeybl] 2013-02-11 16:07:39 PST
(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] (TPAC) 2013-02-11 16:53:37 PST
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] (TPAC) 2013-02-11 16:55:20 PST
But we certainly should make sure the larger leak won't happen in release builds.
Comment 10 Alexandre Poirot [:ochameau] 2013-02-15 03:01:33 PST
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] 2013-02-15 15:27:22 PST
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] 2013-03-04 14:16:19 PST
Created attachment 720890 [details] [diff] [review]
Bug 839280: Fix document leak in nsLoginManager.
Comment 13 Alexandre Poirot [:ochameau] 2013-03-04 14:18:54 PST
Created attachment 720891 [details] [diff] [review]
Bug 839280: Fix frame document leak on Toolbox destroy.
Comment 14 Alexandre Poirot [:ochameau] 2013-03-04 14:25:38 PST
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: gavin@gavinsharp.com] 2013-03-04 14:42:07 PST
(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] (TPAC) 2013-03-04 14:47:30 PST
(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] 2013-03-04 14:50:50 PST
Created attachment 720918 [details] [diff] [review]
Bug 839280: Fix xul button leaks on Toolbox destroy
Comment 18 Alexandre Poirot [:ochameau] 2013-03-04 14:53:34 PST
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] 2013-03-04 14:57:11 PST
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] 2013-03-04 15:10:38 PST
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 2013-03-04 15:20:48 PST
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 22 Matt Brubeck (:mbrubeck) 2013-03-04 16:05:23 PST
*** Bug 838948 has been marked as a duplicate of this bug. ***
Comment 23 Olli Pettay [:smaug] (TPAC) 2013-03-04 16:07:39 PST
(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] 2013-03-04 16:33:52 PST
Created attachment 720971 [details]
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] (needinfo me or ping on irc) 2013-03-06 01:01:08 PST
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 28 Panos Astithas [:past] 2013-03-08 11:15:09 PST
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] 2013-03-11 11:25:25 PDT
(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 2013-03-13 10:42:23 PDT
This is probably not platform specific, so please update the platform field. Thanks!
Comment 31 Alexandre Poirot [:ochameau] 2013-03-13 15:19:49 PDT
Created attachment 724636 [details] [diff] [review]
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] (TPAC) 2013-03-13 15:23:38 PDT
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] 2013-03-13 15:33:46 PDT
(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 2013-03-18 16:38:57 PDT
Since we have not met the criteria in comment 11, this is being untracked for FF20.
Comment 35 Alexandre Poirot [:ochameau] 2013-03-21 08:43:04 PDT
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 [:past] 2013-03-21 12:06:51 PDT
(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] 2013-03-25 05:39:05 PDT
Created attachment 728932 [details] [diff] [review]
Bug 839280 - devtools increase CC graph size
Comment 38 Alexandre Poirot [:ochameau] 2013-03-25 05:43:15 PDT
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] 2013-03-25 08:30:44 PDT
The two patches are ready to be checked-in again.
Comment 42 :Ms2ger (⌚ UTC+1/+2) 2013-03-28 09:30:04 PDT
(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 [:past] 2013-03-28 09:36:50 PDT
Grrr, I must have missed an error message when applying the patch. Relanding.
Comment 44 Panos Astithas [:past] 2013-03-28 10:26:56 PDT
https://hg.mozilla.org/integration/fx-team/rev/2bcc54a65d95
Comment 45 Ryan VanderMeulen [:RyanVM] 2013-04-01 09:59:19 PDT
https://hg.mozilla.org/mozilla-central/rev/2bcc54a65d95
Comment 46 bhavana bajaj [:bajaj] 2013-04-04 22:50:45 PDT
(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 [:past] 2013-04-05 09:32:21 PDT
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] 2013-04-05 14:42:34 PDT
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...)
Comment 49 bhavana bajaj [:bajaj] 2013-04-16 22:02:20 PDT
(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 [QA] (:cornel_ionce) 2013-06-18 06:23:57 PDT
Is there anything QA can do here?

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