[WIP] Revert "Infra: remove old service worker code from testharness.js"

[stephenmcgruer]

This reverts commit d7032c0.

WIP: I want to see the results of this on TaskCluster; please don't review yet.

[jgraham]

What's the plan with this one?

[stephenmcgruer]

What's the plan with this one?

Fantastic question. I was 100% sure that I had run a trigger run on Chrome with this, but it seems like I didn't. So I'm starting with that, and if it shows the same (well, the inverse) results as #21162 (comment) I'm going to investigate those further on the Chromium side (maybe pull in the worker folks to ask).

[stephenmcgruer]

https://wpt.fyi/results/?diff&filter=ADC&run_id=455690005&run_id=480270002 should be the diff. Some flakes, as expected, but the service-worker/ changes seem to back up what happened in #21162 (comment) - a handful of tests in client-navigate.html come back (https://wpt.fyi/results/service-workers/service-worker/client-navigate.https.html?diff&filter=ADC&run_id=455690005&run_id=480270002).

cc @nhiroki who I believe is on the Chromium service worker team.

Hiroki - this PR is about some old Chrome-specific code that was in testharness.js, which mentioned a problem with 'Blink setting MessageEvent.source to null for messages sent via ServiceWorker.postMessage()'. That code was removed in #21162 but I noticed a few possible regressions in service-worker tests in Chromium, and was trying to figure out if the code was therefore still needed. Do you have any context here, or understand the comment in the removed code?

[nhiroki]

'Blink setting MessageEvent.source to null for messages sent via ServiceWorker.postMessage()'

This should already be supported in Blink (https://crbug.com/543198).

I guess that the original PR needs more work. In the original code, postMessage() transfers the message port to the worker.

worker.postMessage({type: "connect"}, [message_channel.port2]);

Looks like some tests still depend on the transferre port instead of the ExtendableMessageEvent.source attribute (e.g., wpt/service-workers/service-worker/resources/client-navigate-worker.js). Probably service workers in the tests cannot handle the message event due to this, and time out.

[nhiroki]

@stephenmcgruer @annevk I'll make a patch to fix this (chromium issue).

[nhiroki]

[edited] I understand the root cause. The failing tests use an inner frame's service worker object's postMessage(), while they wait on the main frame's navigator.serviceWorker.onmessage. In this case, event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout.

In addition, we could remove the leftover in ServiceWorkerTestEnvironment() in testharness.js.

if (event.ports && event.ports[0]) {
    // If a MessageChannel was passed, then use it to
    // send results back to the main window.  This
    // allows the tests to work even if the browser
    // does not fully support MessageEvent.source in
    // ServiceWorkers yet.
    this_obj._add_message_port(event.ports[0]);
    event.ports[0].start();
} else {
    // If there is no MessageChannel, then attempt to
    // use the MessageEvent.source to send results
    // back to the main window.
    this_obj._add_message_port(event.source);
}

[nhiroki]

I understand the root cause. The failing tests use an inner frame's navigatior.serviceWorker.postMessage(), while they wait on the main frame's navigator.serviceWorker.onmessage. In this case, event.source is the inner frame, and a reply from the service worker is dispatched on the inner frame's navigator.serviceWorker.onmessage. This results in timeout.

Let me elaborate on this more because this is hidden in fetch_tests_from_worker() helper and it's a bit difficult to trace what happens there.

The failing tests use fetch_tests_from_worker() helper. This helper takes a service worker object to post a "connect" message to the worker, and then passes the worker object to RemoteContext in testharness.js. RemoteContext adds an onmessage event handler on self.navigator.serviceWorker. This works well when the given worker object is associated with self. However, these failing tests pass the worker object associated with the inner frame (not self) to the helper, so the mismatch explained above happens.

I think this should be fixed in the tests, not in testharness.js. I'll make a fix.

[annevk]

Thanks @stephenmcgruer and @nhiroki for dealing with this in such a thoughtful manner! Glad to see testharness.js might see further simplifications.

[stephenmcgruer]

Thanks very much for the investigation and fixes @nhiroki . I will close this PR now :)