Bug 178977 – Update testharness.js to work around our lack of support for MessagePort in service workers

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

Update testharness.js to work around our lack of support for MessagePort in service workers, similarly to what was already done for Edge.

[Chris Dumez]

Created attachment 325240 [details]
Patch

[Build Bot]

Comment on attachment 325240 [details]
Patch

Attachment 325240 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5020009

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/not-in-shadow-tree.html
imported/w3c/web-platform-tests/shadow-dom/slotchange-event.html

[Build Bot]

Created attachment 325244 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 325240 [details]
Patch

Attachment 325240 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5020014

New failing tests:
imported/w3c/web-platform-tests/streams/piping/general.html
imported/w3c/web-platform-tests/html/semantics/document-metadata/the-meta-element/pragma-directives/attr-meta-http-equiv-refresh/not-in-shadow-tree.html
imported/w3c/web-platform-tests/shadow-dom/slotchange-event.html

[Build Bot]

Created attachment 325245 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 325248 [details]
Patch

[Build Bot]

Comment on attachment 325248 [details]
Patch

Attachment 325248 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5020726

New failing tests:
imported/w3c/web-platform-tests/html/browsers/browsing-the-web/history-traversal/persisted-user-state-restoration/scroll-restoration-fragment-scrolling-cross-origin.html

[Build Bot]

Created attachment 325252 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Sam Weinig]

Comment on attachment 325248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325248&action=review

> LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js:2093
> +            // The ServiceWorker's implicit MessagePort is currently not
> +            // reliably accessible from the ServiceWorkerGlobalScope due to
> +            // Blink setting MessageEvent.source to null for messages sent
> +            // via ServiceWorker.postMessage(). Until that's resolved,
> +            // create an explicit MessageChannel and pass one end to the
> +            // worker.
> +            if (window.MessageChannel && !!window.chrome) {

What does this do for Firefox?

[Chris Dumez]

(In reply to Sam Weinig from comment #9)
> Comment on attachment 325248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325248&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js:2093
> > +            // The ServiceWorker's implicit MessagePort is currently not
> > +            // reliably accessible from the ServiceWorkerGlobalScope due to
> > +            // Blink setting MessageEvent.source to null for messages sent
> > +            // via ServiceWorker.postMessage(). Until that's resolved,
> > +            // create an explicit MessageChannel and pass one end to the
> > +            // worker.
> > +            if (window.MessageChannel && !!window.chrome) {
> 
> What does this do for Firefox?

My patch makes every browser except Chrome use postMessage on the service worker instead of using MessagePort. The comment was explaining that using MessagePort was to work around a bug in Chrome. I could make the change minimal by detecting WebKit but I do not know of a good way to do so.

[Chris Dumez]

Created attachment 325262 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> (In reply to Sam Weinig from comment #9)
> > Comment on attachment 325248 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=325248&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/resources/testharness.js:2093
> > > +            // The ServiceWorker's implicit MessagePort is currently not
> > > +            // reliably accessible from the ServiceWorkerGlobalScope due to
> > > +            // Blink setting MessageEvent.source to null for messages sent
> > > +            // via ServiceWorker.postMessage(). Until that's resolved,
> > > +            // create an explicit MessageChannel and pass one end to the
> > > +            // worker.
> > > +            if (window.MessageChannel && !!window.chrome) {
> > 
> > What does this do for Firefox?
> 
> My patch makes every browser except Chrome use postMessage on the service
> worker instead of using MessagePort. The comment was explaining that using
> MessagePort was to work around a bug in Chrome. I could make the change
> minimal by detecting WebKit but I do not know of a good way to do so.

Note that using window.safari is not suitable because it won't work in WKTR, only in Safari.

[Sam Weinig]

Comment on attachment 325262 [details]
Patch

r=me. Are you going to push this change upstream as well?

[youenn fablet]

I think this is good enough for WebKit trunk and we should probably try to have a better plan when upstreaming WPT if needed.

I am surprised we are not able to unskip some previously timing-out tests.
I guess there is some additional missing support for these tests.
Maybe we should wait for this additional support before committing this patch.
In that patch, we could then unskip some tests.

[Chris Dumez]

(In reply to youenn fablet from comment #14)
> I think this is good enough for WebKit trunk and we should probably try to
> have a better plan when upstreaming WPT if needed.
> 
> I am surprised we are not able to unskip some previously timing-out tests.
> I guess there is some additional missing support for these tests.
> Maybe we should wait for this additional support before committing this
> patch.
> In that patch, we could then unskip some tests.

As discussed offline, tests are stuck *before* even getting there. They're stuck on:
function wait_for_update(test, registration) {
  if (!registration || registration.unregister == undefined) {
    return Promise.reject(new Error(
      'wait_for_update must be passed a ServiceWorkerRegistration'));
  }

  return new Promise(test.step_func(function(resolve) {
      registration.addEventListener('updatefound', test.step_func(function() {
          resolve(registration.installing);
        }));
    }));
}

Because we do not fire the "updatefound" event and registration.installing is null. Brady is working on this.

[Chris Dumez]

Comment on attachment 325262 [details]
Patch

Clearing flags on attachment: 325262

Committed r224152: <https://trac.webkit.org/changeset/224152>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to youenn fablet from comment #14)
> I think this is good enough for WebKit trunk and we should probably try to
> have a better plan when upstreaming WPT if needed.
> 
> I am surprised we are not able to unskip some previously timing-out tests.
> I guess there is some additional missing support for these tests.
> Maybe we should wait for this additional support before committing this
> patch.
> In that patch, we could then unskip some tests.

Also Youenn, I think we are going to hit the expired certificate issue because the first thing the tests do is fetch the script containing the tests from the service worker and those tests run over HTTPs. I believe we need to fix this sooner rather than later. We normally ask the client about the certificate but we currently fail to do so for fetches from service workers.

[Chris Dumez]

(In reply to Sam Weinig from comment #13)
> Comment on attachment 325262 [details]
> Patch
> 
> r=me. Are you going to push this change upstream as well?

Not this exact change. We'll either have to push a better change upstream or implement MessagePort support (there is a bug tracking this on me). For now, I am merely trying to get tests started downstream.

[youenn fablet]

Since this is a change to WPT testharness.js and we want to keep the change, we should probably skip importing testharness.js (see LayoutTests/imported/w3c/resources).

> Because we do not fire the "updatefound" event and registration.installing
> is null. Brady is working on this.

OK, if it takes time to have the full right behavior, we may want to do something similar getRegistration() returning null which allows to go to the meat of the tests sooner.
Is there a way to hack some of testharness.js infrastructure to check whether we are missing other stuff?

> Also Youenn, I think we are going to hit the expired certificate issue
> because the first thing the tests do is fetch the script containing the
> tests from the service worker and those tests run over HTTPs. I believe we
> need to fix this sooner rather than later. We normally ask the client about
> the certificate but we currently fail to do so for fetches from service
> workers.

The issue here is that service worker is not attached to a webpage, so we cannot call directly a webpage related callback that WTR is implementing.
I see several approaches:
-Reuse a webpage/webpage-callback, but it is difficult to know which webpage callback to pick
- Add a dedicated service worker callback (meaning probably new API for apps to control service workers)
- Alternatively, add a specific boolean switch at WebProcessPool level plus necessary C/ObjC API that would lower the security for WTR only.

[Chris Dumez]

(In reply to youenn fablet from comment #20)
> Since this is a change to WPT testharness.js and we want to keep the change,
> we should probably skip importing testharness.js (see
> LayoutTests/imported/w3c/resources).
> 
> > Because we do not fire the "updatefound" event and registration.installing
> > is null. Brady is working on this.
> 
> OK, if it takes time to have the full right behavior, we may want to do
> something similar getRegistration() returning null which allows to go to the
> meat of the tests sooner.
> Is there a way to hack some of testharness.js infrastructure to check
> whether we are missing other stuff?

Given that out may take time to get the full right behavior, I am indeed faking some things to get WPT tests started via Bug 178985.

[Radar WebKit Bug Importer]

<rdar://problem/35567815>