Bug 159579 – currentThreadIsHoldingLock Heap::unprotect Assertion

• Component: JavaScriptCore
• Status: NEW

[Chris Vienneau]

I’ve recently noticed that our version of WebKit based off of 188436 asserts when running the scenario below, but our older version based off of 157437 does not.  After doing some investigation I found that the implementation of JSLock changed significantly between the two versions.  The tip of trunk version is pretty much what we have in 188436 as well; the code for currentThreadIsHoldingLock() I find questionable.  Please feel free to suggest how we might improperly be using the system to get in the assert scenario, or discuss the merit of the changes I suggest.
 
Thank you
 
Chris Vienneau
 
 
Current Code:
bool JSLock::currentThreadIsHoldingLock()
{
    ASSERT(!m_hasExclusiveThread || (exclusiveThread() == std::this_thread::get_id()));
    if (m_hasExclusiveThread)
        return !!m_lockCount;
    return m_ownerThreadID == std::this_thread::get_id();
}
I found the logic missing some things that I wanted really clear so I chose to re-implement it. 
 
Suggested currentThreadIsHoldingLock
I took a look at what happens in 15.x and came up with this version of the function:
bool JSLock::currentThreadIsHoldingLock()
{
    ASSERT(!m_hasExclusiveThread || (exclusiveThread() == std::this_thread::get_id()));
    //+EAWebKitChange
    //07/07/2016 - this logic is more strait forward and fixes an assert in DestroyJavascriptValue after EvaluateJavaScript
    if (m_hasExclusiveThread && exclusiveThread() == std::this_thread::get_id())
    {
        return true;
    }    
    if (m_lockCount && m_ownerThreadID == std::this_thread::get_id())
    {
        return true;
    } 
    return false;
    //-EAWebKitChange
}
 
Additional Required Fix
The above was successful in clearing up the assert however another change was needed to make things run properly for all tests and soaks:
void JSLock::lock(intptr_t lockCount)
{
    ASSERT(lockCount > 0);
    //+EAWebKitChange
    //07/07/2016 - added  && (m_lockCount > 0) to condition
    //currentThreadIsHoldingLock doesn't really return if it is holding the lock, but more so if it should have the lock
    //this change allows us to take the lock the first time
    if (currentThreadIsHoldingLock() && (m_lockCount > 0)) {
    //-EAWebKitChange
        m_lockCount += lockCount;
        return;
    }
 
    if (!m_hasExclusiveThread) {
        m_lock.lock();
        m_ownerThreadID = std::this_thread::get_id();
    }
    ASSERT(!m_lockCount);
    m_lockCount = lockCount;
 
    didAcquireLock();
}
 
Test Code:
    void JavascriptArrays(void*)
    {
        EA_TRACE_MESSAGE("Running JavascriptArrays");
                                
        JavascriptArrayBoundObject jsb;
        jsb.mEAWK = mEAWK;
        jsb.mView = mView;
 
        using namespace EA::WebKit;
        mView->BindJavaScriptObject("TestObject", &jsb);
 
        eastl::string16 script(EA_CHAR16("TestObject.ArrayTest([1, true, \"ping\"]);"));
 
        JavascriptValue *returnValue = mEAWK->CreateJavascriptValue(mView);
        mView->EvaluateJavaScript(script.data(), returnValue);
        mEAWK->DestroyJavascriptValue(returnValue);
    }
 
