Closed Bug 1021151 Opened 6 years ago Closed 4 years ago

NS_GENERIC_AGGREGATED_CONSTRUCTOR_INIT leaks on failure?

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mccr8, Assigned: twilliampowell, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, memory-leak, Whiteboard: [CID 749279] [good first bug])

Attachments

(1 file)

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.)
Yep, we're definitely leaking as far as I can see.
Ehsan, you want to mentor this one?
Flags: needinfo?(ehsan)
Sure!
Flags: needinfo?(ehsan)
Whiteboard: [CID 749279] → [CID 749279][mentor=ehsan]
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.
Whiteboard: [CID 749279][mentor=ehsan] → [CID 749279][mentor=ehsan] [good first bug]
I'll take a shot at this. I feel like compiling some code. :)
Go for it. Do you have a build environment set up already?
Assignee: nobody → twilliampowell
I'll have to double-check. My code is pretty stale, at the very least.
(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!
Mentor: ehsan
Whiteboard: [CID 749279][mentor=ehsan] [good first bug] → [CID 749279] [good first bug]
Any updates on this, Thomas?  Anything I can help with?
Flags: needinfo?(twilliampowell)
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.
Flags: needinfo?(twilliampowell)
If you run the browser and it works that's probably enough. :)
(In reply to comment #11)
> If you run the browser and it works that's probably enough. :)

Yep!
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
Flags: needinfo?(twilliampowell)
I'm ready to submit...  I'm re-reviewing the patch creation process and then I will submit a patch.
Flags: needinfo?(twilliampowell)
Great.  Let us know if you have any questions.
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 <twilliampowell@gmail.com>
date:        Fri Aug 01 14:46:38 2014 -0400
summary:     [mq]: bug1021151.patch

changeset:   187090:94f2f1861f65
tag:         qparent
user:        Thomas Powell <twilliampowell@gmail.com>
date:        Fri Aug 01 09:13:56 2014 -0400
summary:     Bug 1021151 - change _InstanceClass to nsRefPtr<_InstanceClass>
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 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/
Al credits goes to Thomas Powell, i only update the fix and patch it.
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! :-)
Attachment #8722445 - Flags: review?(ehsan)
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/
Attachment #8722445 - Flags: review?(ehsan)
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 :)
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 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
Attachment #8722445 - Flags: review?(ehsan) → review+
Totally agree with you. Thanks for the review.
(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.
https://hg.mozilla.org/mozilla-central/rev/281306a480ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.