1021151 - NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT leaks on failure?

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

I'm not entirely sure if this is valid, because I'm not entirely familiar with how the aggregated stuff works, but it seems possible.

    _InstanceClass* inst = new _InstanceClass(aOuter);                      \
    if (!inst) {                                                            \
        return NS_ERROR_OUT_OF_MEMORY;                                      \
    }                                                                       \
                                                                            \
    nsISupports* inner = inst->InnerObject();                               \
    NS_ADDREF(inner);                                                       \
    nsresult rv = inst->_InitMethod();                                      \
    if (NS_SUCCEEDED(rv)) {                                                 \
        rv = inner->QueryInterface(aIID, aResult);                          \
    }                                                                       \
    NS_RELEASE(inner);                                                      \
                                                                            \
    return rv;                                                              \

If _InitMethod() succeeds, and the QI succeeds, then ownership goes to the caller via aResult, but I think if either of those fail, then nobody will end up freeing |inst|, unless the release of the inner somehow triggers the freeing of the outer.

(Coverity actually reports this leak for the one place the macro is used, in netwerk/build/nsNetModule.)

Comment 1

[(no longer active)]

Yep, we're definitely leaking as far as I can see.

Comment 2

[Mike Hoye [:mhoye]]

Ehsan, you want to mentor this one?

Comment 3

[(no longer active)]

Sure!

Comment 4

[(no longer active)]

The fix here would be to convert the raw pointer on this line <http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAgg.h#316> to an nsRefPtr.

Comment 5

[Thomas Powell]

I'll take a shot at this. I feel like compiling some code. :)

Comment 6

[Mike Hoye [:mhoye]]

Go for it. Do you have a build environment set up already?

Comment 7

[Thomas Powell]

I'll have to double-check. My code is pretty stale, at the very least.

Comment 8

[(no longer active)]

