Summary: | Need to initialize VM stack data even when the VM is on an exclusive thread | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, fpizlo, ggaren, joepeck, mhahnenberg, mkwst, mmirman, msaboff, oliver | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 129325, 129341 | ||||||||
Bug Blocks: | 128766 | ||||||||
Attachments: |
|
Description
Mark Lam
2014-02-24 12:22:10 PST
Created attachment 225100 [details]
the patch.
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. Created attachment 225114 [details]
patch 2: extensiveThread pushed down into JSLock + other fixes.
(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 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 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 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. Thanks for the review. Landed in r164627: <http://trac.webkit.org/r164627>. Re-opened since this is blocked by bug 129325 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 Sounds like this can be re-landed now, after <http://trac.webkit.org/r164679>. Patch re-landed in r164683: <http://trac.webkit.org/r164683>. |