[webkit-reviews] review granted: [Bug 202391] [JSC] Place VM* in TLS : [Attachment 379947] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 202391: [JSC] Place VM* in TLS
https://bugs.webkit.org/show_bug.cgi?id=202391

Attachment 379947: Patch

https://bugs.webkit.org/attachment.cgi?id=379947&action=review




--- Comment #13 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 379947
  --> https://bugs.webkit.org/attachment.cgi?id=379947
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379947&action=review

r=me with the ChangeLog fixed.

> Source/JavaScriptCore/ChangeLog:10
> +	   And JSLockHolder's destructor restores it. Since DropAllLocks want
to put `nullptr` in TLS, we place previous VM*
> +	   in JSLockHolder itself. And now JSLock is only lockable by
JSLockHolder.

This part "Since DropAllLocks want to put `nullptr` in TLS, we place previous
VM* in JSLockHolder itself." is not true.  DropAllLocks knows the VM that it is
dropping the lock for, and it doesn't interact with JSLockHolder, right?  So,
JSLockHolder::m_previousVMInTLS is not needed for this purpose.

That said, let's say that we have 2 VM instances A and B, and that we were
executing in A when it called a C++ host function.  While still holding the
JSLock for A, the thread now enters B.	In this case, the TLS VM should be set
to B, but when we exit, it should be restored to A.  I think this is the reason
for needing JSLockHolder::m_previousVMInTLS.  This is also the reason why we
should not generally use VM::current() as the source of truth of VM* rather
than threading everywhere: it is only correct for some uses.