[webkit-reviews] review denied: [Bug 129265] Need to initialize VM stack data even when the VM is on an exclusive thread : [Attachment 225100] the patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 24 16:03:24 PST 2014


Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 129265: Need to initialize VM stack data even when the VM is on an
exclusive thread
https://bugs.webkit.org/show_bug.cgi?id=129265

Attachment 225100: the patch.
https://bugs.webkit.org/attachment.cgi?id=225100&action=review

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


More information about the webkit-reviews mailing list