Bug 1457501 - Part 1 - Mac Crash deadlock triggered by CrashReporter::GetFlatThreadAnnotation() lock acquisition r?gsvelto draft
authorHaik Aftandilian <haftandilian@mozilla.com>
Tue, 24 Apr 2018 15:59:15 -0700
changeset 790019 6209668714f93ca64b3e068be660366c57c9c095
parent 789147 6ae02bea5291ace205a5a9364b3a092949a6dc54
child 790020 0f57f707dbf328f035d297bff32d7648a10a3f30
push id108390
push userhaftandilian@mozilla.com
push dateMon, 30 Apr 2018 23:54:57 +0000
reviewersgsvelto
bugs1457501
milestone61.0a1
Bug 1457501 - Part 1 - Mac Crash deadlock triggered by CrashReporter::GetFlatThreadAnnotation() lock acquisition r?gsvelto MozReview-Commit-ID: BxIUqco6oiV
toolkit/crashreporter/ThreadAnnotation.cpp
toolkit/crashreporter/ThreadAnnotation.h
toolkit/crashreporter/nsExceptionHandler.cpp
--- a/toolkit/crashreporter/ThreadAnnotation.cpp
+++ b/toolkit/crashreporter/ThreadAnnotation.cpp
@@ -20,18 +20,71 @@
 using mozilla::StaticMutex;
 using mozilla::StaticMutexAutoLock;
 using mozilla::UniquePtr;
 
 namespace CrashReporter {
 
 namespace {
 
+#ifdef XP_MACOSX
+/*
+ * On the Mac, exception handler callbacks are invoked in a context where all
+ * other threads are paused. As a result, attempting to acquire a mutex is
+ * problematic because 1) the mutex may be held by another thread which is
+ * now suspended and 2) acquiring an unheld mutex can trigger memory allocation
+ * which generally requires allocator locks. This class is a wrapper around a
+ * StaticMutex, providing an IsLocked() method which only makes sense to use
+ * in the Mac exception handling context when other threads are paused.
+ */
+class MacCrashReporterLock
+{
+public:
+  void Lock()
+  {
+    sInnerMutex.Lock();
+    sIsLocked = true;
+  }
+  void Unlock()
+  {
+    sIsLocked = false;
+    sInnerMutex.Unlock();
+  }
+  /*
+   * Returns true if the lock is held at the time the method is called.
+   * The return value is out-of-date by the time this method returns unless
+   * we have a guarantee that other threads are not running such as in Mac
+   * breadkpad exception handler context.
+   */
+  bool IsLocked()
+  {
+    return sIsLocked;
+  }
+  void AssertCurrentThreadOwns()
+  {
+    sInnerMutex.AssertCurrentThreadOwns();
+  }
+private:
+  static StaticMutex sInnerMutex;
+  static bool sIsLocked;
+};
+StaticMutex MacCrashReporterLock::sInnerMutex;
+bool MacCrashReporterLock::sIsLocked;
+
+// Use MacCrashReporterLock for locking
+typedef mozilla::BaseAutoLock<MacCrashReporterLock> CrashReporterAutoLock;
+typedef MacCrashReporterLock CrashReporterLockType;
+#else /* !XP_MACOSX */
+// Use StaticMutex for locking
+typedef StaticMutexAutoLock CrashReporterAutoLock;
+typedef StaticMutex CrashReporterLockType;
+#endif /* XP_MACOSX */
+
 // Protects access to sInitialized and sThreadAnnotations.
-static StaticMutex sMutex;
+static CrashReporterLockType sMutex;
 
 class ThreadAnnotationSpan {
 public:
   ThreadAnnotationSpan(uint32_t aBegin, uint32_t aEnd)
     : mBegin(aBegin)
     , mEnd(aEnd)
   {
     MOZ_ASSERT(mBegin < mEnd);
@@ -150,31 +203,31 @@ template<typename T>
 class DeleteWithLock
 {
 public:
   constexpr DeleteWithLock() {}
 
   void operator()(T* aPtr) const
   {
     static_assert(sizeof(T) > 0, "T must be complete");
-    StaticMutexAutoLock lock(sMutex);
+    CrashReporterAutoLock lock(sMutex);
 
     delete aPtr;
   }
 };
 
 static bool sInitialized = false;
 static UniquePtr<ThreadAnnotationData> sThreadAnnotations;
 
 static unsigned sTLSThreadInfoKey = (unsigned) -1;
 void ThreadLocalDestructor(void* aUserData)
 {
   MOZ_ASSERT(aUserData);
 
-  StaticMutexAutoLock lock(sMutex);
+  CrashReporterAutoLock lock(sMutex);
 
   ThreadAnnotationSpan* aThreadInfo =
     static_cast<ThreadAnnotationSpan*>(aUserData);
   delete aThreadInfo;
 }
 
 // This is called on thread termination.
 ThreadAnnotationSpan::~ThreadAnnotationSpan()
@@ -187,17 +240,17 @@ ThreadAnnotationSpan::~ThreadAnnotationS
     sThreadAnnotations->EraseThreadAnnotation(*this);
   }
 }
 
 } // Anonymous namespace.
 
 void InitThreadAnnotation()
 {
-  StaticMutexAutoLock lock(sMutex);
+  CrashReporterAutoLock lock(sMutex);
 
   if (sInitialized) {
     return;
   }
 
   PRStatus status = PR_NewThreadPrivateIndex(&sTLSThreadInfoKey,
                                              &ThreadLocalDestructor);
   if (status == PR_FAILURE) {
@@ -212,42 +265,68 @@ void InitThreadAnnotation()
 void SetCurrentThreadName(const char* aName)
 {
   if (PR_GetThreadPrivate(sTLSThreadInfoKey)) {
     // Explicitly set TLS value to null (and call the dtor function ) before
     // acquiring sMutex to avoid reentrant deadlock.
     PR_SetThreadPrivate(sTLSThreadInfoKey, nullptr);
   }
 
-  StaticMutexAutoLock lock(sMutex);
+  CrashReporterAutoLock lock(sMutex);
 
   if (!sInitialized) {
     return;
   }
 
   ThreadAnnotationSpan* threadInfo =
     sThreadAnnotations->AddThreadAnnotation(CurrentThreadId(),
                                             aName);
   // This may destroy the old insatnce.
   PR_SetThreadPrivate(sTLSThreadInfoKey, threadInfo);
 }
 
-void GetFlatThreadAnnotation(const std::function<void(const char*)>& aCallback)
+void GetFlatThreadAnnotation(const std::function<void(const char*)>& aCallback,
+                             bool aIsHandlingException)
 {
-  StaticMutexAutoLock lock(sMutex);
+  bool lockNeeded = true;
+
+#ifdef XP_MACOSX
+  if (aIsHandlingException) {
+    // Don't acquire the lock on Mac because we are
+    // executing in exception context where all other
+    // threads are paused. If the lock is held, skip
+    // thread annotations to avoid deadlock caused by
+    // waiting for a suspended thread. If the lock
+    // isn't held, acquiring it serves no purpose and
+    // can trigger memory allocations.
+    if (sMutex.IsLocked()) {
+      aCallback("");
+      return;
+    }
+    lockNeeded = false;
+  }
+#endif
+
+  if (lockNeeded) {
+    sMutex.Lock();
+  }
 
   if (sThreadAnnotations) {
     sThreadAnnotations->GetData(aCallback);
   } else {
     // Maybe already shutdown: call aCallback with empty annotation data.
     aCallback("");
   }
+
+  if (lockNeeded) {
+    sMutex.Unlock();
+  }
 }
 
 void ShutdownThreadAnnotation()
 {
-  StaticMutexAutoLock lock(sMutex);
+  CrashReporterAutoLock lock(sMutex);
 
   sInitialized = false;
   sThreadAnnotations.reset();
 }
 
 }
--- a/toolkit/crashreporter/ThreadAnnotation.h
+++ b/toolkit/crashreporter/ThreadAnnotation.h
@@ -11,17 +11,18 @@
 
 // Thread annotation interfaces for the crash reporter.
 namespace CrashReporter {
 
 void InitThreadAnnotation();
 
 void ShutdownThreadAnnotation();
 
-void GetFlatThreadAnnotation(const std::function<void(const char*)>& aCallback);
+void GetFlatThreadAnnotation(const std::function<void(const char*)>& aCallback,
+                             bool aIsHandlingException=false);
 
 class InitThreadAnnotationRAII {
 public:
   InitThreadAnnotationRAII()
   {
     if (GetEnabled()) {
       InitThreadAnnotation();
     }
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@@ -1106,17 +1106,17 @@ MinidumpCallback(
         WriteLiteral(apiData, "ThreadIdNameMapping=");
         WriteLiteral(eventFile, "ThreadIdNameMapping=");
         WriteString(apiData, aAnnotation);
         WriteString(eventFile, aAnnotation);
         WriteLiteral(apiData, "\n");
         WriteLiteral(eventFile, "\n");
       }
     };
-    GetFlatThreadAnnotation(getThreadAnnotationCB);
+    GetFlatThreadAnnotation(getThreadAnnotationCB, false);
   }
 
   if (!doReport) {
 #ifdef XP_WIN
     TerminateProcess(GetCurrentProcess(), 1);
 #endif // XP_WIN
     return returnValue;
   }
@@ -1292,17 +1292,17 @@ PrepareChildExceptionTimeAnnotations(voi
   std::function<void(const char*)> getThreadAnnotationCB =
     [&] (const char * aAnnotation) -> void {
     if (aAnnotation) {
       WriteLiteral(apiData, "ThreadIdNameMapping=");
       WriteString(apiData, aAnnotation);
       WriteLiteral(apiData, "\n");
     }
   };
-  GetFlatThreadAnnotation(getThreadAnnotationCB);
+  GetFlatThreadAnnotation(getThreadAnnotationCB, true);
 }
 
 #ifdef XP_WIN
 static void
 ReserveBreakpadVM()
 {
   if (!gBreakpadReservedVM) {
     gBreakpadReservedVM = VirtualAlloc(nullptr, kReserveSize, MEM_RESERVE,