Bug 233143 – [JSC] Shadow realms: set correct Function prototype on wrapped functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joseph Griego]

[JSC] Shadow realms: set correct Function prototype on wrapped functions

[Joseph Griego]

Created attachment 444299 [details]
Patch

[Joseph Griego]

(NB: in `JSGlobalObject.cpp` i erred on the side of matching surrounding code, but the linter is complaining--not sure what the usual course of action is here)

[Phillip Mates]

Comment on attachment 444299 [details]
Patch

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

> Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:141
> +    wrappedFn->setPrototype(vm, targetGlobalObj, targetGlobalObj->strictFunctionStructure(isBuiltin)->storedPrototype());

Any idea if `setPrototype` plays nice with function inlining optimizations? Might be worth trying to get some coverage for this change in the stress tests (like in JSTests/stress/shadow-realm-evaluate.js where we try to force inlining)

[Joseph Griego]

Created attachment 444710 [details]
Patch

[Joseph Griego]

I am not sure how correct inlining could ruin our day here, but I added some more tests that we're getting this right to the existing stress tests.

[Yusuke Suzuki]

Comment on attachment 444710 [details]
Patch

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

r=me but probably, eventually, I think we need to use bound function.

> Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:56
> +        if (callerRealm !== null) {
> +            @moveFunctionToRealm(wrapped, callerRealm);
> +        }

WebKit coding style do not use brace if there is only one statement.

[Caitlin Potter (:caitp)]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 444710 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444710&action=review
> 
> r=me but probably, eventually, I think we need to use bound function.

I'm working on a variant using something like JSBoundFunction, it's in progress

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > Comment on attachment 444710 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=444710&action=review
> > 
> > r=me but probably, eventually, I think we need to use bound function.
> 
> I'm working on a variant using something like JSBoundFunction, it's in
> progress

However, I think the JS builtin version is fine, and I'd be happy with it.

[Joseph Griego]

Created attachment 444748 [details]
Patch

[Joseph Griego]

After discussing with caitp, we think this version (using a bool instead of swapping realm arguments) is more readable.

Also fixed the tests (which were typo'd) and style.

[Yusuke Suzuki]

Comment on attachment 444748 [details]
Patch

r=me

[EWS]

Committed r286069 (244456@main): <https://commits.webkit.org/244456@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444748 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85612395>