Closed
Bug 1081260
Opened 10 years ago
Closed 9 years ago
|js::MallocProvider| alloc functions leak memory after calling |onOutOfMemory|
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: erahm, Assigned: terrence)
References
(Blocks 2 open bugs)
Details
(Keywords: coverity, Whiteboard: [CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461])
Attachments
(2 files)
3.89 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
4.62 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
|onOutOfMemory| [1] potentially returns allocated memory [2] (it will try to do a GC), all of the alloc functions[3,4,5,6,7] ignore the return value. [1] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/Runtime.cpp#l670 [2] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/Runtime.cpp#l688 [3] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/MallocProvider.h#l71 [4] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/MallocProvider.h#l91 [5] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/MallocProvider.h#l117 [6] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/MallocProvider.h#l137 [7] http://hg.mozilla.org/mozilla-central/annotate/e4cfacb76830/js/src/vm/MallocProvider.h#l162
Reporter | ||
Updated•10 years ago
|
Whiteboard: [MemShrink][CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764] → [MemShrink][CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461]
Comment 1•10 years ago
|
||
At least some of these came in with recent refactorings, it would appear: http://hg.mozilla.org/mozilla-central/rev/f9ec09ff142c#l13.27
Updated•10 years ago
|
Whiteboard: [MemShrink][CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461] → [MemShrink:P1][CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461]
Updated•10 years ago
|
Flags: needinfo?(terrence)
Assignee | ||
Comment 3•10 years ago
|
||
Oh, good catch! I did not realize that onOutOfMemory did more than report!
Blocks: GC.size
Flags: needinfo?(terrence)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8505545 -
Flags: review?(luke)
Updated•10 years ago
|
Attachment #8505545 -
Flags: review?(luke) → review+
Comment 5•10 years ago
|
||
What versions does this affect? Please be sure to backport as appropriate.
Blocks: 1033442
Flags: needinfo?(terrence)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8505545 [details] [diff] [review] return_onOOM_result-v0.diff Review of attachment 8505545 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/MallocProvider.h @@ +63,2 @@ > if (MOZ_LIKELY(p)) { > client()->updateMallocCounter(numElems * sizeof(T)); I think we need to do this in the case the onOutOfMemory successfully allocates as well. This comment applies to to the rest of the functions. @@ +130,5 @@ > if (MOZ_LIKELY(p)) { > client()->updateMallocCounter(bytes); > return p; > } > + return client()->onOutOfMemory(nullptr, bytes); This needs to pass in reinterpret_cast<void *>(1) to indicate it's a |calloc| call [1]. [1] http://hg.mozilla.org/mozilla-central/annotate/62f0b771583c/js/src/vm/Runtime.h#l1309
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6243d2b50e33
Flags: needinfo?(terrence)
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 8•10 years ago
|
||
Thank you for the fix, Terrence.
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6243d2b50e33
Comment 10•9 years ago
|
||
Can this be closed now, Terrence? Do you recall why you left it open?
Flags: needinfo?(terrence)
Assignee | ||
Comment 11•9 years ago
|
||
I want to redesign the interface to get rid of the absurd reinterpret_cast<void *>(1) and interpretation of a void* as an enum. I just haven't had time yet.
Flags: needinfo?(terrence)
Comment 12•9 years ago
|
||
Is the memory leak fixed at least? We could remove the [MemShrink] tag in that case. Or just close this and file a new one.
Reporter | ||
Comment 13•9 years ago
|
||
Comment 6 was never addressed, so we're not quite updating the malloc counter properly and the calloc retry will return non-zeroed memory if successful. So I guess technically the leak is fixed, but there are still issues.
Assignee | ||
Comment 14•9 years ago
|
||
That too.
Whiteboard: [MemShrink:P1][CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461] → [CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461]
Assignee | ||
Comment 15•9 years ago
|
||
Update the malloc counters if we happen to recover from OOM. https://treeherder.mozilla.org/#/jobs?repo=try&revision=163572fe1272
Attachment #8596928 -
Flags: review?(jcoppeard)
Comment 16•9 years ago
|
||
Comment on attachment 8596928 [details] [diff] [review] fix_updateMallocCounter_in_MallocProvider-v0.diff Review of attachment 8596928 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/MallocProvider.h @@ +70,5 @@ > } > + p = (T*)client()->onOutOfMemory(AllocFunction::Malloc, numElems * sizeof(T)); > + if (MOZ_LIKELY(p)) > + client()->updateMallocCounter(numElems * sizeof(T)); > + return p; Sorry to nitpick, but can we factor out the |numElems * sizeof(T)| calculation as it's not used three times in this function? Also ditto below. Also I don't think there's much point using MOZ_LIKELY() after the initial alloc failure. Otherwise all good :)
Attachment #8596928 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=163572fe1272
Assignee | ||
Comment 20•9 years ago
|
||
That should be everything.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•