Bug 1359415 - move threadsafety checks inside nsAutoOwningThread; r=erahm
authorNathan Froyd <froydnj@mozilla.com>
Wed, 26 Apr 2017 11:41:32 -0400
changeset 389207 8457e1358dda18f6e8d3ab787f306314668e5fb9
parent 389206 57624ab56af4ae9305d9d5db5605dccbf012c104
child 389208 5590452f092c5cbd29054f024094031ae7df4971
push id51
push userfmarier@mozilla.com
push dateFri, 05 May 2017 18:41:45 +0000
reviewerserahm
bugs1359415
milestone55.0a1
Bug 1359415 - move threadsafety checks inside nsAutoOwningThread; r=erahm This change moves most of the logic for the threadsafety check into nsAutoOwningThread, rather than having part of the logic live in nsAutoOwningThread and part of the logic live in nsDebug.h. Changing this also forces us to clean up a couple of places that replicated the logic that lived in nsDebug.h as well.
image/imgLoader.h
ipc/chromium/src/chrome/common/ipc_channel_win.cc
xpcom/base/nsDebug.h
xpcom/base/nsISupportsImpl.cpp
xpcom/base/nsISupportsImpl.h
xpcom/base/nsWeakReference.cpp
--- a/image/imgLoader.h
+++ b/image/imgLoader.h
@@ -43,28 +43,26 @@ class imgCacheEntry
 public:
   imgCacheEntry(imgLoader* loader, imgRequest* request,
                 bool aForcePrincipalCheck);
   ~imgCacheEntry();
 
   nsrefcnt AddRef()
   {
     NS_PRECONDITION(int32_t(mRefCnt) >= 0, "illegal refcnt");
-    MOZ_ASSERT(_mOwningThread.GetThread() == PR_GetCurrentThread(),
-      "imgCacheEntry addref isn't thread-safe!");
+    NS_ASSERT_OWNINGTHREAD(imgCacheEntry);
     ++mRefCnt;
     NS_LOG_ADDREF(this, mRefCnt, "imgCacheEntry", sizeof(*this));
     return mRefCnt;
   }
 
   nsrefcnt Release()
   {
     NS_PRECONDITION(0 != mRefCnt, "dup release");
-    MOZ_ASSERT(_mOwningThread.GetThread() == PR_GetCurrentThread(),
-      "imgCacheEntry release isn't thread-safe!");
+    NS_ASSERT_OWNINGTHREAD(imgCacheEntry);
     --mRefCnt;
     NS_LOG_RELEASE(this, mRefCnt, "imgCacheEntry");
     if (mRefCnt == 0) {
       mRefCnt = 1; /* stabilize */
       delete this;
       return 0;
     }
     return mRefCnt;
--- a/ipc/chromium/src/chrome/common/ipc_channel_win.cc
+++ b/ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ -19,17 +19,17 @@
 #include "mozilla/ipc/ProtocolUtils.h"
 
 // ChannelImpl is used on the IPC thread, but constructed on a different thread,
 // so it has to hold the nsAutoOwningThread as a pointer, and we need a slightly
 // different macro.
 #ifdef DEBUG
 #define ASSERT_OWNINGTHREAD(_class) \
   if (nsAutoOwningThread* owningThread = _mOwningThread.get()) {               \
-    NS_CheckThreadSafe(owningThread->GetThread(), #_class " not thread-safe"); \
+    owningThread->AssertOwnership(#_class " not thread-safe"); \
   }
 #else
 #define ASSERT_OWNINGTHREAD(_class) ((void)0)
 #endif
 
 namespace IPC {
 //------------------------------------------------------------------------------
 
--- a/xpcom/base/nsDebug.h
+++ b/xpcom/base/nsDebug.h
@@ -358,25 +358,16 @@ inline void MOZ_PretendNoReturn()
   NS_ENSURE_FALSE(outer, NS_ERROR_NO_AGGREGATION)
 
 /*****************************************************************************/
 
 #if (defined(DEBUG) || (defined(NIGHTLY_BUILD) && !defined(MOZ_PROFILING))) && !defined(XPCOM_GLUE_AVOID_NSPR)
   #define MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED  1
 #endif
 
-#ifdef XPCOM_GLUE
-  #define NS_CheckThreadSafe(owningThread, msg)
-#else
-  #define NS_CheckThreadSafe(owningThread, msg)                 \
-    if (MOZ_UNLIKELY(owningThread != PR_GetCurrentThread())) {  \
-      MOZ_CRASH(msg);                                           \
-    }
-#endif
-
 #ifdef MOZILLA_INTERNAL_API
 void NS_ABORT_OOM(size_t aSize);
 #else
 inline void NS_ABORT_OOM(size_t)
 {
   MOZ_CRASH();
 }
 #endif
--- a/xpcom/base/nsISupportsImpl.cpp
+++ b/xpcom/base/nsISupportsImpl.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* vim: set ts=8 sts=2 et sw=2 tw=80: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsISupportsImpl.h"
+#include "mozilla/Assertions.h"
 
 nsresult NS_FASTCALL
 NS_TableDrivenQI(void* aThis, REFNSIID aIID, void** aInstancePtr,
                  const QITableEntry* aEntries)
 {
   do {
     if (aIID.Equals(*aEntries->iid)) {
       nsISupports* r = reinterpret_cast<nsISupports*>(
@@ -20,8 +21,19 @@ NS_TableDrivenQI(void* aThis, REFNSIID a
     }
 
     ++aEntries;
   } while (aEntries->iid);
 
   *aInstancePtr = nullptr;
   return NS_ERROR_NO_INTERFACE;
 }
+
+#ifdef MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
+void
+nsAutoOwningThread::AssertCurrentThreadOwnsMe(const char* msg) const
+{
+  if (MOZ_UNLIKELY(mThread != PR_GetCurrentThread())) {
+    // `msg` is a string literal by construction.
+    MOZ_CRASH_UNSAFE_OOL(msg);
+  }
+}
+#endif
--- a/xpcom/base/nsISupportsImpl.h
+++ b/xpcom/base/nsISupportsImpl.h
@@ -8,20 +8,19 @@
 
 #ifndef nsISupportsImpl_h__
 #define nsISupportsImpl_h__
 
 #include "nscore.h"
 #include "nsISupportsBase.h"
 #include "nsISupportsUtils.h"
 
-
 #if !defined(XPCOM_GLUE_AVOID_NSPR)
-#include "prthread.h" /* needed for thread-safety checks */
-#endif // !XPCOM_GLUE_AVOID_NSPR
+#include "prthread.h" /* needed for cargo-culting headers */
+#endif
 
 #include "nsDebug.h"
 #include "nsXPCOM.h"
 #include <atomic>
 #include "mozilla/Attributes.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Compiler.h"
 #include "mozilla/Likely.h"
@@ -46,37 +45,52 @@ ToCanonicalSupports(nsISupports* aSuppor
   return nullptr;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Macros to help detect thread-safety:
 
 #ifdef MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
 
+#include "prthread.h" /* needed for thread-safety checks */
+
 class nsAutoOwningThread
 {
 public:
   nsAutoOwningThread() { mThread = PR_GetCurrentThread(); }
-  void* GetThread() const { return mThread; }
+
+  // We move the actual assertion checks out-of-line to minimize code bloat,
+  // but that means we have to pass a non-literal string to
+  // MOZ_CRASH_UNSAFE_OOL.  To make that more safe, the public interface
+  // requires a literal string and passes that to the private interface; we
+  // can then be assured that we effectively are passing a literal string
+  // to MOZ_CRASH_UNSAFE_OOL.
+  template<int N>
+  void AssertOwnership(const char (&aMsg)[N]) const
+  {
+    AssertCurrentThreadOwnsMe(aMsg);
+  }
 
 private:
+  void AssertCurrentThreadOwnsMe(const char* aMsg) const;
+
   void* mThread;
 };
 
 #define NS_DECL_OWNINGTHREAD            nsAutoOwningThread _mOwningThread;
 #define NS_ASSERT_OWNINGTHREAD_AGGREGATE(agg, _class) \
-  NS_CheckThreadSafe(agg->_mOwningThread.GetThread(), #_class " not thread-safe")
+  agg->_mOwningThread.AssertOwnership(#_class " not thread-safe")
 #define NS_ASSERT_OWNINGTHREAD(_class) NS_ASSERT_OWNINGTHREAD_AGGREGATE(this, _class)
-#else // !DEBUG && !(NIGHTLY_BUILD && !MOZ_PROFILING)
+#else // !MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
 
 #define NS_DECL_OWNINGTHREAD            /* nothing */
 #define NS_ASSERT_OWNINGTHREAD_AGGREGATE(agg, _class) ((void)0)
 #define NS_ASSERT_OWNINGTHREAD(_class)  ((void)0)
 
-#endif // DEBUG || (NIGHTLY_BUILD && !MOZ_PROFILING)
+#endif // MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
 
 
 // Macros for reference-count and constructor logging
 
 #if defined(NS_BUILD_REFCNT_LOGGING)
 
 #define NS_LOG_ADDREF(_p, _rc, _type, _size) \
   NS_LogAddRef((_p), (_rc), (_type), (uint32_t) (_size))
--- a/xpcom/base/nsWeakReference.cpp
+++ b/xpcom/base/nsWeakReference.cpp
@@ -11,19 +11,19 @@
 #include "nsWeakReference.h"
 #include "nsCOMPtr.h"
 #include "nsDebug.h"
 
 #ifdef MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
 
 #define MOZ_WEAKREF_DECL_OWNINGTHREAD nsAutoOwningThread _mWeakRefOwningThread;
 #define MOZ_WEAKREF_ASSERT_OWNINGTHREAD \
-  NS_CheckThreadSafe(_mWeakRefOwningThread.GetThread(), "nsWeakReference not thread-safe")
+  _mWeakRefOwningThread.AssertOwnership("nsWeakReference not thread-safe")
 #define MOZ_WEAKREF_ASSERT_OWNINGTHREAD_DELEGATED(that) \
-  NS_CheckThreadSafe((that)->_mWeakRefOwningThread.GetThread(), "nsWeakReference not thread-safe")
+  (that)->_mWeakRefOwningThread.AssertOwnership("nsWeakReference not thread-safe")
 
 #else
 
 #define MOZ_WEAKREF_DECL_OWNINGTHREAD
 #define MOZ_WEAKREF_ASSERT_OWNINGTHREAD do { } while (false)
 #define MOZ_WEAKREF_ASSERT_OWNINGTHREAD_DELEGATED(that) do { } while (false)
 
 #endif