|js::MallocProvider| alloc functions leak memory after calling |onOutOfMemory|

RESOLVED FIXED in mozilla40

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: erahm, Assigned: terrence)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla40
coverity
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CID 1244574][CID 1244708][CID 1244669][CID 1244747][CID 1244667][CID 1244764][CID 124461])

Attachments

(2 attachments)

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

4 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
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]
terrence, please fix! :)
Assignee: nobody → terrence
Flags: needinfo?(terrence)
(Assignee)

Comment 3

4 years ago
Oh, good catch! I did not realize that onOutOfMemory did more than report!
Blocks: 1008337
Flags: needinfo?(terrence)
(Assignee)

Comment 4

4 years ago
Created attachment 8505545 [details] [diff] [review]
return_onOOM_result-v0.diff
Attachment #8505545 - Flags: review?(luke)

Updated

4 years ago
Attachment #8505545 - Flags: review?(luke) → review+
What versions does this affect?  Please be sure to backport as appropriate.
Blocks: 1033442
Flags: needinfo?(terrence)
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)

Updated

4 years ago
Keywords: leave-open
Thank you for the fix, Terrence.
Can this be closed now, Terrence?  Do you recall why you left it open?
Flags: needinfo?(terrence)
(Assignee)

Comment 11

3 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)
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.
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

3 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)

Updated

3 years ago
Depends on: 1156317
(Assignee)

Comment 15

3 years ago
Created attachment 8596928 [details] [diff] [review]
fix_updateMallocCounter_in_MallocProvider-v0.diff

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 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 20

3 years ago
That should be everything.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.