Assert location:
\JavaScriptCore\heap\Heap.cpp (471)
bool Heap::unprotect(JSValue k)
{
    ASSERT(k);
>    ASSERT(m_vm->currentThreadIsHoldingAPILock());
…
 
Callstack:
                EAWebKitDemoUTFWin.exe!EA::Browser::BrowserClient::DebugLog(EA::WebKit::DebugLogInfo & dli={...}) Line 254                C++
               EAWebKitd.dll!EA::WebKit::DebugLogCallback(const eastl::basic_string<char,eastl::allocator> & origMessage={...}, bool shouldAssert=true) Line 611       C++
               EAWebKitd.dll!EA::WebKit::DebugLogCallbackInternal(bool shouldAssert=true, const char * format=0x000007feb4b15170, char * args=0x00000000001af000) Line 629              C++
               EAWebKitd.dll!vprintf_stderr_common(bool shouldAssert=true, const char * format=0x000007feb4b15170, char * args=0x00000000001af000) Line 153           C++
               EAWebKitd.dll!printf_stderr_common(bool shouldAssert=true, const char * format=0x000007feb4b15170, ...) Line 237         C++
               EAWebKitd.dll!printCallSite(const char * file=0x000007feb4d04240, int line=471, const char * function=0x000007feb4d04158, bool shouldAssert=true) Line 259             C++
               EAWebKitd.dll!WTFReportAssertionFailure(const char * file=0x000007feb4d04240, int line=471, const char * function=0x000007feb4d04158, const char * assertion=0x000007feb4d03fe8) Line 281    C++
               EAWebKitd.dll!JSC::Heap::unprotect(JSC::JSValue k={...}) Line 471           C++
               EAWebKitd.dll!JSC::gcUnprotect(JSC::JSCell * val=0x000007fffe453dd0) Line 38  C++
               EAWebKitd.dll!JSC::gcUnprotect(JSC::JSValue value={...}) Line 62             C++
               EAWebKitd.dll!EA::WebKit::JavascriptValue::Destruct() Line 495                C++
               EAWebKitd.dll!EA::WebKit::JavascriptValue::~JavascriptValue() Line 318               C++
               EAWebKitd.dll!EA::WebKit::JavascriptValue::`vector deleting destructor'(unsigned int)  C++
               EAWebKitd.dll!EA::WebKit::DestroyJavascriptValue(EA::WebKit::JavascriptValue * v=0x000007fffe4b7a70) Line 1035             C++
               EAWebKitd.dll!EA::WebKit::EAWebKitLib::DestroyJavascriptValue(EA::WebKit::JavascriptValue * v=0x000007fffe4b7a70) Line 400               C++
>             EAWebKitDemoUTFWin.exe!EAWebkitViewFunctionalTest::JavascriptArrays(void * __formal=0x00000000001afae0) Line 192              C++
                EAWebKitDemoUTFWin.exe!EATest::Test_mfun<EAWebkitViewFunctionalTest>::TestDelegate<EAWebkitViewFunctionalTest>::Invoke(EAWebkitViewFunctionalTest * instance=0x00000001415f2130, void * p=0x00000000001afae0) Line 72                C++
               EAWebKitDemoUTFWin.exe!EATest::Test_mfun<EAWebkitViewFunctionalTest>::operator()(void * context=0x00000000001afae0) Line 107 C++
               EAWebKitDemoUTFWin.exe!EATest::TestCase::operator()(void * context=0x00000000001afae0) Line 76                C++
               EAWebKitDemoUTFWin.exe!EATest::TestSuite::ExecuteTestCase(EATest::TestCase * tc=0x00000000002c4239, void * context=0x00000000001afae0) Line 520 C++
               EAWebKitDemoUTFWin.exe!EATest::TestSuite::RunTests(const char * suitename=0x0000000000000000, const char * testname=0x0000000000000000, void * context=0x00000000001afae0) Line 304        C++
               EAWebKitDemoUTFWin.exe!EATest::TestSuite::RunAllTests(void * context=0x00000000001afae0) Line 147                C++
               EAWebKitDemoUTFWin.exe!EATest::DoTests(int argc=1, char * * argv=0x000007fffefb83a0, void * context=0x00000000001afae0) Line 307 C++
               EAWebKitDemoUTFWin.exe!EA::Browser::RunUnitTests(EA::App::AppCommandLine & commandLine={...}) Line 2284             C++
               EAWebKitDemoUTFWin.exe!EAMain(EA::App::AppCommandLine & commandLine={...}) Line 1123         C++
               EAWebKitDemoUTFWin.exe!WinMain(HINSTANCE__ * __formal=0x000000013fe80000, HINSTANCE__ * __formal=0x0000000000000000, char * cmdLine=0x00000000002167c2, int __formal=10) Line 79 C++
               EAWebKitDemoUTFWin.exe!invoke_main() Line 109      C++
               EAWebKitDemoUTFWin.exe!__scrt_common_main_seh() Line 264        C++
               EAWebKitDemoUTFWin.exe!__scrt_common_main() Line 309  C++
               EAWebKitDemoUTFWin.exe!WinMainCRTStartup() Line 17         C++
               kernel32.dll!BaseThreadInitThunk ()      Unknown
               ntdll.dll!RtlUserThreadStart ()   Unknown

[Chris Vienneau]

Created attachment 283195 [details]
Suggested Changes

[Chris Vienneau]

Unfortunately i don't think i can provide a stand alone test case, the test is run from some wrapping code we have created, I'm not familiar enough with how test cases are normally done on the trunk webkit project.
Test Code:
    void JavascriptArrays(void*)
    {
        EA_TRACE_MESSAGE("Running JavascriptArrays");
                                
        JavascriptArrayBoundObject jsb;
        jsb.mEAWK = mEAWK;
        jsb.mView = mView;
 
        using namespace EA::WebKit;
        mView->BindJavaScriptObject("TestObject", &jsb);
 
        eastl::string16 script(EA_CHAR16("TestObject.ArrayTest([1, true, \"ping\"]);"));
 
        JavascriptValue *returnValue = mEAWK->CreateJavascriptValue(mView);
        mView->EvaluateJavaScript(script.data(), returnValue);
        mEAWK->DestroyJavascriptValue(returnValue);
    }

[Alex Christensen]

Making a patch that applies to current WebKit trunk with a ChangeLog entry would be necessary to put it in trunk.  That can be done with Tools/Scripts/prepare-ChangeLog after making changes to trunk.

You might be able to transform your test case into something that doesn't use EA wrappers.  Tools/TestWebKitAPI/Tests/JavaScriptCore seems to be empty.  Maybe you could add something to Source/JavaScriptCore/API/tests.

[Chris Vienneau]

Before I worry about getting it into trunk, I'm more interested in some inspection.  Am I out to lunch on this change? I like to think that JSLock is well tested, so I would expect the error is on our end.  However attempting to restore to similarly older implementation seems to make the problems go away.  There is the chance that though it has been some time, a knock on has been lingering since it changed; of course there is the other possibility that the old implementation had incorrect behavior we relied on.  I guess i'm asking if someone with good knowledge of this code can take a peek at it and comment.

[Chris Vienneau]

I've decided to go a safer route for now and just modified the asserts which i believe to be firing when not necessary in:
void Heap::protect(JSValue k) and bool Heap::unprotect(JSValue k)
<<    ASSERT(m_vm->currentThreadIsHoldingAPILock());
>>    ASSERT(m_vm->currentThreadIsHoldingAPILock() || (m_vm->hasExclusiveThread() && m_vm->exclusiveThread() == std::this_thread::get_id()));