1022027 - asm.js packaged apps leak a bunch of stuff

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: other leaking JS stuff

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: allocation stacks for leaks in test_packaged_app_asmjs.html

Comment 5

[Luke Wagner [:luke]]

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

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Luke Wagner [:luke]]

Attachment: 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.

Comment 8

[Luke Wagner [:luke]]

Oh, I meant to ask: Andrew, does this patch fix the LSAN leak?

Comment 9

[Luke Wagner [:luke]]

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 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8437451 [details] [diff] [review]
fix-leak

This fixes the leak for me.  Thanks!

Comment 11

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5289b9a1f8a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5289b9a1f8a

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: asm.js packaged apps leak a bunch of stuff

Comment 14

[Luke Wagner [:luke]]

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?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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 16

[Luke Wagner [:luke]]

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

Comment 17

[Luke Wagner [:luke]]

(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

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8437451 [details] [diff] [review]
fix-leak

Aurora approval granted.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 20

[Luke Wagner [:luke]]

(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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b7a5bba0115

Comment 22

[Preeti Raghunath(:Preeti)]

Comment on attachment 8437451 [details] [diff] [review]
fix-leak

Review of attachment 8437451 [details] [diff] [review]:
-----------------------------------------------------------------

Lets fix it on master instead