[JSC] Shadow realms: set correct Function prototype on wrapped functions
Created attachment 444299 [details] Patch
(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)
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)
Created attachment 444710 [details] Patch
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.
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.
(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
(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.
Created attachment 444748 [details] Patch
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.
Comment on attachment 444748 [details] Patch r=me
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].
<rdar://problem/85612395>