1467273 - Memory leak with OOM in js::jit::JitActivation::getRematerializedFrame(JSContext*, js::jit::JSJitFrameIter const&, unsigned long)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[André Bargull [:anba]]

|rematerializedFrames_| is not deleted when |init()| in this line [1] returns false.


[1] https://searchfox.org/mozilla-central/rev/cf464eabfeba64e866c1fa36b9fefd674dca9c51/js/src/vm/Stack.cpp#1658


Test case:
---
var g = newGlobal();
var dbg = new Debugger(g);
g.dbg = dbg;

g.eval(`
    oomAtAllocation(16604 - 168 - 104);

    print("START");

    function t() {
        var gotFrame = false;
        
        function g(i) {
            if (i === 90000) {
                dbg.getNewestFrame();
                return 1;
            }
            return 0;
        }

        function f() {
            var q = 0;
            for (var i = 0; i < 100000; ++i) {
                q += g(i);
            }
            return q;
        }
        for (var i = 0; i < 10; ++i) print(f());
    }
    t();
`);
---


Configure flags: --enable-debug --disable-optimize --disable-tests --enable-valgrind --disable-jemalloc

Run with: valgrind --tool=memcheck --leak-check=yes ~/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js --no-threads /tmp/t.js


Output:
---
==13872== Memcheck, a memory error detector
==13872== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==13872== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==13872== Command: /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js --no-threads /tmp/t.js
==13872== 
==13872== Warning: set address range perms: large range [0x1dc745fd000, 0x1dcb45fd000) (noaccess)
START
1
counter=16499, max-alloc=16500
uncaught exception: out of memory
(Unable to print stack trace)
==13872== Warning: set address range perms: large range [0x1dc745fd000, 0x1dcb45fd000) (noaccess)
==13872== 
==13872== HEAP SUMMARY:
==13872==     in use at exit: 72,867 bytes in 4 blocks
==13872==   total heap usage: 9,810 allocs, 9,806 frees, 6,870,923 bytes allocated
==13872== 
==13872== 88 bytes in 1 blocks are definitely lost in loss record 3 of 4
==13872==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13872==    by 0x4920B4: SystemMalloc::malloc(unsigned long) (malloc_decls.h:37)
==13872==    by 0x491FE8: DummyArenaAllocator<SystemMalloc>::moz_arena_malloc(unsigned long, unsigned long) (malloc_decls.h:37)
==13872==    by 0x491EDC: moz_arena_malloc (malloc_decls.h:115)
==13872==    by 0x41E632: js_malloc(unsigned long) (Utility.h:388)
==13872==    by 0x429261: unsigned char* js_pod_malloc<unsigned char>(unsigned long) (Utility.h:585)
==13872==    by 0x45C123: unsigned char* js::MallocProvider<JSContext>::maybe_pod_malloc<unsigned char>(unsigned long) (MallocProvider.h:54)
==13872==    by 0x45BFDE: unsigned char* js::MallocProvider<JSContext>::pod_malloc<unsigned char>(unsigned long) (MallocProvider.h:87)
==13872==    by 0x113EE52: js::HashMap<unsigned char*, JS::GCVector<js::jit::RematerializedFrame*, 0ul, js::TempAllocPolicy>, js::DefaultHasher<unsigned char*>, js::TempAllocPolicy>* js::MallocProvider<JSContext>::new_<js::HashMap<unsigned char*, JS::GCVector<js::jit::RematerializedFrame*, 0ul, js::TempAllocPolicy>, js::DefaultHasher<unsigned char*>, js::TempAllocPolicy>, JSContext*&>(JSContext*&) (in /home/andre/hg/mozilla-inbound/js/src/build-valgrind-debug-obj/dist/bin/js)
==13872==    by 0x111F7DE: js::jit::JitActivation::getRematerializedFrame(JSContext*, js::jit::JSJitFrameIter const&, unsigned long) (Stack.cpp:1655)
==13872==    by 0x111F6A5: js::FrameIter::ensureHasRematerializedFrame(JSContext*) (Stack.cpp:1118)
==13872==    by 0xE638BF: js::Debugger::getNewestFrame(JSContext*, unsigned int, JS::Value*) (Debugger.cpp:3881)
==13872== 
==13872== LEAK SUMMARY:
==13872==    definitely lost: 88 bytes in 1 blocks
==13872==    indirectly lost: 0 bytes in 0 blocks
==13872==      possibly lost: 0 bytes in 0 blocks
==13872==    still reachable: 72,779 bytes in 3 blocks
==13872==         suppressed: 0 bytes in 0 blocks
==13872== Reachable blocks (those to which a pointer was found) are not shown.
==13872== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==13872== 
==13872== For counts of detected and suppressed errors, rerun with: -v
==13872== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
---

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

