The current implementations of JSLock's dropAllLocks(), dropAllLocksUnconditionally(), and grabAllLocks() currently implement locking / unlocking by duplicating the code from lock() and unlock(). Instead, they should just call lock() and unlock() instead.
Created attachment 223569 [details] work in progress. Let's try this on the EWS bots. This patch is dependent on <https://webkit.org/b/128450> landing. Once 128450 lands, this patch should reduce in size a bit.
Comment on attachment 223569 [details] work in progress. View in context: https://bugs.webkit.org/attachment.cgi?id=223569&action=review > Source/JavaScriptCore/ChangeLog:11 > + lock() and unlock(). Instead, they should just call lock() and unlock() > + instead. Instead instead. > Source/JavaScriptCore/ChangeLog:19 > + - Also clear m_ownerThread when we're going to relinquish the lock. Let's not mix this behavior change into a refactoring patch. > Source/JavaScriptCore/ChangeLog:55 > +2014-02-08 Mark Lam <mark.lam@apple.com> > + > + DropAllLocks should let JSLock lock its spinLock. > + <https://webkit.org/b/128450> > + > + Reviewed by NOBODY (OOPS!). > + > + Instead of having DropAllLock acquire JSLock's spinLock and then passing > + it into dropAllLocks(), dropAllLocksUnconditionally(), and grabAllLocks(), > + let those JSLock methods acquire the spinLock themselves. > + > + * runtime/JSLock.cpp: > + (JSC::JSLock::dropAllLocks): > + (JSC::JSLock::dropAllLocksUnconditionally): > + (JSC::JSLock::grabAllLocks): > + (JSC::JSLock::DropAllLocks::DropAllLocks): > + (JSC::JSLock::DropAllLocks::~DropAllLocks): > + * runtime/JSLock.h: Remove please. > Source/JavaScriptCore/runtime/JSLock.cpp:228 > // Don't drop the locks if they've already been dropped once. > // (If the prior drop came from another thread, and it resumed first, > // it could trash our register file). > if (m_lockDropDepth) > return 0; m_lockDropDepth is a shared value, so it's not sound to read m_lockDropDepth without holding the spinlock. > Source/JavaScriptCore/runtime/JSLock.cpp:276 > + ASSERT(!m_vm || !m_vm->stackPointerAtVMEntry); I don't think anything prevents another thread from setting these values once we've relinquished the lock, so this ASSERT seems invalid.
Comment on attachment 223569 [details] work in progress. View in context: https://bugs.webkit.org/attachment.cgi?id=223569&action=review >> Source/JavaScriptCore/runtime/JSLock.cpp:228 >> return 0; > > m_lockDropDepth is a shared value, so it's not sound to read m_lockDropDepth without holding the spinlock. We don’t need the spinLock here because m_lockDropDepth is (well ... should have been) protected by the JSLock’s own mutex. Only the owner of the JSLock may read or write to m_lockDropDepth. So, I still have a bug here i.e. I cannot move this m_lockDropDepth check before the ownership check below (which was previously above it). Will fix. >> Source/JavaScriptCore/runtime/JSLock.cpp:276 >> + ASSERT(!m_vm || !m_vm->stackPointerAtVMEntry); > > I don't think anything prevents another thread from setting these values once we've relinquished the lock, so this ASSERT seems invalid. Thanks for catching this. I thought of it when I contemplated adding a similar assert to grabAllLocks() (which is why I didn’t add such an assert there), but then went on to miss the invalidity of it here. Will remove.
Created attachment 223735 [details] updated patch now that 128450 has landed.
(In reply to comment #4) > Created an attachment (id=223735) [details] > updated patch now that 128451 has landed. Correction: “that 128450 has landed”, not 128451.
Comment on attachment 223735 [details] updated patch now that 128450 has landed. r=me
Thanks. Landed in r163820: <http://trac.webkit.org/r163820>.