Update testharness.js to work around our lack of support for MessagePort in service workers, similarly to what was already done for Edge.
Created attachment 325240 [details] Patch
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
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 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
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
Created attachment 325248 [details] Patch
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
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 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?
(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.
Created attachment 325262 [details] Patch
(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 on attachment 325262 [details] Patch r=me. Are you going to push this change upstream as well?
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.
(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 on attachment 325262 [details] Patch Clearing flags on attachment: 325262 Committed r224152: <https://trac.webkit.org/changeset/224152>
All reviewed patches have been landed. Closing bug.
(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.
(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.
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.
(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.
<rdar://problem/35567815>