Closed Bug 986313 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, valgrind, Whiteboard: [qa-])

Attachments

(2 files)

Attached file 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
Flags: needinfo?(luke)
Tested on inbound rev 8c129d201f96 which has the fixes for OdinMonkey in bug 970643.
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
Oops, I don't think JSBugMon tests Valgrind issues yet.
Whiteboard: [jsbugmon:]
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
}
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?
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.
Flags: needinfo?(luke)
Attached patch bug986313.patchSplinter Review
Let's go for it!
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8398723 - Flags: review?(luke)
Comment on attachment 8398723 [details] [diff] [review]
bug986313.patch

Perfect, thanks!
Attachment #8398723 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/1d5cd00e4016
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Can the attached testcase be checked in?
Flags: in-testsuite?
Whiteboard: [qa-]
(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.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.