Summary: | [JSC] Shadow realms: set correct Function prototype on wrapped functions | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Griego <joseph.j.griego> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | caitp, ews-watchlist, joepeck, keith_miller, mark.lam, msaboff, pmates, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Joseph Griego
2021-11-15 13:07:42 PST
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]. |