403199 - Leak logging confused with classes derived from classes supporting aggregation. (like nsSimpleNestedURI)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter Van der Beken [:peterv]]

I ran into this while trying to reproduce bug 399758:

Serial Numbers of Leaked Objects:
12 @0x19754380 (0 references; 0 from COMPtrs)

I was logging nsSimpleNestedURI. I think I know what is happening.

nsSimpleNestedURI is derived from nsSimpleURI which supports aggregation. Because of aggregation the canonical nsISupports pointer for nsSimpleURI is not the nsSimpleURI itself. Calling AddRef/Release on the canonical nsISupports pointer will not call AddRef/Release on the nsSimpleURI itself. Calling AddRef/Release on the nsSimpleURI will call AddRef/Release on the canonical nsISupports pointer. In the case of a derived class this means that the AddRef/Release of the derived class will only be called when calling AddRef/Release on the nsSimpleURI itself. In this case the AddRef/Release calls were balanced (both through the derived class and through the canonical nsISupports pointer), but the Release for refcount 1 -> 0 didn't go through the derived class. The object was deleted properly, but the logging code in the implementation of AddRef/Release for the derived class was never called for refcount 0, and so we didn't register destruction for the derived class (see http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/base/nsTraceRefcntImpl.cpp&rev=1.110&mark=253-254#250).

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080912014358 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)

Fwiw, I can reproduce leaking 1 nsSimpleNestedURI, by running (--browser-chrome)
<objdir-Firefox_\_tests\testing\mochitest\browser\docshell\test\browser
with "only"
[
browser_bug134911.js
browser_bug388121-1.js
browser_bug388121-2.js
browser_bug441169.js
test-form_sjis.html
]
If I remove any (more) of these files, the leak (report) stops :-|

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081120 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/20245c2d97d0)

(After removing the (leaking) tests from bug 462545 and bug 465952,)
I can't reproduce this leak anymore.

I don't know if the previously leaking tests were modified, or if this bug was fixed...

Comment 5

[Serge Gautherie (:sgautherie)]

Reftest leak error report was enabled today,
and here is an intermittent leak:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238936836.1238944390.2219.gz
Linux mozilla-central unit test on 2009/04/05 06:07:16

TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 64 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsSimpleNestedURI with size 64 bytes

TinderboxPrint: reftest<br/>2875/0/138&nbsp;<em class="testfail">LEAK</em>
}

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Reftest leak error report was enabled today,

Actually, in this very build ;->

Comment 7

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239173176.1239181637.16945.gz
Linux mozilla-central unit test on 2009/04/07 23:46:16
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1239193666.1239204277.982.gz
Linux mozilla-central unit test on 2009/04/08 05:27:46

Comment 8

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

Why do you think those failures are related to this bug?  Why would any symptoms of this bug be intermittent?

Comment 9

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

Er, I suppose they'd be intermittent if it varies who does the last release on an object, so never mind... maybe.

Comment 10

[Boris Zbarsky [:bzbarsky]]

If Serge is correct that this is the intermittent leak on tbox, then this is by far the most common source of unit test orange...

Would just never calling AddRef/Release on the nsSimpleURI (and instead always calling it on its canonical nsISuports) fix the issue?  Or even more simply, would switching nsSimpleNestedURI to not log its addref/release calls work?

Note that I do thing Serge is correct, because while nsSimpleNestedURI shows as leaking, nsSimpleURI does NOT.  Nor do any other objects.

Comment 11

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

Yeah, I think this is fallout from making the *_INHERITED macros log, which things really weren't designed for.  I think the easy solution here would be just making nsSimpleNestedURI's AddRef and Release not log; it may well be good to have a set of INHERITED_ADDREF and INHERITED_RELEASE macros that don't log for the general case.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)

Ah, iiuc, this is what you were (already) suggesting in bug 458844 comment 13...

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#818

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: How does this look? [Checkin: See comment 16 & 21]

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 371877 [details] [diff] [review]
How does this look?
[Checkin: See comment 16 & 21]

I guess you can get rid of the |nsrefcnt r| intermediate variable.

Comment 15

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

Comment on attachment 371877 [details] [diff] [review]
How does this look?
[Checkin: See comment 16 & 21]

r+sr=dbaron with the elimination of the |r| variable in both

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/daa54fdba14e

Comment 17

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

Shouldn't we search the tree for other classes derived from classes supporting aggregation before marking this fixed?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, probably a good idea, yes.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> Shouldn't we search the tree for other classes derived from classes supporting
> aggregation

How does one do this ?

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371877 [details] [diff] [review]
How does this look?
[Checkin: See comment 16 & 21]

a191=beltzner

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 371877 [details] [diff] [review]
How does this look?
[Checkin: See comment 16 & 21]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce9476ab72d8

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> > Shouldn't we search the tree for other classes derived from classes supporting
> > aggregation
> 
> How does one do this ?

Are the followings part of the answer?

http://mxr.mozilla.org/mozilla-central/ident?i=NS_DECL_AGGREGATED
Referenced (in 5 files total)
http://mxr.mozilla.org/mozilla1.9.1/ident?i=NS_DECL_AGGREGATED
Referenced (in 8 files total)

http://mxr.mozilla.org/mozilla-central/ident?i=NS_DECL_CYCLE_COLLECTING_AGGREGATED
http://mxr.mozilla.org/mozilla1.9.1/ident?i=NS_DECL_CYCLE_COLLECTING_AGGREGATED
Referenced in:
    * rdf/base/src/nsInMemoryDataSource.cpp

Comment 24

[Andrew McCreight [:mccr8]]

Code for this landed a long time ago. I think the aggregated stuff might even be gone now, though NS_IMPL_NONLOGGING_ADDREF_INHERITED is still around.