[oilpan]: fix deadlock when leaving a safepoint and sweeping.

[oilpan]: fix deadlock when leaving a safepoint and sweeping.

In the case where a thread takes a lock/mutex within a safepoint scope and leaves the safepoint with the lock held we risk deadlocking if the same thread takes the same lock in a destructor. The reason is that if another thread did a GC while the first thread was in the safepoint the sweep done by the first thread will potentially call a destructor which could then take the same lock. Since the WTF:Mutex is not recursive/reentrant the thread will deadlock on itself.
This change fixes this by introducing a RecursiveMutex which allows a thread to take a lock it is already holding. Interestingly the Windows WTF:Mutex is currently reentrant wrt. the lock() method, but not wrt. tryLock() since tryLock mimics the behavior of the pthread_mutex by keeping a recursion count. To deal with this I introduce a separate tryLock method for the Mutex and the RecursiveMutex that behaves different on windows. On pthreads (Linux/Mac) the tryLock method is identical since it changes behavior depending on how the mutex is created.

I have added a test which demonstrates the dead-lock issue.

R=ager@chromium.org, erik.corry@gmail.com, haraken@chromium.org, oilpan-reviews@chromium.org, sigbjornf@opera.com, tkent@chromium.org, zerny@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178928

[sof, 2014-07-24 21:27:05]

The recursive/re-entrant implementations looks correct to me; is there a mutex
maven that it'd be good to involve here?

What (and where) went actually wrong? The description covers the general
problem, but what were you running into?

[haraken, 2014-07-25 01:54:41]

Looks good, but I also failed in finding a place where guidMutex() is acquired
in a destructor. Is closeDatabase() called by some destructor?

[tkent, 2014-07-25 05:11:45]

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:621: explicit
SafePointAwareMutexLocker(WTF::MutexBase& mutex, ThreadState::StackState
stackState = ThreadState::HeapPointersOnStack)
We should omit WTF::

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:664: WTF::MutexBase& m_mutex;
Ditto.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
File Source/wtf/ThreadingPrimitives.h (right):

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
Source/wtf/ThreadingPrimitives.h:86: #ifndef NDEBUG
#if ENABLE(ASSERT)

We have a bot with Release + assertion, and locked() is used in ASSERT().

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
Source/wtf/ThreadingPrimitives.h:155: using WTF::Mutex;
Add |using WTF::MutexBase| too.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
File Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:300: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:317: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:324: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:341: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:360: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:387: #ifndef NDEBUG
#if ENABLE(ASSERT)

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:411: #ifndef NDEBUG
#if ENABLE(ASSERT)

[wibling-chromium, 2014-07-25 09:31:29]

Thanks for the reviews!

To answer Sigbjorn's and Haraken's question the issue is actually during weak
processing. 

When calling stopSyncDatabases (frame 28 below) we clear a map of weak pointers
to DatabaseCloser objects. The DatabaseCloser indirectly calls
DatabaseBackendBase::closeDatabase (frame 18) which takes the guidMutex.
While waiting for guidMutex a GC occurs which means that when we get the
guidMutex and leave the safe point we will process weak callbacks. These weak
callbacks cause another DatabaseCloser (frame 7 vs frame 20) to be called which
also calls closeDatabase and tries to acquire the guidMutex that the thread just
got.

Thread 166 (Thread 0x7fffbc1a4700 (LWP 8330)):
#0  __lll_lock_wait () at
../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:132
#1  0x00007ffff230a065 in _L_lock_858 () from
/lib/x86_64-linux-gnu/libpthread.so.0
#2  0x00007ffff2309eba in __pthread_mutex_lock (mutex=0x16561e806728) at
pthread_mutex_lock.c:61
#3  0x0000000001c198d5 in WTF::Mutex::lock (this=0x16561e806728) at
../../third_party/WebKit/Source/wtf/ThreadingPthreads.cpp:312
#4  0x0000000001afaf38 in
blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker
(this=0x7fffbc1a32d8, mutex=..., 
    stackState=blink::ThreadState::HeapPointersOnStack) at
../../third_party/WebKit/Source/platform/heap/ThreadState.h:636
#5  0x0000000003eb2838 in blink::DatabaseBackendBase::closeDatabase
(this=0x3bc45bd826c0)
---Type <return> to continue, or q <return> to quit---
    at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseBackendBase.cpp:269
#6  0x0000000003bbfedd in blink::DatabaseSync::closeImmediately
(this=0x3bc45bd826c0) at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseSync.cpp:196
#7  0x0000000003eb9d49 in
blink::DatabaseContext::DatabaseCloser::~DatabaseCloser (this=0x2ef97b833360)
    at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseContext.cpp:203
#8  0x0000000003eba5ea in
WTF::OwnedPtrDeleter<blink::DatabaseContext::DatabaseCloser>::deletePtr
(ptr=0x2ef97b833360)
    at ../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:52
#9  0x0000000003eba5af in
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser>::~OwnPtr
(this=0x3bc45bd82db8) at ../../third_party/WebKit/Source/wtf/OwnPtr.h:67
#10 0x0000000003eba4bc in
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >::~KeyValuePair (
    this=0x3bc45bd82db0) at ../../third_party/WebKit/Source/wtf/HashTraits.h:239
#11 0x0000000003eba485 in
WTF::HashTable<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
WTF::KeyValuePairKeyExtractor,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashMapValueTraits<WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase>
>, WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> > >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
blink::HeapAllocator>::deleteBucket (bucket=...) at
../../third_party/WebKit/Source/wtf/HashTable.h:502
#12 0x0000000003ebc23b in
WTF::WeakProcessingHashTableHelper<(WTF::WeakHandlingFlag)1,
blink::WeakMember<blink::DatabaseBackendBase>,
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
WTF::KeyValuePairKeyExtractor,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashMapValueTraits<WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase>
>, WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> > >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
blink::HeapAllocator>::process (visitor=0x2ef978ea2920, closure=0x3bc45bd81c68)
    at ../../third_party/WebKit/Source/wtf/HashTable.h:1144
#13 0x0000000001bdc5de in
blink::CallbackStack::popAndInvokeCallback<(blink::CallbackInvocationMode)2>
(this=0x2ef97c9ce020, first=0x2ef97c5aff88, 
    visitor=0x2ef978ea2920) at
../../third_party/WebKit/Source/platform/heap/Heap.cpp:1555
#14 0x0000000001af981b in blink::ThreadState::popAndInvokeWeakPointerCallback
(this=0x2ef97c5afec0, visitor=0x2ef978ea2920)
    at ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:589
#15 0x0000000001afa121 in blink::ThreadState::performPendingSweep
(this=0x2ef97c5afec0) at
../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:897
#16 0x0000000001afa41f in blink::ThreadState::leaveSafePoint
(this=0x2ef97c5afec0, locker=0x7fffbc1a35a8)
    at ../../third_party/WebKit/Source/platform/heap/ThreadState.cpp:858
#17 0x0000000001afaf57 in
blink::SafePointAwareMutexLocker::SafePointAwareMutexLocker
(this=0x7fffbc1a35a8, mutex=..., 
    stackState=blink::ThreadState::HeapPointersOnStack) at
../../third_party/WebKit/Source/platform/heap/ThreadState.h:643
#18 0x0000000003eb2838 in blink::DatabaseBackendBase::closeDatabase
(this=0x3bc45bd82248)
    at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseBackendBase.cpp:269
#19 0x0000000003bbfedd in blink::DatabaseSync::closeImmediately
(this=0x3bc45bd82248) at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseSync.cpp:196
#20 0x0000000003eb9d49 in
blink::DatabaseContext::DatabaseCloser::~DatabaseCloser (this=0x2ef97a62c5a0)
    at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseContext.cpp:203
#21 0x0000000003eba5ea in
WTF::OwnedPtrDeleter<blink::DatabaseContext::DatabaseCloser>::deletePtr
(ptr=0x2ef97a62c5a0)
    at ../../third_party/WebKit/Source/wtf/OwnPtrCommon.h:52
#22 0x0000000003eba5af in
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser>::~OwnPtr
(this=0x3bc45bd82c08) at ../../third_party/WebKit/Source/wtf/OwnPtr.h:67
#23 0x0000000003eba4bc in
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >::~KeyValuePair (
    this=0x3bc45bd82c00) at ../../third_party/WebKit/Source/wtf/HashTraits.h:239
#24 0x0000000003eba485 in
WTF::HashTable<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
WTF::KeyValuePairKeyExtractor,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashMapValueTraits<WTF::---Type <return> to continue, or q <return> to
quit---
HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> > >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
blink::HeapAllocator>::deleteBucket (bucket=...) at
../../third_party/WebKit/Source/wtf/HashTable.h:502
#25 0x0000000003eba41a in
WTF::HashTable<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
WTF::KeyValuePairKeyExtractor,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashMapValueTraits<WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase>
>, WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> > >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
blink::HeapAllocator>::deleteAllBucketsAndDeallocate (table=0x3bc45bd82bd0,
size=32)
    at ../../third_party/WebKit/Source/wtf/HashTable.h:983
