Modernize invoking user code

[domenic]

This updates the four algorithms that invoke user code in the following ways:

• Updates them to use HTML's latest framework for managing script settings objects. The previous version referred to concepts, such as the "stack of incumbent scripts", which have not been defined for a long time (i.e. since before Rewrite script execution on top of ES html#401). Updates in Rewrite script execution on top of ES html#401 and Add a new section detailing the various potential realms html#1091 have changed the setup here. There is still some ongoing discussion in "Rewrite script execution on top of ES" fallout html#473 about whether the current HTML setup is correct for managing incumbent settings objects, but this at least updates the algorithms to correctly manage entry settings objects. The plan implemented here is roughly that discussed in "Rewrite script execution on top of ES" fallout html#473 (comment).
• Gives them explicit <dfn>s with explicit arguments to be passed.
• Updates them to use some slightly-more-modern ES technology such as Call(), Get(), and Set().

This also updates all ECMAScript references to point to the latest spec.

Review appreciated @bzbarsky. As I say above, incumbent might be a bit messed up at the moment, but I am pretty sure this is a strict improvement. I wanted to finish off fixing "entry"; now I need to go off an take care of incumbent.

[Ms2ger]

I think I'd prefer to land #114 first, so this PR only has relevant changes.

[Ms2ger]

Rebased at https://github.com/Ms2ger/webidl/tree/modernize-invoke

[domenic]

Thanks, reset my branch to yours so this should be mergeable now.

[Ms2ger]

Rebased again, since I keep bitrotting you

[domenic]

Reset this PR to your commit, thanks.

Ping @bzbarsky to review?

[bzbarsky]

"list of IDL argument" either needs an 's' at the end or the restoration of the "values" that went missing. Preferably the latter.

[domenic]

Fixed and pushed a new commit.

BTW you may find reviewing the XML file slightly more enjoyable than reviewing the generated output.

[bzbarsky]

Hmm, I must be missing what the connection is between entry settings object and exception handling?

See whatwg/html#958 (comment)

Maybe that was agreed upon in some earlier thread I forgot or was before my time.

Before your time.

To match Chrome's behavior, I guess we would have event dispatch specifically report errors to the EventTarget object's relevant settings object.

I don't think we should match Chrome's behavior. I think Chrome should align with what the spec at least used to say before people started messing with it and what everyone agreed we should do the last time this was discussed. Specifically, in the simple case of a function callback the error is reporter to the global of that function. The hard cases can then be figured out, as long as the simple case works correctly.

One solution is to do here what I did below for "get a user object's attribute value": use value's callback context's settings object's realm execution context.

The "callback context" thing was added to propagate the incumbent, right? I don't think it's the right thing.

I guess it would give B in both cases?

No, as far as I can tell it would give D: the window that made the addEventListener call.

Another solution is to acknowledge that all objects have a realm, not just functions, and then use O's realm.

That's basically what Firefox does.

new Function uses the currently active script-or-module to fill the created function's [[ScriptOrModule]]. The currently active script-or-module is your main-document <script> tag.

Hrmm. That seems ... weird. I think people using new someWindow.Function would expect that to act just like a function declared in someWindow. It claims to be instanceof that window's Function, for sure.

But looking at the actual spec at http://www.ecma-international.org/ecma-262/6.0/#sec-function-constructor it gets the "active function object". This is defined as:

The value of the Realm component of the running execution context is also called the current Realm. The value of the Function component of the running execution context is also called the active function object.

and in this case it gets set up by http://www.ecma-international.org/ecma-262/6.0/#sec-built-in-function-objects-construct-argumentslist-newtarget which references http://www.ecma-international.org/ecma-262/6.0/#sec-built-in-function-objects-call-thisargument-argumentslist which sets the active function object to the built-in function being called/constructed. Of course there is no script-or-module in any of that stuff...

This is window A in both your examples. (But, if we modified them to declare the function in window B or C, it would be window B or C.)

I think this would confuse people a lot. People expect functions created with new someWindow.Function to act like functions defined in that window, from everything I've seen.

[domenic]

Of course there is no script-or-module in any of that stuff...

Right, that's in the modern spec at https://tc39.github.io/ecma262/#sec-functioninitialize

