From a6c96367a7d6cc5132e771e4b8fff2f4d6630137 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Tue, 14 Mar 2017 14:05:51 -0400 Subject: [PATCH] Bug 1347963 - part 1 - introduce mozilla::RecursiveMutex; r=erahm Having a proper recursively-acquirable mutex type makes intent clearer, and RecursiveMutex also happens to be somewhat faster than ReentrantMonitor. --- storage/test/gtest/test_deadlock_detector.cpp | 1 + xpcom/tests/gtest/TestDeadlockDetector.cpp | 26 ++++ xpcom/tests/gtest/TestRecursiveMutex.cpp | 25 ++++ xpcom/tests/gtest/TestSlicedInputStream.cpp | 1 + xpcom/tests/gtest/moz.build | 1 + xpcom/threads/BlockingResourceBase.cpp | 63 ++++++++- xpcom/threads/BlockingResourceBase.h | 2 +- xpcom/threads/RecursiveMutex.cpp | 89 +++++++++++++ xpcom/threads/RecursiveMutex.h | 124 ++++++++++++++++++ xpcom/threads/ReentrantMonitor.h | 1 - xpcom/threads/moz.build | 2 + 11 files changed, 332 insertions(+), 3 deletions(-) create mode 100644 xpcom/tests/gtest/TestRecursiveMutex.cpp create mode 100644 xpcom/threads/RecursiveMutex.cpp create mode 100644 xpcom/threads/RecursiveMutex.h diff --git a/storage/test/gtest/test_deadlock_detector.cpp b/storage/test/gtest/test_deadlock_detector.cpp index 5fbd3cb1d475..1f09285deaca 100644 --- a/storage/test/gtest/test_deadlock_detector.cpp +++ b/storage/test/gtest/test_deadlock_detector.cpp @@ -10,6 +10,7 @@ // duplication. #include "mozilla/CondVar.h" +#include "mozilla/RecursiveMutex.h" #include "mozilla/ReentrantMonitor.h" #include "SQLiteMutex.h" diff --git a/xpcom/tests/gtest/TestDeadlockDetector.cpp b/xpcom/tests/gtest/TestDeadlockDetector.cpp index 404a32b18499..f829817dfe03 100644 --- a/xpcom/tests/gtest/TestDeadlockDetector.cpp +++ b/xpcom/tests/gtest/TestDeadlockDetector.cpp @@ -12,6 +12,7 @@ #include "nsMemory.h" #include "mozilla/CondVar.h" +#include "mozilla/RecursiveMutex.h" #include "mozilla/ReentrantMonitor.h" #include "mozilla/Mutex.h" @@ -202,6 +203,31 @@ TEST_F(TESTNAME(DeadlockDetectorTest), TESTNAME(Sanity4DeathTest)) ASSERT_DEATH_IF_SUPPORTED(Sanity4_Child(), regex); } +int +Sanity5_Child() +{ + DisableCrashReporter(); + + mozilla::RecursiveMutex m1("dd.sanity4.m1"); + MUTEX m2("dd.sanity4.m2"); + m1.Lock(); + m2.Lock(); + m1.Lock(); + return 0; +} + +TEST_F(TESTNAME(DeadlockDetectorTest), TESTNAME(Sanity5DeathTest)) +{ + const char* const regex = + "Re-entering RecursiveMutex after acquiring other resources.*" + "###!!! ERROR: Potential deadlock detected.*" + "=== Cyclical dependency starts at.*--- RecursiveMutex : dd.sanity4.m1.*" + "--- Next dependency:.*--- Mutex : dd.sanity4.m2.*" + "=== Cycle completed at.*--- RecursiveMutex : dd.sanity4.m1.*" + "###!!! ASSERTION: Potential deadlock detected.*"; + ASSERT_DEATH_IF_SUPPORTED(Sanity5_Child(), regex); +} + //----------------------------------------------------------------------------- // Multithreaded tests diff --git a/xpcom/tests/gtest/TestRecursiveMutex.cpp b/xpcom/tests/gtest/TestRecursiveMutex.cpp new file mode 100644 index 000000000000..3f0557aed312 --- /dev/null +++ b/xpcom/tests/gtest/TestRecursiveMutex.cpp @@ -0,0 +1,25 @@ +/* -*- 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 "nsThreadUtils.h" +#include "mozilla/RecursiveMutex.h" +#include "gtest/gtest.h" + +using mozilla::RecursiveMutex; +using mozilla::RecursiveMutexAutoLock; + +// Basic test to make sure the underlying implementation of RecursiveMutex is, +// well, actually recursively acquirable. + +TEST(RecursiveMutex, SmokeTest) +{ + RecursiveMutex mutex("testing mutex"); + + RecursiveMutexAutoLock lock1(mutex); + RecursiveMutexAutoLock lock2(mutex); + + //...and done. +} diff --git a/xpcom/tests/gtest/TestSlicedInputStream.cpp b/xpcom/tests/gtest/TestSlicedInputStream.cpp index 668e7b933637..3279758e535b 100644 --- a/xpcom/tests/gtest/TestSlicedInputStream.cpp +++ b/xpcom/tests/gtest/TestSlicedInputStream.cpp @@ -2,6 +2,7 @@ #include "nsCOMPtr.h" #include "nsIInputStream.h" +#include "nsIPipe.h" #include "nsStreamUtils.h" #include "nsString.h" #include "nsStringStream.h" diff --git a/xpcom/tests/gtest/moz.build b/xpcom/tests/gtest/moz.build index db091b5a7a98..6010328a785f 100644 --- a/xpcom/tests/gtest/moz.build +++ b/xpcom/tests/gtest/moz.build @@ -32,6 +32,7 @@ UNIFIED_SOURCES += [ 'TestPLDHash.cpp', 'TestPriorityQueue.cpp', 'TestRacingServiceManager.cpp', + 'TestRecursiveMutex.cpp', 'TestRWLock.cpp', 'TestSlicedInputStream.cpp', 'TestSnappyStreams.cpp', diff --git a/xpcom/threads/BlockingResourceBase.cpp b/xpcom/threads/BlockingResourceBase.cpp index ff4c83abcb26..88b1ce1b773b 100644 --- a/xpcom/threads/BlockingResourceBase.cpp +++ b/xpcom/threads/BlockingResourceBase.cpp @@ -20,6 +20,7 @@ #include "mozilla/CondVar.h" #include "mozilla/DeadlockDetector.h" +#include "mozilla/RecursiveMutex.h" #include "mozilla/ReentrantMonitor.h" #include "mozilla/Mutex.h" #include "mozilla/RWLock.h" @@ -38,7 +39,7 @@ namespace mozilla { // static members const char* const BlockingResourceBase::kResourceTypeName[] = { // needs to be kept in sync with BlockingResourceType - "Mutex", "ReentrantMonitor", "CondVar" + "Mutex", "ReentrantMonitor", "CondVar", "RecursiveMutex" }; #ifdef DEBUG @@ -527,6 +528,66 @@ ReentrantMonitor::Wait(PRIntervalTime aInterval) } +// +// Debug implementation of RecursiveMutex +void +RecursiveMutex::Lock() +{ + BlockingResourceBase* chainFront = ResourceChainFront(); + + // the code below implements mutex reentrancy semantics + + if (this == chainFront) { + // immediately re-entered the mutex: acceptable + LockInternal(); + ++mEntryCount; + return; + } + + // this is sort of a hack around not recording the thread that + // owns this monitor + if (chainFront) { + for (BlockingResourceBase* br = ResourceChainPrev(chainFront); + br; + br = ResourceChainPrev(br)) { + if (br == this) { + NS_WARNING( + "Re-entering RecursiveMutex after acquiring other resources."); + + // show the caller why this is potentially bad + CheckAcquire(); + + LockInternal(); + ++mEntryCount; + return; + } + } + } + + CheckAcquire(); + LockInternal(); + NS_ASSERTION(mEntryCount == 0, "RecursiveMutex isn't free!"); + Acquire(); // protected by us + mOwningThread = PR_GetCurrentThread(); + mEntryCount = 1; +} + +void +RecursiveMutex::Unlock() +{ + if (--mEntryCount == 0) { + Release(); // protected by us + mOwningThread = nullptr; + } + UnlockInternal(); +} + +void +RecursiveMutex::AssertCurrentThreadIn() +{ + MOZ_ASSERT(IsAcquired() && mOwningThread == PR_GetCurrentThread()); +} + // // Debug implementation of CondVar nsresult diff --git a/xpcom/threads/BlockingResourceBase.h b/xpcom/threads/BlockingResourceBase.h index d70fbcbbf38c..5c8a08524286 100644 --- a/xpcom/threads/BlockingResourceBase.h +++ b/xpcom/threads/BlockingResourceBase.h @@ -50,7 +50,7 @@ class BlockingResourceBase { public: // Needs to be kept in sync with kResourceTypeNames. - enum BlockingResourceType { eMutex, eReentrantMonitor, eCondVar }; + enum BlockingResourceType { eMutex, eReentrantMonitor, eCondVar, eRecursiveMutex }; /** * kResourceTypeName diff --git a/xpcom/threads/RecursiveMutex.cpp b/xpcom/threads/RecursiveMutex.cpp new file mode 100644 index 000000000000..97f5b28d92ee --- /dev/null +++ b/xpcom/threads/RecursiveMutex.cpp @@ -0,0 +1,89 @@ +/* -*- 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 "mozilla/RecursiveMutex.h" + +#ifdef XP_WIN +#include + +#define NativeHandle(m) (reinterpret_cast(&m)) +#endif + +namespace mozilla { + +RecursiveMutex::RecursiveMutex(const char* aName MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL) + : BlockingResourceBase(aName, eRecursiveMutex) +#ifdef DEBUG + , mOwningThread(nullptr) + , mEntryCount(0) +#endif +{ + MOZ_GUARD_OBJECT_NOTIFIER_INIT; +#ifdef XP_WIN + // This number was adapted from NSPR. + static const DWORD sLockSpinCount = 100; + +#if defined(RELEASE_OR_BETA) + // Vista and later automatically allocate and subsequently leak a debug info + // object for each critical section that we allocate unless we tell the + // system not to do that. + DWORD flags = CRITICAL_SECTION_NO_DEBUG_INFO; +#else + DWORD flags = 0; +#endif + BOOL r = InitializeCriticalSectionEx(NativeHandle(mMutex), + sLockSpinCount, flags); + MOZ_RELEASE_ASSERT(r); +#else + pthread_mutexattr_t attr; + + MOZ_RELEASE_ASSERT(pthread_mutexattr_init(&attr) == 0, + "pthread_mutexattr_init failed"); + + MOZ_RELEASE_ASSERT(pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) == 0, + "pthread_mutexattr_settype failed"); + + MOZ_RELEASE_ASSERT(pthread_mutex_init(&mMutex, &attr) == 0, + "pthread_mutex_init failed"); + + MOZ_RELEASE_ASSERT(pthread_mutexattr_destroy(&attr) == 0, + "pthread_mutexattr_destroy failed"); +#endif +} + +RecursiveMutex::~RecursiveMutex() +{ +#ifdef XP_WIN + DeleteCriticalSection(NativeHandle(mMutex)); +#else + MOZ_RELEASE_ASSERT(pthread_mutex_destroy(&mMutex) == 0, + "pthread_mutex_destroy failed"); +#endif +} + +void +RecursiveMutex::LockInternal() +{ +#ifdef XP_WIN + EnterCriticalSection(NativeHandle(mMutex)); +#else + MOZ_RELEASE_ASSERT(pthread_mutex_lock(&mMutex) == 0, + "pthread_mutex_lock failed"); +#endif +} + +void +RecursiveMutex::UnlockInternal() +{ +#ifdef XP_WIN + LeaveCriticalSection(NativeHandle(mMutex)); +#else + MOZ_RELEASE_ASSERT(pthread_mutex_unlock(&mMutex) == 0, + "pthread_mutex_unlock failed"); +#endif +} + +} // namespace mozilla diff --git a/xpcom/threads/RecursiveMutex.h b/xpcom/threads/RecursiveMutex.h new file mode 100644 index 000000000000..e1b28fafb580 --- /dev/null +++ b/xpcom/threads/RecursiveMutex.h @@ -0,0 +1,124 @@ + +/* -*- 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/. */ + +// A lock that can be acquired multiple times on the same thread. + +#ifndef mozilla_RecursiveMutex_h +#define mozilla_RecursiveMutex_h + +#include "mozilla/BlockingResourceBase.h" +#include "mozilla/GuardObjects.h" + +#ifndef XP_WIN +#include +#endif + +namespace mozilla { + +class RecursiveMutex : public BlockingResourceBase +{ +public: + explicit RecursiveMutex(const char* aName MOZ_GUARD_OBJECT_NOTIFIER_PARAM); + ~RecursiveMutex(); + +#ifdef DEBUG + void Lock(); + void Unlock(); +#else + void Lock() { LockInternal(); } + void Unlock() { UnlockInternal(); } +#endif + + void AssertCurrentThreadIn() +#ifdef DEBUG + ; +#else + { + } +#endif + +private: + RecursiveMutex() = delete; + RecursiveMutex(const RecursiveMutex&) = delete; + RecursiveMutex& operator=(const RecursiveMutex&) = delete; + + void LockInternal(); + void UnlockInternal(); + +#ifdef DEBUG + PRThread* mOwningThread; + size_t mEntryCount; +#endif + +#if !defined(XP_WIN) + pthread_mutex_t mMutex; +#else + // We eschew including windows.h and using CRITICAL_SECTION here so that files + // including us don't also pull in windows.h. Just use a type that's big + // enough for CRITICAL_SECTION, and we'll fix it up later. + void* mMutex[6]; +#endif + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER +}; + +class MOZ_RAII RecursiveMutexAutoLock +{ +public: + explicit RecursiveMutexAutoLock(RecursiveMutex& aRecursiveMutex + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : mRecursiveMutex(&aRecursiveMutex) + { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + NS_ASSERTION(mRecursiveMutex, "null mutex"); + mRecursiveMutex->Lock(); + } + + ~RecursiveMutexAutoLock(void) + { + mRecursiveMutex->Unlock(); + } + +private: + RecursiveMutexAutoLock() = delete; + RecursiveMutexAutoLock(const RecursiveMutexAutoLock&) = delete; + RecursiveMutexAutoLock& operator=(const RecursiveMutexAutoLock&) = delete; + static void* operator new(size_t) CPP_THROW_NEW; + + mozilla::RecursiveMutex* mRecursiveMutex; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER +}; + +class MOZ_RAII RecursiveMutexAutoUnlock +{ +public: + explicit RecursiveMutexAutoUnlock(RecursiveMutex& aRecursiveMutex + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) + : mRecursiveMutex(&aRecursiveMutex) + { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + NS_ASSERTION(mRecursiveMutex, "null mutex"); + mRecursiveMutex->Unlock(); + } + + ~RecursiveMutexAutoUnlock(void) + { + mRecursiveMutex->Lock(); + } + +private: + RecursiveMutexAutoUnlock() = delete; + RecursiveMutexAutoUnlock(const RecursiveMutexAutoUnlock&) = delete; + RecursiveMutexAutoUnlock& operator=(const RecursiveMutexAutoUnlock&) = delete; + static void* operator new(size_t) CPP_THROW_NEW; + + mozilla::RecursiveMutex* mRecursiveMutex; + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER +}; + +} // namespace mozilla + +#endif // mozilla_RecursiveMutex_h diff --git a/xpcom/threads/ReentrantMonitor.h b/xpcom/threads/ReentrantMonitor.h index dfc6fe2520a6..75661a1a1f27 100644 --- a/xpcom/threads/ReentrantMonitor.h +++ b/xpcom/threads/ReentrantMonitor.h @@ -256,5 +256,4 @@ private: } // namespace mozilla - #endif // ifndef mozilla_ReentrantMonitor_h diff --git a/xpcom/threads/moz.build b/xpcom/threads/moz.build index e231da3f52ad..3ded2058980e 100644 --- a/xpcom/threads/moz.build +++ b/xpcom/threads/moz.build @@ -49,6 +49,7 @@ EXPORTS.mozilla += [ 'Monitor.h', 'MozPromise.h', 'Mutex.h', + 'RecursiveMutex.h', 'ReentrantMonitor.h', 'RWLock.h', 'SchedulerGroup.h', @@ -82,6 +83,7 @@ UNIFIED_SOURCES += [ 'nsThreadPool.cpp', 'nsThreadUtils.cpp', 'nsTimerImpl.cpp', + 'RecursiveMutex.cpp', 'RWLock.cpp', 'SchedulerGroup.cpp', 'SharedThreadPool.cpp',