Closed Bug 1019843 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mccr8, Assigned: guillaume.turri, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, memory-leak, Whiteboard: [MemShrink:P3] )

Attachments

(1 file, 2 obsolete files)

When set->init() fails, set is not freed.
Not surprisingly, a similar thing happens in MapObject::construct.
Summary: SetObject::construct leaks |set| when |init| fails → SetObject::construct and MapObject::construct leaks when |init| fails
Whiteboard: [MemShrink] → [MemShrink] [mentor=mccr8]
Whiteboard: [MemShrink] [mentor=mccr8] → [MemShrink:P3] [mentor=mccr8]
Mentor: continuation
Whiteboard: [MemShrink:P3] [mentor=mccr8] → [MemShrink:P3]
Let me know if you have any questions, Guillaume!
Attached patch bug1019843.patch (obsolete) — Splinter Review
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...
Attachment #8447713 - Flags: feedback?(continuation)
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.
Attachment #8447713 - Flags: review?(jorendorff)
Attachment #8447713 - Flags: feedback?(continuation)
Attachment #8447713 - Flags: feedback+
Oh and thanks for the patch!
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.
Attachment #8447713 - Flags: review?(jorendorff)
Attached patch bug1019843_2.patch (obsolete) — Splinter Review
Attachment #8447713 - Attachment is obsolete: true
Attachment #8449978 - Flags: review?(jorendorff)
Attachment #8449978 - Attachment is obsolete: true
Attachment #8449978 - Flags: review?(jorendorff)
Attachment #8451224 - Flags: review?(jorendorff)
Attachment #8451224 - Flags: review?(jorendorff) → review+
Testing here:
https://tbpl.mozilla.org/?tree=Try&rev=154167c16f23

Setting ni?me to check back on that and set checkin-needed later on.
Flags: needinfo?(jorendorff)
Er, disregard previous link. Testing here:
https://tbpl.mozilla.org/?tree=Try&rev=09ee28d968d3
Flags: needinfo?(jorendorff)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ff371321520
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.