WHERE: xpinstall/src/nsSoftwareUpdate.cpp (class nsSoftwareUpdate) PROBLEM: It is not really a leak, but shows as, and confuses bloatview/refcnt logger, adding unnecessary noise. An instance of nsSoftwareUpdate has a member variable (mMasterListener) of type nsTopProgressListener. This former class is nsISupport based, and so, the nsSoftwareUpdate class owns a reference to it. To do this, in the constructor, mMasterListener.AddRef() is executed. The test case has been launching mozilla, and clicking exit on the Profile Manager. Until here this is the correct way to go. Doing this we evitate that when that other objects referencing that one (directly or indirectly) won't be able to drop it's count to zero (causing it's destruction which would result in a crash, as we'd be deleting an incorrect address). The problem comes when the nsSoftwareUpdate object is destroyed, as it doesn't release the reference to the nsTopProgressListener mMasterListener object. THIS DOESN'T CAUSE A LEAK, as the object is freed as part of it's parent, but it confuses bloatview and refcnt balancer as they don't see the Release they expect. SOLLUTION: I've ran under these kind of problems before. The sollution is pretty simple, and comes in two flavours: a) ignoring the leak, what adds noise to the reports [the easy one] b) adding a nsISupports primitive, Release_nd() to be used on this kind of objects. This method would operate as Release(), but it would not delete the object when the count drops to zero. IT'S VERY IMPORTANT TO NOTE that this method should be ONLY used to stabilize the counts in objects which are children of another object, and not dinamically allocated within the object. Short, fast example: class A : public nsISupports { // whatever }; class B { public: B (void) { a_children.AddRef(); // stabilize } ~B (void) { a_children.Release_nd(); // don't confuse bloatview/refcnt balancer } A a_children; // more whatever } // ... code ... void any_function (B &b) { b.a_children.AddRef(); // ... blablabla ... #ifdef THIS_IS_CORRECT_CODE b.a_children.Release(); #else // !THIS_IS_CORRECT_CODE b.a_children.Release_nd(); #endif }

reassign to dan

I´ve created a new patch which fixes this one, changing the mMasterListener member into a COMPtr and having that object dynamic. It removes the noise, and otherwise, works ok.

Using nsCOMPtr's on naked classes doesn't work on some compilers, IIRC.

What do you mean with ´naked class´? sorry, I don´t understand that (blame it on my english)... nsTopProgressListener is a pretty XPCOM class, as any other, so it should work normally, unless you tell me there is some magic below.

Sorry about that. What I meant was, nsCOMPtr's expect to hold interface pointers (e.g., nsCOMPtr<nsIFoo>), not implementation pointers (e.g., nsCOMPtr<nsFooImpl>). Some compilers do eager template instantiation, so if you use an nsCOMPtr with an implementation pointer, the compiler will try to create a method that calls nsFooImpl::GetIID(). This method won't exist on the implementation. Regardless, it's generally bad form to use an nsCOMPtr on anything but an interface pointer.

Thanks, that something I did not know. Then it should be a matter of changing it to the interface class; in this case there is no interface, as far as I can tell, so I´m going to use an nsIXPIListener, as it is a base class. Doing it this way will force me to cast upon access, but I don´t know of a better way, unless I create an interface class. Which approach would you prefer?

inaky, I definitely think that casting like you're doing is *not* the right thing to do here. Can you work with dveditz or scc@mozilla.org to figure out a better way? Maybe it would be best to do this without nsCOMPtr's, and just have the nsTopProgressListener be owned as a raw pointer that's NS_ADDREF'd and NS_RELEASE'd manually. thanks, chris

Completely agree. I did not like it a bit as soon as I was doing it, but wanted to be sure you agreed. Definitively, the best way to do it is going to be using manual ADDREFs/RELEASEs. Will work on it now.

Okay, here is the new patch. I´m doing it manually, much cleaner than the casting mess. I´ve also added checks before accessing the pointer, I feel safer that way.

Thanks for doing this. This looks good to me, but I'll let dveditz have the final say.

Looks OK to me too, though I'd prefer a different error to NS_ERROR_OUT_OF_MEMORY. That might be originally why the pointer was null, but at that point other wierd badness might have reset that pointer to null. Maybe NS_ERROR_NULL_POINTER or even generic NS_ERROR_FAILURE is better. So do you have check-in rights? This is assigned to me, does that mean you want me to check in? (not a problem, just want to make sure we know who's doing the next step)

YEah, I do have. I´ll change the error to NS_ERROR_FAILURE and then check it in.

inaky.gonzalez@intel.com: Are you still working on this ? Can you pleasae add a comment ?

I have not done any work in mozilla for a long time already, so I cannot provide any insight. Sorry :)

-> default owner (via mail:Inaky Perez Gonzalez has no time for this)

> An instance of nsSoftwareUpdate has a member variable > (mMasterListener) of type nsTopProgressListener. According to DXR, neither of these classes exists any more. I think this bug can be closed.