Bug 1457501 - Part 1 - Mac Crash deadlock triggered by CrashReporter::GetFlatThreadAnnotation() lock acquisition r?gsvelto
MozReview-Commit-ID: BxIUqco6oiV
--- 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,