Make testharness tests run in a top-level browsing context #13966

jgraham · 2018-11-07T13:21:48Z

This change is

jgraham · 2018-11-07T13:25:09Z

@jugglinmike This is the alternative to #13881. @lukebjerring if you have the ability to generate a complete run of this to look for regressions that would be very helpful. I fixed some problems I found in a m-c run, but there were a few other issues remaining.

gsnedders

By no means a complete review, but from a quick look through.

tools/wptrunner/wptrunner/executors/executormarionette.py

gsnedders · 2018-11-07T16:37:10Z

tools/wptrunner/wptrunner/executors/executorwebdriver.py

-        protocol.base.execute_script(self.script % format_map, async=True)
-        test_window = protocol.testharness.get_test_window(self.window_id, parent_window)
-
+        protocol.base.execute_script("window.open('about:blank', '%s', 'noopener')" % self.window_id)


Note that Edge will shortly be using this and doesn't support noopener.

Opening from an iframe apparently works in older versions of Safari:

var iframe = document.createElement('iframe'); document.body.appendChild(iframe); iframe.contentWindow.open('about:blank', 'window_id'); document.body.removeChild(iframe);

Though Edge's behavior isn't what we want. From the child window:

> !!opener true > opener.document X Permission denied

We can just null out the window's opener property:

var win = open('about:blank'); win.opener = null;

This feels a little artificial. Is it important that the window be disowned specifically? If so, maybe we could inject an anchor with target="_blank" rel="noreferrer" and click on the link. That's spec'd to disown the window, and it works in Edge.

I"m not super worried about this; any test that depends on the opener being null at the moment is already broken. I only used noopener to prove that we aren't depending on the opener any more,

In the medium term I hope that everyone will implement the Create Window webdriver endpoint, which is inteded to have exactly the behaviour we require here and also returns the handle of the new window so we can delete the window finding code. I'm happy to do the more complex solution in the interim if people think that it's important, but otherwise I would tend to argue that this good enough for now.

I don't think we need to worry about older versions of Safari; it's almost exclusively Edge that's my concern here.

Does anyone happen to have Edge set up so we can verify if this PR works there?

let's try explicitly setting window.open().opener = null, because I don't think that can make anything worse?

You don't think wrong; I tried this and Chrome crashes (or maybe there's an uncaught exception due to trying to set a readonly property).

@jgraham https://gist.github.com/fb3941b207c8d9ff3f3343a7bf302165 (this is with Selenium)

@jgraham https://gist.github.com/c939fd3a4276f3bc056e175e483e57a5 (edge_webdriver)

tools/wptrunner/wptrunner/products.py

lukebjerring · 2018-11-07T18:34:32Z

@jgraham
Started the scrapes, you'll eventually see them @ https://wpt.fyi/test-runs?label=testdriver-message-passing

EDIT: In case someone with access to https://wpt.fyi/admin/results/upload wants to get a PR into wpt.fyi in future, I just:

