Closed Bug 1081260 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

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)

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]
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)
Oh, good catch! I did not realize that onOutOfMemory did more than report!
Blocks: GC.size
Flags: needinfo?(terrence)
Attachment #8505545 - Flags: review?(luke)
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
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)
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.
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]
Depends on: 1156317
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+
That should be everything.
Status: NEW → RESOLVED
Closed: 5 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.