[wptrunner] Transmit test results via postMessage #13881

jugglinmike · 2018-11-02T21:58:35Z

A recent improvement to wptrunner modified the way data structures are
transmitted from the browser to the WebDriver server [1]. One
consequence of this change is that the data structures are created in a
JavaScript realm which differs from the realm in which the "Execute
Script" WebDriver command is running. As of version 17, Microsoft Edge
does not correctly serialize cross-realm values, so referenced
improvement unintentionally precluded testing in that browser.

Transmit data structures between realms via the postMessage API to
avoid this problem.

[1] #13419

This is the reason we've recently been reporting such laughably incomplete results for Edge on wpt.fyi. I've also submitted WebDriver spec tests for the expected serialization behavior

A recent improvement to wptrunner modified the way data structures are transmitted from the browser to the WebDriver server [1]. One consequence of this change is that the data structures are created in a JavaScript realm which differs from the realm in which the "Execute Script" WebDriver command is running. As of version 17, Microsoft Edge does not correctly serialize cross-realm values, so referenced improvement unintentionally precluded testing in that browser. Transmit data structures between realms via the `postMessage` API to avoid this problem. [1] web-platform-tests#13419

jugglinmike · 2018-11-02T22:05:50Z

I posted some more context to the relevant issue in the results-collection project.

foolip · 2018-11-02T23:30:04Z

@jugglinmike, infrastructure/ tests are failing on Travis, can you see about fixing those?

jugglinmike · 2018-11-03T02:09:07Z

Looks like I was a little too aggressive in trying to remove global state. I think I've resolved this by caching the origin in the realm of the message recipient. This is a crucial section of code, though, so vetting it through all of WPT seems prudent:

https://tools.taskcluster.net/groups/Mmj2u8piTuyoDlvdad2gGg

This branch is currently based on ffeba23, so we should compare with the Taskcluster results for that commit:

https://tools.taskcluster.net/groups/dFx-WCDHRzSMGzj4wuxF6Q

foolip · 2018-11-03T07:45:39Z

@lukebjerring @Hexcles how can we get https://tools.taskcluster.net/groups/Mmj2u8piTuyoDlvdad2gGg into wpt.fyi for a comparison?

This is a perfect case for #13263 :)

lukebjerring · 2018-11-03T12:38:02Z

The Taskcluster webhook takes all the task groups and turns them into artifact URLs, and then posts to the results receiver with a full list of all of those URLs.. might be worth extracting that code, and exposing it into another api endpoint which is Taskcluster specific.

However, as a one off, you could reproduce the same list of urls and send the POST event yourself?

lukebjerring · 2018-11-03T13:13:16Z

So, with a little scrape of the UI:

