devtools increase CC graph size

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Developer Tools
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: ochameau)

Tracking

unspecified
Firefox 22
Points:
---

Firefox Tracking Flags

(firefox20- wontfix, firefox21+ wontfix, firefox22 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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.
It should be interesting to make the same test against beta, in order to figure out if this is a toolbox regression or not.
(Reporter)

Comment 3

4 years ago
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.)
(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).
(Reporter)

Comment 5

4 years ago
And for some reason I can't reproduce the 10000 objects leak today, but only the smaller leak.
(Reporter)

Comment 6

4 years ago
Leaks causing larger CC graphs are bad. They cause higher CC times and reduce responsiveness.
tracking-firefox20: --- → ?
tracking-firefox21: --- → ?

Comment 7

4 years ago
(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?
(Reporter)

Comment 8

4 years ago
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.
(Reporter)

Comment 9

4 years ago
But we certainly should make sure the larger leak won't happen in release builds.
(Assignee)

Comment 10

4 years ago
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.
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.
Assignee: nobody → poirot.alex
tracking-firefox20: ? → +
tracking-firefox21: ? → +
(Assignee)

Comment 12

4 years ago
Created attachment 720890 [details] [diff] [review]
Bug 839280: Fix document leak in nsLoginManager.
(Assignee)

Comment 13

4 years ago
Created attachment 720891 [details] [diff] [review]
Bug 839280: Fix frame document leak on Toolbox destroy.
(Assignee)

Comment 14

4 years ago
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?
tracking-firefox20: + → ?
tracking-firefox21: + → ?

Updated

4 years ago
Attachment #720891 - Flags: review?(jwalker)

Updated

4 years ago
Attachment #720890 - Flags: review?(dolske)
(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).
(Reporter)

Comment 16

4 years ago
(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?
(Assignee)

Comment 17

4 years ago
Created attachment 720918 [details] [diff] [review]
Bug 839280: Fix xul button leaks on Toolbox destroy
(Assignee)

Updated

4 years ago
Attachment #720891 - Attachment is obsolete: true
Attachment #720891 - Flags: review?(jwalker)
(Assignee)

Comment 18

4 years ago
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!
Attachment #720918 - Flags: review?(jwalker)
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.
Attachment #720890 - Flags: review?(dolske) → review+
(Assignee)

Comment 20

4 years ago
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?
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.
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
tracking-firefox20: ? → +
tracking-firefox21: ? → +
Duplicate of this bug: 838948
(Reporter)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
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 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.
Attachment #720918 - Flags: review?(jwalker) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7952fadd1732
https://hg.mozilla.org/integration/fx-team/rev/6deb17bc6c56
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
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!
Whiteboard: [fixed-in-fx-team]
Whiteboard: [MemShrink]
(Assignee)

Comment 29

4 years ago
(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
Whiteboard: [MemShrink]
Whiteboard: [MemShrink]

Comment 30

4 years ago
This is probably not platform specific, so please update the platform field. Thanks!
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 31

4 years ago
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.
Attachment #720918 - Attachment is obsolete: true
Attachment #724636 - Flags: review?(jwalker)
(Reporter)

Comment 32

4 years ago
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;
(Assignee)

Comment 33

4 years ago
(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?)
Attachment #724636 - Flags: review?(jwalker) → review+
Since we have not met the criteria in comment 11, this is being untracked for FF20.
status-firefox20: affected → wontfix
tracking-firefox20: + → -
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 35

4 years ago
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?
(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.
(Assignee)

Comment 37

4 years ago
Created attachment 728932 [details] [diff] [review]
Bug 839280 - devtools increase CC graph size
(Assignee)

Updated

4 years ago
Attachment #724636 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
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.
Attachment #728932 - Flags: review?(jwalker)
Attachment #728932 - Flags: review?(jwalker) → review+
(Assignee)

Comment 39

4 years ago
The two patches are ready to be checked-in again.
Keywords: checkin-needed
Whiteboard: [MemShrink:P2] → [MemShrink:P2][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/eab3530d9ce7
https://hg.mozilla.org/integration/fx-team/rev/e0ef49d585ad
Whiteboard: [MemShrink:P2][land-in-fx-team] → [MemShrink:P2][fixed-in-fx-team]
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/eab3530d9ce7
https://hg.mozilla.org/mozilla-central/rev/e0ef49d585ad
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-fx-team] → [MemShrink:P2]
Target Milestone: --- → Firefox 22
(In reply to Panos Astithas [:past] from comment #40)
> https://hg.mozilla.org/integration/fx-team/rev/eab3530d9ce7

This changeset is empty.
Grrr, I must have missed an error message when applying the patch. Relanding.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/fx-team/rev/2bcc54a65d95
Whiteboard: [MemShrink:P2] → [MemShrink:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2bcc54a65d95
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [MemShrink:P2][fixed-in-fx-team] → [MemShrink:P2]

Updated

4 years ago
status-firefox22: affected → fixed
(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.
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?
(Assignee)

Comment 48

4 years ago
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...)
(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.

Updated

4 years ago
status-firefox21: affected → wontfix
Is there anything QA can do here?
You need to log in before you can comment on or make changes to this bug.