I think this would confuse people a lot. People expect functions created with new someWindow.Function to act like functions defined in that window, from everything I've seen.

So what filename should be reported when reporting these exceptions?

[bzbarsky]

So what filename should be reported when reporting these exceptions?

Ah, hmm. Having that be the caller of the Function constructor does make some sense. OK, but just to check my understanding: if code in window1 does var f = new window2.Function("stuff") then f has window1 as its script-or-module and window2 as its Realm?

If so, I probably need to go back and think really carefully about all the cases where you propose using script-or-module...

[domenic]

if code in window1 does var f = new window2.Function("stuff") then f has window1 as its script-or-module and window2 as its Realm?

Yes, exactly.

As I said, I see two alternatives here. To rephrase them slightly:

• "report an exception" takes a script. It uses that script's filename, and dispatches error events to that script's global.
• "report an exception" takes a script and a global. It uses that script's filename, but dispatches error events to the supplied global, which may not equal the script's global.

In your example quoted above, the first alternative dispatches error events to window1, whereas the second allows the possibility of dispatching them to window2 if we thread everything through correctly.

[domenic]

OK, but there's also this tricky case, as you pointed out:

el.addEventListener("click", el.cloneNode); // purposefully not bound

When "inner invoke" invokes the IDL callback created from el.cloneNode, which of course throws an exception due to invalid this, there's no script possible to pass to "report an exception".

I can't imagine a relevant filename for this, unless we wanted to try to use the callback context, and make it capture the "incumbent script" (i.e. the script that did addEventListener). Probably it makes more sense to say that there's no filename that's responsible for throwing, since it's really browser code that's doing the if (isNotCorrectlyBranded(this)) { throw new TypeError(); }.

So it seems like the approach of having "report an exception" only take a script, and derive the correct global from that script, won't work, unless we capture the incumbent script. Which sounds pretty different from anything anyone is doing, so probably we shouldn't do that...

[bzbarsky]

Or more precisely, it has whatever the window1 script was that called the Function ctor as script-or-module. But the point is that the [[Realm]] of the [[ScriptOrModule]] of the function involved doesn't match the [[Realm]] of the function itself (and this is the only way to produce this situation!), so it really starts to matter how we go about getting to the Realm.

For what it's worth, at least in Firefox in this case the function looks in all ways like it came from window2, except that the "filename" string is something like "urlofWindow1 line N > Function". Note that this is NOT the same thing as "urlofWindow1", which is what I think you're saying the current setup in the spec does, and is a lot more useful...

Actually, I just went ahead and put up a testcase so we can see what UAs do here: http://web.mit.edu/bzbarsky/www/testcases/javascript/newFunction-parent.html. The output for the top stack frame is the following in various browsers:

