326603 - Enable use of tracerefcnt with derived classes

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Darin Fisher]

Enable use of tracerefcnt with derived classes.

If you use NS_IMPL_ADDREF_INHERITED, then you cannot use the refcount balancer tools to check for leaks on your class.  Instead, you can only monitor leaks on the base class.  If you have several classes inheriting from the base class, then this makes it difficult.  Example: nsBaseChannel.

The simple solution is to just log addref and release events for both the derived class as well as the base class.  Patch coming up.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

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

Comment on attachment 211330 [details] [diff] [review]
v1 patch

>+  NS_LOG_ADDREF(this, r, #Class, sizeof(*this));                              \

Perhaps you should use sizeof(*this) - sizeof(Super), although maybe not, since things are double-counted for object counts, serial numbers, etc.?  I think I'm inclined towards leaving it and going for pure double-counting, since that's easier to understand.

Comment 3

[Darin Fisher]

> Perhaps you should use sizeof(*this) - sizeof(Super), although maybe not, since
> things are double-counted for object counts, serial numbers, etc.?  I think I'm
> inclined towards leaving it and going for pure double-counting, since that's
> easier to understand.

Hmm... if we don't modify the size as you suggest, then the leak logs will show a false increase in the total memory leaked.  For that reason, I think making the change as you suggested is a good idea.

Comment 4

[Darin Fisher]

<dbaron> I sort of prefer it the way you had it, actually, because nsTraceRefcnt is useless for aggregate stats anyway.
<dbaron> And seeing the real size might be a little clearer
<dbaron> especially since there's already a good bit of double-counting with that patch

Good point.  I'll go with the original patch.

Comment 5

[Darin Fisher]

fixed-on-trunk

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I assume this basically fixes bug 299663?

Also, this plays nice with the subtraction of comptr logs that make-tree.pl does?

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 299663 has been marked as a duplicate of this bug. ***

Comment 8

[Darin Fisher]

> So I assume this basically fixes bug 299663?

Thanks for marking the dupe.


> Also, this plays nice with the subtraction of comptr logs that make-tree.pl
> does?

I don't know the first about that.

Comment 9

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

(In reply to comment #6)
> Also, this plays nice with the subtraction of comptr logs that make-tree.pl
> does?

Certainly no problems if you're only logging one of the classes, I'd think.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I just checked (by applying the patch locally), and it looks like the comptr subtraction does work fine.  I assume that's because we're using the pointer involved to track things, so as long as |this| is the same for subclass and superclass (which it ought to be in all reasonable cases), things should work.

Thanks a ton for fixing this, Darin!