Closed
Bug 990807
Opened 10 years ago
Closed 10 years ago
Valgrind detects leak - 4 bytes and/or 32 bytes are definitely lost (direct)
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: gkw, Assigned: mjrosenb)
Details
(Keywords: testcase, valgrind, Whiteboard: [fuzzblocker])
Attachments
(3 files, 1 obsolete file)
5.21 KB,
text/plain
|
Details | |
1.74 KB,
patch
|
gkw
:
feedback+
|
Details | Diff | Splinter Review |
3.27 KB,
patch
|
dougc
:
review+
gkw
:
feedback+
|
Details | Diff | Splinter Review |
(function() { function f() {} [, -0, 0 / 0, , , , , 0x80000001, -0xfff, , 0x80000001, , -0xfff, -0, 0x100000001]; function c() { f } })() valgrind --smc-check=all-non-file --leak-check=full --show-leak-kinds=definite ./js-opt-32-dm-vg-ts-hfp-linux-c16f36021958 --ion-parallel-compile=off --ion-eager testcase.js ==29967== 4 bytes in 1 blocks are definitely lost in loss record 1 of 7 ==29967== at 0x48253C0: operator new[](unsigned int) (vg_replace_malloc.c:379) ==29967== by 0x8334B: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::finishPool() (IonAssemblerBufferWithConstantPools.h:722) ==29967== by 0x830C1: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::dumpPool() (IonAssemblerBufferWithConstantPools.h:939) /snip ==29967== 32 bytes in 2 blocks are definitely lost in loss record 4 of 7 ==29967== at 0x48253C0: operator new[](unsigned int) (vg_replace_malloc.c:379) ==29967== by 0x8332D: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::finishPool() (IonAssemblerBufferWithConstantPools.h:721) ==29967== by 0x830C1: js::jit::AssemblerBufferWithConstantPool<1024, 4, js::jit::Instruction, js::jit::Assembler, 1>::dumpPool() (IonAssemblerBufferWithConstantPools.h:939) /snip Tested on ARM Ubuntu Linux on m-c rev c16f36021958. Marty, any idea how to move this forward? My configure flags are: AR=ar sh /home/fuzz5lin/trees/mozilla-central/js/src/configure --enable-optimize=-O1 --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-valgrind --with-ccache --enable-threadsafe <other NSPR flags>
Flags: needinfo?(mrosenberg)
Reporter | ||
Comment 1•10 years ago
|
||
This blocks fuzzing using Valgrind on ARM. (which has become fairly usable recently on a pandaboard, hence this bug)
Whiteboard: [fuzzblocker]
Comment 2•10 years ago
|
||
The pool packing failure bail out path does not appear to free resources allocated in finishPool(). I can not reproduce this path with the test so it might not be this. There are some other OOM paths that do not free resources in finishPool.
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8400385 [details] [diff] [review] Free resources in the bail out path. I've to double check myself, but it does seem that this fixes the issue, perhaps time for a review? :)
Attachment #8400385 -
Flags: feedback+
Flags: needinfo?(mrosenberg) → needinfo?(dtc-moz)
Comment 4•10 years ago
|
||
Comment on attachment 8400385 [details] [diff] [review] Free resources in the bail out path. Looks like this path was being hit, although I could not reproduce it. The larger issue will be addressed in bug 972710. There are also some OOM paths that do not free these resources, and should they also be addressed?
Attachment #8400385 -
Flags: review?(mrosenberg)
Flags: needinfo?(dtc-moz)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8400385 [details] [diff] [review] Free resources in the bail out path. Review of attachment 8400385 [details] [diff] [review]: ----------------------------------------------------------------- Sorry about the delay on this. I looked into this before gkw filed this bug. I think there are some cases that your patch misses. If for whatever reason, it turns out the extra paths are dead, feel free to re-r? me, and It'll be a quick review :-)
Attachment #8400385 -
Flags: review?(mrosenberg)
Assignee | ||
Comment 6•10 years ago
|
||
This should handle a couple of cases that doug's patch doesn't handle.
Attachment #8402704 -
Flags: review?(dtc-moz)
Attachment #8402704 -
Flags: feedback?(gary)
Comment 7•10 years ago
|
||
Comment on attachment 8402704 [details] [diff] [review] fix_oom_leak-r0.patch Review of attachment 8402704 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch. Looks good, but a few questions below. Please resubmit for review. ::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h @@ +760,5 @@ > IonSpew(IonSpew_Pools, "[%d]***Offset was still out of range!***", id, codeOffset - magicAlign); > IonSpew(IonSpew_Pools, "[%d] Too complicated; bailingp", id); > this->fail_bail(); > + // only free up to the current offset > + for (int pi = 0; pi <= poolIdx; pi++) Perhaps this should be 'for (int pi = poolIdx; pi < numPoolKinds; pi++)' as the outer loop allocates from numPoolKinds-1 down? @@ +786,5 @@ > p->numEntries -= numSkips; > } > poolOffset += p->numEntries * p->immSize; > delete[] preservedEntries; > + preservedEntries = nullptr; nit: is this necessary? Were you going to handle the allocation of preservedEntries failing.
Attachment #8402704 -
Flags: review?(dtc-moz)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8402704 [details] [diff] [review] fix_oom_leak-r0.patch Seems like this patch might go through some more iterations, please request feedback when r+ and ready.
Attachment #8402704 -
Flags: feedback?(gary)
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Douglas Crosher [:dougc] from comment #7) > Perhaps this should be 'for (int pi = poolIdx; pi < numPoolKinds; pi++)' as > the outer loop allocates from numPoolKinds-1 down? Good Catch! That hopefully would have thrown assertions all over the underlying code, but you never know... > > @@ +786,5 @@ > > p->numEntries -= numSkips; > > } > > poolOffset += p->numEntries * p->immSize; > > delete[] preservedEntries; > > + preservedEntries = nullptr; > > nit: is this necessary? Were you going to handle the allocation of > preservedEntries failing. I'm doing that because if we may delete it again later on another code path, so this prevents us from deleting it twice, which afaict, is a bug, and will likely make free complain.
Assignee | ||
Comment 10•10 years ago
|
||
Feel free to set feedback on it if/when you're done reviewing it.
Attachment #8402704 -
Attachment is obsolete: true
Attachment #8402899 -
Flags: review?(dtc-moz)
Comment 11•10 years ago
|
||
Comment on attachment 8402899 [details] [diff] [review] fix_oom_leak-r1.patch Review of attachment 8402899 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h @@ +786,5 @@ > p->numEntries -= numSkips; > } > poolOffset += p->numEntries * p->immSize; > delete[] preservedEntries; > + preservedEntries = nullptr; Thank you for the explanation. I must be missing something as it appears to just loop back to the loop head where preservedEntries is re-assigned so setting it to null appears unnecessary.
Attachment #8402899 -
Flags: review?(dtc-moz) → review+
Updated•10 years ago
|
Attachment #8402899 -
Flags: feedback?(gary)
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8402899 [details] [diff] [review] fix_oom_leak-r1.patch Nice, this seems to work fine. Thanks! :)
Attachment #8402899 -
Flags: feedback?(gary) → feedback+
Reporter | ||
Comment 13•10 years ago
|
||
I guess this can be landed?
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
Comment 14•10 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #13) > I guess this can be landed? Look ok to me.
Flags: needinfo?(dtc-moz)
Reporter | ||
Comment 15•10 years ago
|
||
Helped to land this. https://hg.mozilla.org/integration/mozilla-inbound/rev/bc71ac7b9074
Flags: needinfo?(mrosenberg)
Target Milestone: --- → mozilla31
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc71ac7b9074
Assignee: nobody → mrosenberg
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•