[ShadowRealm] Part 1 - Skeleton [v8/v8 : main]
legendecas (Gerrit)
Attention is currently required from: legendecas.
Set Ready For Review
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
legendecas (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, legendecas.
legendecas removed Michael Hablich from this change.
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
legendecas (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest.
1 comment:
Patchset:
Hi, Toon, Shu. This is the bootstrap of ShadowRealm proposal. May I have your review on this? Thanks!
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: legendecas, Toon Verwaest.
11 comments:
Patchset:
Sorry for the very late review, legendecas.
The main thing here is that a design doc is warranted for the API interface for how hosts should hook into creation of the ShadowRealm globals, as well as how those globals delegate answers up to their creator realm.
Could you please write up a Google doc, and then we can get the relevant V8 and blink engineers to review?
File src/builtins/accessors.h:
Patch Set #5, Line 52:
kHasSideEffectToReceiver)Neither length nor name are specified in the spec draft AFAICT. I've filed https://github.com/tc39/proposal-shadowrealm/issues/338
File src/builtins/builtins-shadow-realms.cc:
Patch Set #5, Line 12:
BUILTIN(ShadowRealmConstructor) {Could you copy/paste the spec steps into these builtins please?
Patch Set #5, Line 33:
DeserializeEmbedderFieldsCallback(), nullptr);This part isn't right, though not all details are known yet pending HTML integration:
- The host needs a way to hook in the extensions and the object template. This probably requires API changes.
- The shadow realm needs to know its parent realm to get at the settings object for web API. Where are you storing this info?
- I'm not sure if a JSGlobalProxy is necessary here, since we don't need the identity-preserving semantics of being able to swap out its guts. However, we probably do want to be able to leverage the existing security checks for access globals.
I think the next step here is to write up a design doc for what kind of global object ShadowRealm globals should be, what API extensions are needed, and how blink should hook into it, so we can get reviews across different teams.
File src/diagnostics/objects-debug.cc:
void JSShadowRealm::JSShadowRealmVerify(Isolate* isolate) {
TorqueGeneratedClassVerifiers::JSShadowRealmVerify(*this, isolate);
}
void JSWrappedFunction::JSWrappedFunctionVerify(Isolate* isolate) {
TorqueGeneratedClassVerifiers::JSWrappedFunctionVerify(*this, isolate);
}These seem like they could be USE_TORQUE_VERIFIER
File src/init/bootstrapper.cc:
Patch Set #5, Line 4413:
{ // --- W r a p p e d F u n c t i onNit: missing space between o and n :)
Patch Set #5, Line 4430:
boundThis should be wrapped instead of bound.
Patch Set #5, Line 4437:
boundThis should be wrapped instead of bound.
File src/init/heap-symbols.h:
Patch Set #5, Line 412:
V(_, wrapped_function_string, "wrapped function") \Is this needed?
File src/objects/contexts.h:
Patch Set #5, Line 267:
V(SHADOW_REALM_FUN_INDEX, JSFunction, shadow_realm_fun) \Is this needed by anything?
File src/objects/js-function.cc:
Patch Set #5, Line 245:
return isolate->factory()->wrapped_function_string();This doesn't seem right. Intuitively I would expect the name to be target.name, not a static "wrapped function" string.
I also don't see the "name" instance variable specified in the spec draft. I've filed https://github.com/tc39/proposal-shadowrealm/issues/338
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
legendecas (Gerrit)
Attention is currently required from: Shu-yu Guo.
Patch set 7:Commit-Queue +1
11 comments:
Patchset:
This is an update to reflect the shape in the design doc.
File src/builtins/accessors.h:
Patch Set #5, Line 52:
kHasSideEffectToReceiver)
Neither length nor name are specified in the spec draft AFAICT. I've filed https://github. […]
The spec is not finalized yet. So I'd remove these for now.
File src/builtins/builtins-shadow-realms.cc:
Patch Set #5, Line 12:
BUILTIN(ShadowRealmConstructor) {Could you copy/paste the spec steps into these builtins please?
Done
Patch Set #5, Line 33:
DeserializeEmbedderFieldsCallback(), nullptr);
This part isn't right, though not all details are known yet pending HTML integration: […]
Updated with aligning to the current design doc https://docs.google.com/document/d/1k9OZxtbKmzENOMzahmm2C0uBNRc_pVLXMSoKsrkJJBw/edit#heading=h.3jxz4ihz2wrb (I know the design doc is not LGTM to any reviewers yet. I'd be happy to update if there is anything changed in the doc).
File src/diagnostics/objects-debug.cc:
void JSShadowRealm::JSShadowRealmVerify(Isolate* isolate) {
TorqueGeneratedClassVerifiers::JSShadowRealmVerify(*this, isolate);
}
void JSWrappedFunction::JSWrappedFunctionVerify(Isolate* isolate) {
TorqueGeneratedClassVerifiers::JSWrappedFunctionVerify(*this, isolate);
}These seem like they could be USE_TORQUE_VERIFIER
Done
File src/init/bootstrapper.cc:
Patch Set #5, Line 4413:
{ // --- W r a p p e d F u n c t i onNit: missing space between o and n :)
Done
Patch Set #5, Line 4430:
boundThis should be wrapped instead of bound.
Done
Patch Set #5, Line 4437:
boundThis should be wrapped instead of bound.
Done
File src/init/heap-symbols.h:
Patch Set #5, Line 412:
V(_, wrapped_function_string, "wrapped function") \Is this needed?
Removed.
File src/objects/contexts.h:
Patch Set #5, Line 267:
V(SHADOW_REALM_FUN_INDEX, JSFunction, shadow_realm_fun) \Is this needed by anything?
Removed
File src/objects/js-function.cc:
Patch Set #5, Line 245:
return isolate->factory()->wrapped_function_string();
This doesn't seem right. Intuitively I would expect the name to be target. […]
The spec is not finalized yet. So I'd remove these for now.
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Chengzhong Wu (Gerrit)
Patchset:
Hi, folks. Thanks for reviewing the design doc! I've resolved the conflicts and updated the CL to reflect the host hooks design doc (https://docs.google.com/document/d/1k9OZxtbKmzENOMzahmm2C0uBNRc_pVLXMSoKsrkJJBw/edit). For the testing completeness of this CL, I've removed the WrappedFunction definition in this CL since it is not reachable in anyways from the current implementation and is not feasible to get tested. I'll submit a follow-up CL for ShadowRealm.prototype.evaluate and the WrappedFunction once this large CL gets landed. Please take a look, thanks a lot :)
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Chengzhong Wu, Toon Verwaest.
8 comments:
Patchset:
Mostly looks good, thank you for the CL!
Looks like there're merge conflicts again, and perhaps double-check with the newest test262 roll if the status list is update to date.
File src/builtins/builtins-shadow-realms.cc:
Patch Set #11, Line 49:
isolate->factory()->NewFastOrSlowJSObjectFromMapCould you use JSObject::New here instead of manually getting the derived map?
if (!args.new_target()->IsUndefined(isolate)) { // [[Construct]]
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kConstructorNotFunction,
isolate->factory()->ShadowRealm_string()));
}You don't need to manually check for 'new', it's already done before we enter here.
if (!args.new_target()->IsUndefined(isolate)) { // [[Construct]]
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kConstructorNotFunction,
isolate->factory()->ShadowRealm_string()));
}You don't need to manually check for 'new', it's already done before we enter here.
File src/execution/isolate.cc:
Patch Set #11, Line 4689:
return Handle<NativeContext>::cast(shadow_realm_context);Hm, is there a good reason to support a default implementation? We could just throw kUnsupported if the callback is not set.
d8 should set its own, if you put the default implementation here for testing in d8.
File test/cctest/test-api.cc:
Patch Set #11, Line 26617:
v8::V8::SetFlagsFromString("--harmony-shadow-realm");cctests can get away with doing i::FLAG_harmony_shadow_realm = true directly.
Patch Set #11, Line 26634:
CHECK(result->IsObject());Please also check that the internal object IsJSShadowRealm()
File test/test262/test262.status:
Patch Set #11, Line 394:
# https://github.com/tc39/test262/pull/3312 unexpected passThanks for fixing this upstream!
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Chengzhong Wu (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest.
Patch set 14:-Commit-Queue
8 comments:
Patchset:
Updated, PTAL again :)
File src/builtins/builtins-shadow-realms.cc:
Patch Set #5, Line 33:
DeserializeEmbedderFieldsCallback(), nullptr);Updated with aligning to the current design doc https://docs.google. […]
Done
File src/builtins/builtins-shadow-realms.cc:
Patch Set #11, Line 49:
isolate->factory()->NewFastOrSlowJSObjectFromMapCould you use JSObject::New here instead of manually getting the derived map?
Done
if (!args.new_target()->IsUndefined(isolate)) { // [[Construct]]
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kConstructorNotFunction,
isolate->factory()->ShadowRealm_string()));
}You don't need to manually check for 'new', it's already done before we enter here.
Done
if (!args.new_target()->IsUndefined(isolate)) { // [[Construct]]
THROW_NEW_ERROR_RETURN_FAILURE(
isolate, NewTypeError(MessageTemplate::kConstructorNotFunction,
isolate->factory()->ShadowRealm_string()));
}You don't need to manually check for 'new', it's already done before we enter here.
Done
File src/execution/isolate.cc:
Patch Set #11, Line 4689:
return Handle<NativeContext>::cast(shadow_realm_context);
Hm, is there a good reason to support a default implementation? We could just throw kUnsupported if […]
Done
File test/cctest/test-api.cc:
Patch Set #11, Line 26617:
v8::V8::SetFlagsFromString("--harmony-shadow-realm");cctests can get away with doing i::FLAG_harmony_shadow_realm = true directly.
Done
Patch Set #11, Line 26634:
CHECK(result->IsObject());Please also check that the internal object IsJSShadowRealm()
Done
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Chengzhong Wu, Toon Verwaest.
Patch set 14:Code-Review +1
1 comment:
Patchset:
lgtm, thanks!
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Chengzhong Wu, Toon Verwaest.
1 comment:
Patchset:
I'm not an owner on all the subdirectories. Toon, mind doing an OWNER stamp?
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Chengzhong Wu.
To view, visit change 3195532. To unsubscribe, or for help writing mail filters, visit settings.
Chengzhong Wu (Gerrit)
Attention is currently required from: Chengzhong Wu.
Patch set 15:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/v8heapconst.py
Insertions: 121, Deletions: 120.
The diff is too large to show. Please review the diff.
```
[ShadowRealm] Part 1 - Skeleton
1. Expose all the functions to empty builtins.
2. Wire up the basic structure of ShadowRealm and internal slots.
Bug: v8:11989
Change-Id: If7545fe18a74b2bd4b70a1a25776e41f03aaff89
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3195532
Reviewed-by: Shu-yu Guo <s...@chromium.org>
Reviewed-by: Toon Verwaest <verw...@chromium.org>
Commit-Queue: Chengzhong Wu <legen...@gmail.com>
Cr-Commit-Position: refs/heads/main@{#78757}
---
M BUILD.bazel
M BUILD.gn
M include/v8-callbacks.h
M include/v8-isolate.h
M src/api/api.cc
M src/builtins/base.tq
M src/builtins/builtins-definitions.h
A src/builtins/builtins-shadow-realms.cc
M src/compiler/types.cc
M src/d8/d8.cc
M src/d8/d8.h
M src/diagnostics/objects-debug.cc
M src/diagnostics/objects-printer.cc
M src/execution/isolate.cc
M src/execution/isolate.h
M src/flags/flag-definitions.h
M src/init/bootstrapper.cc
M src/init/heap-symbols.h
M src/objects/all-objects-inl.h
M src/objects/js-function.cc
M src/objects/js-objects.cc
A src/objects/js-shadow-realms-inl.h
A src/objects/js-shadow-realms.h
A src/objects/js-shadow-realms.tq
M src/objects/map.cc
M src/objects/object-list-macros.h
M src/objects/objects-body-descriptors-inl.h
M src/objects/objects.h
M test/cctest/test-api.cc
M test/test262/test262.status
M test/test262/testcfg.py
M tools/v8heapconst.py
32 files changed, 468 insertions(+), 111 deletions(-)