Bug 1410209, part 3 - Use Runnable::mName for the class name with XPCOM leak checking. r=smaug draft
authorAndrew McCreight <continuation@gmail.com>
Wed, 25 Oct 2017 13:51:17 -0700
changeset 714058 c9b2a42254043923bfae38f6b4e1e1c5194d40f5
parent 714057 479326c3a67ea5125a993155c2039926631d256d
child 714059 053fcbe083e49499b37482fd67e4f05e4701ee0b
push id93832
push userbmo:continuation@gmail.com
push dateThu, 21 Dec 2017 16:57:49 +0000
reviewerssmaug
bugs1410209
milestone59.0a1
Bug 1410209, part 3 - Use Runnable::mName for the class name with XPCOM leak checking. r=smaug Most subclasses of Runnable don't bother to override AddRef and Release, so XPCOM leak checking ends up reporting Runnable, which makes it impossible to know what is actually leaking. Each subclass of Runnable is already required to pass in the name of the class, which is stored in the field mName. This patch changes Runnable to use mName as the class name for XPCOM leak checking, thus giving each subclass a specific name without needing to change the hundreds of existing subclasses of Runnable. The limitation of this approach is the classes that DO use NS_IMPL_ADDREF/RELEASE_INHERITED end up using the same class name that is used by the superclass AddRef/Release, but with a different size, which causes assertions in the leak checker. To work around this, I change NS_IMPL_ADDREF/RELEASE_INHERITED to not call into NS_LOG_ADDREF/RELEASE for classes that are a subclass of Runnable. This needs to use IsConvertible<> and not IsBaseOf<> because the latter requires the classes involved to be defined, and headers can use nsISupportsImpl.h without nsThreadUtils.h. MozReview-Commit-ID: H0pgvwQSZAE
xpcom/base/nsISupportsImpl.h
xpcom/base/nsTraceRefcnt.cpp
xpcom/threads/nsThreadUtils.cpp
--- a/xpcom/base/nsISupportsImpl.h
+++ b/xpcom/base/nsISupportsImpl.h
@@ -643,31 +643,40 @@ NS_INLINE_DECL_THREADSAFE_REFCOUNTING_ME
 public:                                                                       \
   NS_IMETHOD_(MozExternalRefCountType) AddRef(void) = 0;                      \
   NS_IMETHOD_(MozExternalRefCountType) Release(void) = 0;                     \
 public:
 
 /**
  * Use this macro to implement the AddRef method for a given <i>_class</i>
  * @param _class The name of the class implementing the method
+ * @param _name The class name to be passed to XPCOM leak checking
  */
-#define NS_IMPL_ADDREF(_class)                                                \
+#define NS_IMPL_NAMED_ADDREF(_class, _name)                                   \
 NS_IMETHODIMP_(MozExternalRefCountType) _class::AddRef(void)                  \
 {                                                                             \
   MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(_class)                                  \
   MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                        \
+  MOZ_ASSERT(_name != nullptr, "Must specify a name");                        \
   if (!mRefCnt.isThreadSafe)                                                  \
     NS_ASSERT_OWNINGTHREAD(_class);                                           \
   nsrefcnt count = ++mRefCnt;                                                 \
-  NS_LOG_ADDREF(this, count, #_class, sizeof(*this));                         \
+  NS_LOG_ADDREF(this, count, _name, sizeof(*this));                           \
   return count;                                                               \
 }
 
 /**
  * Use this macro to implement the AddRef method for a given <i>_class</i>
+ * @param _class The name of the class implementing the method
+ */
+#define NS_IMPL_ADDREF(_class)                                                \
+  NS_IMPL_NAMED_ADDREF(_class, #_class)
+
+/**
+ * Use this macro to implement the AddRef method for a given <i>_class</i>
  * implemented as a wholly owned aggregated object intended to implement
  * interface(s) for its owner
  * @param _class The name of the class implementing the method
  * @param _aggregator the owning/containing object
  */
 #define NS_IMPL_ADDREF_USING_AGGREGATOR(_class, _aggregator)                  \
 NS_IMETHODIMP_(MozExternalRefCountType) _class::AddRef(void)                  \
 {                                                                             \
@@ -675,63 +684,71 @@ NS_IMETHODIMP_(MozExternalRefCountType) 
   NS_PRECONDITION(_aggregator, "null aggregator");                            \
   return (_aggregator)->AddRef();                                             \
 }
 
 /**
  * Use this macro to implement the Release method for a given
  * <i>_class</i>.
  * @param _class The name of the class implementing the method
+ * @param _name The class name to be passed to XPCOM leak checking
  * @param _destroy A statement that is executed when the object's
  *   refcount drops to zero.
  *
  * For example,
  *
- *   NS_IMPL_RELEASE_WITH_DESTROY(Foo, Destroy(this))
+ *   NS_IMPL_RELEASE_WITH_DESTROY(Foo, "Foo", Destroy(this))
  *
  * will cause
  *
  *   Destroy(this);
  *
  * to be invoked when the object's refcount drops to zero. This
  * allows for arbitrary teardown activity to occur (e.g., deallocation
  * of object allocated with placement new).
  */
-#define NS_IMPL_RELEASE_WITH_DESTROY(_class, _destroy)                        \
+#define NS_IMPL_NAMED_RELEASE_WITH_DESTROY(_class, _name, _destroy)           \
 NS_IMETHODIMP_(MozExternalRefCountType) _class::Release(void)                 \
 {                                                                             \
   MOZ_ASSERT(int32_t(mRefCnt) > 0, "dup release");                            \
+  MOZ_ASSERT(_name != nullptr, "Must specify a name");                        \
   if (!mRefCnt.isThreadSafe)                                                  \
     NS_ASSERT_OWNINGTHREAD(_class);                                           \
   nsrefcnt count = --mRefCnt;                                                 \
-  NS_LOG_RELEASE(this, count, #_class);                                       \
+  NS_LOG_RELEASE(this, count, _name);                                         \
   if (count == 0) {                                                           \
     mRefCnt = 1; /* stabilize */                                              \
     _destroy;                                                                 \
     return 0;                                                                 \
   }                                                                           \
   return count;                                                               \
 }
 
+#define NS_IMPL_RELEASE_WITH_DESTROY(_class, _destroy)                        \
+  NS_IMPL_NAMED_RELEASE_WITH_DESTROY(_class, #_class, _destroy)
+
 /**
  * Use this macro to implement the Release method for a given <i>_class</i>
  * @param _class The name of the class implementing the method
  *
  * A note on the 'stabilization' of the refcnt to one. At that point,
  * the object's refcount will have gone to zero. The object's
  * destructor may trigger code that attempts to QueryInterface() and
  * Release() 'this' again. Doing so will temporarily increment and
  * decrement the refcount. (Only a logic error would make one try to
  * keep a permanent hold on 'this'.)  To prevent re-entering the
  * destructor, we make sure that no balanced refcounting can return
  * the refcount to |0|.
  */
 #define NS_IMPL_RELEASE(_class) \
   NS_IMPL_RELEASE_WITH_DESTROY(_class, delete (this))
 
+#define NS_IMPL_NAMED_RELEASE(_class, _name)                                  \
+  NS_IMPL_NAMED_RELEASE_WITH_DESTROY(_class, _name, delete (this))
+
 /**
  * Use this macro to implement the Release method for a given <i>_class</i>
  * implemented as a wholly owned aggregated object intended to implement
  * interface(s) for its owner
  * @param _class The name of the class implementing the method
  * @param _aggregator the owning/containing object
  */
 #define NS_IMPL_RELEASE_USING_AGGREGATOR(_class, _aggregator)                 \
@@ -1080,35 +1097,45 @@ public:                                 
   NS_IMETHOD QueryInterface(REFNSIID aIID,                                    \
                             void** aInstancePtr) override;                \
   NS_IMETHOD_(MozExternalRefCountType) AddRef(void) override;             \
   NS_IMETHOD_(MozExternalRefCountType) Release(void) override;            \
 
 /**
  * These macros can be used in conjunction with NS_DECL_ISUPPORTS_INHERITED
  * to implement the nsISupports methods, forwarding the invocations to a
- * superclass that already implements nsISupports.
+ * superclass that already implements nsISupports. Don't do anything for
+ * subclasses of Runnable because it deals with subclass logging in its own
+ * way, using the mName field.
  *
  * Note that I didn't make these inlined because they're virtual methods.
  */
 
+namespace mozilla {
+class Runnable;
+} // namespace mozilla
+
 #define NS_IMPL_ADDREF_INHERITED(Class, Super)                                \
 NS_IMETHODIMP_(MozExternalRefCountType) Class::AddRef(void)                   \
 {                                                                             \
   MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING(Class)                                   \
   nsrefcnt r = Super::AddRef();                                               \
-  NS_LOG_ADDREF(this, r, #Class, sizeof(*this));                              \
+  if (!mozilla::IsConvertible<Class*, mozilla::Runnable*>::value) {           \
+    NS_LOG_ADDREF(this, r, #Class, sizeof(*this));                            \
+  }                                                                           \
   return r;                                                                   \
 }
 
 #define NS_IMPL_RELEASE_INHERITED(Class, Super)                               \
 NS_IMETHODIMP_(MozExternalRefCountType) Class::Release(void)                  \
 {                                                                             \
   nsrefcnt r = Super::Release();                                              \
-  NS_LOG_RELEASE(this, r, #Class);                                            \
+  if (!mozilla::IsConvertible<Class*, mozilla::Runnable*>::value) {           \
+    NS_LOG_RELEASE(this, r, #Class);                                          \
+  }                                                                           \
   return r;                                                                   \
 }
 
 /**
  * As above but not logging the addref/release; needed if the base
  * class might be aggregated.
  */
 #define NS_IMPL_NONLOGGING_ADDREF_INHERITED(Class, Super)                     \
--- a/xpcom/base/nsTraceRefcnt.cpp
+++ b/xpcom/base/nsTraceRefcnt.cpp
@@ -428,17 +428,18 @@ GetBloatEntry(const char* aTypeName, uin
         entry = nullptr;
       }
     } else {
       MOZ_ASSERT(aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize,
                  "Mismatched sizes were recorded in the memory leak logging table. "
                  "The usual cause of this is having a templated class that uses "
                  "MOZ_COUNT_{C,D}TOR in the constructor or destructor, respectively. "
                  "As a workaround, the MOZ_COUNT_{C,D}TOR calls can be moved to a "
-                 "non-templated base class.");
+                 "non-templated base class. Another possible cause is a runnable with "
+                 "an mName that matches another refcounted class.");
     }
   }
   return entry;
 }
 
 static int
 DumpSerialNumbers(PLHashEntry* aHashEntry, int aIndex, void* aClosure)
 {
--- a/xpcom/threads/nsThreadUtils.cpp
+++ b/xpcom/threads/nsThreadUtils.cpp
@@ -35,17 +35,19 @@ NS_IMPL_ISUPPORTS(IdlePeriod, nsIIdlePer
 
 NS_IMETHODIMP
 IdlePeriod::GetIdlePeriodHint(TimeStamp* aIdleDeadline)
 {
   *aIdleDeadline = TimeStamp();
   return NS_OK;
 }
 
-NS_IMPL_ISUPPORTS(Runnable, nsIRunnable, nsINamed)
+NS_IMPL_NAMED_ADDREF(Runnable, mName)
+NS_IMPL_NAMED_RELEASE(Runnable, mName)
+NS_IMPL_QUERY_INTERFACE(Runnable, nsIRunnable, nsINamed)
 
 NS_IMETHODIMP
 Runnable::Run()
 {
   // Do nothing
   return NS_OK;
 }