-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Event handlers are not compiled against the right global, per spec #1956
Comments
It's a bit frustrating that ES doesn't have any "create a function in realm X" abilities. You can pass a different prototype argument, but the resulting function's [[Realm]] will still get set to the current Realm. So I guess from the spec side the fix here is to find the right realm and push its settings object's realm execution context onto the stack temporarily. Meh. Looking at the current algorithm steps, I am trying to discern if the realm we want to use is that of document or that of element. I wrote the following test case: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4602 which seems to reveal that it's that of document. Can you check that my test case is properly distinguishing? It looks like we supposedly report errors to document's browsing context's Window according to the current spec, which is a little surprising and baroque. I wonder if we could use document's relevant settings object for that as well, and align both. I mean... what if document doesn't have a browsing context?? Seems bogus. |
In Gecko these are always the same (adoption changes the element's Realm), so the question doesn't arise. But yes, your testcase properly distinguishes between the two situations if adoption doesn't change the object's Realm.
Then we return early in https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler step 1 substep 2, because https://html.spec.whatwg.org/multipage/webappapis.html#concept-n-noscript tests true for a document with no browsing context. Yes, not quite obvious from the caller side here...
Yes we should! What we have now is black-box distinguishable from that by messing with event handler attributes in navigated-away from documents, and what the spec says (which is nonsensical, because there is no such concept as a "browsing context's window", but I'm assuming it means active document's window or something?) and I bet browsers do NOT fire onerror on the currently showing document if someone messes with event handler attributes in a navigated-away from document. But worth testing, I guess.... would be good to have a web platform test here anyway. |
@bzbarsky can you help me write tests for the error reporting? I tried at http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4608, but Gecko is confusing me. It says that Chrome seems to fire on the currently-showing document's window, not the navigated-away-from document's window, if my test makes sense. |
@domenic You're running into the usual problems with Window reuse on navigation from initial about:blank and the fact that Blink/Gecko/Edge/spec don't all quite agree on how/when it happens. In Gecko, in your testcase, there is no new Window object; it's the same Window object both before and after the navigation. I can't speak for exactly what Blink is doing in this specific situation. If I test this with a testcase that makes sure a new Window is actually created on navigation by loading some actual thing and then loading some other thing, I see the following:
|
Thanks; the about:blank issue is of course what's catching me. Regarding the post-fix test case, that isn't what I see. Maybe the live-dom-viewer was confusing things. I've uploaded a modified test case to http://w3c-test.org/submissions/4089/html/webappapis/scripting/events/event-handler-idl-attribute-realm.html. If I change it to a non-syntax error for onclick, the assert that onclick equals null fails. And Gecko hits my assert_unreached, so it is firing that error event. As far as I can tell everyone is interoperably using the currently-showing document :(. (Except maybe Edge, where we run into their famous "Permission denied" errors for trying to access another global.) Am I testing wrong, perhaps? |
You're setting onclick on |
Ah, right, OK, testing error! The deployed code at that URL is now updated to use oldDocument. But, now I am getting timeouts in Firefox (I guess that is your "does not fire the error event, because we never fire the error event for a non-current Window") and Blink is still using the active document's window. I bet we could talk Blink folks into the Firefox behavior... (Edit: I should also test Safari, since they have different multiple-global behavior in some scenarios than Blink. Although right now my Safari-testing-computer is installing an OS update.) |
That doesn't match the Blink behavior I was seeing, but maybe it's different there for documentElement vs document.body? Can you link to your updated testcase, or better yet a diff against web platform tests that adds the testcase? http://w3c-test.org/submissions/4089/html/webappapis/scripting/events/event-handler-idl-attribute-realm.html still has the newDocument thing... |
The updated test case is at https://github.com/w3c/web-platform-tests/pull/4089/files ... strange that it's not deploying. I'll try poking it. |
http://w3c-test.org/submissions/4089/html/webappapis/scripting/events/event-handler-idl-attribute-realm.html is now fixed. And indeed you're right Chrome is also timing out. OK, so let's spec that! No events if you're not the active document? So... "if document is the active document, then report the error for ... using document's relevant global object"? Sound reasonable? |
Well, we need to report the error in all cases, right? I mean, we want it to end up in the console. We just don't want to fire the error event. Do other browsers fire error events in inactive documents in any situations? For example, what happens if you insert a We should also add tests for what happens when a valid string is used for the event handler attribute, of course. |
That's assuming we would compile a valid string in this situation to produce a function. If we wouldn't do that, then there is clearly nothing to report in the error case, since compiling happens at all. ;) |
"Report the error" always means to fire the error event. The spec has a separate vaguer concept of "If the error is still not handled after this, then the error may be reported to a developer console." So in the current architecture the correct thing to do would be to just not report the error; maybe we could throw in "Note: you may report this to the developer console if you want." But I guess you are arguing for adding this check to "report the error" in general, so that it immediately bails out if target is a Window whose document is not active, and then the "not handled" part takes over? |
Right, I'm arguing we should at least consider that, so we don't have to play whack-a-mole. |
So I opened #1989 for the separate issue of not executing scripts in navigated-away-from documents. I guess we should also check other cases that could call "report the exception", such as custom element reactions, enqueued microtasks that throw, event listeners that throw, RAF that throws, setTimeout... I'll try to do that soon. |
See whatwg/html#1956. In particular, this tests that: - Event handler IDL attributes are compiled in the right realm - Event handler IDL attribute compilation, setTimeout, and requestAnimationFrame do not "report the exception" for inactive documents
OK, I updated web-platform-tests/wpt#4089 with some additional tests. For Safari and Chrome they show that they fire error events on the post-navigation window, but for RAF and event handler IDL attributes they do not. Edge has its usual issues with windows accessing other windows post-navigation. I'll update the "report the error" spec to the Firefox behavior. |
Fixes #1956. Also changes the error reporting global used to be more straightforward.
That is, on globals whose responsible document is not fully active. Discussed in #1956.
@domenic Which tests show this? What happens if the post-navigation window is different-origin? |
Ah, is this the html/webappapis/timers/compile-errors.html test? That could be down to exactly how setTimeout coerces undefined to a useful "this" value. It might go through WindowProxy in Chrome/Safari, but just use the current global in Firefox. Well, I know for a fact it just uses the current global in Firefox; we went to some lengths to make that work. |
Sorry, yes, I seem to have left out "for setTimeout". It seems like setTimeout and RAF should behave the same though, if this were some kind of pervasive bindings problem. I guess it could be a setTimeout-specific one. In any case I'm not too worried; the Firefox semantics seem more reasonable so I can file a Chrome bug to align. |
The tests are testing two quite different things. The setTimeout test takes the setTimeout function from the old window, and passes a string with a syntax error to it. If this setTimeout call is executed with the new window as "this", then when the syntax error is discovered it's natural to report the exception to "this"; in fact there is nothing else to report it to. The RAF test calls the old window's RAF and passes it a function from the parent window as the callback. When this callback is later invoked, there is a nice function object there, the browser (either one) does https://heycam.github.io/webidl/#es-invoking-callback-functions and ends up with realm being the parent window. In spec terms... I guess we land at https://html.spec.whatwg.org/#run-the-animation-frame-callbacks and then it says to go to https://html.spec.whatwg.org/#report-the-exception which says to go to https://html.spec.whatwg.org/#report-the-error using the global of the "script's settings object", where "script" is the "relevant script". What is the "relevant script"? I have no idea; this links to https://html.spec.whatwg.org/#concept-script but what we have is a Function object... What Firefox does is that it uses the Realm of the function for the error reporting here. That's the parent window, and since the test has This test would be a lot more interesting if a function from the navigated-away-from window were passed as the RAF callback. Of course then there would be the question of whether it even gets called..... I guess there is also the question of whether the function from the parent should be called. Why should RAF tick on an unloaded window? It's a bit surprising to me that it does in both Chrome and Firefox. (This stuff is all so annoying. I really wish, on a daily basis, that people had not conflated "the global" and "the thing you navigate" back in the 90s.) |
Fixes #1956. Also changes the error reporting global used to be more straightforward.
I guess the concept-script breakage I describe above is #958 |
See whatwg/html#1956. In particular, this tests that: - Event handler IDL attributes are compiled in the right realm - Event handler IDL attribute compilation, setTimeout, and requestAnimationFrame do not "report the exception" for inactive documents
Fixes whatwg#1956. Also changes the error reporting global used to be more straightforward.
Consider this testcase:
What should happen here per current spec? Per https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-content-attributes the
setAttribute
call sets the event handler to be an " internal raw uncompiled handler". Then the.onclick
get will call into the getter behavior defined at https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-idl-attributes which calls into https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handlerStepping through this last algorithm, we end up under step 1 and in substep 9 call
FunctionCreate
. ButFunctionCreate
creates the function in the current Realm, and what is the current Realm in this case? The current Realm, as far as I can tell, is that of the parent page, not the subframe: we are calling a getter whose Realm is that of the parent page, right?Edge, Chrome, Firefox, and Safari, WebKit nightly all create the function in the Realm of the subframe in this case. This is not what the spec says to do, and the spec is what's wrong here.
Note that it gets even worse if I were to actually click on the element instead of getting
.onclick
. In that situation, we land in event handling, and eventually https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm which I believe invokes https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler when we have no current Realm at all.@Ms2ger, @domenic, @annevk
The text was updated successfully, but these errors were encountered: