-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Editorial: Return created EC from InitializeHostDefinedRealm #3274
base: main
Are you sure you want to change the base?
Editorial: Return created EC from InitializeHostDefinedRealm #3274
Conversation
(can someone tell me why the IPR form check is failing? This isn't even my first contribution 😅) |
|
This still requires the HTML spec to grab the running execution context and remove it from the execution context stack. This seems to me like bad coupling (effectively global-data coupling via the EC stack): The HTML spec is relying on InitializeHostDefinedRealm (IHDR) having pushed the EC onto the EC stack, and the ES spec is sort-of relying on the HTML spec popping it from the EC stack. Moreover, HTML would be getting realm and realm execution context via different means, so one might want to assert that the returned realm is in fact realm execution context's Realm component, which kind of defeats the purpose. Instead, I think things would be cleaner if IHDR just returned the EC (having popped it from the EC stack, so that's not the caller's job any more). Sure, it isn't backwards compatible, but this PR is suggesting a change to the HTML spec anyway. So then the HTML spec would say:
Are there any such hosts? I.e., ones that are documented to invoke IHDR? I couldn't find any. If they do exist and they're currently invoking IHDR without popping the new EC from the EC stack, that could create problems. (Apparently, it lead to issues for HTML, see whatwg/html#3784, though I don't know what the issues were.) |
Thanks for the suggestions! I'm open to alternative solutions if the end result is a net positive :)
Interestingly that's also what we did in LibJS, although without popping it from the stack beforehand:
Depends on what you mean by documented - by means of another spec? Probably not, but outside of ECMA-402/HTML I'm not even aware of anything else having a dependency on ECMA-262. Known to exist? Sure, I know of at least two (https://codeberg.org/kiesel-js/kiesel/src/commit/3177581d63753baf525fac5d0151c78f039d09a9/src/cli.zig#L757-L758, https://github.com/SerenityOS/serenity/blob/99fbd33d7d887723372e861be27f2743d335ecb5/Userland/Libraries/LibJS/Runtime/VM.h#L339-L344), and not popping the EC was never problematic for either - that always seemed like a HTML-ism to me. |
da96f27
to
4ed1126
Compare
I've implemented your suggestion, PTAL! |
(This reply probably doesn't have much bearing on the PR.)
Cool.
Yup, that's what I meant. The details of how IHDR conveys an execution context to its caller are only relevant for a spec that defines how a host interacts with ECMA-262. The implementation can convey it however it likes (if it even has that separation), as your LibJS example shows.
I'm curious whether, for those implementations, leaving the EC on the stack was not just unproblematic but actually necessary. (ScriptEvaluation asserts that there's an underlying EC on the stack, but I don't understand why.)
Isn't LibJS used in Serenity's browser? (Doesn't it need to be concerned with HTML-isms?) |
4ed1126
to
4112041
Compare
Have updated
At least in LibJS it was primarily to ease creating objects in the "main" realm as things like "current realm" rely on an EC being on the stack. There certainly are ways around that if you can guarantee nothing downstream asks for the current realm.
Indeed, but for the most part is is a clean ECMA-262 implementation that doesn't try to concern itself with HTML-specific functionality (there are minor exceptions to this). Host-defined behavior is usually made opt-in by providing a callback to the AO that requires it, and host-defined data passed around as |
I also wonder if the only reason IHDR pushed newContext onto the EC stack was to make it accessible to HTML. So if IHDR instead returns newContext, does it even need to involve the EC stack? But that isn't something this PR needs to answer. |
4112041
to
947b436
Compare
This AO is not being used in ECMA-262 directly, but from real-world experience implementing & using is as-is I can say that having to pull the Realm out of the running execution context is somewhat awkward. This change is not backwards compatible with how the HTML spec uses the AO, but will allow simplifying the following: 1. Perform InitializeHostDefinedRealm() with the provided customizations for creating the global object and the global this binding. 2. Let realm execution context be the running JavaScript execution context. 3. Remove realm execution context from the JavaScript execution context stack. 4. Let realm be realm execution context's Realm component. to 1. Let realm execution context be InitializeHostDefinedRealm() with the provided customizations for creating the global object and the global this binding. 2. Let realm be realm execution context's Realm component. (https://html.spec.whatwg.org/#creating-a-new-javascript-realm) For hosts that don't have HTML's requirement of removing the running execution context, it becomes a simple: 1. Let executionContext be InitializeHostDefinedRealm(). 2. Let realm be executionContext.[[Realm]].
947b436
to
668b11d
Compare
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.
This seems fine to me, but this entire AO is odd (because it's old) because the host provides steps instead of providing definitions for host hooks.
Specifically, the steps for creating a new global object and for what the this value ought to be can be combined into a single host hook that returns a pair containing those things.
Since we're making HTML changes anyway, would you be up for doing that at the same time @linusg? I'd like to minimize the number of HTML spec PRs we'll need to make.
See also PR #3139 re eliminating some of this AO's oddness. I think it's mostly independent of this PR or the change you're suggesting, but you might want to consider it at the same time. |
I agree, that seems nicer!
Sure :) Looking at @jmdyck's PR again I very much prefer the unification of these AOs, would you be fine with merging that and me making these changes + an upstream HTML PR on top of it? |
@syg said:
It only needs to be a host hook if the ECMAScript implementation needs to pass some info to the host, and I don't think that's the case here. Specifically, in the HTML spec, the customizations for the global object are:
And the customizations for the global
The point being, none of these rely on info that IHDR has. So instead of adding a host hook, IHDR could just add two parameters. The HTML spec could then explicitly create the custom object(s) and pass them to IHDR. (Or, when it just wants the default, pass a special value, e.g. |
Or maybe they do. Apparently, when the HTML spec says something like "create a new Window object", that's a shorthand for "create a new object implementing the interface What's notable is that that algorithm has a required realm parameter, and yet none of the above customizations supply a realm. If the idea is that the implicit realm is the one that IHDR is about to create, then HTML can't create the objects beforehand and pass them to IHDR. @syg:
But if we treat it like other host hooks (declare a host-defined AO, say HostSupplyRealmGlobalStuff), that makes it inconvenient for the HTML spec, because it then has to define HostSupplyRealmGlobalStuff in a way that does different things depending on the context that's calling IHDR. So instead, I think it would be clearer if the 'hook' was an abstract closure that got passed to IHDR. The abstract closure would take one parameter, the realm that IHDR is in the process of creating, and would return the global object and global So a sample invocation of IHDR could be:
(In the HTML spec, that would be split by the |
Yes, this is exactly the case. |
This AO is not being used in ECMA-262 directly, but from real-world experience implementing & using is as-is I can say that having to pull the Realm out of the running execution context is somewhat awkward.
This change is not backwards compatible with how the HTML spec uses the AO, but will allow simplifying the following:
to
(https://html.spec.whatwg.org/#creating-a-new-javascript-realm)
For hosts that don't have HTML's requirement of removing the running
execution context, it becomes a simple: