Bug 178977

Summary: Update testharness.js to work around our lack of support for MessagePort in service workers
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Tools / TestsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, ggaren, lforschler, rniwa, sam, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch none

Description Chris Dumez 2017-10-27 21:33:36 PDT
Update testharness.js to work around our lack of support for MessagePort in service workers, similarly to what was already done for Edge.
Comment 1 Chris Dumez 2017-10-27 21:35:50 PDT
Created attachment 325240 [details]
Patch
Comment 2 Build Bot 2017-10-27 22:30:33 PDT
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
Comment 3 Build Bot 2017-10-27 22:30:34 PDT
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
Comment 4 Build Bot 2017-10-27 22:36:20 PDT
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
Comment 5 Build Bot 2017-10-27 22:36:22 PDT
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
Comment 6 Chris Dumez 2017-10-27 22:51:26 PDT
Created attachment 325248 [details]
Patch
Comment 7 Build Bot 2017-10-28 00:13:34 PDT
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
Comment 8 Build Bot 2017-10-28 00:13:35 PDT
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
Comment 9 Sam Weinig 2017-10-28 07:10:54 PDT
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?
Comment 10 Chris Dumez 2017-10-28 09:23:45 PDT
(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.
Comment 11 Chris Dumez 2017-10-28 09:48:39 PDT
Created attachment 325262 [details]
Patch
Comment 12 Chris Dumez 2017-10-28 10:06:04 PDT
(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.
Comment 13 Sam Weinig 2017-10-28 10:17:50 PDT
Comment on attachment 325262 [details]
Patch

r=me. Are you going to push this change upstream as well?
Comment 14 youenn fablet 2017-10-28 10:29:36 PDT
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.
Comment 15 Chris Dumez 2017-10-28 10:36:54 PDT
(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.
Comment 16 Chris Dumez 2017-10-28 10:38:05 PDT
Comment on attachment 325262 [details]
Patch

Clearing flags on attachment: 325262

Committed r224152: <https://trac.webkit.org/changeset/224152>
Comment 17 Chris Dumez 2017-10-28 10:38:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Chris Dumez 2017-10-28 10:40:03 PDT
(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.
Comment 19 Chris Dumez 2017-10-28 11:09:18 PDT
(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.
Comment 20 youenn fablet 2017-10-28 12:00:24 PDT
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.
Comment 21 Chris Dumez 2017-10-29 16:44:30 PDT
(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.
Comment 22 Radar WebKit Bug Importer 2017-11-15 12:34:55 PST
<rdar://problem/35567815>