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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
10 months ago

People

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

Tracking

(Blocks 1 bug, {coverity, memory-leak})

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3] )

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
When set->init() fails, set is not freed.
(Reporter)

Comment 1

5 years ago
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

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink] [mentor=mccr8]
Whiteboard: [MemShrink] [mentor=mccr8] → [MemShrink:P3] [mentor=mccr8]
Mentor: continuation
Whiteboard: [MemShrink:P3] [mentor=mccr8] → [MemShrink:P3]
(Reporter)

Comment 2

5 years ago
Let me know if you have any questions, Guillaume!
(Assignee)

Comment 3

5 years ago
Posted 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)
(Reporter)

Comment 4

5 years ago
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+
(Reporter)

Comment 5

5 years ago
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)
(Assignee)

Comment 7

5 years ago
Posted patch bug1019843_2.patch (obsolete) — Splinter Review
Attachment #8447713 - Attachment is obsolete: true
Attachment #8449978 - Flags: review?(jorendorff)
(Assignee)

Comment 8

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.