[webkit-reviews] review denied: [Bug 128451] Change JSLock::dropAllLocks() and friends to use lock() and unlock() : [Attachment 223569] work in progress.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 128451: Change JSLock::dropAllLocks() and friends to use lock() and
unlock()
https://bugs.webkit.org/show_bug.cgi?id=128451

Attachment 223569: work in progress.
https://bugs.webkit.org/attachment.cgi?id=223569&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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 at 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.