Bug 230602 – [JSC] implement Shadow Realm

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Phillip Mates]

https://github.com/tc39/proposal-shadowrealm

[Phillip Mates]

Created attachment 438932 [details]
Patch

[Phillip Mates]

Created attachment 438944 [details]
Patch

[Phillip Mates]

Created attachment 438947 [details]
Patch

[Phillip Mates]

Created attachment 438948 [details]
Patch

[Phillip Mates]

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.

[Phillip Mates]

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.

[Yusuke Suzuki]

@pmates Can you enable ShadowRealm tests on test262 in config.yaml and update expectations.yaml?

[Phillip Mates]

Created attachment 439337 [details]
Patch

[Phillip Mates]

(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

[Rick Waldron]

https://github.com/tc39/test262/pull/3220 has been reviewed and merged. Thanks Phillip!

[Phillip Mates]

Created attachment 439380 [details]
Patch

[Yusuke Suzuki]

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.

[Radar WebKit Bug Importer]

<rdar://problem/83660888>

[Phillip Mates]

Created attachment 439587 [details]
Patch

[Phillip Mates]

Thanks for the review Yusuke! I've pushed an updated patch addressing your suggestions

[Yusuke Suzuki]

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

[Phillip Mates]

(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!

[Phillip Mates]

Created attachment 440773 [details]
Patch

[Yusuke Suzuki]

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.

[Phillip Mates]

Created attachment 441060 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 441060 [details]
Patch

r=me.
You also need to modify JavaScriptCore/DerivedSources-input.xcfilelist and JavaScriptCore/DerivedSources-output.xcfilelist

[Phillip Mates]

Created attachment 441193 [details]
Patch

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 441193 [details]
Patch

Already reviewed, and style matches surrounding code.

[EWS]

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].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 231740

[Lauro Moura]

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.

[EWS]

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].

[Yusuke Suzuki]

Looks like this patch did not update expectations.yaml properly.