• Firefox: anonymous@http://web.mit.edu/bzbarsky/www/testcases/javascript/newFunction-parent.html line 5 > Function:1:7
• Chrome: at eval (eval at onload (http://web.mit.edu/bzbarsky/www/testcases/javascript/newFunction-parent.html:5:13), <anonymous>:2:7)
• Safari: anonymous
• Edge: `at Function code (Function code:1:1)

none of those have "http://web.mit.edu/bzbarsky/www/testcases/javascript/newFunction-parent.html" as the filename, and Chrome and Firefox have something much more useful instead. I don't think we really want to be using the script-or-module of the caller of new Function to set the filename if it would require all of them to claim that this error occured in "http://web.mit.edu/bzbarsky/www/testcases/javascript/newFunction-parent.html".

[domenic]

Do you have any suggestions for spec language that could usefully specify the filename? Maybe instead of trying to pass through a script object, we should just leave it super vague ("relevant filename, line number and column number" with a note explaining that this is purposefully vague)?

[bzbarsky]

"report an exception" takes a script. It uses that script's filename

Uses it for what? There is a filename associated with the exception itself, or should be... (in Firefox there isn't one for non-Error exceptions, but that's a bug we should fix). Note that the filename involved in exceptions is where the exception was thrown/created, not where script was entered anyway, whereas reporting happens at the entry point.

Are we talking about the filename in the error event, basically?

el.addEventListener("click", el.cloneNode); // purposefully not bound

That will not do the right thing, because event listeners are called with the event target as this, so this will clone the event target. You want something like:

el.addEventListener("click", document.write);

to make this very simple. I just tried that, and it reports no useful filename in the console in Safari or Firefox, for what it's worth. It does report one in Chrome, but that filename doesn't match the .filename of the error event that gets fired or the second arg passed to the onerror handler if there is one. That's set to empty string in Firefox and Chrome. It's undefined in Safari in this case. Is this the filename we're talking about here?

I can't imagine a relevant filename for this

I agree.

I think being vague about precisely what the filename/line/column are is fine for now. We can revisit it once people get closer to standardizing the .stack of Error instances, since that will involve solving basically this problem....

[domenic]

Yeah, I meant the filename (and line and column number) exposed as properties on the error event object. The current spec seems oriented around filling those in, based on the "relevant script".

If we can let those be vague, then I think I know how to address the rest of the issues you raised. Let me finish my workout, and in fortyish minutes I'll post a plan for fixing both this PR and report an exception (plus all callers of report an exception).

[domenic]

OK, so here's the plan. Let me know what you think, and I can try to implement it tomorrow.

Step 1: Fixing this PR

We note that every object (not just platform object, but every object) has an associated Realm. For functions it coincides with the value returned by GetFunctionRealm(); for other objects it's not yet specified.

The four "call user code" operations here all use the corresponding object's Realm to set up the entry stuff. (I.e., get the object's realm's settings object's realm execution context, increment its entrance counter, and push it onto JS execution context stack.)

For the case of "call a user object's operation", which is more complicated since it can potentially invoke user code twice, we potentially set up the entry stuff twice: once for the Get(), using the object, and another time for the Call(), using the function. So if you have an object whose realm is A with a handleEvent property that is a function whose realm is B, invoking the getter will happen with a different entry global than invoking the handleEvent.

(We could change that to use the object in both cases, but that seems weird to me. I'm flexible though.)

This should set up the correct entry settings object per your earlier PR comments.

Step 2: Fix HTML's "report the exception"

"Report the exception" gets modified to take an exception object, a global object, a filename/line number/column number tuple, and an optional script.

The optional script is necessary to perform muted error checks. (See "report an error".) If it's not present then we assume the error is un-muted. We no longer derive the filename from the script, to allow the better results you mention.

We probably consolidate "report the exception" and "report an error".

Step 3: Add "guarded" user code invocation to Web IDL

(This would be a follow-up PR performed after step 2, not as part of this step 1 PR.)

We add a new optional flag to all of these four user-code-calling operations: "guarded", default unset. The flag is not allowed to be set if the attribute or operation in question returns a promise type.

If the guarded flag is set, then when an exception is thrown during any of:

• converting arguments
• Get()/Set()/Call()
• converting return values

Then we do "report the exception" on the thrown exception, passing in the exception, the entry global (i.e. the global we went to all this trouble to make the entry global), "some relevant filename/line number/column number" (purposefully underspecified), and the callback's script-or-module.

Note that this means that in our var f = new window2.Function("stuff") case, we use the script of this var declaration for the muted errors check. This seems correct.

(I'm not sure if we should immediately call out to "report the exception" before un-entryifying the appropriate global, or if we should just save the appropriate global, entryify it, run possibly-throwing steps, unentryify it, and then call out to "report the exception" with the appropriate global. Maybe they are equivalent. I will assume we should do it before un-entryifying since that seems to be the direction your earlier PR comments were hinting at.)

Step 4: Fix all call sites of "report an exception"

Very few places should directly call "report an exception". Most should switch over to calling one of IDL's invoke-user-code operations with the new "guarded" flag set.

Here are the call sites I know of to "report the exception":

HTML

• Custom element callback reactions should switch to using the guarded flag.
• Custom element upgrade reactions will need a bit of care; will have to think on it a bit more.
• Failed module resolution should report to the script's settings object's global, which will be the same as the initiating global for the fetch (except for module workers, where it will be the inner worker global, not the outer global)
• creating a module script will report to the script's settings object's global
• running a classic/module script will report to the entry settings object set during running the script
• EnqueueJob will report to the entry settings object set during job execution
• Timer functions, when supplied a string: their CSP checks will "report the exception" if CSP blocks string compilation. This will need a bit of care to capture the active script at the time setTimeout() et al. are called so it can be used for muted-errors checks later. It already captures the current realm, which it can use for error reporting.
• Timer functions, when supplied a function: these currently call IDL "invoke" but seem to omit "report the exception." That seems like a straight bug. Regardless, it should switch to using the guarded flag.
• Animation frame callbacks should switch to using the guarded flag.
• Custom element errors during parsing will be reported to the document's Window. Might need to thread through the correct script here to perform "muted errors" checks.

DOM

• Calling handleEvent should switch to using the guarded flag.
• Calling mutation observer callbacks should switch to using the guarded flag.

Pretty much everywhere else

• Should switch to using the guarded flag.

[bzbarsky]

The current spec seems oriented around filling those in, based on the "relevant script".

Ah. The "relevant script" there is the one throwing the exception (but even then, that's not true; if an Error is created at one place but thrown at another, it's the creation point that gets reflected in the filename in browsers in practice). It has absolutely nothing to do with incumbent, entry, where the exception is reported, etc.

(We could change that to use the object in both cases, but that seems weird to me. I'm flexible though.)

Fwiw, Firefox uses the same entry settings for the Get and the call to the gotten thing. Setting up this stuff is not that cheap, so I would rather not do it twice, honestly, unless there are strong reasons to do so.

"Report the exception" gets modified to take an exception object, a global object, a filename/line number/column number tuple, and an optional script.

s/exception object/exception value/, right?

We add a new optional flag to all of these four user-code-calling operations: "guarded", default unset.

It should probably be default set. See the categorization of the various places in the platform that invoke this stuff as of a few years ago in https://www.w3.org/Bugs/Public/show_bug.cgi?id=17713#c16 -- pretty much everything wants to report the exception.

I'm not sure if we should immediately call out to "report the exception" before un-entryifying the appropriate global

I'm not sure it matters in practice, if we're explicitly passing in the global anyway.

Timer functions, when supplied a string: their CSP checks will "report the exception" if CSP blocks string compilation.

I don't think that should be happening. There should be CSP reporting, sure, but no firing of "error" events in this situation, right? I haven't tested what UAs do in detail here, but Gecko at least will simply return 0 from the setTimeout call, not schedule anything, and report the CSP violation if the policy asked for reports.

This will need a bit of care to capture the active script at the time setTimeout() et al. are called so it can be used for muted-errors checks later.

I don't think it's worth worrying about, honestly. The muted error checks are really important for syntax errors. For everything else they're mostly not that important. Certainly I don't think UAs go out of their way to do anything complicated for the string setTimeout case here, and I don't think the spec should either.

The plan sounds good in general! Thank you for thinking through this.

[domenic]

OK, @bzbarsky, this is ready for you to take a look at. https://rawgit.com/domenic/webidl/07ffceee1adb2e0c03ec1fc8a0c8eda94e4da219/index.html#es-user-objects use https://rawgit.com/domenic/webidl/6a54acae5c1ff4199b5bf85b61bc4681addbafb7/index.html instead should be a viewable version of the output.

Some notes:

• I went the extra distance and re-did all the abrupt completion stuff, in preparation for fixing all the "report an exception" stuff. This mostly seems better. Although it's sometimes a bit strange that we have ES completion values containing IDL values, I think that's how it has to be.
• I haven't tried tightening the promise language yet (e.g. introducing to ES a "CreateRejectedPromise" operation). We'll leave that for "some day".
• I went with your suggestion of only setting up entry stuff once for "call a user object's operation"
• This does not yet fix incumbent settings objects. My current queue on all this mess looks like:
  • Get this PR merged, in order to fix entry settings objects
  • Get Fix and expand incumbent settings object definition html#1189 merged, in order to lay the groundwork for fixing incumbent settings objects
  • Do a new Web IDL PR that adds a few steps in order to fix incumbent settings objects
  • Fix Should we be doing prepare to run a script/clean up after running a script more often? html#855 to take care of incumbent/entry settings objects in places like StructuredClone
  • Fix "report an exception" as discussed in steps 2-4 above.
  • Go triage and test all uses of incumbent and entry in various specs to see if we can get rid of them in favor of current.

[Ms2ger]

ES doesn't use ! for IsCallable(); I think it's better to follow that. (Especially because in this case my first interpretation was that it was a negation.)

[domenic]

There's a general movement in ES toward using ! everywhere, though. The current usage conflates completion values and ES values, and we'd like to move away from that. I agree it is easy to misread :(.

[bzbarsky]

My personal feeling, from trying to read the ES spec draft, is that the single-char thing is perhaps taking brevity a bit too far (it's easy to miss entirely) and '!' is a particularly poor choice of character. The right answer might be to change what ES is doing to use more-obvious and less-confusing sigils here.

[bzbarsky]

For example, "unbox" might be better than '!' to get the meaning across. Or something.

[domenic]

I'd encourage you to open an issue there to try to change it across the ecosystem, but we've already adopted it pretty wide and far, so it'll be an uphill slog. I think we should stick with the established conventions here, until such a time as such a change is agreed upon.

[bzbarsky]

If we stick with it here, we should linkify the '!' at the very least.

[domenic]

Nobody else does, but if that's what it takes to get this PR merged, I'll do it.

[bzbarsky]

My concern is that the IDL spec needs to be readable/understandable by people designing web APIs. Using this bizarre syntax with no context does NOT help that...

[domenic]

I'm not sure people designing web APIs need to understand the algorithms involved here; they contain a lot of obtuse stuff we've spent several months hashing out. Rather, they need to understand the publicly-exposed "spec API", which is the four <dfn>s. Binding implementers need to understand the notation, sure.

[bzbarsky]

That's fair, but in practice I've had all sorts of non-binding implementors (both spec authors and people trying to review specs) come to me with questions about the exact behavior some part of IDL produces...

I wonder whether we can do the linkification automagically (find all " ! " and " ? " with the spaces, and replace them with linkified versions) as a stopgap...

Anyway, I filed tc39/ecma262#568

[domenic]

I can linkify and add the intro paragraph for ! and ? as I did in #121. Are there other code review comments to be had, or does this LGTY otherwise?

[bzbarsky]

I need to read through the details, still. Probably on Monday morning at this point...

[domenic]

Thanks.

Oh, I just realized. Instead of pushing/incrementing and popping/decrementing, I should probably directly call "prepare to run script" and "clean up after running script".

The difference is that "clean up after running script" would run global script clean-up jobs and perform a microtask checkpoint, which seems desirable.

I'll push another commit making that change (and the ?/! linkification). Let me know if I'm wrong though and it should not do that.

[bzbarsky]

I thought about this pretty hard, and I am really not happy with the way this ends up working for proxies.

If the intent is to capture "the realm of the code that will actually run" then defining the realm in this way for a callable proxy is pretty suboptimal. If you just get your hands on any callable from some realm, you can pretend like code from that realm will run while actually running code of your own choosing, by simply implementing the "apply" trap.

I would much rather we ended up handling a callable proxy the same way we handle a function that (possibly) ends up calling another callable inside of itself instead of trying to peek through it like this.

Changing GetFunctionRealm doesn't seem viable given its uses in ArraySpeciesCreate and GetPrototypeFromConstructor. But that means maybe we don't want GetFunctionRealm here.

I'm not exactly sure about the bound function case. While it's true that in that case the actual function being wrapped does run, it runs in a way controlled by the binder, which makes things a bit worrisome too.

Note that this is primarily a worry for me because I associate certain security guarantees with this stuff. If none of the uses involve security checks, then it may not matter as much.

[domenic]

I'll leave functions as un-specified as normal objects, then, and remove this clause about GetFunctionRealm.

[bzbarsky]

Well. For the honest-to-God Function case there is an obvious answer that everyone agrees on and we should spec that.

For the other cases, we should try to see what various engines do in practice and see if we can get feedback from other implementors as to what they think should happen.

[domenic]

Ah, where "honest-to-God Function" is not a proxy or a bound function. OK, yeah, I'll add back in a note about it matching [[Realm]] for those.

[bzbarsky]

Thank you for linkifying the '!'/?' and for putting up a generated version. Made this much more pleasant!

The one remaining concern I have is the microtask checkpoint bit. I think morally that's the right thing to do (and have been meaning to make that change in Firefox; it's blocked by some other issues in how we handle microtasks), but I'm not sure what UAs do right now. For example, during event dispatch, do promise callbacks run after every event listener?

I tried creating a testcase for that:

<body>
<script>
function logger(i) {
  console.log("Call for " + i);
  Promise.resolve().then(function() { console.log("Promise callback for " + i) });
}
document.body.addEventListener("click", logger.bind(undefined, 1));
document.body.addEventListener("click", logger.bind(undefined, 2));
document.documentElement.addEventListener("click", logger.bind(undefined, 3));
console.log("before click");
document.body.click();
console.log("after click");
</script>

and all of Firefox, Chrome, Safari log:

before click
Call for 1
Call for 2
Call for 3
after click
Promise callback for 1
Promise callback for 2
Promise callback for 3

which is not the output I was expecting if any of them performed a microtask checkpoint there... though maybe I misunderstand what microtask checkpoints do.

[domenic]

Updated in response to code review comments at https://rawgit.com/domenic/webidl/modernize-invoke/index.html#es-user-objects

Regarding microtasks... hmm. "clean up after running script" says:

If the JavaScript execution context stack is now empty, perform a microtask checkpoint. (If this runs scripts, these algorithms will be invoked reentrantly.)

I think in your test case the execution context stack is not empty. I think it looks like this at the time the handler executes:

1. Bottom: realm execution context (pushed by executing the <script>)
2. document.body.click's execution context ??
3. realm execution context (pushed by "invoke")
4. Top: function execution context for logger.bind(undefined, 1).

That is, when invoke finishes, the execution context stack still contains 1 and 2. (Or at least 1... I'm having trouble remembering whether host-defined functions have execution contexts, so I'm not sure about 2.)

I created a similar test at http://jsbin.com/gukipafemo/edit?html,console,output where instead of using document.body.click() from inside a script, you manually click the button--so at the time the "invoke" finishes, you should have an empty stack. In this case Chrome works "as expected" with the interleaved calls, although Edge and Firefox do not. This reminds me of the cross-browser investigations in https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/ where Jake concluded that only Chrome was following the spec in these sort of tricky edge cases.

[bzbarsky]

Ah, I had missed the "if the stack is now empty" thing.

I think the difference in behavior between the dispatch from click and dispatch from script is pretty weird. In particular, it means that if a capturing listener grabs the event and redispatches it you get different behavior than if you just allow the event to propagate.... :(

Relying on "stack is empty" also involves other problems if a UA doesn't want alert and company to completely block the UI (and yes, I know the spec model of alert is that it blocks everything; this is user-hostile and broken): suddenly your microtasks stop running correctly while the alert is up.

It seems like we need to sit down and figure out how this stuff should actually work in the real world of browsers, which doesn't match the simplistic world of the spec and will keep diverging from it (e.g. iOS Safari recently moved to more-nonblocking alerts), as I understand.

[domenic]

Hmm, to me the idea that microtasks run when the stack becomes empty has always been quite intuitive. Sure, it can cause edge case weirdness like the capturing event listener you mention. But it makes sense from a microtask perspective...

As for alert, I agree that figuring out how to modernize those is a good project, but it seems like a separate one.

Is there anything else to do in this PR, though?

[bzbarsky]

My point is that while the alert is up the stack is not empty. This can be the state of things for a long time.

For this PR, I'm not sure there's anything else to do, but it sounds to me like in general the microtask setup is broken and needs fixing. :(

[domenic]

Can we merge this PR then?

[bzbarsky]

I think so, but please make sure issues are filed on the microtask bit. And it might be worth adding a note that that part is being sorted out...

[domenic]

Filed whatwg/html#1294.

[domenic]

Ping to merge. I think we should wait on the outcome of discussions in whatwg/html#1294 before adding any notes, and if we do, it'd be in HTML not Web IDL.

[bzbarsky]

Oh, you don't have commit rights to this repo? That's moderately dumb. :(

I agree the note needs to be in HTML, but I think it's pretty important to add ASAP. Otherwise we get in a cycle of some UAs claiming "but the spec says X, so we must do X" and other UAs saying "but it says this based on bogus premises that aren't actually true except maybe in your particular implementation" and then we end up with things like https://www.w3.org/Bugs/Public/show_bug.cgi?id=11960. Been there, done that, don't want to end up there again.

[domenic]

In that case it'd be good to clarify things over in that issue, as right now it sounds like nobody quite agrees with you that there's a problem...