171319 2017-04-26 03:11:27 -0700 sendMessageScoped's signal handler calls LocklessBag::consumeAll, which causes heap deallocation in signal handler and leads deadlock 2017-04-26 11:46:27 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 ysuzuki ysuzuki benjamin buildbot cdumez cgarcia cmarcelo dbates fpizlo keith_miller mark.lam saam oldest_to_newest 1301549 0 ysuzuki 2017-04-26 03:11:27 -0700 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. 1301550 1 ysuzuki 2017-04-26 03:13:58 -0700 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. 1301553 2 308236 ysuzuki 2017-04-26 04:18:24 -0700 Created attachment 308236 Patch 1301562 3 308236 keith_miller 2017-04-26 06:22:55 -0700 Comment on attachment 308236 Patch Wow! How did I miss this r=me. 1301565 4 ysuzuki 2017-04-26 06:34:27 -0700 Committed r215798: <http://trac.webkit.org/changeset/215798> 1301611 5 308236 saam 2017-04-26 09:09:29 -0700 Comment on attachment 308236 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? 1301612 6 308236 keith_miller 2017-04-26 09:11:20 -0700 Comment on attachment 308236 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. 1301614 7 308236 ysuzuki 2017-04-26 09:12:37 -0700 Comment on attachment 308236 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. 1301708 8 308236 saam 2017-04-26 11:46:27 -0700 Comment on attachment 308236 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. 308236 2017-04-26 04:18:24 -0700 2017-04-26 06:22:55 -0700 Patch bug-171319-20170426201823.patch text/plain 4455 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1Nzk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGRiMDNlYTljMWMzMjRkMDg4NDFkZTdl YzQ5MTg4NWIzYWY1YzBjZmEuLmI2YzZhNGEyZjIyZDRhNmIwNTI4ZWEwYzk2ZjRhZTRlOWU5ZjRi YzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjQgQEAKKzIwMTctMDQtMjYgIFl1c3VrZSBTdXp1a2kgIDx1dGF0 YW5lLnRlYUBnbWFpbC5jb20+CisKKyAgICAgICAgc2VuZE1lc3NhZ2VTY29wZWQncyBzaWduYWwg aGFuZGxlciBjYWxscyBMb2NrbGVzc0JhZzo6Y29uc3VtZUFsbCwgd2hpY2ggY2F1c2VzIGhlYXAg ZGVhbGxvY2F0aW9uIGluIHNpZ25hbCBoYW5kbGVyIGFuZCBsZWFkcyBkZWFkbG9jaworICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTcxMzE5CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgSW4gc2VuZE1lc3NhZ2VT Y29wZWQsIHdlIGNhbGwgTG9ja2xlc3NCYWc8U2lnbmFsSGFuZGxlcj46OmNvbnN1bWVBbGwgYHRo cmVhZC0+dGhyZWFkTWVzc2FnZXMoKS5jb25zdW1lQWxsKClgLgorICAgICAgICBJbiBMb2NrbGVz c0JhZzo6Y29uc3VtZUFsbCwgd2UgY2FsbCBgZGVsZXRlYCBvbiBOb2Rlcy4KKyAgICAgICAgVGhl IHByb2JsZW0gaXMgdGhhdCB0aGlzIGlzIGNhbGxlZCB1bmRlciB0aGUgY29udGV4dCBvZiBzaWdu YWwgaGFuZGxlci4gVGh1cywgd2hlbiBjYWxsaW5nIHRoaXMsIHRoZSBvcmlnaW5hbAorICAgICAg ICB0aHJlYWQgbWF5IGhvbGQgdGhlIGxvY2sgaW4gYm1hbGxvYy4gSW4gdGhhdCBjYXNlLCB0aGlz IGBkZWxldGVgIGNhbGwgYXR0ZW1wdHMgdG8gbG9jayB0aGUgaGVhcCBsb2NrIHJlY3Vyc2l2ZWx5 LAorICAgICAgICBhbmQgY2F1c2VzIGRlYWRsb2NrLgorCisgICAgICAgIEluc3RlYWQsIHRoaXMg cGF0Y2ggdHJhbnNmZXJzIHRoZSBMb2NrbGVzc0JhZydzIE5vZGUgdG8gdGhlIHNlbmRlciB0aHJl YWQuIEFuZCB0aGUgc2VuZGVyIHRocmVhZCBkZWxldGVzIGl0IGluc3RlYWQuCisKKyAgICAgICAg KiB3dGYvTG9ja2xlc3NCYWcuaDoKKyAgICAgICAgKFdURjo6TG9ja2xlc3NCYWc6OmNvbnN1bWVB bGxXaXRoTm9kZSk6CisgICAgICAgICogd3RmL1RocmVhZE1lc3NhZ2UuY3BwOgorICAgICAgICAo V1RGOjpUaHJlYWRNZXNzYWdlRGF0YTo6VGhyZWFkTWVzc2FnZURhdGEpOgorICAgICAgICAoV1RG OjpzZW5kTWVzc2FnZVNjb3BlZCk6CisKIDIwMTctMDQtMjUgIERvbiBPbG1zdGVhZCAgPGRvbi5v bG1zdGVhZEBhbS5zb255LmNvbT4KIAogICAgICAgICBbV2luXSBVc2UgQ2xhbmcncyBfX2hhc19k ZWNsc3BlY19hdHRyaWJ1dGUgZm9yIGV4cG9ydCBtYWNyb3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9X VEYvd3RmL0xvY2tsZXNzQmFnLmggYi9Tb3VyY2UvV1RGL3d0Zi9Mb2NrbGVzc0JhZy5oCmluZGV4 IDE5Y2FjMDMyZGJlOWY3ODViNzNkZDYyMWJlY2RjODJkZTMwMzYwMjUuLjYxZDhhM2RjYjFkOGQ0 MTEyMjE0ODNiN2MyYmMzYzY0Mzc5YTYyZTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0xv Y2tsZXNzQmFnLmgKKysrIGIvU291cmNlL1dURi93dGYvTG9ja2xlc3NCYWcuaApAQCAtMzQsMTQg KzM0LDE0IEBAIG5hbWVzcGFjZSBXVEYgewogdGVtcGxhdGU8dHlwZW5hbWUgVD4KIGNsYXNzIExv Y2tsZXNzQmFnIHsKICAgICBXVEZfTUFLRV9OT05DT1BZQUJMRShMb2NrbGVzc0JhZyk7Ci0KK3B1 YmxpYzoKICAgICBzdHJ1Y3QgTm9kZSB7CiAgICAgICAgIFdURl9NQUtFX0ZBU1RfQUxMT0NBVEVE OwogICAgIHB1YmxpYzoKICAgICAgICAgVCBkYXRhOwogICAgICAgICBOb2RlKiBuZXh0OwogICAg IH07Ci1wdWJsaWM6CisKICAgICBMb2NrbGVzc0JhZygpCiAgICAgICAgIDogbV9oZWFkKG51bGxw dHIpCiAgICAgewpAQCAtOTAsNiArOTAsMTcgQEAgY2xhc3MgTG9ja2xlc3NCYWcgewogICAgICAg ICB9CiAgICAgfQogCisgICAgdGVtcGxhdGU8dHlwZW5hbWUgRnVuY3Rvcj4KKyAgICB2b2lkIGNv bnN1bWVBbGxXaXRoTm9kZShjb25zdCBGdW5jdG9yJiBmdW5jKQorICAgIHsKKyAgICAgICAgTm9k ZSogbm9kZSA9IG1faGVhZC5leGNoYW5nZShudWxscHRyKTsKKyAgICAgICAgd2hpbGUgKG5vZGUp IHsKKyAgICAgICAgICAgIE5vZGUqIG9sZE5vZGUgPSBub2RlOworICAgICAgICAgICAgbm9kZSA9 IG5vZGUtPm5leHQ7CisgICAgICAgICAgICBmdW5jKFdURk1vdmUob2xkTm9kZS0+ZGF0YSksIG9s ZE5vZGUpOworICAgICAgICB9CisgICAgfQorCiBwcml2YXRlOgogICAgIEF0b21pYzxOb2RlKj4g bV9oZWFkOwogfTsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1RocmVhZE1lc3NhZ2UuY3Bw IGIvU291cmNlL1dURi93dGYvVGhyZWFkTWVzc2FnZS5jcHAKaW5kZXggZjJmOGE3YjIyNWVjZmM2 MzE3MTQzNjZlYWE2N2ZlNWI5OTE2ZDhlYy4uZjE0YmM0NWQzNjE0YzdjNjYzNWViMTdhYjBlMjcy ODE2YTEzNzNkZCAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvVGhyZWFkTWVzc2FnZS5jcHAK KysrIGIvU291cmNlL1dURi93dGYvVGhyZWFkTWVzc2FnZS5jcHAKQEAgLTM5LDE2ICszOSwxOCBA QAogCiBuYW1lc3BhY2UgV1RGIHsKIAordXNpbmcgTm9kZSA9IExvY2tsZXNzQmFnPFRocmVhZE1l c3NhZ2VEYXRhKj46Ok5vZGU7CisKIGNsYXNzIFRocmVhZE1lc3NhZ2VEYXRhIHsKICAgICBXVEZf TUFLRV9GQVNUX0FMTE9DQVRFRDsKIHB1YmxpYzoKICAgICBUaHJlYWRNZXNzYWdlRGF0YShjb25z dCBUaHJlYWRNZXNzYWdlJiBtKQotICAgICAgICA6IHJhbihmYWxzZSkKKyAgICAgICAgOiByYW4o bnVsbHB0cikKICAgICAgICAgLCBtZXNzYWdlKG0pCiAgICAgewogICAgIH0KIAotICAgIEF0b21p Yzxib29sPiByYW47CisgICAgQXRvbWljPE5vZGUqPiByYW47CiAgICAgY29uc3QgVGhyZWFkTWVz c2FnZSYgbWVzc2FnZTsKIH07CiAKQEAgLTk0LDkgKzk2LDEyIEBAIE1lc3NhZ2VTdGF0dXMgc2Vu ZE1lc3NhZ2VTY29wZWQoVGhyZWFkJiB0aHJlYWQsIGNvbnN0IFRocmVhZE1lc3NhZ2UmIG1lc3Nh Z2UpCiAgICAgICAgICAgICAgICAgcmV0dXJuIFNpZ25hbEFjdGlvbjo6Tm90SGFuZGxlZDsKICAg ICAgICAgICAgIH0KIAotICAgICAgICAgICAgdGhyZWFkLT50aHJlYWRNZXNzYWdlcygpLmNvbnN1 bWVBbGwoWyZdIChUaHJlYWRNZXNzYWdlRGF0YSogZGF0YSkgeworICAgICAgICAgICAgLy8gTm9k ZSBzaG91bGQgYmUgZGVsZXRlZCBpbiB0aGUgc2VuZGVyIHRocmVhZC4gRGVsZXRpbmcgTm9kZXMg aW4gc2lnbmFsIGhhbmRsZXIgY2F1c2VzIGRlYWQgbG9jay4KKyAgICAgICAgICAgIHRocmVhZC0+ dGhyZWFkTWVzc2FnZXMoKS5jb25zdW1lQWxsV2l0aE5vZGUoWyZdIChUaHJlYWRNZXNzYWdlRGF0 YSogZGF0YSwgTm9kZSogbm9kZSkgewogICAgICAgICAgICAgICAgIGRhdGEtPm1lc3NhZ2UoaW5m bywgc3RhdGljX2Nhc3Q8dWNvbnRleHRfdCo+KHVhcCkpOwotICAgICAgICAgICAgICAgIGRhdGEt PnJhbi5zdG9yZSh0cnVlKTsKKyAgICAgICAgICAgICAgICAvLyBCeSBzZXR0aW5nIHJhbiB2YXJp YWJsZSwgKDEpIHRoZSBzZW5kZXIgYWNrbm93bGVkZ2VzIHRoZSBjb21wbGV0aW9uIGFuZAorICAg ICAgICAgICAgICAgIC8vICgyKSBnZXRzIHRoZSBOb2RlIHRvIGJlIGRlbGV0ZWQuCisgICAgICAg ICAgICAgICAgZGF0YS0+cmFuLnN0b3JlKG5vZGUpOwogICAgICAgICAgICAgfSk7CiAKICAgICAg ICAgICAgIHdoaWxlICh3cml0ZShmaWxlRGVzY3JpcHRvcnNbV3JpdGVdLCBtYWdpY0J5dGUsIDEp ID09IC0xKQpAQCAtMTM3LDEyICsxNDIsMTQgQEAgTWVzc2FnZVN0YXR1cyBzZW5kTWVzc2FnZVNj b3BlZChUaHJlYWQmIHRocmVhZCwgY29uc3QgVGhyZWFkTWVzc2FnZSYgbWVzc2FnZSkKICAgICAg ICAgICAgIGZjbnRsKGZpbGVEZXNjcmlwdG9yc1tSZWFkXSwgRl9TRVRGTCwgZmxhZ3MpOwogICAg ICAgICB9OwogCi0gICAgICAgIGlmIChkYXRhLnJhbi5sb2FkKCkpIHsKKyAgICAgICAgaWYgKE5v ZGUqIG5vZGUgPSBkYXRhLnJhbi5sb2FkKCkpIHsKICAgICAgICAgICAgIGNsZWFyUGlwZSgpOwor ICAgICAgICAgICAgZGVsZXRlIG5vZGU7CiAgICAgICAgICAgICByZXR1cm4gTWVzc2FnZVN0YXR1 czo6TWVzc2FnZVJhbjsKICAgICAgICAgfQogCi0gICAgICAgIHJlYWQoZmlsZURlc2NyaXB0b3Jz W1JlYWRdLCBidWZmZXIsIDEpOworICAgICAgICBpbnQgcmV0ID0gcmVhZChmaWxlRGVzY3JpcHRv cnNbUmVhZF0sIGJ1ZmZlciwgMSk7CisgICAgICAgIFVOVVNFRF9QQVJBTShyZXQpOwogICAgICAg ICBBU1NFUlQoYnVmZmVyWzBdID09IG1hZ2ljQnl0ZVswXSk7CiAgICAgICAgIGNsZWFyUGlwZSgp OwogICAgIH0K