Closed Bug 1022027 Opened 5 years ago Closed 5 years ago

asm.js packaged apps leak a bunch of stuff

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- affected
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- affected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: mccr8, Assigned: luke)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files, 3 obsolete files)

Attached file leaking allocation stacks (obsolete) —
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.
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.
Attached file other leaking JS stuff (obsolete) —
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]
Attachment #8436168 - Attachment is obsolete: true
Attachment #8436175 - Attachment is obsolete: true
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.
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.
Attached patch fix-leakSplinter Review
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)
Oh, I meant to ask: Andrew, does this patch fix the LSAN leak?
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.
Comment on attachment 8437451 [details] [diff] [review]
fix-leak

This fixes the leak for me.  Thanks!
Attachment #8437451 - Flags: feedback?(continuation) → feedback+
Attachment #8437451 - Flags: review?(bhackett1024) → review+
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/b5289b9a1f8a
Assignee: nobody → luke
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8438616 - Attachment is obsolete: true
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?
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.
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?
(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 on attachment 8437451 [details] [diff] [review]
fix-leak

Aurora approval granted.
Attachment #8437451 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
(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 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.