Bug 230602 - [JSC] implement Shadow Realm
Summary: [JSC] implement Shadow Realm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 231506 231740
Blocks:
  Show dependency treegraph
 
Reported: 2021-09-22 01:05 PDT by Phillip Mates
Modified: 2021-10-26 11:38 PDT (History)
19 users (show)

See Also:


Attachments
Patch (77.12 KB, patch)
2021-09-22 01:07 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (82.86 KB, patch)
2021-09-22 06:15 PDT, Phillip Mates
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (82.76 KB, patch)
2021-09-22 07:21 PDT, Phillip Mates
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (86.81 KB, patch)
2021-09-22 07:55 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (84.50 KB, patch)
2021-09-27 03:30 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (85.83 KB, patch)
2021-09-27 12:36 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (86.80 KB, patch)
2021-09-29 03:22 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (85.23 KB, patch)
2021-10-11 04:46 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (83.37 KB, patch)
2021-10-13 05:42 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff
Patch (85.18 KB, patch)
2021-10-14 02:11 PDT, Phillip Mates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Phillip Mates 2021-09-22 01:05:52 PDT
https://github.com/tc39/proposal-shadowrealm
Comment 1 Phillip Mates 2021-09-22 01:07:48 PDT
Created attachment 438932 [details]
Patch
Comment 2 Phillip Mates 2021-09-22 06:15:28 PDT
Created attachment 438944 [details]
Patch
Comment 3 Phillip Mates 2021-09-22 07:21:12 PDT
Created attachment 438947 [details]
Patch
Comment 4 Phillip Mates 2021-09-22 07:55:31 PDT
Created attachment 438948 [details]
Patch
Comment 5 Phillip Mates 2021-09-23 07:04:51 PDT
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 6 Phillip Mates 2021-09-23 07:07:34 PDT
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.
Comment 7 Yusuke Suzuki 2021-09-24 12:32:46 PDT
@pmates Can you enable ShadowRealm tests on test262 in config.yaml and update expectations.yaml?
Comment 8 Phillip Mates 2021-09-27 03:30:36 PDT
Created attachment 439337 [details]
Patch
Comment 9 Phillip Mates 2021-09-27 03:33:55 PDT
(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
Comment 10 Rick Waldron 2021-09-27 10:36:30 PDT
https://github.com/tc39/test262/pull/3220 has been reviewed and merged. Thanks Phillip!
Comment 11 Phillip Mates 2021-09-27 12:36:59 PDT
Created attachment 439380 [details]
Patch
Comment 12 Yusuke Suzuki 2021-09-28 13:58:52 PDT
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.
Comment 13 Radar WebKit Bug Importer 2021-09-29 01:06:16 PDT
<rdar://problem/83660888>
Comment 14 Phillip Mates 2021-09-29 03:22:57 PDT
Created attachment 439587 [details]
Patch
Comment 15 Phillip Mates 2021-09-29 05:51:49 PDT
Thanks for the review Yusuke! I've pushed an updated patch addressing your suggestions
Comment 16 Yusuke Suzuki 2021-10-08 15:48:54 PDT
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
Comment 17 Phillip Mates 2021-10-11 03:11:21 PDT
(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!
Comment 18 Phillip Mates 2021-10-11 04:46:25 PDT
Created attachment 440773 [details]
Patch
Comment 19 Yusuke Suzuki 2021-10-12 18:38:22 PDT
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.
Comment 20 Phillip Mates 2021-10-13 05:42:40 PDT
Created attachment 441060 [details]
Patch
Comment 21 Yusuke Suzuki 2021-10-13 10:09:24 PDT
Comment on attachment 441060 [details]
Patch

r=me.
You also need to modify JavaScriptCore/DerivedSources-input.xcfilelist and JavaScriptCore/DerivedSources-output.xcfilelist
Comment 22 Phillip Mates 2021-10-14 02:11:46 PDT
Created attachment 441193 [details]
Patch
Comment 23 Ms2ger (he/him; ⌚ UTC+1/+2) 2021-10-14 02:19:36 PDT
Comment on attachment 441193 [details]
Patch

Already reviewed, and style matches surrounding code.
Comment 24 EWS 2021-10-14 03:22:18 PDT
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].
Comment 25 WebKit Commit Bot 2021-10-14 07:17:34 PDT
Re-opened since this is blocked by bug 231740
Comment 26 Lauro Moura 2021-10-18 20:21:59 PDT
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.
Comment 27 EWS 2021-10-18 21:07:31 PDT
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].
Comment 28 Yusuke Suzuki 2021-10-26 11:38:47 PDT
Looks like this patch did not update expectations.yaml properly.