Bug 238313 - [JSC] JSRemoteFunction thunk should materialize code-pointer
Summary: [JSC] JSRemoteFunction thunk should materialize code-pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-23 23:58 PDT by Yusuke Suzuki
Modified: 2022-03-24 16:42 PDT (History)
51 users (show)

See Also:


Attachments
Patch (7.64 KB, patch)
2022-03-23 23:59 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.93 KB, patch)
2022-03-24 00:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (424.11 KB, patch)
2022-03-24 16:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-03-23 23:58:45 PDT
[JSC] JSRemoteFunction thunk should materialize code-pointer
Comment 1 Yusuke Suzuki 2022-03-23 23:59:58 PDT
Created attachment 455615 [details]
Patch
Comment 2 Yusuke Suzuki 2022-03-24 00:33:13 PDT
Created attachment 455619 [details]
Patch
Comment 3 Mark Lam 2022-03-24 14:28:01 PDT
Comment on attachment 455619 [details]
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.
Comment 4 Yusuke Suzuki 2022-03-24 14:40:30 PDT
Sounds good!
Comment 5 Yusuke Suzuki 2022-03-24 14:42:09 PDT
Committed r291815 (248842@trunk): <https://commits.webkit.org/248842@trunk>
Comment 6 Radar WebKit Bug Importer 2022-03-24 14:43:17 PDT
<rdar://problem/90791656>
Comment 7 Chris Dumez 2022-03-24 16:04:14 PDT
Reopening to attach new patch.
Comment 8 Chris Dumez 2022-03-24 16:04:16 PDT
Created attachment 455699 [details]
Patch
Comment 9 Chris Dumez 2022-03-24 16:42:57 PDT
Bad webkit-patch upload.