Closed
Bug 911370
Opened 11 years ago
Closed 11 years ago
Valgrind detects leak - 472 bytes are definitely lost (direct) with js::jit::IonScript::New on the stack
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: gkw, Assigned: jandem)
Details
(Keywords: regression, testcase, valgrind)
Attachments
(4 files, 1 obsolete file)
1.93 KB,
text/plain
|
Details | |
1.23 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
2.74 KB,
text/plain
|
Details | |
1.57 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
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.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 1•11 years ago
|
||
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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #798458 -
Flags: review?(bhackett1024)
Attachment #798458 -
Flags: feedback?(gary)
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 2•11 years ago
|
||
Better fix. The alternative is to copy the cache and backedge entries before allocating the IonCode, but I think I like this more.
Attachment #798458 -
Attachment is obsolete: true
Attachment #798458 -
Flags: review?(bhackett1024)
Attachment #798458 -
Flags: feedback?(gary)
Attachment #798460 -
Flags: review?(bhackett1024)
Attachment #798460 -
Flags: feedback?(gary)
Updated•11 years ago
|
Attachment #798460 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 3•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6acba1186f4
Comment 4•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e6acba1186f4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Reporter | ||
Comment 5•11 years ago
|
||
Nope, this is still not fixed as of m-c tip rev e3785e299ab6.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•11 years ago
|
Attachment #798460 -
Flags: feedback?(gary)
Reporter | ||
Comment 6•11 years ago
|
||
I tested with a 64-bit deterministic threadsafe shell, same parameters as the one in comment 0, on Ubuntu 12.04.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 7•11 years ago
|
||
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.
Attachment #799634 -
Flags: review?(kvijayan)
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 8•11 years ago
|
||
Kannan: review ping, small patch and fixes a leak. Would be nice to get this in before the merge next week :)
Comment 9•11 years ago
|
||
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.
Attachment #799634 -
Flags: review?(kvijayan) → review+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d3d7e8a990
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5d3d7e8a990
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•