[webkit-reviews] review granted: [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 granted  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


r=me

>>> 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.
> 
> This is correct.  std::thread::id() means no thread has taken ownership.  See
http://en.cppreference.com/w/cpp/thread/thread/id/id.  I think you have it
confused with std::thread::get_id() which returns the id for the current
thread.

Yeah, I read that as get_id(). Oops!

>>> 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.
> 
> I'm being conservative in case the caller passes in std::thread::id() which
means the null thread id i.e. no exclusive thread.  Alternatively, I can add an
assert that ensures that the incoming threadId is not std::thread::id() i.e.
not null.

Same here.