Bug 129265 – Need to initialize VM stack data even when the VM is on an exclusive thread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

We check VM::exclusiveThread as an optimization to forego the need to do JSLock locking.  However, we recently started pigging backing on JSLock's lock() and unlock() to initialize VM stack data (stackPointerAtVMEntry and lastStackTop) to appropriate values for the current thread.  As a result, we ended up not initializing the VM stack data when VM::exclusiveThread causes us to bypass the locking activity.

The fix is to make the APIEntryShim initialize the VM stack data even when locking is bypassed.

This bug was the reason for a release assertion failing in ErrorHandlingScope (as seen in https://bugs.webkit.org/show_bug.cgi?id=129227).

ref: <rdar://problem/16142775>

[Mark Lam]

Created attachment 225100 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 225100 [details]
the patch.

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

> Source/JavaScriptCore/runtime/JSLock.cpp:79
>  void JSLockHolder::init()
>  {
> -    if (m_vm)
> +    if (!m_vm->exclusiveThread)
>          m_vm->apiLock().lock();
> +
> +    m_didInitializeStackPointerAtVMEntry = m_vm->initializeStackDataIfNeeded();
>  }

JSLockHolder is a convenience object, but this patch would turn it into an API that's required for correctness. I don't think that's a great match because we already have a corresponding API that's required for correctness: JSLock. Instead of copying JSLock logic up into JSLockHolder, let's move the exclusive thread optimization down into JSLock.

> Source/WebCore/bindings/js/JSDOMBinding.cpp:157
>  void reportException(ExecState* exec, JSValue exception, CachedScript* cachedScript)
>  {
> +    APIEntryShim entryShim(exec);

Let's make this an ASSERT, and put the shim higher in the stack. Since this function takes JSC:: arguments, it's likely incorrect for its caller not to lock.

[Mark Lam]

Created attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.

[Mark Lam]

(In reply to comment #2)
> > Source/WebCore/bindings/js/JSDOMBinding.cpp:157
> >  void reportException(ExecState* exec, JSValue exception, CachedScript* cachedScript)
> >  {
> > +    APIEntryShim entryShim(exec);
> 
> Let's make this an ASSERT, and put the shim higher in the stack. Since this function takes JSC:: arguments, it's likely incorrect for its caller not to lock.

After I rebased to ToT, the test failure that necessitated this change no longer manifested.  It appears the issue is already resolved.  No additional fix is necessary.  I did add the assertion though.

[Geoffrey Garen]

Comment on attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.

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

> Source/JavaScriptCore/runtime/JSLock.cpp:83
> +    : m_ownerThreadID(std::thread::id())

m_ownerThreadID is supposed to be the thread that took the lock. But in the constructor, no thread has taken the lock. So, I don't think this is right.

> Source/JavaScriptCore/runtime/JSLock.cpp:104
> +    m_hasExclusiveThread = (threadId != std::thread::id());

Why does it matter if threadId != std::thread::id()? setExclusiveThread() should always establish an exclusive thread.

> Source/JavaScriptCore/runtime/JSLock.cpp:105
> +    m_ownerThreadID = threadId;

I don't think you should use m_ownerThreadID in this way. (See above.)

Perhaps the right answer is to use an m_exclusiveThread data member instead of an m_hasExclusiveThread boolean, and store the exclusive thread in that data member. You can use the null threadID if there is no exclusive thread.

[Mark Lam]

Comment on attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:83
>> +    : m_ownerThreadID(std::thread::id())
> 
> m_ownerThreadID is supposed to be the thread that took the lock. But in the constructor, no thread has taken the lock. So, I don't think this is right.

This is correct.  std::thread::id() means no thread has taken ownership.  See http://en.cppreference.com/w/cpp/thread/thread/id/id.  I think you have it confused with std::thread::get_id() which returns the id for the current thread.

>> Source/JavaScriptCore/runtime/JSLock.cpp:104
>> +    m_hasExclusiveThread = (threadId != std::thread::id());
> 
> Why does it matter if threadId != std::thread::id()? setExclusiveThread() should always establish an exclusive thread.

I'm being conservative in case the caller passes in std::thread::id() which means the null thread id i.e. no exclusive thread.  Alternatively, I can add an assert that ensures that the incoming threadId is not std::thread::id() i.e. not null.

>> Source/JavaScriptCore/runtime/JSLock.cpp:105
>> +    m_ownerThreadID = threadId;
> 
> I don't think you should use m_ownerThreadID in this way. (See above.)
> 
> Perhaps the right answer is to use an m_exclusiveThread data member instead of an m_hasExclusiveThread boolean, and store the exclusive thread in that data member. You can use the null threadID if there is no exclusive thread.

The m_hasExclusiveThread boolean is meant as an optimization because the platform gets to define how std::thread::id is implemented.  I also didn't see an conversion operators in the spec to convert the thread id to a boolean for our test (See http://en.cppreference.com/w/cpp/thread/thread/id).  I compare it to std::thread::id() because std::thread::id() is the portable way to express a null thread id (for whatever way the platform wants to define the null thread id).  I don't think we should assume that std::thread::id() is actually encoded as null.

[Geoffrey Garen]

Comment on attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.

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

r=me

>>> Source/JavaScriptCore/runtime/JSLock.cpp:83
>>> +    : m_ownerThreadID(std::thread::id())
>> 
>> m_ownerThreadID is supposed to be the thread that took the lock. But in the constructor, no thread has taken the lock. So, I don't think this is right.
> 
> This is correct.  std::thread::id() means no thread has taken ownership.  See http://en.cppreference.com/w/cpp/thread/thread/id/id.  I think you have it confused with std::thread::get_id() which returns the id for the current thread.

Yeah, I read that as get_id(). Oops!

>>> Source/JavaScriptCore/runtime/JSLock.cpp:104
>>> +    m_hasExclusiveThread = (threadId != std::thread::id());
>> 
>> Why does it matter if threadId != std::thread::id()? setExclusiveThread() should always establish an exclusive thread.
> 
> I'm being conservative in case the caller passes in std::thread::id() which means the null thread id i.e. no exclusive thread.  Alternatively, I can add an assert that ensures that the incoming threadId is not std::thread::id() i.e. not null.

Same here.

[Mark Lam]

Thanks for the review.  Landed in r164627: <http://trac.webkit.org/r164627>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129325

[Alexey Proskuryakov]

This broke SubtleCrypto tests. Mark is not online, so I'm going to roll out.

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r164659%20(2882)/crypto/subtle/rsa-oaep-generate-non-extractable-key-crash-log.txt

[Alexey Proskuryakov]

Sounds like this can be re-landed now, after <http://trac.webkit.org/r164679>.

[Mark Lam]

Patch re-landed in r164683: <http://trac.webkit.org/r164683>.