[Webkit-unassigned] [Bug 230602] [JSC] implement Shadow Realm

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 8 15:48:54 PDT 2021


https://bugs.webkit.org/show_bug.cgi?id=230602

--- Comment #16 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 439587
  --> https://bugs.webkit.org/attachment.cgi?id=439587
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. at 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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211008/8ab20e1b/attachment.htm>


More information about the webkit-unassigned mailing list