Bug 1410209, part 3 - Use Runnable::mName for the class name with XPCOM leak checking. r=smaug
authorAndrew McCreight <continuation@gmail.com>
Wed, 25 Oct 2017 13:51:17 -0700
changeset 448934 cc72b46a44f177cbf4d647e4e8435ad81f036e50
parent 448933 4b7fb8a3e422b2bfee39e3f6672ba9a65f209039
child 448935 649873f16998ab8ab7ce96d560d04d44be90dd57
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1410209
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;
 }