Add tests for the realms involved in compiling event handlers #4988

domenic · 2017-02-22T23:20:25Z

Follows whatwg/html#2286 and whatwg/html#2248.

This is WIP; ideally I'd like to also test incumbent settings object. Also the spec PR needs updates based on the results of my testing.

/cc @bzbarsky @Ms2ger

Follows whatwg/html#2286 and whatwg/html#2248.

wpt-pr-bot · 2017-02-22T23:20:30Z

Notifying @Ms2ger, @ayg, @jdm, @jgraham, @zcorpan, and @zqzhang. (Learn how reviewing works.)

w3c-bots · 2017-02-22T23:31:02Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision 4046209
Using browser at version BuildID 20170222110216; SourceStamp 9a9db410f20781076424307265decbc5de1e94cc
Starting 10 test iterations
All results were stable

All results

1 test ran

/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

Subtest	Results
	OK
`The entry settings object while executing the compiled callback via Web IDL's invoke must be that of the node document`	PASS
`The incumbent settings object while executing the compiled callback via Web IDL's invoke must be that of the node document`	FAIL
`The Function instance must be created in the Realm of the node document`	PASS

w3c-bots · 2017-02-22T23:31:03Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision 7b7b8a0
Using browser at version 58.0.3018.3 dev
Starting 10 test iterations
All results were stable

All results

1 test ran

/html/webappapis/scripting/events/compile-event-handler-settings-objects.html

Subtest	Results
	TIMEOUT

domenic · 2017-02-22T23:50:28Z

OK, these are ready to go. Encouragingly, they pass in Firefox, and the incumbent one fails in Chrome (which is encouraging because Chrome is known to "give up" on determining the incumbent in sufficiently-esoteric situations, of which this is probably one).

bzbarsky · 2017-02-23T06:06:20Z

I don't follow the entry settings test. What's foo.html? Why would that onload ever fire? I'm pretty sure error loads don't necessarily trigger onload... It might be better to use a URL that actually resolves to something there.

Actually, I'm not sure I follow the incumbent settings test either. How does that test the incumbent global of a compiled event handler? Or is that not what it's trying to test?

domenic · 2017-02-23T16:13:49Z

foo.html resolves to a 404 page, which fires the onload just fine... we can create a foo.html if you want though.

I'm not sure about the incumbent global test. I just kind of tried to follow your instructions, but I admit not understanding them super-well. Why did you think return value conversion was relevant to the incumbent global?

bzbarsky · 2017-02-23T16:27:22Z

foo.html resolves to a 404 page, which fires the onload just fine...

I guess I'm not sure why both Chrome and Firefox time out on that test, then, per test results above.

Why did you think return value conversion was relevant to the incumbent global?

Because when the return value conversion is happening the incumbent global is whatever was pushed for the callback.

So this test is testing the incumbent global that gets stored when window.onbeforeunload is assigned, but that has nothing to do with event handler compilation.

You may want to leave that test and add one which does:

<body onbeforeunload="return { toString: etc }">

to test the incumbent set up with event handler compilation.

bzbarsky · 2017-02-23T16:28:29Z

And also possibly of interest is a bodyElement.setAttribute('onbeforeunload', stuff) where the current/incumbent/entry global at the setAttribute call point don't match the relevant global. And then see which global ends up being used as incumbent.

domenic · 2017-02-23T16:47:49Z

I guess I'm not sure why both Chrome and Firefox time out on that test, then, per test results above.

They don't if you actually run it in a browser instead of in CI; very interesting.

I'll try to see if I can get a good incumbent test working...

…IDL attributes, for the incumbent case

domenic · 2017-02-23T20:44:32Z

OK, I worked on that, but now the test is failing in Firefox (the "source" ends up being the parent frame, not the iframe), which is unexpected given your comments in whatwg/html#2248 (comment)...

bzbarsky · 2017-02-23T22:15:19Z

That's odd. I just tried this locally

<!DOCTYPE html>
<iframe src="subframe.html"></iframe>
<script>
  onmessage = function(e) {
    console.log(e.source == frames[0]);
    console.log(e.source == this);
  }
</script>

where subframe.html is:

<body onbeforeunload="return { toString: parent.postMessage.bind(parent, 'PASS', '*') };">
<a href="http://example.com">Click me</a>

and I get true and false logged in that order.

domenic · 2017-02-24T19:22:02Z

