Bug 179404 – Implement ServiceWorker handle fetch for navigation loads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement ServiceWorker handle fetch for navigation loads

[youenn fablet]

Created attachment 326276 [details]
wip

[youenn fablet]

Will probably split it into smaller patches.

[youenn fablet]

Created attachment 326358 [details]
wip

[Build Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 326493 [details]
Patch

[youenn fablet]

Created attachment 326496 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 326528 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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

[Chris Dumez]

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.

[Chris Dumez]

Comment on attachment 326528 [details]
Patch

r- because it does not look we have test coverage.

[youenn fablet]

Created attachment 326556 [details]
Patch

[youenn fablet]

(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.

[Chris Dumez]

(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?

[Chris Dumez]

(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.

[Chris Dumez]

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?

[youenn fablet]

Created attachment 326570 [details]
Patch

[youenn fablet]

> 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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[youenn fablet]

Created attachment 326578 [details]
Patch

[youenn fablet]

Created attachment 326579 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[youenn fablet]

(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.

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[youenn fablet]

Created attachment 326768 [details]
Patch

[youenn fablet]

Created attachment 326823 [details]
Patch

[youenn fablet]

Created attachment 326838 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 326846 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[youenn fablet]

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.

[Alex Christensen]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 326914 [details]
Patch for landing

[youenn fablet]

Created attachment 326926 [details]
Patch for landing

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 326927 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 326927 [details]
Patch for landing

Clearing flags on attachment: 326927

Committed r224852: <https://trac.webkit.org/changeset/224852>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35562275>

[Ryan Haddad]

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

[youenn fablet]

We should probably revert https://bugs.webkit.org/show_bug.cgi?id=178596