Bug 129265 - Need to initialize VM stack data even when the VM is on an exclusive thread
Summary: Need to initialize VM stack data even when the VM is on an exclusive thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 129325 129341
Blocks: 128766
  Show dependency treegraph
 
Reported: 2014-02-24 12:22 PST by Mark Lam
Modified: 2014-02-25 16:58 PST (History)
9 users (show)

See Also:


Attachments
the patch. (7.62 KB, patch)
2014-02-24 15:09 PST, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
patch 2: extensiveThread pushed down into JSLock + other fixes. (12.18 KB, patch)
2014-02-24 19:28 PST, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-02-24 12:22:10 PST
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>
Comment 1 Mark Lam 2014-02-24 15:09:23 PST
Created attachment 225100 [details]
the patch.
Comment 2 Geoffrey Garen 2014-02-24 16:00:32 PST
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.
Comment 3 Mark Lam 2014-02-24 19:28:06 PST
Created attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.
Comment 4 Mark Lam 2014-02-24 19:34:35 PST
(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.
Comment 5 Geoffrey Garen 2014-02-24 19:52:42 PST
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.
Comment 6 Mark Lam 2014-02-24 20:04:56 PST
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.
Comment 7 Geoffrey Garen 2014-02-24 20:32:49 PST
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.
Comment 8 Mark Lam 2014-02-24 20:39:19 PST
Thanks for the review.  Landed in r164627: <http://trac.webkit.org/r164627>.
Comment 9 WebKit Commit Bot 2014-02-25 13:06:33 PST
Re-opened since this is blocked by bug 129325
Comment 10 Alexey Proskuryakov 2014-02-25 13:08:01 PST
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
Comment 11 Alexey Proskuryakov 2014-02-25 16:32:22 PST
Sounds like this can be re-landed now, after <http://trac.webkit.org/r164679>.
Comment 12 Mark Lam 2014-02-25 16:58:58 PST
Patch re-landed in r164683: <http://trac.webkit.org/r164683>.