Ah, I see. The problem is that a slight modification of that, where instead of clicking the button the outer frame does frames[0].src = "about:blank", logs false/true. See https://ns-gooldndpxq.now.sh.

Can you think of a reason that would be?

bzbarsky · 2017-02-24T20:35:21Z

So first of all, that test is not navigating the subframe, just assigning a .src property on its window (as opposed to the iframe element). The beforeunload only fires when the whole page is unloaded.

But that shouldn't affect the behavior. What does affect it is that gecko does NOT in fact set up an incumbent global on event handlers compiled from strings. Which means the incumbent global ends up being "yeah, whatever is on the stack" if there's script on the stack (and the entry global if there's nothing on the stack). But then if that incumbent global is not same origin-domain with the current global, we reset it to the current global.

In the "unload the page via browser UI" case, there's actually privileged script that's part of the browser UI on the stack. So we get that as the incumbent, then it fails the same origin-domain check and we reset the incumbent to current. And our current at that point is the global of the postMessage function, which is the parent.

Anyway, this is a bug in Gecko. Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1342513

domenic · 2017-02-24T21:01:39Z

Ah, oops.

But in the end, the upside of this that the tests and spec patch are good to merge? :) The test in this PR does not appear to have the iframe vs. contentWindow confusion.

bzbarsky · 2017-02-24T21:39:34Z

Yes, the test in the PR looks good to me!

This fixes #2248 by ensuring that we convert the created JavaScript Function object to a Web IDL callback, with the appropriate callback context. While in the area, it also straightens out and clarifies some confusion around what exactly the event listener's "callback" is; it is now always a Web IDL callback, instead of sometimes being just an algorithm. It also adds a clarifying note as to why we go through the JavaScript stack-pushing steps. Tests: web-platform-tests/wpt#4988

This fixes whatwg#2248 by ensuring that we convert the created JavaScript Function object to a Web IDL callback, with the appropriate callback context. While in the area, it also straightens out and clarifies some confusion around what exactly the event listener's "callback" is; it is now always a Web IDL callback, instead of sometimes being just an algorithm. It also adds a clarifying note as to why we go through the JavaScript stack-pushing steps. Tests: web-platform-tests/wpt#4988

Add tests for the realms involved in compiling event handlers

69ff7e6

Follows whatwg/html#2286 and whatwg/html#2248.

domenic added the do not merge yet label Feb 22, 2017

wpt-pr-bot added the html label Feb 22, 2017

domenic mentioned this pull request Feb 22, 2017

Fix event callback invocation to set entry/incumbent correctly whatwg/html#2286

Merged

Add incumbent tests; fix entry test

dec9bd6

domenic removed the do not merge yet label Feb 22, 2017

No console.log

cfde58b

domenic added 3 commits February 23, 2017 15:27

Create a dummy file

6882c1e

Modify to test event handler compilation via content attributes, not …

2fffed5

…IDL attributes, for the incumbent case

Make failures easier to diagnose

d34987e

domenic merged commit 9e1c15f into master Feb 24, 2017

domenic deleted the compiled-event-handler-settings-objects branch February 24, 2017 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for the realms involved in compiling event handlers #4988

Add tests for the realms involved in compiling event handlers #4988

domenic commented Feb 22, 2017

wpt-pr-bot commented Feb 22, 2017

w3c-bots commented Feb 22, 2017 •

edited

w3c-bots commented Feb 22, 2017 •

edited

domenic commented Feb 22, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 23, 2017

domenic commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 24, 2017

bzbarsky commented Feb 24, 2017

domenic commented Feb 24, 2017

bzbarsky commented Feb 24, 2017

Add tests for the realms involved in compiling event handlers #4988

Add tests for the realms involved in compiling event handlers #4988

Conversation

domenic commented Feb 22, 2017

wpt-pr-bot commented Feb 22, 2017

w3c-bots commented Feb 22, 2017 • edited

Firefox (nightly channel)

All results

w3c-bots commented Feb 22, 2017 • edited

Chrome (unstable channel)

All results

domenic commented Feb 22, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 23, 2017

domenic commented Feb 23, 2017

bzbarsky commented Feb 23, 2017

domenic commented Feb 24, 2017

bzbarsky commented Feb 24, 2017

domenic commented Feb 24, 2017

bzbarsky commented Feb 24, 2017

w3c-bots commented Feb 22, 2017 •

edited

w3c-bots commented Feb 22, 2017 •

edited