We should double check after Bug 1418971 is landed. Which is currently pending on the second patch from Bug 1464829.

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

I was not able to reproduce the issue, but just reading the code still convinced me that there is indeed a non-handled OOM.
I suggest to convert the JitActivation::rematerializedFrames_ field to a UniquePtr, which should handle this issue.

Comment 3

[Nicolas B. Pierron [:nbp] {away}]

Attachment: JitActivation owns the rematerialized frames of the activation.

Change the Raw pointer of the JitActivation to be a js::UniquePtr, which should
correctly handled free-ing the pointer if we fail to initialize the HashMap.

Comment 4

[André Bargull [:anba]]

Comment on attachment 8994803 [details] [diff] [review]
JitActivation owns the rematerialized frames of the activation.

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

r+ with the review comments applied.

::: js/src/vm/Stack.cpp
@@ +1654,5 @@
>      MOZ_ASSERT(iter.activation() == this);
>      MOZ_ASSERT(iter.isIonScripted());
>  
>      if (!rematerializedFrames_) {
> +        rematerializedFrames_.reset(cx->new_<RematerializedFrameTable>(cx));

I think `rematerializedFrames_ = cx->make_unique<RematerializedFrameTable>(cx);` is the preferred pattern here.

@@ +1655,5 @@
>      MOZ_ASSERT(iter.isIonScripted());
>  
>      if (!rematerializedFrames_) {
> +        rematerializedFrames_.reset(cx->new_<RematerializedFrameTable>(cx));
> +        if (!rematerializedFrames_ || !rematerializedFrames_->init()) {

Shouldn't we still set `rematerializedFrames_ = nullptr;` to ensure the RematerializedFrameTable pointer is reset, so that when getRematerializedFrame() is called the next time, we retry to initialize the hash map?

And we still need two if-statements to avoid re-reporting an OOM exception per bug 1466626 comment #5.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

(In reply to André Bargull [:anba] from comment #4)
> Comment on attachment 8994803 [details] [diff] [review]
> JitActivation owns the rematerialized frames of the activation.
> 
> Review of attachment 8994803 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the review comments applied.
> 
> ::: js/src/vm/Stack.cpp
> @@ +1654,5 @@
> >      MOZ_ASSERT(iter.activation() == this);
> >      MOZ_ASSERT(iter.isIonScripted());
> >  
> >      if (!rematerializedFrames_) {
> > +        rematerializedFrames_.reset(cx->new_<RematerializedFrameTable>(cx));
> 
> I think `rematerializedFrames_ =
> cx->make_unique<RematerializedFrameTable>(cx);` is the preferred pattern
> here.

Thanks, I was not aware of this new function.

> @@ +1655,5 @@
> >      MOZ_ASSERT(iter.isIonScripted());
> >  
> >      if (!rematerializedFrames_) {
> > +        rematerializedFrames_.reset(cx->new_<RematerializedFrameTable>(cx));
> > +        if (!rematerializedFrames_ || !rematerializedFrames_->init()) {
> 
> Shouldn't we still set `rematerializedFrames_ = nullptr;` to ensure the
> RematerializedFrameTable pointer is reset, so that when
> getRematerializedFrame() is called the next time, we retry to initialize the
> hash map?

Indeed, I will add a  rematerializedFrames_.release().

> And we still need two if-statements to avoid re-reporting an OOM exception
> per bug 1466626 comment #5.

Ok.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/553c52a7d9eb
JitActivation owns the rematerialized frames of the activation. r=anba

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/553c52a7d9eb