Closed
Bug 1022027
Opened 11 years ago
Closed 10 years ago
asm.js packaged apps leak a bunch of stuff
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mccr8, Assigned: luke)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(2 files, 3 obsolete files)
234.27 KB,
text/plain
|
Details | |
796 bytes,
patch
|
bhackett1024
:
review+
mccr8
:
feedback+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
According to LSAN, we're leaking some stuff under js::Shape::hashify(). entryCount is involved there somehow, too. I can't even tell how entryCount() ends up allocating memory. Among all the mochitests, this is only happening in Moth, which is odd.
Reporter | ||
Comment 1•11 years ago
|
||
Well, there's a bunch of other shape and global sort of stuff, so maybe there's just some global data structure that is leaking, and LSAN is getting confused about what is holding onto what.
Reporter | ||
Comment 2•11 years ago
|
||
Reporter | ||
Comment 3•10 years ago
|
||
It turns out that (aside from the one leak in ScriptSource), this all is being leaked in a single Mochitest chrome test: test_packaged_app_asmjs.html I'm not sure what is going on here, but we're leaking a bunch of random JS runtime residue.
Summary: Small leak in entryCount under Shape::hashify() → asm.js packaged apps leak a bunch of stuff
Whiteboard: [MemShrink]
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8436168 -
Attachment is obsolete: true
Attachment #8436175 -
Attachment is obsolete: true
Assignee | ||
Comment 5•10 years ago
|
||
Hmm, sounds like the off-main-thread compilation (with its associated script and global) is getting leaked by the install-time compilation. I'll take a look at this.
Comment 6•10 years ago
|
||
For the first attachment, my best guess is that this is the line it's talking about:
ShapeTable *table = cx->new_<ShapeTable>(shape->entryCount());
even though the stack doesn't quite match that.
But yes, with all those other leaks, it looks like there's a larger problem that is the root cause.
Assignee | ||
Comment 7•10 years ago
|
||
There seems to be a missing leaveCompartment call in the off-main-thread parsing machinery. Without this patch, I can see two ParseTask globals staying alive (forever marked because enterCompartmentDepth > 0) and, with the patch, all these globals are finalized.
What I don't understand is why this only leaks in the asm.js case; it seems like this bug would cause every ParseTask global to leak. Any idea Brian? I put a watchpoint on the enterCompartmentDepth of some random non-asm.js ParseTask global's compartment and it never gets decremented; the whole compartment just gets freed at some point.
Attachment #8437451 -
Flags: review?(bhackett1024)
Attachment #8437451 -
Flags: feedback?(continuation)
Assignee | ||
Comment 8•10 years ago
|
||
Oh, I meant to ask: Andrew, does this patch fix the LSAN leak?
Assignee | ||
Comment 9•10 years ago
|
||
Ah: in normal situations, the caller passes a non-null cx (b/c the caller wants the result) which eventually triggers MergeCompartments which ends up nulling out parseTask->compartment()->global_ (thereby avoiding the leak). In the asm.js case, we don't care about the resulting script, so a null cx is passed. Apparently there is another case which would hit this leak, although it looks like a rare/error path:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#863
It'd be nice to get this on b2g1.4, since bug 965970 exposes this.
status-b2g-v1.4:
--- → affected
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8437451 [details] [diff] [review]
fix-leak
This fixes the leak for me. Thanks!
Attachment #8437451 -
Flags: feedback?(continuation) → feedback+
Updated•10 years ago
|
Attachment #8437451 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment 12•10 years ago
|
||
Assignee: nobody → luke
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Comment 13•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8438616 -
Attachment is obsolete: true
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8437451 [details] [diff] [review]
fix-leak
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 965970
User impact if declined: leak some memory in the Chrome process
Testing completed: m-c
Risk to taking this patch (and alternatives if risky): low
Is this what I need to do to get this patch in FFOS1.4? Also, is there anywhere else this should land other than central?
Attachment #8437451 -
Flags: approval-mozilla-b2g30?
Comment 15•10 years ago
|
||
For B2G, we're presumably going to want this on Aurora for v2.0 (m-c is current on version v2.1 as of Monday's uplift). Your call about whether we want it on Beta for Fx31 as well.
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8437451 [details] [diff] [review]
fix-leak
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 965970
User impact if declined: leak in chrome process
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
Attachment #8437451 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Your call about whether we want it on Beta for Fx31 as well.
Thanks Ryan. install-time caching is only triggered (in normal browsing) on FFOS atm, so I'll leave off beta.
Comment 18•10 years ago
|
||
Comment on attachment 8437451 [details] [diff] [review]
fix-leak
Aurora approval granted.
Attachment #8437451 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #14)
> User impact if declined: leak some memory in the Chrome process
How bad is this? What do you think the result will be on B2G 1.4 if we don't take this fix?
> Is this what I need to do to get this patch in FFOS1.4?
At this point in the 1.4 schedule we are only considering blocking bugs. If this leak is severe enough (what impact does it have on games and other apps?), I think we can consider blocking on this. In this case you'll want to use the blocking-b2g flag to nom for 1.4.
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] from comment #19)
> How bad is this? What do you think the result will be on B2G 1.4 if we don't
> take this fix?
Slightly bad if we expect WMW to be the only app that uses this install-time caching on 1.4 (which, so far as I've heard, is the case). This will be a one-time leak (in the chrome process) when the app is installed of perhaps 50kb that will persist until the device restarts.
Comment 21•10 years ago
|
||
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
status-firefox33:
--- → fixed
Comment 22•10 years ago
|
||
Comment on attachment 8437451 [details] [diff] [review]
fix-leak
Review of attachment 8437451 [details] [diff] [review]:
-----------------------------------------------------------------
Lets fix it on master instead
Attachment #8437451 -
Flags: approval-mozilla-b2g30?
You need to log in
before you can comment on or make changes to this bug.
Description
•