39323 - ghost leak: nsTopProgressListener, by class nsSoftwareUpdate

Status: RESOLVED WONTFIX

(Core Graveyard :: Installer: XPInstall Engine, defect, P3)

Description

[Inaky Perez Gonzalez]

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     
   }

Comment 1

[Inaky Perez Gonzalez]

Attachment: Proposed (draft) patch for fixing it and error dump of the problems running it

Comment 2

[Cathleen]

reassign to dan

Comment 3

[Inaky Perez Gonzalez]

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.

Comment 4

[Inaky Perez Gonzalez]

Attachment: Fix by changing mMasterListener to be a COMPtr

Comment 5

[Chris Waterson]

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

Comment 6

[Inaky Perez Gonzalez]

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.

Comment 7

[Chris Waterson]

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.

Comment 8

[Inaky Perez Gonzalez]

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?

Comment 9

[Inaky Perez Gonzalez]

Attachment: Fix using nsCOMPtr to interface

Comment 10

[Chris Waterson]

inaky, I definitely think that casting like you're doing is *not* the right 
thing to do here. Can you work with dveditz or [e-mail] 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

Comment 11

[Inaky Perez Gonzalez]

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.

Comment 12

[Inaky Perez Gonzalez]

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.

Comment 13

[Inaky Perez Gonzalez]

Attachment: Fix with manual pointer handling and checks

Comment 14

[Chris Waterson]

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

Comment 15

[Daniel Veditz [:dveditz]]

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)

Comment 16

[Inaky Perez Gonzalez]

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

Comment 17

[Matthias Versen [:Matti]]

[e-mail]: Are you still working on this ? Can you pleasae add a
comment ?

Comment 18

[Inaky Perez Gonzalez]

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

Comment 19

[Matthias Versen [:Matti]]

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

Comment 20

[Nicholas Nethercote [inactive]]

> 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.