-
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" fallout #473
Comments
Worth double-checking whether the new setup produces the same results as the old one.... In general, my feeling is that in an ideal world calling a callback would use the global of that callable as the entry settings thing, so calling a callback doesn't require passing in external state. This requires that all callables have an associated global, though. That's true for functions, and and IDL-defined objects with |
Also, a related issue is that https://html.spec.whatwg.org/multipage/webappapis.html#runtime-script-errors-in-documents assumes there is a script, which is not the case for a builtin function, bound or not, being called directly as a callback. I thought this used to be defined more clearly in a way that worked for all callables, but I might have been confused. |
It looks like the bug here is that bound functions are not created with a ScriptOrModule. They should be created with the current execution context's ScriptOrModule. I can PR ES to fix that. If that gets fixed then step 2 of GetActiveScriptOrModule() will return the bound function's execution context. Did you have something else in mind though?
This is unclear to me because IDL's "invoke" (which is what setTimeout references) is has not been up to date with HTML for a long time (way before my changes). It's unclear whether it uses "prepare to run a callback" and "clean up after running a callback". I guess if we fixed IDL it would:
In that case yes the entry settings object would be the settings object of the bound function. Which I believe would be the same as the incumbent per my above reasoning.
The intent was that only the very top-level contexts, which are basically a spec artifact, should be missing ScriptOrModule. You found a bug for bound functions, but otherwise it should be the case always.
Right. The idea is that when digging up the stack for the closest ScriptOrModule you have to skip over built-in functions.
That was supposed to link to https://html.spec.whatwg.org/#script-corresponding-to-the-running-execution-context. I have no idea why it does not :(. As explained above though there should always be such a script.
I think this has always been that way :-/. |
The commit message for tc39/ecma262@698819c gives details on how every execution context is supposed to get a ScriptOrModule. (There is a level above the top-level, which is the spec artifact I alluded to; the commit message doesn't mention those.) As you can see determining an execution context's ScriptOrModule often relies on determining some relevant function's [[ScriptOrModule]]. That is why bound functions not having [[ScriptOrModule]] breaks things. Even in the case of a built-in calling a built-in, as long as Web IDL's "invoke" uses https://html.spec.whatwg.org/multipage/webappapis.html#prepare-to-run-a-callback, step 4 will ensure that there is a Script-correlated execution context on the stack. |
Upon trying to fix bound functions I see that they're not actually broken. They don't even create an execution context when called or constructed, they just delegate to the original function. So they don't need [[ScriptOrModule]]. The execution context that executes will just be the execution context of the normal function. So, bound built-in functions have the problem to the same extent as other built-in functions do. With this in mind my above ideas for updating IDL seem wrong. I guess it needs to be more like:
Then when you invoke c you prepare to run a callback using c's call context and cleanup using the same. |
@domenic Could you stop, back up, for a second, and describe the actual behavior you're trying to achieve? There are three relevant things to consider when making a call to a callback, I believe:
Last time this was all discussed, I believe we all agreed on the following behavior, which was then implemented in Gecko, attempted to be implemented in the HTML spec, and at least bugs filed on the IDL spec:
Note that this setup means that when you want to run a callback you don't need to cart around any state except the callback itself: the entry settings object is deduced from the callable, and the incumbent settings object is captured by the callback creation. In particular, "prepare to run a callback" does not need an environment settings object as input as far as I can tell if this is the setup being implemented. Now, are you aiming for this behavior? If not, can you describe the behavior you are aiming for and point to where it was discussed and decided to switch to this other behavior? |
I am definitely aiming for that behavior! I was just trying to update it to work with ES6, and in the process it seemed much nicer if we could just phrase things in terms of the ES execution context (I think you mentioned this in a thread a while back). So there was some significant refactoring. I hope the end result ends up simpler than before, and requires less monkeypatching of ES; that is the goal. Not any normative changes. I can see how the scheme you describe, of going from callback -> Realm -> settings object, will be simpler for calling IDL callbacks. I'm not sure how to integrate it with the definitions of incumbent and entry settings object that also work in other situations (like calling functions which are not converted to IDL callbacks, or at the various points in the spec where we "jump to a code entry-point" for a script, or in eval contexts, or similar).
Prepare to run a callback has always taken a settings object in order to mark it as a candidate entry settings object. Do you have a different way of determining entry settings objects, than marking them during this step? I don't see how it's avoidable for scripts that are not IDL callbacks or JS function objects (like setTimeout strings or |
OK, good, we agree on the goals (including trying to do this in terms of ES execution contexts). Just wanted to double-check that before I continue digging into details. Good point about calling functions that are not IDL callbacks. That needs to set up an entry settings object, of course, and may need some sort of "capture the incumbent settings object" thing like IDL callbacks, for similar reasons. I think the only really relevant case here is Anyway, now I understand why Hixie kept saying that settings objects were in a many to one relationship with globals. In his setup, you had to have settings objects for the same global some of which were marked as entry settings objects and some were not. Specifically, consider the situation in which you do something like:
and So in the new world, we have two separate "JavaScript execution context"s here for the two invocations of |
I think I agree. Currently this is taken care of by https://html.spec.whatwg.org/#enqueuejob(queuename,-job,-arguments) which does the prepare/cleanup steps before/after running the job.
Yeah I agree with this as well. In #443 I will rename it to "prepare to run a script" since I am making changes in those areas anyway.
Oh dear. Looking things over, I broke this. Previously there was a stack of settings objects and you could mark entries in the stack as candidate entry settings object. I always interpreted this as the stack being of { settings object, is candidate entry settings object } pairs, so you could mark entries in the stack independent of marking the settings objects themselves. But in the current spec (i.e. after my recent refactor) it seems like it has just collapsed to "Label settings as a candidate entry settings object", which is not good. I think we should maintain the 1:1 settings object to global correspondence. It seems like the best approach is to mark JavaScript execution contexts as "candidate entry execution contexts". (From an execution context you can go execution context -> ScriptOrModule -> HTML "script" -> script's settings object.)
To make sure I am following: at what points in the previous spec do "candidate entry settings object" get marked? I am thinking they get marked in the IDL "Invoke" inside setTimeout's task, and the IDL "Invoke" inside dispatchEvent? Would you mind spelling this out in a bit more detail, preferably calling out which code lives in a different iframe or whatever so that we have two clearly distinct settings objects. |
Yeah, that would work too.
Worksforme, if we can manage it (i.e. if we never end up with different responsible browsing contexts or whatnot given a global)!
That makes sense, yes.
Yes, that was my understanding too. In my example, the idea was that |
OK, I think we have a plan, although the details of how I'm going to fix this are a little unclear--- I started typing them then confused myself. I think it will work itself out when I actually go to do the work, which I need to delay until after #443. I'll report back soon. |
See #167 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=27204. This makes it clear that environment settings objects, realms, and global objects are 1:1:1, giving explicit names and paths for getting from each to the other (of which 5/6 are already used in this spec). It also introduces the "new" concept of "current settings object" as a shorthand for "the current Realm Record's settings object", since that was already being used in two places. This does not yet fix #473, nor does it yet create definitions for {incumbent, entry, current} {settings object, Realm, global object} like I planned in #167 (comment).
See #167 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=27204. This makes it clear that environment settings objects, realms, and global objects are 1:1:1, giving explicit names and paths for getting from each to the other (of which all are already used in this spec). It also introduces the "new" concept of "current settings object" as a shorthand for "the current Realm Record's settings object", since that was already being used in two places. This does not yet fix #473, nor does it yet create definitions for {incumbent, entry, current} {settings object, Realm, global object} like I planned in #167 (comment).
See #167 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=27204. This makes it clear that environment settings objects, realms, and global objects are 1:1:1, giving explicit names and paths for getting from each to the other (of which all are already used in this spec). It also introduces the "new" concept of "current settings object" as a shorthand for "the current Realm Record's settings object", since that was already being used in two places. This does not yet fix #473, nor does it yet create definitions for {incumbent, entry, current} {settings object, Realm, global object} like I planned in #167 (comment).
Accidentally closed by my wording in 0866f1b ("does not yet fix" was seen by GitHub as "fix"). |
OK, I have cleared enough off my plate that I can make this my highest priority. @bzbarsky, if you would be OK paging this stuff back in, here is my current plan for entry settings object:
What remains is to ensure that we tag the appropriate execution contexts as candidates at all times. The mechanism for doing so is as follows:
This is a little roundabout---the frequent realm -> settings object -> realm execution context dance during the marking phase seems a bit silly, especially since we later go execution context -> realm -> settings object to get back "entry settings object". But I am pretty sure it is necessary if we want JavaScript to maintain the stack for us, since it is JavaScript's execution context stack that is the real stack in play here. |
This adds a new section under scripts detailing the entry, incumbent, current, and relevant concepts with regard to realms/global objects/ environment settings objects. This centralizes information that was previously somewhat spread out, and adds lots of details, as well as an example. This takes care of some of the pain points noted in #473, in particular fixing the definition of entry settings object to be more clearly defined in terms of the JavaScript execution context stack. There is still some work to do in that issue: namely, updating Web IDL with regard to tracking the entry settings object, and also making sure that incumbent settings object is correctly defined. This closes #167 by finally implementing the plan in #167 (comment) for making the correspondences clear.
This adds a new section under scripts detailing the entry, incumbent, current, and relevant concepts with regard to realms/global objects/ environment settings objects. This centralizes information that was previously somewhat spread out, and adds lots of details, as well as an example. This takes care of some of the pain points noted in #473, in particular fixing the definition of entry settings object to be more clearly defined in terms of the JavaScript execution context stack. There is still some work to do in that issue: namely, updating Web IDL with regard to tracking the entry settings object, and also making sure that incumbent settings object is correctly defined. This closes #167 by finally implementing the plan in #167 (comment) for making the correspondences clear.
This adds a new section under scripts detailing the entry, incumbent, current, and relevant concepts with regard to realms/global objects/ environment settings objects. This centralizes information that was previously somewhat spread out, and adds lots of details, as well as an example. This takes care of some of the pain points noted in #473, in particular fixing the definition of entry settings object to be more clearly defined in terms of the JavaScript execution context stack. There is still some work to do in that issue: namely, updating Web IDL with regard to tracking the entry settings object, and also making sure that incumbent settings object is correctly defined. This closes #167 by finally implementing the plan in #167 (comment) for making the correspondences clear.
@domenic So thinking about #473 (comment) I have the following questions:
As far as direct calls to author code go, Gecko handles those by requiring that all such calls be preceded by effectively selecting the global you think you're operating in, which in your terms would mark that global's Realm's realm execution context as an entry candidate and push it on the stack. Also, is my understanding correct that if we have this setup in place, then popping an entry candidate execution context off the stack is precisely when you report uncaught exceptions? |
That's a good question. I certainly assume all implementations have some kind of execution stack concept, but I'm not sure how well it maps to spec-level execution contexts. I think in the end it doesn't matter, since it's a spec device. It's a little worrying that this moves the spec's model further away from Gecko's though...
This was imprecise. Basically every push should be "push + tag" and every pop should be "pop + untag". #1091 takes care of this in more detail (for the run a classic/module script case and EnqueueJob case); see https://github.com/whatwg/html/pull/1091/files#diff-36cd38f49b9afa08222c0dc9ebfe35ebR87451). I will edit that description to be more precise.
I haven't investigated engine behavior here. I think R2 is more correct according to the ES spec authors, because GetFunctionRealm() will return R2. The same logic applies to bound functions, if you use the Function.prototype.bind from R1 on a function from R2. I'm not sure if they had any principles in mind when deciding how GetFunctionRealm() operates, besides perhaps the misguided notion that "only real functions have realm internal slots" or something.
Yeah, I think I would spec this as doing "prepare to run script" / "clean up after running script" before/after calling author code.
Yes. In "run a classic script", the error reporting step is immediately followed by the "clean up after running script" step. The latter is the one that pops the entry candidate execution context. I am not sure if the relative ordering between these two steps is observable, but I believe it's always been report, then pop/un-mark, even in the old days. |
Or put another way: if I |
Since there is no user function in the stack, that should still reduce to the top-level case. So, it should be I think I need to come up with a clearer proposal for how I mean to handle the top-level stuff so that I can more concretely say why I think this will work out fine. Thanks for reminding me of this case. |
For which top-level? How is that determined? Where did |
Here is the concrete proposal. New operation: GetFunctionScriptOrModule()
New version of GetActiveScriptScriptOrModule()
New definition of incumbent settings object
How this handles the tricky caseThe case in question is <!DOCTYPE html>
<iframe></iframe>
<script>
window.setTimeout(frames[0].postMessage.bind(frames[0], "x", "*"));
</script> The question is: what is the incumbent settings object referred to by the In this case GetActiveScriptOrModule() returns null: there are no functions with non-null [[ScriptOrModule]] values in the execution context stack. So we fall through to step 2, and return the entry settings object: which is This might not be right; it feels a little strange to use entry settings object as the fallback. But if it's not please give the example where it fails. |
The entry settings object in the case I posited is |
Or put another way: When invoking an async callback the entry settings object is a property of the callback being invoked. The incumbent settings object should be a property of what code was running when we queued the async operation that invokes the callback. These are not the same thing, in general. |
I see. Thanks, I am now convinced that we need to store the incumbent settings object on conversion to an IDL callback type. We can use IDL's "callback context" for this; currently it stores the "incumbent script" which I believe has not been a concept since before I started editing. New concept: the backup incumbent settings object stackThis is a new stack associated with each event loop. IDL modifications
HTML modifications
New definition of incumbent settings object
How this handles the tricky case<!DOCTYPE html>
<iframe></iframe>
<script>
window.setTimeout(frames[0].postMessage.bind(frames[0], "x", "*"));
</script> Upon calling setTimeout:
Inside the task posted by setTimeout:
Inside the postMessage algorithm:
This seems a bit unfortunately complicated, but I'm not sure there's any better way. The "dual stack" approach seems to have shades of the Firefox implementation you described in #473 (comment), but I can't really tell if there's a clear correspondence. Hope this hasn't gone too far off the rails... |
That approach is basically what hixie's spec was aiming at, I believe. You're right that it's pretty much identical to the Firefox implementation; the one difference is that in Firefox the "global script" case is represented by the equivalent of the execution context stack because we have a way of getting directly from a JS stackframe, including a global one, to a corresponding global. So we only use the backup stack for the saved incumbent thing. One other issue that probably needs addressing: the setup described in #473 (comment) only affects IDL callbacks. We should at least think about what happens with promise callbacks. Ideally those would do the same thing (capture the incumbent settings at the point of the |
This makes sense. I guess the corresponding way of doing this here would be to not push/pop in prepare to run a script/clean up after running a script, and then modify "New definition of incumbent settings object" to return the entry settings object if the backup incumbent settings object stack is empty.
Hmm, interesting. This seems like it would require changes in ES that are not very clean :(. That is, I can't figure out a way to phrase it in terms of ScriptOrModule; we need to store the actual incumbent settings object in order to take care of cases like As an aside, this kind of store-alongside-the-callback for the purposes of "context propagation" is very similar to the zones proposal, which also modifies promises. The big difference being that zones are author-controlled, I guess. |
Ah, it turns out that when we run a script or module, we push two stacks onto the execution context stack: the realm execution context, and then ScriptEvaluation/ModuleEvaluation pushes a new context specific to that script. That one has an appropriate ScriptOrModule pointer. This needs some tweaks to GetActiveScriptOrModule, but we can make it work. |
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.
What I don't quite understand in this discussion is that it sounds like Firefox is the only one with this setup. Presumably that is not true right, or do all other browsers have severe issues? |
I can't tell you what other browsers do. Someone just needs to write testcases and measure. I think in practice for the The other obvious use of incumbent global is to derive the referrer in the various |
Right, my impression is that Firefox has taken a principled approach here and collaborated with the spec authors on previous iterations of this. So the first goal is to just get things coherent across ES6 + IDL + HTML. (Previously IDL was lagging behind, and HTML was monkeypatching FunctionBody from ES5, which might have continued to work in ES6 but was pretty gross.) After #1189 lands (and I think this bug is closeable then), I plan to work on the Bugzilla bugs for "investigate incumbent usage" and "investigate entry usage". That will involve writing up lots of test cases for all the places in the spec that use those. If we find divergences that indicate some flexibility to change them, we can start discussing moving those to current, since that's desirable in almost all cases. |
<3 @domenic |
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.
I think this is largely fixed, with a number of remaining issues open. Check out the new "topic: multiple globals" tag to track anything remaining. |
@bzbarsky noticed a number of problems #401 (comment). Opening this issue so they don't get lost.
The text was updated successfully, but these errors were encountered: