Bug 179404 - Implement ServiceWorker handle fetch for navigation loads
Summary: Implement ServiceWorker handle fetch for navigation loads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-07 17:27 PST by youenn fablet
Modified: 2017-11-15 10:23 PST (History)
9 users (show)

See Also:


Attachments
wip (210.01 KB, patch)
2017-11-07 17:35 PST, youenn fablet
no flags Details | Formatted Diff | Diff
wip (57.32 KB, patch)
2017-11-08 13:25 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (421.65 KB, application/zip)
2017-11-08 14:13 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (370.23 KB, application/zip)
2017-11-08 14:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (3.38 MB, application/zip)
2017-11-08 14:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.26 MB, application/zip)
2017-11-08 15:32 PST, Build Bot
no flags Details
Patch (41.00 KB, patch)
2017-11-09 14:58 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (41.06 KB, patch)
2017-11-09 15:31 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.77 MB, application/zip)
2017-11-09 16:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (2.21 MB, application/zip)
2017-11-09 16:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.56 MB, application/zip)
2017-11-09 16:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (3.26 MB, application/zip)
2017-11-09 17:17 PST, Build Bot
no flags Details
Patch (40.79 KB, patch)
2017-11-09 18:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2017-11-09 22:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (32.16 KB, patch)
2017-11-10 01:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.68 MB, application/zip)
2017-11-10 02:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.16 MB, application/zip)
2017-11-10 02:51 PST, Build Bot
no flags Details
Patch (189.36 KB, patch)
2017-11-10 07:47 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (189.61 KB, patch)
2017-11-10 07:51 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.15 MB, application/zip)
2017-11-10 09:19 PST, Build Bot
no flags Details
Patch (209.90 KB, patch)
2017-11-13 10:09 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (209.88 KB, patch)
2017-11-13 16:41 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (217.01 KB, patch)
2017-11-13 18:48 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.24 MB, application/zip)
2017-11-13 20:13 PST, Build Bot
no flags Details
Patch (217.42 KB, patch)
2017-11-13 20:32 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (214.01 KB, patch)
2017-11-14 13:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (209.06 KB, patch)
2017-11-14 14:40 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (209.08 KB, patch)
2017-11-14 14:44 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-11-07 17:27:41 PST
Implement ServiceWorker handle fetch for navigation loads
Comment 1 youenn fablet 2017-11-07 17:35:22 PST
Created attachment 326276 [details]
wip
Comment 2 youenn fablet 2017-11-07 17:35:51 PST
Will probably split it into smaller patches.
Comment 3 youenn fablet 2017-11-08 13:25:24 PST
Created attachment 326358 [details]
wip
Comment 4 Build Bot 2017-11-08 13:26:42 PST
Attachment 326358 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/DocumentLoader.cpp:875:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:79:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/platform/network/cocoa/ResourceResponseCocoa.mm:82:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2017-11-08 14:13:55 PST
Comment on attachment 326358 [details]
wip

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

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2017-11-08 14:13:56 PST
Created attachment 326370 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Build Bot 2017-11-08 14:19:38 PST
Comment on attachment 326358 [details]
wip

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

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2017-11-08 14:19:40 PST
Created attachment 326374 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-11-08 14:58:13 PST
Comment on attachment 326358 [details]
wip

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

New failing tests:
http/tests/workers/service/service-worker-getRegistration.html
http/tests/security/credentials-main-resource.html
imported/w3c/web-platform-tests/service-workers/service-worker/getregistration.https.html
Comment 10 Build Bot 2017-11-08 14:58:15 PST
Created attachment 326387 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-11-08 15:31:59 PST
Comment on attachment 326358 [details]
wip

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

