-
Notifications
You must be signed in to change notification settings - Fork 159
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
Modernize invoking user code #113
Conversation
I think I'd prefer to land #114 first, so this PR only has relevant changes. |
Thanks, reset my branch to yours so this should be mergeable now. |
Rebased again, since I keep bitrotting you |
Reset this PR to your commit, thanks. Ping @bzbarsky to review? |
By default, <span class="esvalue">undefined</span> is used as the <a class="dfnref" href="#dfn-callback-this-value">callback this value</a>, | ||
however this <span class="rfc2119">MAY</span> be overridden by other | ||
specifications. | ||
To <dfn>call a user object's operation</dfn>, given a <a class="dfnref" href="#idl-interface">callback interface type</a> value <var>value</var>, sometimes-optional operation name <var>opName</var>, list of IDL argument <var>idlarg</var><sub>0..<var>n</var>−1</sub>, and optional <dfn data-export="" id="dfn-callback-this-value">callback this value</dfn> <var>thisArg</var>, perform the following steps. |
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.
"list of IDL argument" either needs an 's' at the end or the restoration of the "values" that went missing. Preferably the latter.
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.
Fixed and pushed a new commit.
BTW you may find reviewing the XML file slightly more enjoyable than reviewing the generated output.
Before your time.
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.
The "callback context" thing was added to propagate the incumbent, right? I don't think it's the right thing.
No, as far as I can tell it would give D: the window that made the addEventListener call.
That's basically what Firefox does.
Hrmm. That seems ... weird. I think people using 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:
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...
I think this would confuse people a lot. People expect functions created with |
Right, that's in the modern spec at https://tc39.github.io/ecma262/#sec-functioninitialize
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 If so, I probably need to go back and think really carefully about all the cases where you propose using script-or-module... |
Yes, exactly. As I said, I see two alternatives here. To rephrase them slightly:
In your example quoted above, the first alternative dispatches error events to |
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 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 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... |
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:
none of those have |
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)? |
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?
That will not do the right thing, because event listeners are called with the event target as
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
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.... |
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). |
OK, so here's the plan. Let me know what you think, and I can try to implement it tomorrow. Step 1: Fixing this PRWe 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:
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 (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
DOM
Pretty much everywhere else
|
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.
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.
s/exception object/exception value/, right?
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 it matters in practice, if we're explicitly passing in the global anyway.
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.
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. |
OK, @bzbarsky, this is ready for you to take a look at. Some notes:
|
ES doesn't use |
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 :(. |
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. |
For example, "unbox" might be better than '!' to get the meaning across. Or something. |
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. |
If we stick with it here, we should linkify the '!' at the very least. |
Nobody else does, but if that's what it takes to get this PR merged, I'll do it. |
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... |
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 |
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 |
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? |
I need to read through the details, still. Probably on Monday morning at this point... |
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. |
in the case of <a class='dfnref' href='#dfn-platform-object'>platform objects</a>, the | ||
associated Realm is equal to the object's <a class='external'>relevant Realm</a>, and | ||
for function objects the associated Realm is equal to the Realm returned by <a | ||
class='external'>GetFunctionRealm</a>. |
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 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.
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'll leave functions as un-specified as normal objects, then, and remove this clause about GetFunctionRealm.
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.
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.
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.
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.
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:
and all of Firefox, Chrome, Safari log:
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. |
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:
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:
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. |
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 whatwg/html#401). Updates in whatwg/html#401 and whatwg/html#1091 have changed the setup here. There is still some ongoing discussion in whatwg/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 whatwg/html#473 (comment) with details from #113 (comment) "Step 1". - 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(). - Update their exception handling to use the abrupt completion formalism.
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 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. |
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? |
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. :( |
Can we merge this PR then? |
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... |
Filed whatwg/html#1294. |
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. |
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. |
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... |
This updates the four algorithms that invoke user code in the following ways:
<dfn>
s with explicit arguments to be passed.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.