Summary: | [JSC] implement Shadow Realm | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Phillip Mates <pmates> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, bugmail, chi187, commit-queue, ews-watchlist, gyuyoung.kim, hotaru, joepeck, keith_miller, lmoura, mark.lam, Ms2ger, msaboff, rwaldron, ryuan.choi, saam, sergio, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=231740 https://bugs.webkit.org/show_bug.cgi?id=232005 |
||||||||||||||||||||||||
Bug Depends on: | 231506, 231740, 241448 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Phillip Mates
2021-09-22 01:05:52 PDT
Created attachment 438932 [details]
Patch
Created attachment 438944 [details]
Patch
Created attachment 438947 [details]
Patch
Created attachment 438948 [details]
Patch
Comment on attachment 438948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438948&action=review > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:38 > + delete wrapped['name']; > + delete wrapped['length']; these two `delete` calls are slow (no IC support for `delete` yet). we should either add IC `delete` support or create a custom host function that joins both deletes. Comment on attachment 438948 [details]
Patch
Ready for an initial review
Note that we're still investigating a potential memory leak that popped up in the armv7-test CI:
stress/shadow-realm-evaluate.js.no-llint: Ran out of executable memory while allocating 736 bytes.
@pmates Can you enable ShadowRealm tests on test262 in config.yaml and update expectations.yaml? Created attachment 439337 [details]
Patch
(In reply to Yusuke Suzuki from comment #7) > @pmates Can you enable ShadowRealm tests on test262 in config.yaml and > update expectations.yaml? Yup, updated it (I'm new to this, so I followed https://bocoup.com/blog/new-test262-import-and-runner-in-webkit, not sure if there is more I need to do) The registered expected failures will be addressed in https://github.com/tc39/test262/pull/3220 https://github.com/tc39/test262/pull/3220 has been reviewed and merged. Thanks Phillip! Created attachment 439380 [details]
Patch
Comment on attachment 439380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439380&action=review > Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:71 > +IndirectEvalExecutable* IndirectEvalExecutable::createSafe(JSGlobalObject* globalObject, const SourceCode& source, DerivedContextType derivedContextType, bool isArrowFunctionContext, EvalContextType evalContextType, NakedPtr<JSObject>& resultingError) Let's rename it to create. > Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:79 > +IndirectEvalExecutable* IndirectEvalExecutable::create(JSGlobalObject* globalObject, const SourceCode& source, DerivedContextType derivedContextType, bool isArrowFunctionContext, EvalContextType evalContextType) And let's rename it to tryCreate instead. > Source/JavaScriptCore/runtime/ShadowRealmConstructor.h:38 > + static constexpr unsigned StructureFlags = Base::StructureFlags | HasStaticPropertyTable; Since we have no table, HasStaticPropertyTable is not necessary. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:69 > + RELEASE_ASSERT(thisRealm); Let's use ASSERT. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:93 > + RELEASE_ASSERT(thisRealm); Ditto. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:98 > + String s = asString(evalArg)->value(globalObject); let's make it `script` or `string` instead of `s` for readability. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:99 > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); Use `{ }` instead of `encodedJSValue()` in new code. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:111 > + JSValue parsedObject; > + if (s.is8Bit()) { > + LiteralParser<LChar> preparser(globalObject, s.characters8(), s.length(), NonStrictJSON, nullptr); > + parsedObject = preparser.tryLiteralParse(); > + } else { > + LiteralParser<UChar> preparser(globalObject, s.characters16(), s.length(), NonStrictJSON, nullptr); > + parsedObject = preparser.tryLiteralParse(); > + } > + RETURN_IF_EXCEPTION(scope, encodedJSValue()); > + if (parsedObject) > + return JSValue::encode(parsedObject); I think this part is not necessary. It is done for JSON like object evaluation, but Realm.evaluate's usage would be largely different from that. > Source/JavaScriptCore/runtime/ShadowRealmPrototype.cpp:131 > + RETURN_IF_EXCEPTION(scope, > + throwVMError(globalObject, scope, createTypeError(globalObject, "Error encountered during evaluation"_s))); Running something in RETURN_IF_EXCEPTION's second parameter is not the right thing. Since we are getting an exception, RETURN_IF_EXCEPTION(scope, { }); would be the right thing if we would like to propagate error from the evaluation. If we would like to convert TypeError when some errors occur, then, the code should be, if (UNLIKELY(scope.exception())) { scope.clearException(); return throwVMError(globalObject, scope, createTypeError(globalObject, "Error encountered during evaluation"_s))); } > Source/JavaScriptCore/runtime/VM.cpp:372 > + , shadowRealmSpace ISO_SUBSPACE_INIT(heap, cellHeapCellType.get(), ShadowRealmObject) Let's make it dynamic IsoSubspace by using DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW. > Source/JavaScriptCore/runtime/VM.h:515 > + IsoSubspace shadowRealmSpace; Let's make it dynamic IsoSubspace by using DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER. Created attachment 439587 [details]
Patch
Thanks for the review Yusuke! I've pushed an updated patch addressing your suggestions Comment on attachment 439587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439587&action=review OK! For this patch itself, I think this is OK. But we cannot enable it immediately until we have a change making WebCore work with ShadowRealm & we would like to know how this ShadowRealm JSGlobalObject is handled in DOM side, e.g. incumbent window's lookup. > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:32 > + let wrapped = (...args) => { Use var in builtin JS. And let's just use `arguments`. var length = arguments.length; var wrappedArgs = @newArrayWithSize(length); for (var index = 0; index < length; ++index) @putByValDirect(wrappedArgs, index, @wrap(arguments[index])); > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:34 > + const result = target.@apply(@undefined, wrappedArgs); Use var instead of const in builtin JS > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:56 > + let result = @evalInRealm(this, sourceText) Use var in builtin JS > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:68 > + let exportNameStr = @toString(exportName); > + let specifierStr = @toString(specifier); Let's describe String instead of Str. And use var in builtin JS. > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:70 > + let lookupBinding = (module) => { Ditto. > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:78 > + let crossRealmThrow = (error) => { Ditto. > Source/JavaScriptCore/bytecode/LinkTimeConstant.h:66 > + v(ShadowRealm, nullptr) \ Unnecessary. > Source/JavaScriptCore/jsc.cpp:2064 > + Structure* structure = arg.structureOrNull(vm); > + return JSValue::encode(structure->globalObject()->globalThis()); Do, `return JSValue::encode(asCell(arg)->globalObject(vm)->globalThis());` > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:108 > + JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().mapPrivateName(), arrayPrototypeMapCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum)); Let's remove this. > Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:-60 > - throwVMError(globalObject, scope, error.toErrorObject(globalObject, executable->source())); scope.release() is necessary here. > Source/JavaScriptCore/runtime/JSType.cpp:46 > + CASE(ShadowRealmType) Let's put it in the same ordering to the header's definition. > Source/JavaScriptCore/runtime/OptionsList.h:543 > + v(Bool, useShadowRealm, false, Normal, "Expose the ShadowRealm object.") \ Yup. I think we cannot enable this for now since WebCore has a lot of code, which is assuming that, if it is JSGlobalObject, it is JSDOMGlobalObject. (for example, incumbentDOMWindow is traversing JSGlobalObjects in the callstack, and it assumes that these ones are JSDOMWindow. But this is not true after this patch). So, to enable that, we also need the WebCore side change. Can you note this here, like, /* FIXME: ShadownRealm can be enabled once WebCore's JSGlobalObject == JSDOMGlobalObject assumption is removed, https://bugs.webkit.org/tracking-bug */ \ v(Bool, useShadowRealm, false, Normal, "Expose the ShadowRealm object.") \ > Source/JavaScriptCore/runtime/ShadowRealmConstructor.cpp:63 > +static JSObject* constructShadowRealm(JSGlobalObject* globalObject, JSValue, const ArgList&) > +{ > + VM& vm = globalObject->vm(); > + Structure* shadowRealmStructure = ShadowRealmObject::createStructure(vm, globalObject, globalObject->shadowRealmPrototype()); > + return ShadowRealmObject::create(vm, shadowRealmStructure, globalObject->globalObjectMethodTable()); > +} > + > +JSC_DEFINE_HOST_FUNCTION(constructWithShadowRealmConstructor, (JSGlobalObject* globalObject, CallFrame* callFrame)) > +{ > + ArgList args(callFrame); > + return JSValue::encode(constructShadowRealm(globalObject, callFrame->newTarget(), args)); > +} I don't think we need to have this static function. Let's just implement constructShadowRealm thing inside constructWithShadowRealmConstructor (In reply to Yusuke Suzuki from comment #16) > Comment on attachment 439587 [details] > Patch > > > OK! For this patch itself, I think this is OK. But we cannot enable it > immediately until we have a change making WebCore work with ShadowRealm & we > would like to know how this ShadowRealm JSGlobalObject is handled in DOM > side, e.g. incumbent window's lookup. Sounds good, here is a ticket I created to look into that: https://bugs.webkit.org/show_bug.cgi?id=231506 > > Source/JavaScriptCore/bytecode/LinkTimeConstant.h:66 > > + v(ShadowRealm, nullptr) \ > > Unnecessary. When I remove this line I get the following error: > /home/mates/igalia/WebKit/Source/JavaScriptCore/runtime/JSGlobalObject.cpp: In member function ‘void JSC::JSGlobalObject::init(JSC::VM&)’: > /home/mates/igalia/WebKit/Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1034:65: error: ‘ShadowRealm’ is not a member of ‘JSC::LinkTimeConstant’ > 1034 | m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::ShadowRealm)].set(vm, this, shadowRealmConstructor); > > Source/JavaScriptCore/jsc.cpp:2064 > > + Structure* structure = arg.structureOrNull(vm); > > + return JSValue::encode(structure->globalObject()->globalThis()); > > Do, `return JSValue::encode(asCell(arg)->globalObject(vm)->globalThis());` I couldn't find a `globalObject` fn assocated with the `JSCell` class, so I did: `return JSValue::encode(arg.asCell()->structure(vm)->globalObject()->globalThis());` > > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:108 > > + JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().mapPrivateName(), arrayPrototypeMapCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum)); > > Let's remove this. Without it I can't use `@map` in `ShadowRealmPrototype.js`. Is there another way to move forward on using a non-modifiable `map` implementation? > > Source/JavaScriptCore/runtime/IndirectEvalExecutable.cpp:-60 > > - throwVMError(globalObject, scope, error.toErrorObject(globalObject, executable->source())); > > scope.release() is necessary here. Thanks for the catch! I added a regression test to cover this. > > Source/JavaScriptCore/runtime/OptionsList.h:543 > > + v(Bool, useShadowRealm, false, Normal, "Expose the ShadowRealm object.") \ > > Yup. I think we cannot enable this for now since WebCore has a lot of code, > which is assuming that, if it is JSGlobalObject, it is JSDOMGlobalObject. > (for example, incumbentDOMWindow is traversing JSGlobalObjects in the > callstack, and it assumes that these ones are JSDOMWindow. But this is not > true after this patch). > So, to enable that, we also need the WebCore side change. Oh, interesting; I'll look into it, thanks for the heads up! Created attachment 440773 [details]
Patch
Comment on attachment 440773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440773&action=review commented > Source/JavaScriptCore/builtins/ShadowRealmPrototype.js:33 > + var wrappedArgs = args.@map(@wrap) Please check the comment in https://bugs.webkit.org/show_bug.cgi?id=230602#c16 :) > Source/JavaScriptCore/bytecode/LinkTimeConstant.h:66 > + v(ShadowRealm, nullptr) \ We should remove this since it is not used. It must be defined only when @ShadowRealm is used in builtin JS. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:108 > + JSC_BUILTIN_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->builtinNames().mapPrivateName(), arrayPrototypeMapCodeGenerator, static_cast<unsigned>(PropertyAttribute::DontEnum)); I mean, we should not use @map and remove this. See comment in https://bugs.webkit.org/show_bug.cgi?id=230602#c16 > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1042 > + m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::ShadowRealm)].set(vm, this, shadowRealmConstructor); You should remove this too. Created attachment 441060 [details]
Patch
Comment on attachment 441060 [details]
Patch
r=me.
You also need to modify JavaScriptCore/DerivedSources-input.xcfilelist and JavaScriptCore/DerivedSources-output.xcfilelist
Created attachment 441193 [details]
Patch
Comment on attachment 441193 [details]
Patch
Already reviewed, and style matches surrounding code.
Committed r284151 (242971@main): <https://commits.webkit.org/242971@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441193 [details]. Re-opened since this is blocked by bug 231740 Comment on attachment 441193 [details]
Patch
CQ+'ing again after an investigation showed the issue was not with this patch, but in the bots infrastructure.
Sorry for the noise.
Committed r284435 (243197@main): <https://commits.webkit.org/243197@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441193 [details]. Looks like this patch did not update expectations.yaml properly. |