1244793 - fix NS_IMPL_RELEASE_INHERITED for corner cases of refcnt logging

Status: NEW

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

With classes like:

class nsRunnable : public nsIRunnable ...
class MyRunnable : nsRunnable ...

NS_IMPL_ISUPPORTS_INHERITED0(MyRunnable, nsRunnable);

and refcnt logging setup like:

XPCOM_MEM_LOG_CLASSES=nsRunnable,MyRunnable command.py ...

and the current implementation of NS_IMPL_RELEASE_INHERITED, we'll get
weird errors when the final Release() is executed on MyRunnable
instances about the class instances not being present in the refcnt
logging table.

Why is this?  Due to C++ class layout rules, for any MyRunnable instance
p, we have:

  static_cast<void*>(p) == static_cast<void*>(static_cast<nsRunnable*>(p))

and therefore the logging code receives the same pointers no matter
which class we're logging for.

That wouldn't normally be a problem, except we execute the superclass's
Release() method prior to logging the release of the derived class.  If
we are Release()'ing the last such reference to the instance, the
superclass's Release() will call the class's destructor, which calls a
logging function for the destructor (NS_LogDtor), which will remove the
instance from the refcnt table.  Then we try and run the derived class's
logging of Release(), only to find that we don't actually have a
recorded pointer in the table for this instance.

This behavior is a corner case (who does leak logging for a superclass
and its derived classes at the same time?), but avoiding this problem is
simple: we log the release of the derived class first, then Release()
the superclass.  The necessary pointers will then still be present in
the table.  For compiler-generated C++ destructors, we run destructors
for the class first and then superclasses, and making this change brings
our behavior more in line with that.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: fix NS_IMPL_RELEASE_INHERITED for corner cases of refcnt logging

I haven't actually tested this to see if it fixes my problem, but the logic
makes a lot of sense to me...

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8714383 [details] [diff] [review]
fix NS_IMPL_RELEASE_INHERITED for corner cases of refcnt logging

Review of attachment 8714383 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsISupportsImpl.h
@@ +980,1 @@
>    NS_LOG_RELEASE(this, r, #Class);                                            \

Isn't r undefined here after your patch? (I haven't read the rest of your comments yet.)

Comment 3

[Nathan Froyd [:froydnj]]

Bother, that's what I get for not compiling first =/

Comment 4

[Andrew McCreight [:mccr8]]

"the superclass's Release() will call the class's destructor"

Please change the latter part of this to "the derived class's destructor". The ambiguity made it a little confusing to me.

It makes sense to fix this, but using MOZ_COUNT_DTOR in a refcounted class is kind of a bug. So we could fix those, too.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.