New failing tests:
http/tests/workers/service/service-worker-getRegistration.html
http/tests/security/credentials-main-resource.html
Comment 12 Build Bot 2017-11-08 15:32:00 PST
Created attachment 326394 [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 13 youenn fablet 2017-11-09 14:58:49 PST
Created attachment 326493 [details]
Patch
Comment 14 youenn fablet 2017-11-09 15:31:03 PST
Created attachment 326496 [details]
Patch
Comment 15 Build Bot 2017-11-09 16:19:25 PST
Comment on attachment 326496 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 16 Build Bot 2017-11-09 16:19:27 PST
Created attachment 326506 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-11-09 16:28:55 PST
Comment on attachment 326496 [details]
Patch

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

New failing tests:
http/tests/security/credentials-main-resource.html
Comment 18 Build Bot 2017-11-09 16:28:56 PST
Created attachment 326507 [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 19 Build Bot 2017-11-09 16:55:32 PST
Comment on attachment 326496 [details]
Patch

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

New failing tests:
http/tests/misc/window-dot-stop.html
http/tests/security/XFrameOptions/x-frame-options-ignore-deny-meta-tag-parent-same-origin-deny.html
http/tests/navigation/redirect-to-invalid-url.html
http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-allow.html
http/tests/loading/server-redirect-for-provisional-load-caching.html
http/tests/security/XFrameOptions/x-frame-options-multiple-headers-conflict.html
http/tests/security/credentials-main-resource.html
http/tests/misc/will-send-request-returns-null-on-redirect.html
fast/loader/main-document-url-for-non-http-loads.html
imported/w3c/web-platform-tests/cors/remote-origin.htm
http/tests/security/XFrameOptions/x-frame-options-invalid.html
http/tests/security/blocked-on-redirect.html
http/tests/contentdispositionattachmentsandbox/cross-origin-frames-disabled.html
http/tests/navigation/redirect-preserves-fragment.html
http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny.html
http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-allow.html
webarchive/loading/test-loading-archive.html
http/tests/security/XFrameOptions/x-frame-options-ignore-deny-meta-tag-in-body.html
loader/go-back-cached-main-resource.html
http/tests/security/XFrameOptions/x-frame-options-deny.html
http/tests/navigation/redirect-to-fragment.html
http/tests/security/XFrameOptions/x-frame-options-parent-same-origin-deny.html
http/tests/security/XFrameOptions/x-frame-options-allowall.html
http/tests/security/XFrameOptions/x-frame-options-ignore-deny-meta-tag.html
fast/loader/javascript-url-iframe-remove-on-navigate.html
fast/events/beforeunload-dom-manipulation-crash.html
http/tests/security/XFrameOptions/x-frame-options-ignore-deny-meta-tag-parent-same-origin-allow.html
Comment 20 Build Bot 2017-11-09 16:55:34 PST
Created attachment 326512 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 21 Build Bot 2017-11-09 17:17:05 PST
Comment on attachment 326496 [details]
Patch

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

New failing tests:
http/tests/security/credentials-main-resource.html
Comment 22 Build Bot 2017-11-09 17:17:07 PST
Created attachment 326515 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 23 youenn fablet 2017-11-09 18:33:50 PST
Created attachment 326528 [details]
Patch
Comment 24 Chris Dumez 2017-11-09 20:20:45 PST
Comment on attachment 326528 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:69
> +class CachedResourceRequest;

Unused?

> Source/WebCore/loader/DocumentLoader.h:230
> +    void doLoadingMainResource(ResourceRequest&&);

Why is this public?

> Source/WebCore/loader/DocumentLoader.h:487
> +    std::optional<ServiceWorkerRegistrationData> m_registrationData;

m_registrationData is too generic a name for this class. Probably m_serviceWorkerRegistrationData;

> Source/WebCore/loader/DocumentThreadableLoader.cpp:322
> +    if (response.type() == ResourceResponse::Type::Default || response.type() == ResourceResponse::Type::Basic || response.type() == ResourceResponse::Type::Cors || response.type() == ResourceResponse::Type::Opaque) {

Feels like we should use a switch() statement?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:292
> +void CachedResourceRequest::setNavigationServiceWorkerRegistrationData(const ServiceWorkerRegistrationData* data)

Would look nicer if it took a const std::optional<ServiceWorkerRegistrationData>& as parameter. It is also convenient since it is the type the call site has.

Also, some to think of it, why isn't this a setServiceWorkerIdentifier() or setServiceWorker() method? Seems unnecessary to pass all the data?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:-77
> -    return connection.hasServiceWorkerRegisteredForOrigin(*resource->origin());

Why is this OK? Have we already done this check earlier? If so, can this be an assertion?

> LayoutTests/imported/w3c/ChangeLog:8
> +        * web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https-expected.txt:

You rebaselined a lot of skipped / flaky tests. A lot (most?) of them have different output for reasons that are unrelated to your change. This makes things confusing as it is hard to identify the changes caused by this particular change.
While it may be useful to rebaseline those tests, I think this should be done separately.

> LayoutTests/TestExpectations:169
> +imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https.html [ Pass Failure ]

Do we know why this is flaky now? If so, a comment would be appropriate. Also, this should be moved to the flaky SW tests section below, not kept in the section of SW tests that time out.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-cache.https-expected.txt:1
> +CONSOLE MESSAGE: Origin https://localhost:9443 is not allowed by Access-Control-Allow-Origin.

I do not think those are related to your change?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https-expected.txt:1
> +CONSOLE MESSAGE: Origin https://localhost:9443 is not allowed by Access-Control-Allow-Origin.

I do not think those are related to your change?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/performance-timeline.https-expected.txt:4
> +FAIL empty service worker fetch event included in performance timings assert_greater_than: Slow service worker request should measure increased delay. expected a number greater than 1150.1 but got 146.3

We should not rebaseline this test.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-foreign-fetch-errors.https-expected.txt:-2
> -Harness Error (TIMEOUT), message = null

Should we unskip this test?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https-expected.txt:-2
> -FAIL register-via-api-updateViaCache-undefined promise_test: Unhandled rejection with value: object "NotSupportedError: ServiceWorkerRegistration::update not yet implemented"

Not related to your change.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt:-3
> -Harness Error (TIMEOUT), message = null

Does not look related to your change.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/update-bytecheck.https-expected.txt:-2
> -FAIL Test(cors: false, main: default, imported: default) promise_test: Unhandled rejection with value: object "NotSupportedError: ServiceWorkerRegistration::update not yet implemented"

Does not look related to your change.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/websocket.https-expected.txt:1
> +CONSOLE MESSAGE: WebSocket connection to 'wss://localhost:62136/echo' failed: Unexpected response code: 404

Should not be rebaselined. this test is flaky and the output depends on the machine.
Comment 25 youenn fablet 2017-11-09 21:42:32 PST
(In reply to Chris Dumez from comment #24)
> Comment on attachment 326528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326528&action=review
> 
> > Source/WebCore/loader/DocumentLoader.h:69
> > +class CachedResourceRequest;
> 
> Unused?

OK

> > Source/WebCore/loader/DocumentLoader.h:230
> > +    void doLoadingMainResource(ResourceRequest&&);
> 
> Why is this public?

Should be private.
 
> > Source/WebCore/loader/DocumentLoader.h:487
> > +    std::optional<ServiceWorkerRegistrationData> m_registrationData;
> 
> m_registrationData is too generic a name for this class. Probably
> m_serviceWorkerRegistrationData;

OK

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:322
> > +    if (response.type() == ResourceResponse::Type::Default || response.type() == ResourceResponse::Type::Basic || response.type() == ResourceResponse::Type::Cors || response.type() == ResourceResponse::Type::Opaque) {
> 
> Feels like we should use a switch() statement?

OK

> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:292
> > +void CachedResourceRequest::setNavigationServiceWorkerRegistrationData(const ServiceWorkerRegistrationData* data)
> 
> Would look nicer if it took a const
> std::optional<ServiceWorkerRegistrationData>& as parameter. It is also
> convenient since it is the type the call site has.
> 
> Also, some to think of it, why isn't this a setServiceWorkerIdentifier() or
> setServiceWorker() method? Seems unnecessary to pass all the data?

This method is navigation specific.
We could pass an identifier but it should be made optional.

> > Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:-77
> > -    return connection.hasServiceWorkerRegisteredForOrigin(*resource->origin());
> 
> Why is this OK? Have we already done this check earlier? If so, can this be
> an assertion?

There is no need to check this now since the request has an active service worker identifier, otherwise we would return false above.
There is an assert below to ensure there is an identifier when sending the IPC.
That said, we can probably remove the resource check as well now.

> > LayoutTests/imported/w3c/ChangeLog:8
> > +        * web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https-expected.txt:
> 
> You rebaselined a lot of skipped / flaky tests. A lot (most?) of them have
> different output for reasons that are unrelated to your change. This makes
> things confusing as it is hard to identify the changes caused by this
> particular change.
> While it may be useful to rebaseline those tests, I think this should be
> done separately.

I disagree.
Given the number of tests that are timeout/flaky, we should rebase every test for every patch. Otherwise, we end up with a flakiness dashboard with tests marked as flaky and that are failing 100% of the time, maybe just needing a rebaseline.

Let's start now with this patch, I don't think this hurts in reading the results for this patch.

> > LayoutTests/TestExpectations:169
> > +imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https.html [ Pass Failure ]
> 
> Do we know why this is flaky now? If so, a comment would be appropriate.
> Also, this should be moved to the flaky SW tests section below, not kept in
> the section of SW tests that time out.

It is stable on my machine.
I kept it pass/fail since we have a lot of uncertainty currently with tests.
Unskipping it is important so that we can later on easily identify whether flaky or not using the dashboard.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-cache.https-expected.txt:1
> > +CONSOLE MESSAGE: Origin https://localhost:9443 is not allowed by Access-Control-Allow-Origin.
> 
> I do not think those are related to your change?

They might actually since with this patch, sub resources loads from iframes are now intercepted by SW.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https-expected.txt:1
> > +CONSOLE MESSAGE: Origin https://localhost:9443 is not allowed by Access-Control-Allow-Origin.
> 
> I do not think those are related to your change?

Ditto.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/performance-timeline.https-expected.txt:4
> > +FAIL empty service worker fetch event included in performance timings assert_greater_than: Slow service worker request should measure increased delay. expected a number greater than 1150.1 but got 146.3
> 
> We should not rebaseline this test.

OK

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/register-foreign-fetch-errors.https-expected.txt:-2
> > -Harness Error (TIMEOUT), message = null
> 
> Should we unskip this test?

Let's wait for landing the patch and see dashboard results.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https-expected.txt:-2
> > -FAIL register-via-api-updateViaCache-undefined promise_test: Unhandled rejection with value: object "NotSupportedError: ServiceWorkerRegistration::update not yet implemented"
> 
> Not related to your change.

Probably but does not hurt.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https-expected.txt:-3
> > -Harness Error (TIMEOUT), message = null
> 
> Does not look related to your change.

Ditto.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/update-bytecheck.https-expected.txt:-2
> > -FAIL Test(cors: false, main: default, imported: default) promise_test: Unhandled rejection with value: object "NotSupportedError: ServiceWorkerRegistration::update not yet implemented"

Ditto.
 
> Does not look related to your change.
> 
> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/websocket.https-expected.txt:1
> > +CONSOLE MESSAGE: WebSocket connection to 'wss://localhost:62136/echo' failed: Unexpected response code: 404
> 
> Should not be rebaselined. this test is flaky and the output depends on the
> machine.

OK
Comment 26 Chris Dumez 2017-11-09 21:57:16 PST
Comment on attachment 326528 [details]
Patch

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

>>> LayoutTests/imported/w3c/ChangeLog:8
>>> +        * web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https-expected.txt:
>> 
>> You rebaselined a lot of skipped / flaky tests. A lot (most?) of them have different output for reasons that are unrelated to your change. This makes things confusing as it is hard to identify the changes caused by this particular change.
>> While it may be useful to rebaseline those tests, I think this should be done separately.
> 
> I disagree.
> Given the number of tests that are timeout/flaky, we should rebase every test for every patch. Otherwise, we end up with a flakiness dashboard with tests marked as flaky and that are failing 100% of the time, maybe just needing a rebaseline.
> 
> Let's start now with this patch, I don't think this hurts in reading the results for this patch.

I am rebaselining the tests in https://bugs.webkit.org/show_bug.cgi?id=179521. Right now, I assume you have no test coverage for this change.
Comment 27 Chris Dumez 2017-11-09 21:58:42 PST
Comment on attachment 326528 [details]
Patch

r- because it does not look we have test coverage.
Comment 28 youenn fablet 2017-11-09 22:30:16 PST
Created attachment 326556 [details]
Patch
Comment 29 youenn fablet 2017-11-09 22:37:35 PST
(In reply to Chris Dumez from comment #27)
> Comment on attachment 326528 [details]
> Patch
> 
> r- because it does not look we have test coverage.

I am a bit puzzled by this review.
The fact that the tests are stable is a proof in itself that we are going in the right direction.

This patch does not remove the selection hack as some http/tests/workers/service rely on it right now. This is a natural follow-up to this patch.
When removing the hack, we will need to rewrite (or remove if redundant) these tests.

I did the experiment in the previous version of the patch to remove the hack and run wpt service workers. It was changing a few results but nothing big.
Comment 30 Chris Dumez 2017-11-09 22:45:48 PST
(In reply to youenn fablet from comment #29)
> (In reply to Chris Dumez from comment #27)
> > Comment on attachment 326528 [details]
> > Patch
> > 
> > r- because it does not look we have test coverage.
> 
> I am a bit puzzled by this review.
> The fact that the tests are stable is a proof in itself that we are going in
> the right direction.
> 
> This patch does not remove the selection hack as some
> http/tests/workers/service rely on it right now. This is a natural follow-up
> to this patch.
> When removing the hack, we will need to rewrite (or remove if redundant)
> these tests.
> 
> I did the experiment in the previous version of the patch to remove the hack
> and run wpt service workers. It was changing a few results but nothing big.

Maybe I am missing something.
This patch is supposed to support fetching of main resources, this sounds testable, no?
Comment 31 Chris Dumez 2017-11-09 22:49:11 PST
(In reply to youenn fablet from comment #29)
> (In reply to Chris Dumez from comment #27)
> > Comment on attachment 326528 [details]
> > Patch
> > 
> > r- because it does not look we have test coverage.
> 
> I am a bit puzzled by this review.
> The fact that the tests are stable is a proof in itself that we are going in
> the right direction.

Tests being stable are usually proof of no behavior change, no? The bug title says we are implementing something. I would expect some WPT tests to progress or a new layout test coverage the behavior change.
Comment 32 Chris Dumez 2017-11-09 22:51:27 PST
Comment on attachment 326556 [details]
Patch

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

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:-71
> -    if (isNonSubresourceRequest(options.destination) || !options.serviceWorkerIdentifier)

This is the part that looks interesting. We used to return false for main resources and now we no longer do. This changes behavior, no?
Comment 33 youenn fablet 2017-11-10 01:34:40 PST
Created attachment 326570 [details]
Patch
Comment 34 youenn fablet 2017-11-10 01:46:23 PST
> This is the part that looks interesting. We used to return false for main
> resources and now we no longer do. This changes behavior, no?

This will once we have an active service worker identifier in the registration data.
I updated the patch to use the installing service worker instead. This mimics the hack to go from installing to active on ServiceWorkerContainer. Not great, but there are some visible behavioral changes now.

Some tests are timing out due to the lack of service worker blob responses support.
Comment 35 Build Bot 2017-11-10 02:30:08 PST
Comment on attachment 326570 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 36 Build Bot 2017-11-10 02:30:10 PST
Created attachment 326572 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 37 Build Bot 2017-11-10 02:51:10 PST
Comment on attachment 326570 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 38 Build Bot 2017-11-10 02:51:12 PST
Created attachment 326573 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 39 youenn fablet 2017-11-10 07:47:36 PST
Created attachment 326578 [details]
Patch
Comment 40 youenn fablet 2017-11-10 07:51:32 PST
Created attachment 326579 [details]
Patch
Comment 41 Chris Dumez 2017-11-10 08:09:05 PST
Comment on attachment 326579 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:294
> +    // FIXME: Remove the use of installingServiceWorkerIdentifier when we get an activeServiceWorkerIdentifier.

I do not understand this comment, why don't we get an activeServiceWorkerIdentifier ? Brady landed patches in the last few days to make sure all 3 identifiers are populated and kept up to date.
Comment 42 Chris Dumez 2017-11-10 08:10:56 PST
Comment on attachment 326579 [details]
Patch

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

> LayoutTests/http/tests/workers/service/service-worker-main-resource.https.html:9
> +    var registration = await navigator.serviceWorker.register("resources/service-worker-fetch-worker.js", { scope: "/" });

After this, shouldn't we wait for registration.installing to before activated before loading the iframe? I thought this was what the WPT tests were doing.
Comment 43 Chris Dumez 2017-11-10 08:35:54 PST
Comment on attachment 326579 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:875
> +            m_frame->document()->setActiveServiceWorker(ServiceWorker::create(*m_frame->document(), *m_serviceWorkerRegistrationData->activeServiceWorkerIdentifier, m_serviceWorkerRegistrationData->scriptURL));

We probably want to pass ServiceWorkerState::Activated as last parameter to the service worker constructor, at least until we can do better.
Comment 44 youenn fablet 2017-11-10 09:15:48 PST
(In reply to Chris Dumez from comment #41)
> Comment on attachment 326579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326579&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:294
> > +    // FIXME: Remove the use of installingServiceWorkerIdentifier when we get an activeServiceWorkerIdentifier.
> 
> I do not understand this comment, why don't we get an
> activeServiceWorkerIdentifier ? Brady landed patches in the last few days to
> make sure all 3 identifiers are populated and kept up to date.

I haven’t checked precisely. Infrastructure seems there in storage process.
Maybe we have not yet implemented the state changes from service worker thread proxy up to storage process.

(In reply to Chris Dumez from comment #42)
> Comment on attachment 326579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326579&action=review
> 
> > LayoutTests/http/tests/workers/service/service-worker-main-resource.https.html:9
> > +    var registration = await navigator.serviceWorker.register("resources/service-worker-fetch-worker.js", { scope: "/" });
> 
> After this, shouldn't we wait for registration.installing to before
> activated before loading the iframe? I thought this was what the WPT tests
> were doing.

Right. This test will probably become redundant against wpt soon so that it can be removed. This might be done with other tests as well which are not spec compliant.

(In reply to Chris Dumez from comment #43)
> Comment on attachment 326579 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326579&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:875
> > +            m_frame->document()->setActiveServiceWorker(ServiceWorker::create(*m_frame->document(), *m_serviceWorkerRegistrationData->activeServiceWorkerIdentifier, m_serviceWorkerRegistrationData->scriptURL));
> 
> We probably want to pass ServiceWorkerState::Activated as last parameter to
> the service worker constructor, at least until we can do better.

OK. Creating a sw here is a kind of a hack.
Ideally, we should keep a registration, which would isolate from waiting/active differences for instance. And we should not require navigator/service worker container for that.
Comment 45 Build Bot 2017-11-10 09:19:13 PST
Comment on attachment 326579 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-respond-with-response-body-with-invalid-chunk.https.html
Comment 46 Build Bot 2017-11-10 09:19:15 PST
Created attachment 326594 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 47 Chris Dumez 2017-11-10 09:37:46 PST
Comment on attachment 326579 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:294
>>> +    // FIXME: Remove the use of installingServiceWorkerIdentifier when we get an activeServiceWorkerIdentifier.
>> 
>> I do not understand this comment, why don't we get an activeServiceWorkerIdentifier ? Brady landed patches in the last few days to make sure all 3 identifiers are populated and kept up to date.
> 
> I haven’t checked precisely. Infrastructure seems there in storage process.
> Maybe we have not yet implemented the state changes from service worker thread proxy up to storage process.
> 
> (In reply to Chris Dumez from comment #42)

Not sure why you are talking about the service worker thread proxy. ServiceWorker objects on WebProcess side *are* kept in sync with their corresponding SWServerWorker on Storage process side. This is implemented and well tested.
I am also sure that Brady implemented support for all 3 identifiers in ServiceWorkerRegistrationData.

>>> LayoutTests/http/tests/workers/service/service-worker-main-resource.https.html:9
>>> +    var registration = await navigator.serviceWorker.register("resources/service-worker-fetch-worker.js", { scope: "/" });
>> 
>> After this, shouldn't we wait for registration.installing to before activated before loading the iframe? I thought this was what the WPT tests were doing.
> 
> Right. This test will probably become redundant against wpt soon so that it can be removed. This might be done with other tests as well which are not spec compliant.
> 
> (In reply to Chris Dumez from comment #43)

Can we please fix the test then? It seems trivial and I don't see why we wouldn't make the test correct.
Comment 48 youenn fablet 2017-11-13 10:09:19 PST
Created attachment 326768 [details]
Patch
Comment 49 youenn fablet 2017-11-13 16:41:55 PST
Created attachment 326823 [details]
Patch
Comment 50 youenn fablet 2017-11-13 18:48:22 PST
Created attachment 326838 [details]
Patch
Comment 51 Build Bot 2017-11-13 20:13:23 PST
Comment on attachment 326838 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-event-respond-with-response-body-with-invalid-chunk.https.html
Comment 52 Build Bot 2017-11-13 20:13:25 PST
Created attachment 326843 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 53 youenn fablet 2017-11-13 20:32:52 PST
Created attachment 326846 [details]
Patch
Comment 54 Chris Dumez 2017-11-13 22:23:53 PST
Comment on attachment 326846 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:876
> +            m_frame->document()->setActiveServiceWorker(ServiceWorker::create(*m_frame->document(), *m_serviceWorkerRegistrationData->activeServiceWorkerIdentifier, m_serviceWorkerRegistrationData->scriptURL, ServiceWorker::State::Activated));

Needs rebaselining after Bug 179649.
Comment 55 Chris Dumez 2017-11-14 09:14:49 PST
Comment on attachment 326846 [details]
Patch

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

>> Source/WebCore/loader/DocumentLoader.cpp:876
>> +            m_frame->document()->setActiveServiceWorker(ServiceWorker::create(*m_frame->document(), *m_serviceWorkerRegistrationData->activeServiceWorkerIdentifier, m_serviceWorkerRegistrationData->scriptURL, ServiceWorker::State::Activated));
> 
> Needs rebaselining after Bug 179649.

As previously commented, ServiceWorker::create() no longer exists as of Bug 179649 and this code needs to be updated accordingly. This may cause Web-facing behavior change too because we won't forcefully construct a new ServiceWorker every time.

Longer term question, do you foresee any use for this ServiceWorker object on ScriptExecutionContext or would an identifier be enough. Currently, it seems an identifier would be enough no?

> LayoutTests/ChangeLog:8
> +        * TestExpectations: Skipping some timing out tests.

Please provide more details. You both skipped tests and marked a few as flaky. Why are those tests timing out? Why are those tests flaky? Why is it OK?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/client-navigate.https-expected.txt:3
> +Harness Error (TIMEOUT), message = null

Why is this timing out now?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-response-taint.https-expected.txt:25
> +FAIL fetching url:"https://localhost:9443/service-workers/service-worker/resources/fetch-access-control.py?" mode:"same-origin" credentials:"same-origin" should succeed. assert_equals: expected "username2s" but got "undefined"

Why is this regressing?

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/performance-timeline.https-expected.txt:4
> +FAIL empty service worker fetch event included in performance timings assert_greater_than: Slow service worker request should measure increased delay. expected a number greater than 1158.2 but got 148.0999999999999

Should not be rebased just noise.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-updateviacache.https-expected.txt:2
> +FAIL register-via-api-updateViaCache-undefined assert_not_equals: Main script should have updated got disallowed value 1510557621.41599

Should not be rebased, just noise.
Comment 56 youenn fablet 2017-11-14 10:22:29 PST
Thanks for the review.

> >> Source/WebCore/loader/DocumentLoader.cpp:876
> >> +            m_frame->document()->setActiveServiceWorker(ServiceWorker::create(*m_frame->document(), *m_serviceWorkerRegistrationData->activeServiceWorkerIdentifier, m_serviceWorkerRegistrationData->scriptURL, ServiceWorker::State::Activated));
> > 
> > Needs rebaselining after Bug 179649.
> 
> As previously commented, ServiceWorker::create() no longer exists as of Bug
> 179649 and this code needs to be updated accordingly. This may cause
> Web-facing behavior change too because we won't forcefully construct a new
> ServiceWorker every time.

Sure, will do.
It should be straightforward.
 
> Longer term question, do you foresee any use for this ServiceWorker object
> on ScriptExecutionContext or would an identifier be enough. Currently, it
> seems an identifier would be enough no?

Good question.
At the moment, and having worker fetch interception in mind, an identifier seems like the best option.

According my reading of the spec, a sw client is tied to a registration, not to a worker.
If we go with the identifier option, we will need to be sure to keep it in sync with whatever happens to the registration. It is unclear whether this will be fine or if we should stick with the spec model.
 
> > LayoutTests/ChangeLog:8
> > +        * TestExpectations: Skipping some timing out tests.
> 
> Please provide more details. You both skipped tests and marked a few as
> flaky. Why are those tests timing out? Why are those tests flaky? Why is it
> OK?

I haven't looked at all the timing outs.
Some time outs are due to the fact that we are now going deeper into the tests but we do not implement some APIs like client/clients.

Some failures are due to our opaque response handling which is not yet correct.
I plan to fix opaque responses as a follow-up as per change log.

Some flakiness are due to assertion failure message, existing flakiness as per dashboard that I or EWS experienced locally. I'll check whether these flakinesses are gone or not with recent progress we made.
Comment 57 Alex Christensen 2017-11-14 11:40:49 PST
Comment on attachment 326846 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:313
> +    void doLoadingMainResource(ResourceRequest&&);

I think this should be named loadMainResource or startLoadingMainResource.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:-331
> -        ASSERT(response.type() == ResourceResponse::Type::Opaqueredirect);

We should rename Opaqueredirect to OpaqueRedirect.
Comment 58 youenn fablet 2017-11-14 11:46:14 PST
Thanks for the review.

(In reply to Alex Christensen from comment #57)
> Comment on attachment 326846 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326846&action=review
> 
> > Source/WebCore/loader/DocumentLoader.h:313
> > +    void doLoadingMainResource(ResourceRequest&&);
> 
> I think this should be named loadMainResource or startLoadingMainResource.

Will change to loadMainResource.

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:-331
> > -        ASSERT(response.type() == ResourceResponse::Type::Opaqueredirect);
> 
> We should rename Opaqueredirect to OpaqueRedirect.

Opaqueredirect is used in the binding generator and we do not have a way right now in the binding generator to set the proper casing.
Comment 59 youenn fablet 2017-11-14 13:12:28 PST
Created attachment 326914 [details]
Patch for landing
Comment 60 youenn fablet 2017-11-14 14:40:57 PST
Created attachment 326926 [details]
Patch for landing
Comment 61 WebKit Commit Bot 2017-11-14 14:43:05 PST
Comment on attachment 326926 [details]
Patch for landing

Rejecting attachment 326926 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 326926, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/5233865
Comment 62 youenn fablet 2017-11-14 14:44:37 PST
Created attachment 326927 [details]
Patch for landing
Comment 63 WebKit Commit Bot 2017-11-14 15:25:33 PST
Comment on attachment 326927 [details]
Patch for landing

Clearing flags on attachment: 326927

Committed r224852: <https://trac.webkit.org/changeset/224852>
Comment 64 WebKit Commit Bot 2017-11-14 15:25:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Radar WebKit Bug Importer 2017-11-15 09:42:24 PST
<rdar://problem/35562275>
Comment 66 Ryan Haddad 2017-11-15 10:20:45 PST
This change seems to have caused API test WebKit.WebsiteDataStoreCustomPaths to fail:

/Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WebsiteDataStoreCustomPaths.mm:220
Value of: [WKWebsiteDataStore _defaultDataStoreExists]
  Actual: false
Expected: true

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/5741
Comment 67 youenn fablet 2017-11-15 10:23:24 PST
We should probably revert https://bugs.webkit.org/show_bug.cgi?id=178596