-
Notifications
You must be signed in to change notification settings - Fork 84
Specify execution context #905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Rob--W
wants to merge
1
commit into
main
Choose a base branch
from
spec-execution-context
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a realm is the definition we should align on, but I was previously under the impression that world's today have some differences from realms. Is that the case and if so, are they different enough that we are just documenting something idealistic?
Feel free to ignore this if the above is not the case :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate what you are thinking of?
A realm is the pre-existing abstract description of the context where JS code executes.
In Firefox, we create XPConnect sandboxes, which creates a realm and sets up the globals. If objects are shared between the page's realm and the content script sandbox's realm, the object is wrapped in a XrayWrapper that offers controlled access (and restrictions) as needed. That mechanism is documented at https://firefox-source-docs.mozilla.org/dom/scriptSecurity/xray_vision.html
In Chrome, the JS execution environments are kept strictly separate, which is how Chromium enforces the isolation. That is documented at https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/bindings/core/v8/V8BindingDesign.md . That makes zero mention of a "realm", but at some point JS needs to execute, so if I were to treat Chromium's implementation as a black box, a realm seems like a good abstraction.
Although "isolated world" is originally a Chromium terminology that leaks outside through various API names, we should not specify it in terms of Chromium/v8 internals, but in existing standard concepts. This is the first time I write this kind of specs, so feedback is welcome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some further reading on realms and I think it's the right definition (@rdcronin to confirm).
I agree... almost. We definitely shouldn't specify things in terms of the internals of a given engine. However, we also shouldn't use a standard concept just because it is desirable if there is a meaningful delta between the behavior developers would expect if we properly implemented that concept vs. the reality of how engines work today.
As best as I understand it, a V8 context is essentially a realm, so using realms in the specification matches reality. If that's the case I'm onboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same concern as Oliver. While the concept of a realm in Ecma262 is very close to what we would use to describe an execution context for an extension, I don't think this is inherently true, nor that it is guaranteed to remain accurate. The spec for realms goes into details about the manner in which realms are created and initialized, and I'm not sure that this exact process is followed for v8 isolated worlds, nor do I think it is a fundamental requirement for them (that is, if a browser initialized its content script execution contexts differently from the exact realm spec, but in a way that still aligned with the properties we expect of an isolated world, I think that would still be fine from an extension perspective -- even though it would mean it deviated from the realm spec).
I'd prefer we avoid the "realm" usage here, instead using either nomenclature which is suitable abstract ("context") or defining our own with the set of principles required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in terms of standard terms, the concept of a ECMAScript Realm is the right choice here. I'll show that Chromium (+Blink+v8) does indeed have an implementation that (trivially) resembles (and augments) it.
(and Safari also has concepts similar to Chromium, as the basics of isolated worlds (DOMWrapperWorld) existed in WebKit before Chromium created Blink as a fork of WebKit)
The literal text in the ECMA spec you linked (which I already cited in my proposed specification text) is:
The first sentence already provides a basis for the argument that any JS engine should already recognize the concept of a realm. If we remove all special behavior, an isolated world is a place where JS code executes - which is exactly what a realm represents.
Note that I intentionally chose the ECMAScript definition (which has minimal requirements, it is browser-agnostic) instead of the augmented realm + environment settings object from the HTML spec (https://html.spec.whatwg.org/multipage/webappapis.html#realms-and-their-counterparts), exactly because the HTML spec version of it imposes many expectations that none of our implementations have.
Now I'm going to enumerate some Chromium implementation details:
script_state.hclass declaration starts with the following comment: "ScriptState is an abstraction class that holds all information about script execution (e.g., v8::Isolate, v8::Context, DOMWrapperWorld, ExecutionContext etc). If you need any info about the script execution, you're expected to pass around ScriptState in the code base. ScriptState is in a 1:1 relationship with v8::Context."ScriptStateImpl::Createis the glue between Blink and v8, and callsV8Initializer::InitializeContext(context, execution_context);(withcontextav8::Contextandexecution_contextthe "ExecutionContext" described above) and pairs the instance with aDOMWrapperWorld, together forming aScriptState.script_state.halso has several static methods onScriptStatethat use the "realm" terminology (ForCurrentRealm/ForRelevantRealm) that maps an arbitrary v8 object, viav8::Contextback toScriptState.v8::Context, and together with the documentation at https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/bindings/core/v8/V8BindingDesign.md establishes a firm link between the "realm" concept and "world".Chromium's point where it creates an isolated world is
IsolatedWorldManager::GetOrCreateIsolatedWorldForHost, which shows (among others) that the instantiation of an isolated world also includes an identifier for the extension (host_id).The definition of a Realm covers the Realm record and its fields/properties (including
[[HostDefined]]where anything goes) and doe not require the exact steps for creation to be followed. For example, Chromium's v8 engine does not even follow these steps, it takes the smarter approach of deserializing a snapshot. There is a human-readable write-up at https://v8.dev/blog/lazy-deserialization for example.What we care about are the observed behaviors. We need a place to execute (which realm provides), some indication that makes it different from the main world and other extensions (the world type, extension origin association), and a guarantee that the instrincs (builtin prototypes etc) operate on the same underlying state (which is a bit handwavy but we can expand later if we really want to). The text here is my attempt of expressing the requirements in terms of the bare minimum that is interoperable. I haven't even specified how to create an isolated world - as you remarked that would be too detailed at this stage.
In my first draft of this PR, I initially used
An <dfn>extension context</dfn> is any JavaScript execution context associated with an extension.This sounds good at first, but if we look up the ecma262 definition of Execution context, the concept specified there does not match what we want from it. The "realm" concept, to me at least, does.And then I continued looking for a specification concept capturing built-in DOM APIs and their prototype methods. These are specified by WebIDL and the platform object concept seems to be a perfect fit for that:
I think that "realm" is still not only a reasonable but also accurate way to serve as the basis for specifying the expected behaviors. I think that my attempt to specify extension contexts and isolated worlds is an accurate representation of how Chromium and Firefox behave. I'll need to refine the text, e.g. the
with its own [=global object=]part ofA <dfn>world</dfn> is a [=realm=] with its own [=global object=].is redundant since the definition of a realm already includes[[GlobalObject]]. But before I get there we first need to agree on the primitives used for the specification.