911370 - Valgrind detects leak - 472 bytes are definitely lost (direct) with js::jit::IonScript::New on the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: Valgrind stack

function n() {
    try {
        f()
    } catch (r) {}
}
f = Function("");
n();
f = Function("let{x:[]}=gcslice()");
n();

causes 472 bytes in 1 block to be definitely lost by a js opt 64-bit shell on m-c changeset 21b5af569ca2 with the following parameters:

valgrind --smc-check=all-non-file --leak-check=full ./js --ion-eager testcase.js

My configure flags are:

--enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe --enable-valgrind --with-ccache <other NSPR flags>

Jan, any idea who may be able to take a look? I think this got introduced in the recent months.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

In CodeGenerator::link, we leak the IonScript if the IonCode allocation fails.

Gary, I was unable to reproduce this with Valgrind but based on the stack this patch should probably fix it. Can you verify? In case there's another issue here.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Better fix. The alternative is to copy the cache and backedge entries before allocating the IonCode, but I think I like this more.

Comment 3

[Jan de Mooij [:jandem]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

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

Nope, this is still not fixed as of m-c tip rev e3785e299ab6.

Comment 6

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

Attachment: Valgrind stack from m-c changeset rev e3785e299ab6

I tested with a 64-bit deterministic threadsafe shell, same parameters as the one in comment 0, on Ubuntu 12.04.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Don't leak invalidated IonScript when handling an exception

Thanks Gary, what would we do without you?

I was able to reproduce this on Linux. The problem is that HandleException has some code to "decref" an invalidated IonScript when we unwind a frame, but we need to do the same when we're resuming into a catch block or we risk leaking it.

Comment 8

[Jan de Mooij [:jandem]]

Kannan: review ping, small patch and fixes a leak.

Would be nice to get this in before the merge next week :)

Comment 9

[Kannan Vijayan [:djvj]]

Comment on attachment 799634 [details] [diff] [review]
Part 2 - Don't leak invalidated IonScript when handling an exception

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

Nice catch.

Comment 10

[Jan de Mooij [:jandem]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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