Bug 147110 – Possible fix for JSC api test failures in debug mode after r187020.

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

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

Created attachment 257095 [details]
Patch

I have not been able to verify that this patch fixes the tests, since Windows is not using pthreads.

[Mark Lam]

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.

[Mark Lam]

(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.

(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 :)

[Mark Lam]

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.

[Mark Lam]

Rolled out r187020 and r187021 in r187026: <http://trac.webkit.org/r187026>.

(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.

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);
    }

[Mark Lam]

(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.

[Mark Lam]

(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.

(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!