147110 2015-07-20 08:24:02 -0700 Possible fix for JSC api test failures in debug mode after r187020. 2015-07-20 12:41:39 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 peavo webkit-unassigned achristensen benjamin bfulgham cmarcelo commit-queue ddkilzer fpizlo ggaren mark.lam msaboff oldest_to_newest 1110624 0 peavo 2015-07-20 08:24:02 -0700 After r187020, a debug assertion is failing during JSC api tests: ASSERTION FAILED: !identifierByPthreadHandle(pthreadHandle) /Volumes/Data/slave/mavericks-cloop-debug/build/Source/WTF/wtf/ThreadingPthreads.cpp(154) : ThreadIdentifier WTF::establishIdentifierForPthreadHandle(const pthread_t &) 1 0x1078e1510 WTFCrash 2 0x10793cac7 WTF::establishIdentifierForPthreadHandle(_opaque_pthread_t* const&) 3 0x10793c730 WTF::createThreadInternal(void (*)(void*), void*, char const*) 4 0x10793af57 WTF::createThread(char const*, std::__1::function<void ()>) 5 0x10793b0b5 WTF::createThread(void (*)(void*), void*, char const*) 6 0x107446317 JSC::GCThreadSharedData::GCThreadSharedData(JSC::VM*) 7 0x107445c7d JSC::GCThreadSharedData::GCThreadSharedData(JSC::VM*) 8 0x107451a44 JSC::Heap::Heap(JSC::VM*, JSC::HeapType) 9 0x1074517b3 JSC::Heap::Heap(JSC::VM*, JSC::HeapType) 10 0x10788b80f JSC::VM::VM(JSC::VM::VMType, JSC::HeapType) 11 0x10788b731 JSC::VM::VM(JSC::VM::VMType, JSC::HeapType) 12 0x10788ea0c JSC::VM::createContextGroup(JSC::HeapType) 13 0x107554ceb JSContextGroupCreate 14 0x107647885 -[JSVirtualMachine init] 15 0x10755356d -[JSContext init] 16 0x1072739dd currentThisInsideBlockGetterTest() 17 0x107294888 testObjectiveCAPIMain() 18 0x1072859e4 testObjectiveCAPI 19 0x10727bec3 main 20 0x7fff9439c5fd start 1110627 1 257095 peavo 2015-07-20 08:32:29 -0700 Created attachment 257095 Patch 1110636 2 peavo 2015-07-20 09:25:39 -0700 I have not been able to verify that this patch fixes the tests, since Windows is not using pthreads. 1110638 3 257095 mark.lam 2015-07-20 09:36:33 -0700 Comment on attachment 257095 Patch Thanks for your diagnosis of the bug. However, I do agree with the fix. Your approach still has a race condition where the same pthreadHandle can be added to the threadMap() twice. Let me take care of the bug since I can test it on mavericks. 1110639 4 mark.lam 2015-07-20 09:36:56 -0700 (In reply to comment #3) > Comment on attachment 257095 [details] > Patch > > Thanks for your diagnosis of the bug. However, I do agree with the fix. > Your approach still has a race condition where the same pthreadHandle can be > added to the threadMap() twice. Let me take care of the bug since I can > test it on mavericks. Correction: I *don't* agree with the fix. 1110640 5 peavo 2015-07-20 09:38:49 -0700 (In reply to comment #3) > Comment on attachment 257095 [details] > Patch > > Thanks for your diagnosis of the bug. However, I do agree with the fix. > Your approach still has a race condition where the same pthreadHandle can be > added to the threadMap() twice. Let me take care of the bug since I can > test it on mavericks. Ok, thanks :) 1110659 6 mark.lam 2015-07-20 11:02:04 -0700 I dug into this, and see that Threading.cpp's threadEntryPoint() should have ensured that currentThread() is not called on the new thread until after the ThreadIdentifier has been established. Hence, Per's suspected race condition is not the root cause. We should roll out r187020 and r187021 until this issue is fixed. 1110671 7 mark.lam 2015-07-20 11:16:05 -0700 Rolled out r187020 and r187021 in r187026: <http://trac.webkit.org/r187026>. 1110676 8 peavo 2015-07-20 11:23:27 -0700 (In reply to comment #7) > Rolled out r187020 and r187021 in r187026: <http://trac.webkit.org/r187026>. Thanks for sorting this out, Mark :) And sorry for the noise, I can have a look, and see if I can spot other possible causes for the assert. 1110697 9 peavo 2015-07-20 11:57:26 -0700 Is this a possible scenario for the assert? A thread not created with WTF::createThread() calls WTF::currentThread(), and then exits. Next a thread created with WTF::createThread reuses the pthread_t handle, and then asserts. For example, I see that JavaScriptCore\API\tests\testapi.mm is creating a thread using just pthread_create: @autoreleasepool { JSContext *context = [[JSContext alloc] init]; pthread_t threadID; pthread_create(&threadID, NULL, &threadMain, (__bridge void*)context); pthread_join(threadID, nullptr); JSSynchronousGarbageCollectForDebugging([context JSGlobalContextRef]); checkResult(@"Did not crash after entering the VM from another thread", true); } 1110701 10 mark.lam 2015-07-20 12:05:49 -0700 (In reply to comment #9) > Is this a possible scenario for the assert? > > A thread not created with WTF::createThread() calls WTF::currentThread(), > and then exits. > Next a thread created with WTF::createThread reuses the pthread_t handle, > and then asserts. Yes, it looks like this is what's happening. I'm currently debugging the issue on Mavericks. 1110713 11 mark.lam 2015-07-20 12:29:48 -0700 (In reply to comment #10) > (In reply to comment #9) > > Is this a possible scenario for the assert? > > > > A thread not created with WTF::createThread() calls WTF::currentThread(), > > and then exits. > > Next a thread created with WTF::createThread reuses the pthread_t handle, > > and then asserts. > > Yes, it looks like this is what's happening. I'm currently debugging the > issue on Mavericks. The real problem has to do with a thread destructor bug in Mavericks too. All this brings to light that we should be using std::thread::id on every port except on Windows. I'll closed this bug as resolved since we rolled out the offending patches. I will work on a fix for bug 146448 that still allows Macs to use std::thread::id. 1110717 12 peavo 2015-07-20 12:41:39 -0700 (In reply to comment #11) > > The real problem has to do with a thread destructor bug in Mavericks too. > All this brings to light that we should be using std::thread::id on every > port except on Windows. I'll closed this bug as resolved since we rolled > out the offending patches. I will work on a fix for bug 146448 that still > allows Macs to use std::thread::id. Thanks! 257095 2015-07-20 08:32:29 -0700 2015-07-20 09:36:33 -0700 Patch bug-147110-20150720173022.patch text/plain 1967 peavo SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxODcwMjEpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE1LTA3LTIwICBQZXIgQXJuZSBWb2xsYW4gIDxw ZWF2b0BvdXRsb29rLmNvbT4KKworICAgICAgICBQb3NzaWJsZSBmaXggZm9yIEpTQyBhcGkgdGVz dCBmYWlsdXJlcyBpbiBkZWJ1ZyBtb2RlIGFmdGVyIHIxODcwMjAuCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDcxMTAKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJZiBhIG5ld2x5IGNyZWF0ZWQgdGhyZWFk IGNhbGxzIGN1cnJlbnRUaHJlYWQoKSBiZWZvcmUgdGhlIHRocmVhZCBjcmVhdG9yIGNhbGxzCisg ICAgICAgIGVzdGFibGlzaElkZW50aWZpZXJGb3JQdGhyZWFkSGFuZGxlKCkgZm9yIHRoZSBuZXcg dGhyZWFkLCB0aGUgVGhyZWFkSWRlbnRpZmllciB3aWxsCisgICAgICAgIGFscmVhZHkgaGF2ZSBi ZWVuIGVzdGFibGlzaGVkLiBKdXN0IHJldHVybiB0aGUgYWxyZWFkeSBlc3RhYmxpc2hlZCBpZCBp biB0aGlzIGNhc2UuCisKKyAgICAgICAgKiB3dGYvVGhyZWFkaW5nUHRocmVhZHMuY3BwOgorICAg ICAgICAoV1RGOjplc3RhYmxpc2hJZGVudGlmaWVyRm9yUHRocmVhZEhhbmRsZSk6CisKIDIwMTUt MDctMjAgIFBlciBBcm5lIFZvbGxhbiAgPHBlYXZvQG91dGxvb2suY29tPgogCiAgICAgICAgIEph dmFTY3JpcHRDb3JlIHBlcmZvcm1hbmNlIGlzIHZlcnkgYmFkIG9uIFdpbmRvd3MKSW5kZXg6IFNv dXJjZS9XVEYvd3RmL1RocmVhZGluZ1B0aHJlYWRzLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv V1RGL3d0Zi9UaHJlYWRpbmdQdGhyZWFkcy5jcHAJKHJldmlzaW9uIDE4NzAxOSkKKysrIFNvdXJj ZS9XVEYvd3RmL1RocmVhZGluZ1B0aHJlYWRzLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTUxLDcg KzE1MSwxNCBAQCBzdGF0aWMgVGhyZWFkSWRlbnRpZmllciBpZGVudGlmaWVyQnlQdGhyCiAKIHN0 YXRpYyBUaHJlYWRJZGVudGlmaWVyIGVzdGFibGlzaElkZW50aWZpZXJGb3JQdGhyZWFkSGFuZGxl KGNvbnN0IHB0aHJlYWRfdCYgcHRocmVhZEhhbmRsZSkKIHsKLSAgICBBU1NFUlQoIWlkZW50aWZp ZXJCeVB0aHJlYWRIYW5kbGUocHRocmVhZEhhbmRsZSkpOworICAgIC8vIElmIGEgbmV3bHkgY3Jl YXRlZCB0aHJlYWQgY2FsbHMgY3VycmVudFRocmVhZCgpIGJlZm9yZSB0aGUgdGhyZWFkIGNyZWF0 b3IgY2FsbHMKKyAgICAvLyBlc3RhYmxpc2hJZGVudGlmaWVyRm9yUHRocmVhZEhhbmRsZSgpIGZv ciB0aGUgbmV3IHRocmVhZCwgdGhlIFRocmVhZElkZW50aWZpZXIgd2lsbAorICAgIC8vIGFscmVh ZHkgaGF2ZSBiZWVuIGVzdGFibGlzaGVkLiBKdXN0IHJldHVybiB0aGUgYWxyZWFkeSBlc3RhYmxp c2hlZCBpZCBpbiB0aGlzIGNhc2UuCisgICAgLy8gRklYTUU6IFdpbGwgdGhpcyBleHRyYSBjaGVj ayBiZSBhIHBlcmZvcm1hbmNlIHJlZ3Jlc3Npb24/CisgICAgVGhyZWFkSWRlbnRpZmllciBpZCA9 IGlkZW50aWZpZXJCeVB0aHJlYWRIYW5kbGUocHRocmVhZEhhbmRsZSk7CisgICAgaWYgKGlkKQor ICAgICAgICByZXR1cm4gaWQ7CisKICAgICBNdXRleExvY2tlciBsb2NrZXIodGhyZWFkTWFwTXV0 ZXgoKSk7CiAgICAgc3RhdGljIFRocmVhZElkZW50aWZpZXIgaWRlbnRpZmllckNvdW50ID0gMTsK ICAgICB0aHJlYWRNYXAoKS5hZGQoaWRlbnRpZmllckNvdW50LCBzdGQ6Om1ha2VfdW5pcXVlPFB0 aHJlYWRTdGF0ZT4ocHRocmVhZEhhbmRsZSkpOwo=