asm.js packaged apps leak a bunch of stuff

RESOLVED FIXED in Firefox 32

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: luke)

Tracking

(Blocks: 1 bug)

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(firefox31 affected, firefox32 fixed, firefox33 fixed, b2g-v1.4 affected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 8436168 [details]
leaking allocation stacks

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.
Created attachment 8436175 [details]
other leaking JS stuff
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]
Created attachment 8437316 [details]
allocation stacks for leaks in test_packaged_app_asmjs.html
Attachment #8436168 - Attachment is obsolete: true
Attachment #8436175 - Attachment is obsolete: true
(Assignee)

Comment 5

5 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.
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

5 years ago
Created attachment 8437451 [details] [diff] [review]
fix-leak

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

5 years ago
Oh, I meant to ask: Andrew, does this patch fix the LSAN leak?
(Assignee)

Comment 9

5 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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Created attachment 8438616 [details] [diff] [review]
asm.js packaged apps leak a bunch of stuff
Attachment #8438616 - Attachment is obsolete: true
(Assignee)

Comment 14

5 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?
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

5 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

5 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 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.
(Assignee)

Comment 20

5 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.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b7a5bba0115
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → affected
status-firefox32: --- → fixed
status-firefox33: --- → fixed
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.