[webkit-reviews] review granted: [Bug 238313] [JSC] JSRemoteFunction thunk should materialize code-pointer : [Attachment 455619] 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 238313: [JSC] JSRemoteFunction thunk should materialize code-pointer
https://bugs.webkit.org/show_bug.cgi?id=238313

Attachment 455619: Patch

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




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

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

r=me

> Source/JavaScriptCore/ChangeLog:10
> +	   Any GC can jettison CodeBlock's JITCode. It means that it is
possible that JITCode is discarded because
> +	   of wrapper creation for arguments of remote function. It causes
occasional crashes on JSTests/stress/shadow-realm-evaluate.js
> +	   test. This patch fixes it by materializing code pointer if it is
already jettisoned.

I suggest rephrasing this as:

When invoking a JSRemoteFunction, we must first wrap the arguments passed to
it.  The wrapping operation may trigger a GC, and GC can jettison JIT code.  As
a result, even though we know that the target JSFunction has JIT code that we
want to execute, the JIT code may be jettisoned (while wrapping the arguments
for it) before we get to the call.  This resulted in occasional crashes on the
JSTests/stress/shadow-realm-evaluate.js test.

This patch fixes this by doing a null check on the JIT code just before calling
it, and if null (i.e. the JIT code has been jettisoned), re-materializing the
JIT code first before making the call.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:1539
> +    // Any GC can potentially jettison JIT code in JSFunction. This means
that calling operationGetWrappedValueForTarget already discarded
> +    // the code. We should check and materialize it here. At this point,
once we got the code. So, we can always materialize the code.

I suggest rephrasing this as:

The calls to operationGetWrappedValueForTarget above may GC, and any GC can
potentially jettison the JIT code in the target JSFunction. If we find that the
JIT code is null (i.e. has been jettisoned), then we need to re-materialize it
for the call below. Note that we know that
operationMaterializeRemoteFunctionTargetCode should be able to re-materialize
the JIT code (except for any OOME) because we only went down this code path
after we found a non-null JIT code (in the noCode check) above i.e. it should
be possible to materialize the JIT code.