1019843 - SetObject::construct and MapObject::construct leaks when |init| fails

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

When set->init() fails, set is not freed.

Comment 1

[Andrew McCreight [:mccr8]]

Not surprisingly, a similar thing happens in MapObject::construct.

Comment 2

[Andrew McCreight [:mccr8]]

Let me know if you have any questions, Guillaume!

Comment 3

[guillaume.turri]

Attachment: bug1019843.patch

As far as I understand, this patch would fix the leak, using the redefined free operator.

However, I'm afraid I might be missing something. In particular, during the allocation with JSContext->new_, we end up calling JSRuntime::updateMallocCounter, which in turns has a few side effects.
As far as I understand, I don't need to take those into account, but I admit I'm not too sure about that...

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8447713 [details] [diff] [review]
bug1019843.patch

Review of attachment 8447713 [details] [diff] [review]:
-----------------------------------------------------------------

> In particular, during the allocation with JSContext->new_, we end up calling JSRuntime::updateMallocCounter, which in turns has a few side effects.
I think updateMallocCount basically just pokes the GC.  The GC tries to run every so often depending on how much stuff is being allocated, but things like these maps and sets that hang off of the JS objects and are allocated by malloc would be invisible to it if we didn't tell the GC about it.

This patch looks good to me.  I've asked a JS peer who knows a lot about this file to do the review.

Comment 5

[Andrew McCreight [:mccr8]]

Oh and thanks for the patch!

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8447713 [details] [diff] [review]
bug1019843.patch

Review of attachment 8447713 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. Please post a revised patch and we'll land it!

::: js/src/builtin/MapObject.cpp
@@ +1172,5 @@
>      ValueMap *map = cx->new_<ValueMap>(cx->runtime());
>      if (!map)
>          return false;
>      if (!map->init()) {
> +        js_free(map);

Whoa, I have no idea how this got written this way. after the new_ call it should say

    if (!map || !map->init()) {
        js_delete(map);
        js_ReportOutOfMemory(cx);
        return false;
    }

(That is, in addition to your fix, we should report OOM if new_ fails, and we should call the destructor even though in this case it probably doesn't do anything.)

@@ +1683,5 @@
>      ValueSet *set = cx->new_<ValueSet>(cx->runtime());
>      if (!set)
>          return false;
>      if (!set->init()) {
> +        js_free(set);

Same here.

Comment 7

[guillaume.turri]

Attachment: bug1019843_2.patch

Comment 8

[guillaume.turri]

Attachment: bug1019843_3.patch

Comment 9

[Jason Orendorff [:jorendorff]]

Testing here:
https://tbpl.mozilla.org/?tree=Try&rev=154167c16f23

Setting ni?me to check back on that and set checkin-needed later on.

Comment 10

[Jason Orendorff [:jorendorff]]

Er, disregard previous link. Testing here:
https://tbpl.mozilla.org/?tree=Try&rev=09ee28d968d3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff371321520

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ff371321520