-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Rewrite script execution on top of ES #401
Conversation
da503bf
to
dcfaadc
Compare
Addressed most review comments, in a separate commit for ease of reviewing. The GitHub UI seems to have lost them so here is the link to the original commit where they were present: da503bf |
(I made the comments on the commit directly as the other commits were not applicable to this PR. But I guess that means they do not get tracked as part of the PR review, which seems sad.) |
@@ -77557,6 +77521,9 @@ dictionary <dfn>DragEventInit</dfn> : <span>MouseEventInit</span> { | |||
navigations occur.</p> | |||
</li> | |||
|
|||
<li><p>Let <var>realm execution context</var> be the <span>JavaScript execution context</span> | |||
created.</p></li> |
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 realize I suggested this, but looking at it "created" probably makes more sense to appear before the term, don't you think?
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's a bit of a weird case; I'm happy to go with whatever. Maybe I should patch ES to return the execution context from InitializeHostDefinedRealm() instead.
Is it fine to not always write realm as Realm? I think other than that this is okay, though having explicit review from the IDL editors would be great. Not sure if @heycam is on vacation too though. |
I opened tc39/ecma262#252 about the capitalization of realm. I think it's an editorial issue with ES and they should not capitalize. |
edd2410
to
0cd82d3
Compare
I added an additional commit addressing #193, please take a look. Also it looks like tc39/ecma262#242 is about to be merged so I'm looking for final LGTMs. |
OK, I guess ES is using Realm to mean Realm Record, per tc39/ecma262#252. I have capitalized it for the one place it is used in that sense here. The other places are in "realm execution context" where it is an adjective so I left it lowercase. |
Newline nit fixed; ready for more review. |
<p>If the user agent does not <span>support the scripting language</span> given by <var | ||
data-x="concept-script-type">the script block's type</var> for this <code>script</code> element, | ||
then the user agent must abort these steps at this point. The script is not executed.</p> | ||
<p>Determine whether the <var>the script block's type</var> is a component-wise <span>ASCII |
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.
What does component-wise mean? Seems like that can be removed without changing the meaning?
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.
The idea was to match the previous text (in "support the scripting language"):
if each component of the script block's type is an ASCII case-insensitive match for the corresponding component in the MIME type string of a scripting language that the user agent implements.
I considered other phrasing, like
For each JavaScript MIME type, check whether the script block's type is an ASCII case-insensitive match for the corresponding component in the JavaScript MIME type string. If all such checks fail, then the user agent must abort these steps at this point.
But that seemed verbose and a bit confusing.
Upon reflection I think I agree you can just do a string comparison, so I will amend it as you suggest.
I guess I should add back "do not merge yet" until @bterlson pushes another snapshot for the JS spec. Bah, continuous deployment is so much nicer :). |
I'll just note that what I reviewed looks good. I would be more comfortable with review from the IDL folks, but since @domenic is committed to maintaining this I'm fine with merging this once ECMAScript is ready and then seeking review from IDL folks on the merged text. |
This brings the script execution parts of the spec up to date with the latest changes in ES. Notable changes include: - Removed of some generalization to allow non-JavaScript scripting languages (including XML-based ones). - Let ES track the execution context stack, and the corresponding stack of scripts; see tc39/ecma262#242. This allows us to stop tracking the stack of script settings objects, instead defining entry and incumbent settings objects with reference to ES. This is especially important since our current mechanism, of monkey-patching the SourceElement grammatical construction, no longer works since that grammar production has disappeared. Fixes #155. - Established a direct correspondence between HTML's "script" concept and ES's Script Record/Module Record concepts. The former is stored in the [[HostDefined]] slot of the latter. - The process of parsing and executing scripts, including handling any resulting errors, is now formalized and appropriately calls out to ES's ParseScript and ScriptEvaluation abstract operations. Note that we do *not* use the ECMAScript ScriptEvaluationJob or job queue for our script parsing/evaluation. That setup is overengineered and does not serve the needs of HTML; we hope to remove it from ES eventually. (See tc39/ecma262#240 (comment) for details.) Instead we simply use ParseScript and EvaluateScript directly. This almost completes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25981, with the remaining work item being to integrate promise jobs and ensure they are run as microtasks.
This fixes #193, which notes that importScripts() previously sent errors down the "report an exception" path instead of rethrowing them. It also makes a slight change of rethrowing any early errors that are not SyntaxErrors, instead of converting early ReferenceErrors into SyntaxErrors.
This integrates with the ES job queue for promise jobs, by overriding ES's EnqueueJob and NextJob to delegate to the microtask queue. This finally gives us a normative specification for how promises interface with the browser event loop. This approach of monkeypatching ES is necessary for now. See discussions in tc39/ecma262#240 (comment) about how this is the least-bad alternative. We hope to update ES to delegate to the host for those operations in the future, instead of overwriting ES. That possibility is discussed in replies to the linked comment. Also fixes a few uses of "ECMAScript"; this specification prefers "JavaScript". This completes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25981.
@domenic The new definition of incumbent settings object seems broken to me. In particular, I don't see how propagating it across Web IDL callbacks works. Think things like:
and what the incumbent settings object will be when I assume the entry settings object in this case will be something sane-ish, in that it will be associated with the |
Oh, and importantly are there ever JavaScript execution contexts that do not have a script or a module associated with them? If not, why not? What is the ScriptOrModule when the callback function is called in the case above? |
Digging into the ES end of this a bit more, builtin functions have A bound function isn't even a Function object in terms of the ES spec, afaict. It's created in https://tc39.github.io/ecma262/#sec-boundfunctioncreate and just has a special |
@domenic As far as entry settings objects... I'm not sure what "the script corresponding to the running execution context" is in the EnqueueJob section in the HTML spec. Why would there be such a script at all? See builtin functions above. I'm not quite sure what used to set up entry settings objects for the case of callbacks backed by either builtin functions or worse yet bound builtin functions. And worse yet bound builtin where the
I believe the real intent of the spec as it stood before these changes is that this should log "Parent error" followed by "Child error", though it's worth testing what UAs actually do here. At least IE and Firefox and the spec used to agree on the first thing being logged being "Parent error" in this case, last I checked. |
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce the "backup incumbent settings object stack", which takes care of these trickier cases. This commit also adds a few examples of how exactly incumbent settings object calculation works, especially in the case where the backup incumbent settings object stack ends up mattering. For this story to be fully coherent, we will also need the minor fixes found in tc39/ecma262#556, as well as further revisions to Web IDL to update it for this new framework. This commit is the first step, however.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce the "backup incumbent settings object stack", which takes care of these trickier cases. This commit also adds a few examples of how exactly incumbent settings object calculation works, especially in the case where the backup incumbent settings object stack ends up mattering. For this story to be fully coherent, we will also need the minor fixes found in tc39/ecma262#556, as well as further revisions to Web IDL to update it for this new framework. This commit is the first step, however.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce the "backup incumbent settings object stack", which takes care of these trickier cases. This commit also adds a few examples of how exactly incumbent settings object calculation works, especially in the case where the backup incumbent settings object stack ends up mattering. For this story to be fully coherent, we will also need the minor fixes found in tc39/ecma262#556, as well as further revisions to Web IDL to update it for this new framework. This commit is the first step, however.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce the "backup incumbent settings object stack", which takes care of these trickier cases. This commit also adds a few examples of how exactly incumbent settings object calculation works, especially in the case where the backup incumbent settings object stack ends up mattering. For this story to be fully coherent, we will also need the minor fixes found in tc39/ecma262#556, as well as further revisions to Web IDL to update it for this new framework. This commit is the first step, however.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce several new concepts, which takes care of these trickier examples. These examples are now included in the spec, and spell out exactly how exactly incumbent settings object calculation works in increasingly-complex scenarios. The new algorithms "prepare to run a callback" and "clean up after running a callback" will be used by Web IDL, similarly to how it already uses "prepare to run script" and "clean up after running script." Another notable change is that EnqueueJob now correctly tracks the necessary goings-on in order to make the incumbent settings object work correctly when promises are used to schedule callbacks.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce several new concepts, which takes care of these trickier examples. These examples are now included in the spec, and spell out exactly how exactly incumbent settings object calculation works in increasingly-complex scenarios. The new algorithms "prepare to run a callback" and "clean up after running a callback" will be used by Web IDL, similarly to how it already uses "prepare to run script" and "clean up after running script." Another notable change is that EnqueueJob now correctly tracks the necessary goings-on in order to make the incumbent settings object work correctly when promises are used to schedule callbacks.
As discussed in #473, starting especially from around #473 (comment), the definition of incumbent introduced in #401 falls down in certain important cases. In order to fix this, we introduce several new concepts, which takes care of these trickier examples. These examples are now included in the spec, and spell out exactly how exactly incumbent settings object calculation works in increasingly-complex scenarios. The new algorithms "prepare to run a callback" and "clean up after running a callback" will be used by Web IDL, similarly to how it already uses "prepare to run script" and "clean up after running script." Another notable change is that EnqueueJob now correctly tracks the necessary goings-on in order to make the incumbent settings object work correctly when promises are used to schedule callbacks. However, we noticed that it does not correctly track the entry settings object; the previous text, introduced in #1091, incorrectly referred to job.[[Realm]], which does not exist. The correct fix is unfortunately not obvious. So we add a warning there for now, with #1426 tracking further work.
This is on top of #373; the first three commits are just that PR. However, I am making a separate PR because the changes here depend on tc39/ecma262#242, so this is not mergeable quite yet. It's ready for review though.
This brings the script execution parts of the spec up to date with the latest changes in ES. Notable changes include:
Note that we do not use the ECMAScript ScriptEvaluationJob or job queue for our script parsing/evaluation. That setup is overengineered and does not serve the needs of HTML; we hope to remove it from ES eventually. (See tc39/ecma262#240 (comment) for details.) Instead we simply use ParseScript and EvaluateScript directly.
This almost completes https://www.w3.org/Bugs/Public/show_bug.cgi?id=25981, with the remaining work item being to integrate promise jobs and ensure they are run as microtasks.