Visited https://tools.taskcluster.net/groups/X4FPXNvyShWg4X_CodEXBQ
Went to the wpt-chrome-dev-results task, grabbed its ID (FH05ydOdR_GoBgVh82pBwA)
Turned that into a wpt_report artifact URL:
- https://queue.taskcluster.net/v1/task/FH05ydOdR_GoBgVh82pBwA/artifacts/public/results/wpt_report.json.gz
Fill out the form, with:
- Uploader as the author of the PR (in this case, jgraham)
- Results URL as the artifact URL.
- Revision as the full SHA of the commit that triggered the check (in this case, 0e5347a)
- Labels with experimental,(branch-name) (in this case, I actually labelled it the same as [wptrunner] Transmit test results via postMessage #13881 - testdriver-message-passing)

lukebjerring · 2018-11-07T18:44:53Z

Whoops - @jgraham I didn't realise affected tests would be like, 19 css tests.
@jugglinmike how did you trigger a complete run for #13881 ?

jugglinmike · 2018-11-07T18:50:06Z

@lukebjerring I pushed a separate branch to Bocoup's fork which included a commit to enable results
collection.

lukebjerring · 2018-11-07T19:52:21Z

Err, my bad on pushing 3d0c723, reverted.

tools/wptrunner/wptrunner/executors/executormarionette.py

tools/wptrunner/wptrunner/executors/executorwebdriver.py

tools/wptrunner/wptrunner/executors/testharness_webdriver_resume.js

tools/wptrunner/wptrunner/testharnessreport.js

jugglinmike · 2018-11-07T22:43:34Z

tools/wptrunner/wptrunner/executors/executorwebdriver.py

-        protocol.base.execute_script(self.script % format_map, async=True)
-        test_window = protocol.testharness.get_test_window(self.window_id, parent_window)
-
+        protocol.base.execute_script("window.open('about:blank', '%s', 'noopener')" % self.window_id)


Opening from an iframe apparently works in older versions of Safari:

var iframe = document.createElement('iframe'); document.body.appendChild(iframe); iframe.contentWindow.open('about:blank', 'window_id'); document.body.removeChild(iframe);

Though Edge's behavior isn't what we want. From the child window:

> !!opener true > opener.document X Permission denied

We can just null out the window's opener property:

var win = open('about:blank'); win.opener = null;

This feels a little artificial. Is it important that the window be disowned specifically? If so, maybe we could inject an anchor with target="_blank" rel="noreferrer" and click on the link. That's spec'd to disown the window, and it works in Edge.

gsnedders · 2018-11-08T16:05:58Z

tools/wptrunner/wptrunner/executors/executorwebdriver.py

+} else if (document.readyState !== "loading") {
+  callback(true);
+} else {
+  document.addEventListener("DOMContentLoaded", () => callback(true));


Let's make this readyStateChange being ready to match WebDriver.

tools/wptrunner/wptrunner/executors/executorselenium.py

Traditionally testharness tests ran in a auxillary browsing context opened using window.open and with access to the opener. This works well because the long-lived nature of the opener helps to avoid some of the race conditions that would otherwise occur. But it doesn't work *that* well; the recent refactor to stop continually focusing the opener broke tests that alter document.domain or otherwise prevent the opener being same-domain with the test window. And future platform features may cause the opener to be nulled out entirely so even a postMessage based fix wouldn't work. To solve all of this, this patch refactors things so that the initial window doesn't contain any logic at all and is just used to keep the browser alive between tests. Most of the logic moves to testharnessreport.js which is loaded once per test. In order to get the right timeout when timeout_multiplier is set this requires an addition to the product API in wptrunner to expose a function for getting the timeout multiplier. This allows us to get the timeout_multiplier for testharness tests upfront and avoids the need to change the content of testharnessreport when we start running testharness tests, or to restart the server for each test type. The main issue with the single-window implementation is that we need to start injecting script once the test page has loaded testharnessreport.js. For most browsers we are able to use pageLoadStrategy=eager to control this; in that case we can start running tests once DOMContentLoaded is reached. Chrome doesn't support this pageLoadStrategy, however, so we have to fake support with custom script.

jgraham · 2018-11-09T11:39:01Z

This seems to fix /html/dom/usvstring-reflection.html in Chrome.

jugglinmike · 2018-11-20T03:11:40Z

It also fixes FileAPI/url/sandboxed-iframe.html.

gsnedders · 2018-11-20T14:01:17Z

@jgraham so what's the status of this now?

jgraham · 2018-11-20T14:13:08Z

Works on Firefox and Chrome on Linux and macOS. Status for Edge and Safari is unknown.

foolip · 2018-11-23T00:11:19Z

Safari looks happy on Azure Pipelines. If you trigger Azure Pipelines again even more stuff will run.

gsnedders · 2018-11-23T17:47:49Z

In Edge almost all (all?) the testharness.js tests now give

JavascriptErrorException: javascript error (500): Object doesn't support property or method '__wptrunner_process_next_event'

gsnedders · 2018-12-05T15:27:20Z

I couldn't later reproduce that, so I've no idea what's up with that; let's assume this works.

gsnedders

Reviewed 27 of 27 files at r1, 3 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jgraham, @frivoal, @annevk, @zqzhang, @zcorpan, @dbaron, @jdm, @foolip, and @domenic)

tools/wptrunner/wptrunner/products.py, line 34 at r1 (raw file):

    def __init__(self, config, product):
        module = product_module(config, product)
        data = module.__wptrunner__

What would it take to have a function (or just a variable) in that module that returned a Product?

tools/wptrunner/wptrunner/products.py, line 52 at r1 (raw file):

def load_product(config, product, load_cls=False):

What's the point of the load_cls arg here? When does it make sense to call this with it being True; why wouldn't you just construct the class yourself?

tools/wptrunner/wptrunner/browsers/firefox.py, line 103 at r1 (raw file):

    capabilities = {}
    if test_type == "testharness":
        capabilities["pageLoadStrategy"] = "eager"

Can we not make this default to eager, somehow? The fact we have all of this is to deal with ChromeDriver not supporting it, right?

tools/wptrunner/wptrunner/executors/base.py, line 525 at r1 (raw file):

    def process_complete(self, url, payload):
        rv = [strip_server(url)] + payload

Is this an earlier underlying bug? What caught this?

tools/wptrunner/wptrunner/executors/executormarionette.py, line 221 at r1 (raw file):

                return test_window

            time.sleep(0.1)

This is addressing the fact that closing windows is async, right?

tools/wptserve/wptserve/stash.py, line 175 at r1 (raw file):

        return value

Given you're adding loads of whitespace, can you enable whichever flake8 check checks for them, or does that still have lots of failures?

jgraham

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gsnedders, @frivoal, @annevk, @zqzhang, @zcorpan, @dbaron, @jdm, @foolip, and @domenic)

tools/wptrunner/wptrunner/products.py, line 34 at r1 (raw file):