MOZ_COUNT_DTOR and NS_LOG_RELEASE should assert (or worse) when used on an unrecognized pointer, when possible

RESOLVED FIXED in mozilla2.0

Status

()

Core
XPCOM
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla2.0
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

There aren't that many cases where we can do this as it only works when every object of that class is being assigned a serial number (so LOG_CLASSES has the class and LOG_OBJECTS isn't defined), but it would have been enough for me to have gotten an assert when generating the stacks in bug 429233.
Created attachment 316699 [details] [diff] [review]
Patch

We can't simply change GetSerialNumber due to the way it's used in the comptr logging, as far as I could tell at a glance.

With this patch and the patch in bug 429930 in place, if I revert the patch for bug 429233 and run |python runtests.py --test-path=q --setenv=XPCOM_MEM_LOG_CLASSES=nsTArray_base|, I get this instead of a series of "leaked" pointers with 0 references:

###!!! ABORT: Serial number requested for unrecognized pointer!  Are you memmoving an object with external pointers?: 'serialno != 0', file /Users/jwalden/moz/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 1090
ERROR FAIL Exited with code 6 during test run
Attachment #316699 - Flags: superreview?(dbaron)
Attachment #316699 - Flags: review?(dbaron)
I don't see how you can use NS_ABORT_IF_FALSE right now given that there are a bunch of cases where we hit this -- and it doesn't seem particularly helpful if somebody's trying to debug a leak, but can't debug leaks of objects of a certain class because somebody else sticks those objects in an nsTHashtable entry.

The assertion text also seems to imply that there are external pointers, when really the only external pointer might be the leak logging code itself.
Created attachment 318369 [details] [diff] [review]
Change macro, assertion text

I can change the macro easily enough.  What cases hit this?  We should be fixing them, not weakening assertions simply because they'd fire too often.  I'm trying to get in the habit of giving the assertions I write meaningful teeth (well, meaningful once ABORT actually aborts) so that someone else can't break them underneath me, also in anticipation of assertions becoming fatal eventually.

It doesn't seem particularly helpful if someone's trying to debug a leak but can't because the one or two instances that are leaking are scattered through a bunch of other bogus instance leaks.  The leak log in bug 429233 had this characteristic, and I don't see how the two classes would be distinguishable without examining every individual stack trace.

I've changed the assertion text to "Serial number requested for unrecognized pointer!  Are you memmoving a refcounted object?"  If you can think of a better message, let me know.
Attachment #316699 - Attachment is obsolete: true
Attachment #318369 - Flags: review?(dbaron)
Attachment #316699 - Flags: superreview?(dbaron)
Attachment #316699 - Flags: review?(dbaron)
I agree that we can assert this in the NS_LOG_RELEASE case.  But not in the MOZ_COUNT_DTOR case, which is where we'd actually hit it.  And in that case it's not a refcounted object.
Though maybe what we really want here is a separate set of macros for MOZ_MOVABLE_OBJ_COUNT_CTOR / MOZ_MOVABLE_OBJ_COUNT_DTOR, with another macro for declaring the variable where the object stores its own serial number.  And then we could assert in MOZ_COUNT_CTOR / MOZ_COUNT_DTOR, with the assertion text for those saying that the MOVABLE_OBJ macros need to be used instead.  That would fix the case of debugging things like leaked nsVoidArray that's become hard due to all the cases where they're put in hashtables, arrays, etc.
Created attachment 321302 [details] [diff] [review]
Okay, only the addref/release cases, then
Attachment #318369 - Attachment is obsolete: true
Attachment #321302 - Flags: review?(dbaron)
Attachment #318369 - Flags: review?(dbaron)
Comment on attachment 321302 [details] [diff] [review]
Okay, only the addref/release cases, then

r=dbaron
Attachment #321302 - Flags: review?(dbaron) → review+
Addref/release cases fixt in mozilla-central.  That covers the original problem I was trying to address, and the movable macros can be someone else's bug; I don't have time to do that now.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.