#26 0x0000000003eba37e in
WTF::HashTable<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::KeyValuePair<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
WTF::KeyValuePairKeyExtractor,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashMapValueTraits<WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase>
>, WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> > >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
blink::HeapAllocator>::clear (this=0x3bc45bd81c68) at
../../third_party/WebKit/Source/wtf/HashTable.h:1055
#27 0x0000000003eba315 in
WTF::HashMap<blink::WeakMember<blink::DatabaseBackendBase>,
WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser>,
WTF::PtrHash<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashTraits<blink::WeakMember<blink::DatabaseBackendBase> >,
WTF::HashTraits<WTF::OwnPtr<blink::DatabaseContext::DatabaseCloser> >,
blink::HeapAllocator>::clear (this=0x3bc45bd81c68) at
../../third_party/WebKit/Source/wtf/HashMap.h:412
#28 0x0000000003eb9dcd in blink::DatabaseContext::stopSyncDatabases
(this=0x3bc45bd81c40)
    at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseContext.cpp:218
#29 0x0000000003eb99ec in blink::DatabaseContext::stopDatabases
(this=0x3bc45bd81c40) at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseContext.cpp:230
#30 0x0000000003eb9ad5 in blink::DatabaseContext::stop (this=0x3bc45bd81c40) at
../../third_party/WebKit/Source/modules/webdatabase/DatabaseContext.cpp:152
#31 0x0000000002456ce0 in
blink::ContextLifecycleNotifier::notifyStoppingActiveDOMObjects
(this=0x2ef97c63c720)
    at ../../third_party/WebKit/Source/core/dom/ContextLifecycleNotifier.cpp:113
#32 0x0000000002367551 in blink::ExecutionContext::stopActiveDOMObjects
(this=0x3bc45bd81888) at
../../third_party/WebKit/Source/core/dom/ExecutionContext.cpp:101
#33 0x0000000002c7da4f in blink::WorkerThreadShutdownStartTask::performTask
(this=0x16561e8152d0, context=0x3bc45bd81888)
    at ../../third_party/WebKit/Source/core/workers/WorkerThread.cpp:207
#34 0x0000000002e14268 in blink::WorkerRunLoopTask::run (this=0x16561e8bdd10) at
../../third_party/WebKit/Source/core/workers/WorkerRunLoop.cpp:61
#35 0x0000000002e12288 in blink::WorkerRunLoop::runCleanupTasks
(this=0x16561ea10430) at
../../third_party/WebKit/Source/core/workers/WorkerRunLoop.cpp:262

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:621: explicit
SafePointAwareMutexLocker(WTF::MutexBase& mutex, ThreadState::StackState
stackState = ThreadState::HeapPointersOnStack)
On 2014/07/25 05:11:44, tkent wrote:
> We should omit WTF::

Done.

https://codereview.chromium.org/415083002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:664: WTF::MutexBase& m_mutex;
On 2014/07/25 05:11:44, tkent wrote:
> Ditto.

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
File Source/wtf/ThreadingPrimitives.h (right):

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
Source/wtf/ThreadingPrimitives.h:86: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)
> 
> We have a bot with Release + assertion, and locked() is used in ASSERT().

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPrimitiv...
Source/wtf/ThreadingPrimitives.h:155: using WTF::Mutex;
On 2014/07/25 05:11:44, tkent wrote:
> Add |using WTF::MutexBase| too.

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
File Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:300: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:317: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:324: #ifndef NDEBUG
On 2014/07/25 05:11:45, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:341: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:360: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:387: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:411: #ifndef NDEBUG
On 2014/07/25 05:11:44, tkent wrote:
> #if ENABLE(ASSERT)

Done.

[tkent, 2014-07-25 09:33:25]

lgtm

[wibling-chromium, 2014-07-25 10:08:57]

On 2014/07/24 21:27:05, sof wrote:
> The recursive/re-entrant implementations looks correct to me; is there a mutex
> maven that it'd be good to involve here?
> 
> What (and where) went actually wrong? The description covers the general
> problem, but what were you running into?

Btw. I am not sure who else I should include to review this? Suggestions are
welcome.

[sof, 2014-07-25 10:18:26]

On 2014/07/25 10:08:57, wibling-chromium wrote:
> On 2014/07/24 21:27:05, sof wrote:
> > The recursive/re-entrant implementations looks correct to me; is there a
mutex
> > maven that it'd be good to involve here?
> > 
> > What (and where) went actually wrong? The description covers the general
> > problem, but what were you running into?
> 
> Btw. I am not sure who else I should include to review this? Suggestions are
> welcome.

Don't know, but that suggestion shouldn't block the CL with tkent's review in
place.

Many thanks for the detailed explanation of cause.

[Erik Corry, 2014-07-25 10:32:38]

LGTM

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
File Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/415083002/diff/1/Source/wtf/ThreadingPthreads...
Source/wtf/ThreadingPthreads.cpp:334: // succeed or not for the non-recursive
mutex. On Linux the two implementations
On pthreads the two...

[wibling-chromium, 2014-07-25 11:03:18]

The CQ bit was checked by wibling@chromium.org

[commit-bot: I haz the power, 2014-07-25 11:04:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wibling@chromium.org/415083002/20001

[commit-bot: I haz the power, 2014-07-25 11:55:16]

Change committed as 178928