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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Eric Rahm [:erahm]]

|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

Comment 1

[Luke Wagner [:luke]]

At least some of these came in with recent refactorings, it would appear:
  http://hg.mozilla.org/mozilla-central/rev/f9ec09ff142c#l13.27

Comment 2

[Nicholas Nethercote [inactive]]

terrence, please fix! :)

Comment 3

[Terrence Cole [:terrence]]

Oh, good catch! I did not realize that onOutOfMemory did more than report!

Comment 4

[Terrence Cole [:terrence]]

Attachment: return_onOOM_result-v0.diff

Comment 5

[Andrew McCreight [:mccr8]]

What versions does this affect?  Please be sure to backport as appropriate.

Comment 6

[Eric Rahm [:erahm]]

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

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6243d2b50e33

Comment 8

[Nicholas Nethercote [inactive]]

Thank you for the fix, Terrence.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6243d2b50e33

Comment 10

[Andrew McCreight [:mccr8]]

Can this be closed now, Terrence?  Do you recall why you left it open?

Comment 11

[Terrence Cole [: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.

Comment 12

[Andrew McCreight [:mccr8]]

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 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Terrence Cole [:terrence]]

That too.

Comment 15

[Terrence Cole [:terrence]]

Attachment: 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

Comment 16

[Jon Coppeard (:jonco)]

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

Comment 17

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=163572fe1272

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c18a8a916d4c

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c18a8a916d4c

Comment 20

[Terrence Cole [:terrence]]

That should be everything.