Bug 128451 – Change JSLock::dropAllLocks() and friends to use lock() and unlock()

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

[Mark Lam]

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.

[Mark Lam]

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.

[Geoffrey Garen]

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
> [phone]  Mark Lam  <[e-mail]>
> +
> +        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.

[Mark Lam]

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.

[Mark Lam]

Created attachment 223735 [details]
updated patch now that 128450 has landed.

[Mark Lam]

(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.

[Geoffrey Garen]

Comment on attachment 223735 [details]
updated patch now that 128450 has landed.

r=me

[Mark Lam]

Thanks.  Landed in r163820: <http://trac.webkit.org/r163820>.