46593 - XPCOM leak logging doesn't handle nsChromeUIDataSource hack

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Giving this to myself initially, but if anyone else sees the problem, feel free
to take it...

DESCRIPTION:  Danm's nsChromeUIDataSource::Release hack makes the
nsChromeUIDataSource show up as 1/1 leaked object with 0/4 leaked refs.  This is
because the XPCOM leak logging uses a release called with a refcount of 0 to
indicate object destruction.

In theory, the following hack ought to fix it:


Index: nsChromeUIDataSource.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/chrome/src/nsChromeUIDataSource.cpp,v
retrieving revision 1.5
diff -u -d -r1.5 nsChromeUIDataSource.cpp
--- nsChromeUIDataSource.cpp	2000/07/19 23:39:49	1.5
+++ nsChromeUIDataSource.cpp	2000/07/27 00:19:24
@@ -96,6 +96,9 @@
     mComposite = 0; // release
     composite->RemoveObserver(this);
     --mRefCnt;
+    // Tell the leak logging the refcount hit 0, if it did.
+    NS_LOG_ADDREF(this, mRefCnt+1, "nsChromeUIDataSource", sizeof(*this));
+    NS_LOG_RELEASE(this, mRefCnt, "nsChromeUIDataSource");
   }
   if (mRefCnt == 0) {
     delete this;

But it doesn't.  It shows up as an additional object created:  0/5 refs leaked,
1/2 objects leaked.

Maybe this is because the serial number is recycled when the count of refs hits
0, but it doesn't look that way.  (If that is the problem, then I think the
right fix is only to recycle the serial number ptr when aRefCnt is 0, which is
what you seem to do now anyway...)

STEPS TO REPRODUCE:
 * get all the leak fixes I have in my tree :-)
 * ./mozilla
 * type a few chars in URL bar
 * exit

Comment 1

[Warren Harris]

Won't this patch cause us to count too many total nsChromeUIDataSource objects 
(even if it gets the final count right)?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It should, I would think, cause too many refs to be counted, but not objects,
since the existing one hasn't been marked as destroyed yet.  But it seems it may
have been marked as destroyed in some way... that's what I don't understand.

What the refcounting hack is doing is breaking a cycle within the ::Release
method if the refcount is 1.  Since the refcount is incremented before breaking
the cycle to avoid recursion into release, there is never a Release 0 logged. 
What I was doing was attempting to log an AddRef 1 and a Release 0, but that
didn't work as I expected.

Comment 3

[Dan M]

This patch seems to clear up the log. Since the whole operation is a hack in the 
first place, the log hacks don't seem out of place.

Index: nsChromeUIDataSource.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/chrome/src/nsChromeUIDataSource.cpp,v
retrieving revision 1.5
diff -r1.5 nsChromeUIDataSource.cpp
94a95
>     NS_LOG_ADDREF(this, mRefCnt, "nsChromeUIDataSource", sizeof(nsChromeUIDataSource));
98a100
>     NS_LOG_RELEASE(this, mRefCnt, "nsChromeUIDataSource");

(that is, log an addef right after the extra ++mRefCnt, and log a release right 
after the extra --mRefCnt).

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Of course... that prevents it from ever hitting a balance of 0 until the number
is 0.

Comment 5

[Dan M]

waterson stirred things up a a bit. no more refcounting hack. no more problem.

Comment 6

[Tracy Walker [:tracy]]

please verify

Comment 7

[leger]

Marking Verified.  Please reopen if problem reoccurs.