[webkit-reviews] review denied: [Bug 129265] Need to initialize VM stack data even when the VM is on an exclusive thread : [Attachment 225114] patch 2: extensiveThread pushed down into JSLock + other fixes.

[bugzilla-daemon at webkit.org]

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 225114: patch 2: extensiveThread pushed down into JSLock + other
fixes.
https://bugs.webkit.org/attachment.cgi?id=225114&action=review

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