(In reply to comment #7)
> I'll have to double-check. My code is pretty stale, at the very least.

Great!  Please let us know if you run into any problems, or if you have any questions!

Comment 9

[(no longer active)]

Any updates on this, Thomas?  Anything I can help with?

Comment 10

[Thomas Powell]

I pretty quickly made changes and they appear to work, but was wondering if there's any profiling or other testing that can examine the impact of the code change.

Comment 11

[Andrew McCreight [:mccr8]]

If you run the browser and it works that's probably enough. :)

Comment 12

[(no longer active)]

(In reply to comment #11)
> If you run the browser and it works that's probably enough. :)

Yep!

Comment 13

[Andrew McCreight [:mccr8]]

Any updates, Thomas?  It sounded like you had the patch written and working, so what you need to do next is to upload it to this bug and ask for review from :Ehsan or :Froydnj

Comment 14

[Thomas Powell]

I'm ready to submit...  I'm re-reviewing the patch creation process and then I will submit a patch.

Comment 15

[Andrew McCreight [:mccr8]]

Great.  Let us know if you have any questions.

Comment 16

[Thomas Powell]

I can't remember what I was trying to do to get in this state, but how do I make a patch from the current state of my repository?


I have this commit locally:
diff --git a/xpcom/base/nsAgg.h b/xpcom/base/nsAgg.h
--- a/xpcom/base/nsAgg.h
+++ b/xpcom/base/nsAgg.h
@@ -308,17 +308,17 @@ static nsresult                         
 static nsresult                                                             \
 _InstanceClass##Constructor(nsISupports *aOuter, REFNSIID aIID,             \
                             void **aResult)                                 \
 {                                                                           \
     *aResult = nullptr;                                                     \
     if (NS_WARN_IF(aOuter && !aIID.Equals(NS_GET_IID(nsISupports))))        \
         return NS_ERROR_INVALID_ARG;                                        \
                                                                             \
-    _InstanceClass* inst = new _InstanceClass(aOuter);                      \
+    nsRefPtr<_InstanceClass> inst = new _InstanceClass(aOuter);                      \
     if (!inst) {                                                            \
         return NS_ERROR_OUT_OF_MEMORY;                                      \
     }                                                                       \
                                                                             \
     nsISupports* inner = inst->InnerObject();                               \
     NS_ADDREF(inner);                                                       \
     nsresult rv = inst->_InitMethod();                                      \


And I have these two changesets. 
changeset:   187091:dc4a305225e8
tag:         bug1021151.patch
tag:         qbase
tag:         qtip
tag:         tip
user:        Thomas Powell <[e-mail]>
date:        Fri Aug 01 14:46:38 2014 -0400
summary:     [mq]: bug1021151.patch

changeset:   187090:94f2f1861f65
tag:         qparent
user:        Thomas Powell <[e-mail]>
date:        Fri Aug 01 09:13:56 2014 -0400
summary:     Bug 1021151 - change _InstanceClass to nsRefPtr<_InstanceClass>

Comment 17

[Andrew McCreight [:mccr8]]

Generally, the workflow people use for Mercurial is to use Mercurial queues, so you never actually commit something locally until you are going to actually push it to mozilla-inbound, if you have commit access.  Though maybe that's what you are doing?  You can turn a qpushed patch into a patch by doing hg export, or just copying it out of your patches directory.  I haven't used hg in a while so I don't remember exactly where it is.

Comment 18

[Andrew McCreight [:mccr8]]

This page has some information on creating a patch in the proper format:
  https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 19

[Andi [:andi]]

Attachment: MozReview Request: Bug 1021151 - avoid memory leak in NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT, use nsAutoPtr instead of naked ptr. r?ehsan

Review commit: https://reviewboard.mozilla.org/r/36037/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36037/

Comment 20

[Andi [:andi]]

Comment on attachment 8722445 [details]
MozReview Request: Bug 1021151 - avoid memory leak in  NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT, use nsAutoPtr instead of naked ptr. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36037/diff/1-2/

Comment 21

[Andi [:andi]]

Al credits goes to Thomas Powell, i only update the fix and patch it.

Comment 22

[(no longer active)]

Comment on attachment 8722445 [details]
MozReview Request: Bug 1021151 - avoid memory leak in  NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT, use nsAutoPtr instead of naked ptr. r?ehsan

https://reviewboard.mozilla.org/r/36037/#review32681

::: xpcom/base/nsAgg.h:298
(Diff revision 2)
> -    _InstanceClass* inst = new _InstanceClass(aOuter);                      \
> +    nsAutoPtr<_InstanceClass> inst = new _InstanceClass(aOuter);            \

Why are you using nsAutoPtr?  You should use RefPtr, I think.

r-, except that MozReview cannot r-minus! :-)

Comment 23

[Andi [:andi]]

Comment on attachment 8722445 [details]
MozReview Request: Bug 1021151 - avoid memory leak in  NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT, use nsAutoPtr instead of naked ptr. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36037/diff/2-3/

Comment 24

[Andi [:andi]]

hello Ehsan,

I've updated the patch but am i missing something obvious here? I used nsAutoPtr because it had the functionality that we needed and that is:

1. manages an object obtained via new expression.
2. deletes that object when itself is destroyed.

These statements are implemented below:

>>private:
>>  T* MOZ_OWNING_REF mRawPtr;
>>
>>public:
>>  typedef T element_type;
>>
>>  ~nsAutoPtr()
>>  {
>>    delete mRawPtr;
>>  }
>>
>>  // Constructors
>>
>>  nsAutoPtr()
>>    : mRawPtr(0)
>>    // default constructor
>>  {
>>  }
>>
>>  MOZ_IMPLICIT nsAutoPtr(Ptr aRawPtr)
>>    : mRawPtr(aRawPtr)
>>    // construct from a raw pointer (of the right type)
>>  {
>>  }

Why did i use it? It was something with that i was more familiar that resembled with std::auto_ptr, that in C++11 is deprecated if i'm not wrong and it is replaced by unique_ptr. 

Thanks again for the review :)

Comment 25

[(no longer active)]

Firstly, nsAutoPtr has the same issue as std::auto_ptr in that it's copyable by default but the copy destroys the right hand side of the assignment which is totally unexpected.  Similar to std::auto_ptr, it should not be used in new code, and you should use UniquePtr (or std::unique_ptr in non-Mozilla code) instead.

But more importantly, this macro is used with refcounted objects, and you should never delete a refcounted object manually.  The correct thing to do is to use RefPtr for all references to such object, and the last reference being dropped will delete the object for you.

Hope this makes sense!

Comment 26

[(no longer active)]

Comment on attachment 8722445 [details]
MozReview Request: Bug 1021151 - avoid memory leak in  NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT, use nsAutoPtr instead of naked ptr. r?ehsan

https://reviewboard.mozilla.org/r/36037/#review32731

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/281306a480ac

Comment 28

[Andi [:andi]]

Totally agree with you. Thanks for the review.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to :Ehsan Akhgari from comment #22)
> ::: xpcom/base/nsAgg.h:298
> (Diff revision 2)
> > -    _InstanceClass* inst = new _InstanceClass(aOuter);                      \
> > +    nsAutoPtr<_InstanceClass> inst = new _InstanceClass(aOuter);            \
> 
> Why are you using nsAutoPtr?  You should use RefPtr, I think.

Nit: in the version of this patch that landed, the commit message still said "nsAutoPtr", though the patch was changed to use RefPtr per this review comment from ehsan.

For future patches, please remember to update the commit messages when appropriate, to align with what the patch ultimately does.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/281306a480ac