Bug 171319 – sendMessageScoped's signal handler calls LocklessBag::consumeAll, which causes heap deallocation in signal handler and leads deadlock

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

In sendMessageScoped, we call LocklessBag<SignalHandler>::consumeAll `thread->threadMessages().consumeAll()`.
In LocklessBag::consumeAll, we call `delete` on Nodes.
The problem is that this is called under the context of signal handler. Thus, when calling this, the original
thread may hold the lock in bmalloc. In that case, this `delete` call attempts to lock the heap lock recursively,
and causes deadlock.

Making heap lock to recursive one easily breaks invariant guarded by that lock in bmalloc.
Should not do that. I believe the correct way to solve it is not calling heap functions inside signal handler.

[Yusuke Suzuki]

http://sprunge.us/XLYG log.
The signal handler is invoked on Thread 19 when the thread is dealing with bmalloc.

#5  <signal handler called>
#6  0x00007f2ef35732f0 in bmalloc::Heap::deallocateSmallLine(std::lock_guard<bmalloc::StaticMutex>&, bmalloc::Object) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007f2ef3572bc6 in bmalloc::Deallocator::processObjectLog(std::lock_guard<bmalloc::StaticMutex>&) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#8  0x00007f2ef3571dd8 in bmalloc::Allocator::refillAllocatorSlowCase(bmalloc::BumpAllocator&, unsigned long) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18

And calls deallocation.

#0  0x00007f2ee923eb57 in sched_yield () at ../sysdeps/unix/syscall-template.S:84
#1  0x00007f2ef35766a5 in bmalloc::StaticMutex::lockSlowCase() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007f2ef3572ced in bmalloc::Deallocator::deallocateSlowCase(void*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#3  0x00007f2ef353a13e in WTF::Function<WTF::SignalAction (int, siginfo_t*, void*)>::CallableWrapper<WTF::sendMessageScoped(WTF::Thread&, WTF::ScopedLambda<void (siginfo_t*, ucontext*)> const&)::{lambda()#1}::operator()() const::{lambda(int, siginfo_t*, void*)#1}>::call(int, siginfo_t*, void*) ()
   from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18

Then, deadlock.

[Yusuke Suzuki]

Created attachment 308236 [details]
Patch

[Keith Miller]

Comment on attachment 308236 [details]
Patch

Wow! How did I miss this r=me.

[Yusuke Suzuki]

Committed r215798: <http://trac.webkit.org/changeset/215798>

[Saam Barati]

Comment on attachment 308236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308236&action=review

> Source/WTF/wtf/ThreadMessage.cpp:154
>          clearPipe();

Don't we need to delete it also here after ensuring we've ran?

[Keith Miller]

Comment on attachment 308236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308236&action=review

>> Source/WTF/wtf/ThreadMessage.cpp:154
>>          clearPipe();
> 
> Don't we need to delete it also here after ensuring we've ran?

No, because the only exit to the loop is above.

[Yusuke Suzuki]

Comment on attachment 308236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308236&action=review

>> Source/WTF/wtf/ThreadMessage.cpp:154
>>          clearPipe();
> 
> Don't we need to delete it also here after ensuring we've ran?

This is not necessary because this goes to `while (true)`.
And we will go to `if (Node* node = data.ran.load()) {` again.

[Saam Barati]

Comment on attachment 308236 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308236&action=review

>>>> Source/WTF/wtf/ThreadMessage.cpp:154
>>>>          clearPipe();
>>> 
>>> Don't we need to delete it also here after ensuring we've ran?
>> 
>> No, because the only exit to the loop is above.
> 
> This is not necessary because this goes to `while (true)`.
> And we will go to `if (Node* node = data.ran.load()) {` again.

ah, right. Oops, I did not read the whole loop. Thanks for clarifying.