986313 - OdinMonkey: Valgrind detects leak - 64 bytes are definitely lost (direct)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Function("\
    \"use asm\";\
    <!--\nfunction f(){\
        f(0,0,0,0,0,0,0,0,0);\
        0()\
    }\
")()

valgrind --smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --leak-check=full --show-possibly-lost=no --num-callers=50 ./js-opt-64-dm-vg-linux-8c129d201f96 testcase.js

==15860== 64 bytes in 1 blocks are definitely lost in loss record 1 of 1
==15860==    at 0x4C2CFA7: malloc (vg_replace_malloc.c:292)
==15860==    by 0x4896DC: mozilla::VectorBase<(anonymous namespace)::VarType, 8ul, js::TempAllocPolicy, js::Vector<(anonymous namespace)::VarType, 8ul, js::TempAllocPolicy> >::growStorageBy(unsigned long) (Utility.h:134)
==15860==    by 0x4A68F2: CheckCallArgs((anonymous namespace)::FunctionCompiler&, js::frontend::ParseNode*, bool (*)((anonymous namespace)::FunctionCompiler&, js::frontend::ParseNode*, (anonymous namespace)::Type), (anonymous namespace)::FunctionCompiler::Call*) (Vector.h:1018)
/snip

Luke, any idea how to move this forward?

My configure flags are:

sh /home/gkwong/trees/mozilla-inbound/js/src/configure --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --disable-threadsafe

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Tested on inbound rev 8c129d201f96 which has the fixes for OdinMonkey in bug 970643.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Oops, I don't think JSBugMon tests Valgrind issues yet.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Investigating. Not happening only in cases of an error, but happening because the function has more than 8 arguments and thus the argTypes_ vector needs reallocation on the heap. There seems to be something wrong with all these Move semantics (guessing a copy of the Signature rather than a real move).

function f() {
    "use asm";
    function g(a,b,c,d,e,f,g,h,i) {
        a=a|0
        b=b|0
        c=c|0
        d=d|0
        e=e|0
        f=f|0
        g=g|0
        h=h|0
        i=i|0

        var z = 0;
        if ((i|0) == 1)
            z = 1;
        else
            z = g(a,b,c,d,e,f,g,h,i-1|0)|0
        return z|0
    }
    return g
}

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

I've found what the error is, after some hard debugging session. The Signature structure has a js::Vector for storing argument types and any Vector with more than 32 bytes of storage is alloc'ed on the malloc heap and freed by the Vector destructor. When we allocate the Func (in ModuleCompiler::addFunction), it gets allocated in the LifoAllocator heap, and the Signature is moved in the LifoAlloc space. At this point, the Vector is moved in the LifoAlloc space, but not the storage space (which is a pointer). LifoAlloc just pops out memory (without calling delete), so ~Vector is never called => Vector's storage is never freed => 64 bits leak, which is 16 x sizeof(VarType), the minimum power of two capacity above 9 (number of arguments in the example).

Didn't come with a simple solution though: we could use an IonAllocPolicy on the VarTypeVector (hence a few other structures), but it looks kind of a hammer solution. Any better ideas?

Comment 6

[Luke Wagner [:luke]]

Great job tracking this down!

Using a different alloc policy sounds like the best plan.  The only alternative I can think of would be to explicitly call all the ~Func() destructors (since they are kept in the functions_ list) but this seems equally or more kludgy.

One issue is that we can't use IonAllocPolicy because it needs a ModuleCompiler-scoped TempAllocator, of which there is none.  So what I'd do is make a simple clone of IonAllocPolicy called LifoAllocPolicy that stores a reference to a LifoAlloc (ModuleCompiler::moduleLifo_) instead of TempAllocator.  We could put this in ds/LifoAlloc.h since it seems generally useful.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug986313.patch

Let's go for it!

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8398723 [details] [diff] [review]
bug986313.patch

Perfect, thanks!

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5cd00e4016

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d5cd00e4016

Comment 11

[u279076]

Can the attached testcase be checked in?

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #11)
> Can the attached testcase be checked in?

I don't think Valgrind is running the JS shell on TBPL, so I am afraid checking in the test wouldn't help, sorry.