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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 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

Comment 2

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

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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 4

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

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.

Comment 5

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

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.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Okay, only the addref/release cases, then

Comment 7

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

Comment on attachment 321302 [details] [diff] [review]
Okay, only the addref/release cases, then

r=dbaron

Comment 8

[Jeff Walden [:Waldo]]

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.