Array.from(document.querySelectorAll('a')).map(a => /groups\/[^\/]+\/tasks\/([^\/]+)\//.exec(a.href)).filter(m => !!m).map(m => m[1]).filter(m => m !== "null")

Looks like the task ID is in the URL you gave (Mmj2u8piTuyoDlvdad2gGg) and the group IDs are:

["ZVChn1bdRcSX5I56CabVbQ", "ViEYc7BHSVCJi-CXO6cqaw", "XWoTo0t7TZqOWoeCvBcOWg", "IEOm-11CR82h2FwGpmosBg", "Yd_5Hnt-S4qsGeHPJhTBig", "R6GiWrXzTg-kejTnU30zvA", "Fau84efbRaCGblDHiGFHcA", "MwmJtqrKRNS77YBOVd5XGQ", "BAeb_zRBS0aFh0iIIlyCKA", "b4Hp03NYRRSekLSnCYn5qg", "bp0Gqv5WRZG0DBrj63HBhA", "DOPVhU7JQ92aPrcp7ppExw", "NRh6qioERYOqz6bmUG4ItQ", "HYtXlm4hQ6yU_nMvdIWoPQ", "Iea3Cya4RWm29-jrA6e1zQ", "OkivyodfR6K5hOA7k7Vfsw", "GiBI8IU4QsGaTMp0L26_2Q", "ToArMA6BShylgk2RBSj4zA", "QY-4hYBNSfKoQFKj2zCvIw", "J3apkKfVTZCOpf8yqvGICw", "PrNFDkkwTGW7Z8R8q8sQYw", "KmyFIDOmQ_yy5PZqCAOKjw", "dL_1ex-DSzmoGykBrbpVlA", "UVNAkAbMRYiKOdqBQIyaDA", "RQzufNthTeORh7EvgCe76w", "FGaW2YD0RV-VbMibuB2Gbg", "CtzBh4pVTbWQgK1lCW5xDg", "SYEXrRsvRWG-4l2u5hXFuw", "ZIF0d4wuTpe4FKeb_L0jJg", "OdplewErRBePy9Ip16EyNw", "Pth9wUmHSPGQRNJOauGayA", "ZnT3IIhXS4KxYiRTM1ly6w", "APC22dKoTOi6M8RcaOSBFg", "YZZOZ4lCQfyibIYOAbsXsg", "dMVwLtBAR8COF0nKk7wKTA", "NXoFXs8fQRqDinQ7HwO_BA", "NYS5CXqmR5e577VxRC0K2g", "Iq6I4zZCSzm7yhDYVgHGFg", "N2I9IpteRE6WnekVg7TunQ", "a5lDXY5dRcWRwLScsvPz6Q", "Okpv13iQSDyUUAj4vwcRbQ", "M_gtodHiSW-OhMnjqNYMvQ", "OlbqEI_ATSq1cmUgif-QtA", "O0Xj9tcbQoyPxhx3Jmg0pw", "d3l7NyrAQQeeZbwU2a7gXA", "MC0-dASJTNS0csqv5kZ3dQ", "AIGlcx0gSJ-dTsC92liuug", "AmAOmo5rT4qBG2XEpnmEvw", "KG_kQA2HTqmrhsIMGOsbUg", "RXc3as_fTTG4VSKyc7tibg", "LiL-UsQ_TGi_30EImKhgJQ", "dKVNfAaxS9G5_bXfNYz4VQ"]

lukebjerring · 2018-11-03T13:48:44Z

In theory, the following scrape of URLs, and a hack for /admin/results/upload, might result in the output we want:
https://gist.github.com/lukebjerring/d4dd09930c4959d9c6611241da4dfc8e

I (cheekily) ran it as jugglinmike, and labelled it testdriver-message-passing, so https://wpt.fyi/test-runs?label=jugglinmike&max-count=100 will show if it's successful (and revision was set to ffeba23ebd)

EDIT: Yeah FF worked, so ran chrome too.
https://wpt.fyi/results/?sha=ffeba23ebd&products=firefox[taskcluster],firefox[jugglinmike]&diff
https://wpt.fyi/results/?sha=ffeba23ebd&products=chrome[taskcluster],chrome[jugglinmike]&diff

foolip · 2018-11-03T14:54:19Z

Amazing, thanks @lukebjerring!

@jugglinmikes, looks like there are regressions to investigate here.

gsnedders

@jugglinmike plz look through the regressions @lukebjerring linked

jugglinmike · 2018-11-05T16:48:29Z

I took the weekend off, but this is on the docket for today.

jgraham · 2018-11-05T16:53:44Z

FWIW we eventually need to work on getting this to work without window.opener being used at all, either via postMessage or otherwise. That's because there's a feature coming to allow nulling out the opener on window.open and testing that probably requires the harness to run tests in a top level browsing context. I started working on that, but it's a bigger change and has a few tricky parts.

jgraham

I think what you're doing here is OK, but it's a larger than necessary change from the current design which makes it harder to reason about the correctness.

jgraham · 2018-11-05T17:00:26Z

tools/wptrunner/wptrunner/executors/runner.js

-  if (!event) {
+
+  if (event.data.type === "testdriver-resume") {
+    source = event.source;


I don't really understand the point of this; it seems like the identity of the test window is statically known here; it's window.win. Why do we need to keep updating it?

jgraham · 2018-11-05T17:03:53Z

tools/wptrunner/wptrunner/executors/runner.js

-};
+
+  source.postMessage({
+      type: "testdriver-next message",


This type string doesn't match anything else.

jgraham · 2018-11-05T17:05:14Z

tools/wptrunner/wptrunner/executors/testharness_webdriver_resume.js

-window.opener.testdriver_callback = callback;
-window.opener.process_next_event();
+
+window.addEventListener("message", function handle(event) {


I don't like the pattern where we keep adding and removing message listeners; we've had bags with such code before. Ideally we define the message handler once in testharnessreport.js and just have this script update the current callback and resume the test.

jgraham

Wrong button, sorry!

jugglinmike · 2018-11-06T01:12:54Z

On Friday, we learned that in addition to tripping up Edge, gh-13419 interferes with the semantics of some tests (see gh-13876). That prompted @jgraham to begin work on an alternate solution which reduces the number of browsing contexts to one. As James noted above, that approach will also enable contributors to test more parts of the platform.

We discussed simplifying this patch to use as a short-term fix. @jgraham shared some details about what they had in mind, but I'm having trouble interpreting them. What I have locally is no less complicated than this patch, and it's still not quite right, somehow.

I'm time-boxing my effort on this because we expect the change to be short-lived. That's why I'm proposing an even simpler change: gh-13938. That will not fix support for the invalidated tests, but it will correct the regression to Microsoft Edge. That may be preferable to reverting gh-13419 altogether.

jugglinmike · 2018-11-06T18:20:48Z

Superseded by gh-13938.

wpt-pr-bot added infra wptrunner The automated test runner, commonly called through ./wpt run labels Nov 2, 2018

wpt-pr-bot requested review from gsnedders and jgraham November 2, 2018 21:58

jugglinmike mentioned this pull request Nov 2, 2018

Widespread timeouts in Edge 17 web-platform-tests/results-collection#563

Open

fixup! [wptrunner] Transmit test results via postMessage

47402ea

gsnedders requested changes Nov 5, 2018

View reviewed changes

jugglinmike mentioned this pull request Nov 5, 2018

Edge runs are too slow (20-30h) web-platform-tests/results-collection#623

Open

jgraham approved these changes Nov 5, 2018

View reviewed changes

jgraham requested changes Nov 5, 2018

View reviewed changes

jugglinmike closed this Nov 6, 2018

jgraham mentioned this pull request Nov 7, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wptrunner] Transmit test results via postMessage #13881

[wptrunner] Transmit test results via postMessage #13881

jugglinmike commented Nov 2, 2018

jugglinmike commented Nov 2, 2018

foolip commented Nov 2, 2018

jugglinmike commented Nov 3, 2018

foolip commented Nov 3, 2018

lukebjerring commented Nov 3, 2018

lukebjerring commented Nov 3, 2018

lukebjerring commented Nov 3, 2018 •

edited

Loading

foolip commented Nov 3, 2018

gsnedders left a comment

jugglinmike commented Nov 5, 2018

jgraham commented Nov 5, 2018

jgraham left a comment

jgraham Nov 5, 2018

jgraham Nov 5, 2018

jgraham Nov 5, 2018

jgraham left a comment

jugglinmike commented Nov 6, 2018

jugglinmike commented Nov 6, 2018

[wptrunner] Transmit test results via postMessage #13881

[wptrunner] Transmit test results via postMessage #13881

Conversation

jugglinmike commented Nov 2, 2018

jugglinmike commented Nov 2, 2018

foolip commented Nov 2, 2018

jugglinmike commented Nov 3, 2018

foolip commented Nov 3, 2018

lukebjerring commented Nov 3, 2018

lukebjerring commented Nov 3, 2018

lukebjerring commented Nov 3, 2018 • edited Loading

foolip commented Nov 3, 2018

gsnedders left a comment

Choose a reason for hiding this comment

jugglinmike commented Nov 5, 2018

jgraham commented Nov 5, 2018

jgraham left a comment

Choose a reason for hiding this comment

jgraham Nov 5, 2018

Choose a reason for hiding this comment

jgraham Nov 5, 2018

Choose a reason for hiding this comment

jgraham Nov 5, 2018

Choose a reason for hiding this comment

jgraham left a comment

Choose a reason for hiding this comment

jugglinmike commented Nov 6, 2018

jugglinmike commented Nov 6, 2018

lukebjerring commented Nov 3, 2018 •

edited

Loading