Event handlers are not compiled against the right global, per spec #1956

bzbarsky · 2016-10-24T14:38:16Z

Consider this testcase:

<!DOCTYPE html>
<iframe></iframe>
<pre><script>
  var elem = frames[0].document.documentElement;
  elem.setAttribute("onclick", "void(0)");
  var get = Object.getOwnPropertyDescriptor(HTMLElement.prototype, "onclick").get;
  var f = get.call(elem);
  document.writeln(f instanceof Function);
  document.writeln(f instanceof frames[0].Function);
</script>

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-handler

Stepping through this last algorithm, we end up under step 1 and in substep 9 call FunctionCreate. But FunctionCreate 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:

domenic · 2016-10-24T21:11:23Z

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.

bzbarsky · 2016-10-25T01:45:19Z

I am trying to discern if the realm we want to use is that of document or that of element.

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.

I mean... what if document doesn't have a browsing context??

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...

I wonder if we could use document's relevant settings object for that as well

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.

domenic · 2016-10-26T20:11:09Z

@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 windowProxy.onerror is not getting reset after navigation, which seems bad. If I switch to using addEventListener("error", ...) then both event listeners fire.

Chrome seems to fire on the currently-showing document's window, not the navigated-away-from document's window, if my test makes sense.

bzbarsky · 2016-10-26T20:44:41Z

@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:

Gecko tries to compile the event handler string but does not fire the error event, because we never fire the error event for a non-current Window (which isn't quite a spec concept, by the way; it's not quite the same as non-current document). We just log the compilation failure to the console.
Blink doesn't try to compile the event handler string at all; even if the string is valid .onclick ends up null. And it's even screwier: Blink seems to remove existing handlers on navigation or something? At least as far as I can tell if I setAttribute, then check .onclick I get a function, but then if I navigate and check .onclick again on the same element I get null. And this is not limited to setAttribute: if I set .onclick to a function and then navigate suddenly it becomes null in Blink. At least for the documentElement of the document that's navigated away from.

See whatwg/html#1956.

domenic · 2016-10-26T20:58:58Z

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?

bzbarsky · 2016-10-26T21:04:08Z

As far as I can tell everyone is interoperably using the currently-showing document

You're setting onclick on newDocument.body, not oldDocument.body, no?

domenic · 2016-10-26T21:10:34Z

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.)

bzbarsky · 2016-10-26T22:45:29Z

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...

domenic · 2016-10-26T22:55:49Z

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.

domenic · 2016-10-26T23:17:37Z

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?

bzbarsky · 2016-10-27T01:18:06Z

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 <script> with a textnode child into an inactive document? I see nothing obvious in the spec at first glance that would prevent it from being executed and possibly reporting errors, though I strongly suspect browsers do NOT actually execute such scripts in practice....

We should also add tests for what happens when a valid string is used for the event handler attribute, of course.

bzbarsky · 2016-10-27T01:18:57Z

I mean, we want it to end up in the console.

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. ;)

domenic · 2016-10-27T19:19:40Z

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.

"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?

bzbarsky · 2016-10-27T19:52:15Z

But I guess you are arguing for adding this check to "report the error" in general

Right, I'm arguing we should at least consider that, so we don't have to play whack-a-mole.

domenic · 2016-10-27T22:56:37Z

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

domenic · 2016-10-31T21:55:15Z

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.

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.

bzbarsky · 2016-11-01T01:01:35Z

For Safari and Chrome they show that they fire error events on the post-navigation window

@domenic Which tests show this? What happens if the post-navigation window is different-origin?

bzbarsky · 2016-11-01T01:07:49Z

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.

domenic · 2016-11-01T01:11:17Z

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.

bzbarsky · 2016-11-01T01:34:51Z

It seems like setTimeout and RAF should behave the same though

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 setup({ allow_uncaught_exception: true }); that error report on the parent window is ignored.

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.

bzbarsky · 2016-11-01T18:43:34Z

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.

domenic self-assigned this Oct 24, 2016

domenic added topic: multiple globals topic: events labels Oct 24, 2016

domenic added a commit to web-platform-tests/wpt that referenced this issue Oct 26, 2016

Add tests for the event handler IDL attribute compilation realm

de34653

See whatwg/html#1956.

domenic mentioned this issue Oct 26, 2016

Add tests for the event handler IDL attribute compilation realm web-platform-tests/wpt#4089

Open

domenic mentioned this issue Oct 27, 2016

Do not execute scripts in navigated-away-from documents #1989

Open

domenic added a commit that referenced this issue Oct 31, 2016

Set the Realm when compiling event handler IDL attributes

58b2aac

Fixes #1956.

domenic added a commit that referenced this issue Oct 31, 2016

Set the Realm when compiling event handler IDL attributes

b0f7684

Fixes #1956. Also changes the error reporting global used to be more straightforward.

domenic mentioned this issue Oct 31, 2016

Set the Realm when compiling event handler IDL attributes #1994

Merged

domenic added a commit that referenced this issue Oct 31, 2016

Do not fire error events on inactive globals

5d66b71

That is, on globals whose responsible document is not fully active. Discussed in #1956.

domenic mentioned this issue Oct 31, 2016

Do not fire error events on inactive globals #1995

Open

domenic closed this as completed in #1994 Nov 1, 2016

domenic added a commit that referenced this issue Nov 1, 2016

Set the Realm when compiling event handler IDL attributes

09a8c47

Fixes #1956. Also changes the error reporting global used to be more straightforward.

alice pushed a commit to alice/html that referenced this issue Jan 8, 2019

Set the Realm when compiling event handler IDL attributes

9d69cb9

Fixes whatwg#1956. Also changes the error reporting global used to be more straightforward.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event handlers are not compiled against the right global, per spec #1956

Event handlers are not compiled against the right global, per spec #1956

bzbarsky commented Oct 24, 2016

domenic commented Oct 24, 2016 •

edited

Loading

bzbarsky commented Oct 25, 2016

domenic commented Oct 26, 2016

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016 •

edited

Loading

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016 •

edited

Loading

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016

domenic commented Oct 26, 2016

bzbarsky commented Oct 27, 2016

bzbarsky commented Oct 27, 2016

domenic commented Oct 27, 2016

bzbarsky commented Oct 27, 2016

domenic commented Oct 27, 2016

domenic commented Oct 31, 2016

bzbarsky commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

domenic commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

Event handlers are not compiled against the right global, per spec #1956

Event handlers are not compiled against the right global, per spec #1956

Comments

bzbarsky commented Oct 24, 2016

domenic commented Oct 24, 2016 • edited Loading

bzbarsky commented Oct 25, 2016

domenic commented Oct 26, 2016

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016 • edited Loading

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016 • edited Loading

bzbarsky commented Oct 26, 2016

domenic commented Oct 26, 2016

domenic commented Oct 26, 2016

bzbarsky commented Oct 27, 2016

bzbarsky commented Oct 27, 2016

domenic commented Oct 27, 2016

bzbarsky commented Oct 27, 2016

domenic commented Oct 27, 2016

domenic commented Oct 31, 2016

bzbarsky commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

domenic commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

bzbarsky commented Nov 1, 2016

domenic commented Oct 24, 2016 •

edited

Loading

domenic commented Oct 26, 2016 •

edited

Loading

domenic commented Oct 26, 2016 •

edited

Loading