[webkit-reviews] review granted: [Bug 194634] [JSC] Should have default NativeJITCode : [Attachment 362006] 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 194634: [JSC] Should have default NativeJITCode
https://bugs.webkit.org/show_bug.cgi?id=194634

Attachment 362006: Patch

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




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

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

r=me with ref fix.

>> Source/JavaScriptCore/runtime/VM.cpp:693
>> +	static NativeJITCode* result;
> 
> It is a little nit picking, but maybe would be good initialise this to
nullptr.

I think statics already null by default.

>> Source/JavaScriptCore/runtime/VM.cpp:695
>> +	std::call_once(onceKey, [&] {
> 
> I was wondering why do you prefer this instead of:
> 
> ```
> if (!result)
>     result = new
NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline),
JITCode::HostCallThunk, NoIntrinsic);
> 
> return makeRef(*result);
> ...
> ```

A process may instantiate more than 1 VMs on multiple threads.	Hence,
std::call_once is necessary.

>> Source/JavaScriptCore/runtime/VM.cpp:698
>> +	return makeRef(*result);
> 
> IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that
```result``` will always pointing to a valid object? Once last
```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but
```result``` will be pointing to an invalid address.

I think Caio is correct here.  This can happen if the VM is destroyed before we
create any new VMs.  Let's ref the NativeJITCode inside the std::call_once so
that it will never be deref'ed to 0.