Closed Bug 39323 Opened 22 years ago Closed 7 years ago

ghost leak: nsTopProgressListener, by class nsSoftwareUpdate

Categories

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

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: inaky.gonzalez, Unassigned)

Details

(Keywords: memory-leak)

Attachments

(4 files)

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
Assignee: cathleen → dveditz
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.
Keywords: mlk
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.
Assignee: dveditz → inaky.gonzalez
Blocks: 92580
No longer blocks: 92580
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)
Assignee: inaky.gonzalez → dveditz
Status: NEW → ASSIGNED
Assignee: dveditz → nobody
Status: ASSIGNED → NEW
QA Contact: jimmykenlee → xpi-engine
> 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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.