Bug 178475 – Add preliminary support for ServiceWorker Handle Fetch

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Let's implement Handle Fetch up to the creation of the fetch event

[youenn fablet]

Created attachment 324142 [details]
Patch

[youenn fablet]

Created attachment 324146 [details]
Rebasing

[youenn fablet]

Created attachment 324150 [details]
GTK fix try

[Chris Dumez]

Comment on attachment 324146 [details]
Rebasing

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

> Source/WebCore/ChangeLog:11
> +        A script context is now storing its selected service worker identifer. This should be fully implemented later on.

typo: identifer

> Source/WebKit/ChangeLog:19
> +        Future work should try to reduce the cases where the IPC danse is done unnecessarily.

typo: s/danse/dance/g

> Source/WebCore/loader/ResourceLoaderOptions.h:95
> +enum class ServiceWorkersMode {

Is ServiceWorkersMode better than ServiceWorkerMode?

> Source/WebCore/workers/service/ServiceWorkerProvider.h:28
> +#include <wtf/Function.h>

Why is this needed?

> Source/WebCore/workers/service/ServiceWorkerProvider.h:38
> +class CachedResource;

Why are those needed?

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.cpp:102
> +void WebSWServerConnection::didReceiveFetchResponse(uint64_t fetchIdentifier, const WebCore::ResourceResponse& response)

Unnecessary WebCore::

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:57
> +    void failedFetch(uint64_t fetchIdentifier);

naming is not very consistent with other fetch-related methods.

> Source/WebKit/StorageProcess/ServiceWorker/WebSWServerConnection.h:70
> +

One too many blank lines?

> Source/WebKit/StorageProcess/StorageProcess.messages.in:44
> +    FailedFetch(uint64_t serverConnectionIdentifier, uint64_t fetchIdentifier)

Naming is not consistent with other fetch messages.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:222
> +        WebProcess::singleton().webLoaderStrategy().scheduleLoadFromNetworkProcess(resourceLoader, request, WTFMove(trackingParameters), shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime);

This is confusing, it makes it look like we ask the service worker to handle to load, and then when it is not, we ask the networkProcess. We have to clarify this somehow. Lambda passed to methods are usually completion handlers.
Maybe this could be a completion handler with an error parameter and we would only ask the network process if the error is set?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:27
> +#include "ServiceWorkerClientFetch.h"

I think we usually put this include inside the #if ENABLE().

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:37
> +static bool shouldRedirectAsGET(const ResourceRequest& oldRequest, const WebCore::ResourceResponse& response, URL& newURL, bool crossOrigin)

Unnecessary WebCore::

I prefer originalRequest than oldRequest.

The boolean should have a prefix, like isCrossOrigin.

Seems like this method is generic and reusable. We may want to move it to the right header in WebCore. Also, isn't there already logic for this in WebCore somewhere?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:62
> +static inline ResourceRequest computeNewRequest(const ResourceRequest& oldRequest, const WebCore::ResourceResponse& response)

originalRequest, necessary WebCore::

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:68
> +    bool crossOrigin = !protocolHostAndPortAreEqual(oldRequest.url(), newURL);

isCrossOrigin

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:71
> +        if (shouldRedirectAsGET(oldRequest, response, newURL, crossOrigin)) {

Should probably be merged with a && with the previous condition.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:78
> +    // FIXME: Properly handle Referer

Missing period at the end.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:48
> +    ServiceWorkerClientFetch(Ref<WebCore::ResourceLoader>&& loader, uint64_t identifier, Ref<IPC::Connection>&& connection, WTF::Function<void(WebCore::ResourceLoader&, const WebCore::ResourceRequest&)>&& fallback)

We should probably move this to the cpp.

> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:63
> +    // Hard coding some fetches for testing purpose until we implement the creation of fetch event.

FIXME?

> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.h:31
> +#include <WebCore/ExceptionOr.h>

Seems unused?

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:82
> +//    ASSERT(loader->options().serviceWorkerIdentifier);

Why is this commented?

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:34
> +#include <WebCore/ResourceLoader.h>

Can this be a forward declaration?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:61
> +static inline bool shouldHandleFetch(const WebSWClientConnection& connection, CachedResource& resource, const ResourceLoaderOptions& options)

Feels like this should maybe be a method on WebSWClientConnection.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h:50
> +    HashMap<uint64_t, Ref<ServiceWorkerClientFetch>> m_ongoingFetches;

I don't think we should use "Fetches" since this is a verb. Maybe "FetchRequests"? Also, doesn't the spec use inflight instead of ongoing?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.h:51
> +    uint64_t m_nextOngoingFetchIdentifier { 0 };

Since we always need to increment it before using it, maybe m_lastFetchRequestIdentifier or m_previousFetchRequestIdentifier or m_currentFetchRequestIdentifier would be better.

> Source/WebKit/WebProcess/WebProcess.cpp:1653
> +    if (!m_serviceWorkerManager)

Can this happen? It seems like this method used to always start a context and now it may fail. Why is it OK?

> Source/WebKit/WebProcess/WebProcess.messages.in:120
> +StartFetchInServiceWorker(uint64_t serverConnectionIdentifier, uint64_t fetchIdentifier, uint64_t serviceWorkerIdentifier, WebCore::ResourceRequest request, struct WebCore::FetchOptions options)

Indentation problem.

[youenn fablet]

Created attachment 324154 [details]
GTK fix try

[youenn fablet]

Created attachment 324169 [details]
Patch

[youenn fablet]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 324146 [details]
> Rebasing

Thanks for the review.
I updated according your comments.
I removed redirection support for now.
I moved to a completion handler taking an enum as well.

[Chris Dumez]

Comment on attachment 324169 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.h:306
> +    uint64_t m_serviceWorkerIdentifier { 0 };

#if ENABLE(SERVICE_WORKER)

> Source/WebCore/workers/service/server/SWServer.h:43
> +class ResourceRequest;

Does not look needed.

> Source/WebCore/workers/service/server/SWServer.h:47
> +struct FetchOptions;

Does not look needed.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:220
> +    WebServiceWorkerProvider::singleton().handleFetch(resourceLoader, resource, webPage ? webPage->sessionID() : PAL::SessionID::defaultSessionID(), [trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime = maximumBufferingTime(resource), resourceLoader = makeRef(resourceLoader)] (ServiceWorkerClientFetchResult result) mutable {

How about we expose a 'canHandleFetch() method on WebServiceWorkerProvider? Then it's like:
if (provider.canHandleFetch()) {
    provider.handleFetch()
    return;
}
scheduleLoadFromNetworkProcess();

Then I do not think we need the None/Unhandled enum value. We may not need an enum at all.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:27
> +#include "ServiceWorkerClientFetch.h"

This is usually in the #if ENABLE() I believe.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:53
> +    ASSERT(response.httpStatusCode() <= 300 || response.httpStatusCode() >= 400 || response.httpStatusCode() == 304 || response.httpStatusCode() == 305 || response.httpStatusCode() == 306);

I do not think this should be an assertion since it can happen.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:39
> +enum class ServiceWorkerClientFetchResult { Done, Cancelled, None };

I think this should be moved to ServiceWorkerClientFetch class and simply called "Result". Also, I do not like the "None" result as I think it is unclear.

How about "Succeeded / Cancelled / Unhandled"?

> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:45
> +    // FIXME: Provide a sensical session ID

Missing period at the end.

> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:46
> +

No need for this extra blank line since the comment also applies to the next line.

> Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp:37
> +#if ENABLE(SERVICE_WORKER)

Is this really needed?

[youenn fablet]

Created attachment 324174 [details]
Patch

[youenn fablet]

 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:220
> > +    WebServiceWorkerProvider::singleton().handleFetch(resourceLoader, resource, webPage ? webPage->sessionID() : PAL::SessionID::defaultSessionID(), [trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime = maximumBufferingTime(resource), resourceLoader = makeRef(resourceLoader)] (ServiceWorkerClientFetchResult result) mutable {
> 
> How about we expose a 'canHandleFetch() method on WebServiceWorkerProvider?
> Then it's like:
> if (provider.canHandleFetch()) {
>     provider.handleFetch()
>     return;
> }
> scheduleLoadFromNetworkProcess();
> 
> Then I do not think we need the None/Unhandled enum value. We may not need
> an enum at all.

canHandleFetch might be asynchronous since we might need to go up to the service worker or storage process to discover we took the wrong path.

I'll take care of the other comments.

[Chris Dumez]

(In reply to youenn fablet from comment #10)
>  
> > > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:220
> > > +    WebServiceWorkerProvider::singleton().handleFetch(resourceLoader, resource, webPage ? webPage->sessionID() : PAL::SessionID::defaultSessionID(), [trackingParameters, shouldClearReferrerOnHTTPSToHTTPRedirect, maximumBufferingTime = maximumBufferingTime(resource), resourceLoader = makeRef(resourceLoader)] (ServiceWorkerClientFetchResult result) mutable {
> > 
> > How about we expose a 'canHandleFetch() method on WebServiceWorkerProvider?
> > Then it's like:
> > if (provider.canHandleFetch()) {
> >     provider.handleFetch()
> >     return;
> > }
> > scheduleLoadFromNetworkProcess();
> > 
> > Then I do not think we need the None/Unhandled enum value. We may not need
> > an enum at all.
> 
> canHandleFetch might be asynchronous since we might need to go up to the
> service worker or storage process to discover we took the wrong path.
> 
> I'll take care of the other comments.

Then can we rename handleFetch() to something that indicates it may not handle it? e.g. tryHandleFetch() or handleFetchIfNecessary()

[youenn fablet]

Comment on attachment 324169 [details]
Patch

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

>> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:27
>> +#include "ServiceWorkerClientFetch.h"
> 
> This is usually in the #if ENABLE() I believe.

Style is usually to put both together.
Since the header is wrapped in compile flag, this does not change any behavior.

>> Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp:37
>> +#if ENABLE(SERVICE_WORKER)
> 
> Is this really needed?

This is needed for build systems that do not generate this file yet.
I guess we could add generation in CMakeLists.txt

[Chris Dumez]

(In reply to youenn fablet from comment #12)
> Comment on attachment 324169 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324169&action=review
> 
> >> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:27
> >> +#include "ServiceWorkerClientFetch.h"
> > 
> > This is usually in the #if ENABLE() I believe.
> 
> Style is usually to put both together.
> Since the header is wrapped in compile flag, this does not change any
> behavior.
> 
> >> Source/WebKit/WebProcess/Storage/WebToStorageProcessConnection.cpp:37
> >> +#if ENABLE(SERVICE_WORKER)
> > 
> > Is this really needed?
> 
> This is needed for build systems that do not generate this file yet.
> I guess we could add generation in CMakeLists.txt

It looks like people do try to keep CMakeLists up to date when it comes to Service Workers.

[youenn fablet]

> Then can we rename handleFetch() to something that indicates it may not
> handle it? e.g. tryHandleFetch() or handleFetchIfNecessary()

I prefer keeping handleFetch.
It refers to the https://w3c.github.io/ServiceWorker/#handle-fetch algorithm which can return null.
As per fetch spec, if returning null, an http fetch will be triggered.

[youenn fablet]

Created attachment 324178 [details]
Patch

[Chris Dumez]

Comment on attachment 324178 [details]
Patch

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

r=me with comments.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:80
> +    void scheduleLoadFromNetworkProcess(WebCore::ResourceLoader&, const WebCore::ResourceRequest&, const WebResourceLoader::TrackingParameters&, bool shouldClearReferrerOnHTTPSToHTTPRedirect, Seconds);

I think we should provide the name of the last parameter as it is non-obvious.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:53
> +    if (!(response.httpStatusCode() <= 300 || response.httpStatusCode() >= 400 || response.httpStatusCode() == 304 || response.httpStatusCode() == 305 || response.httpStatusCode() == 306)) {

Can we add a isRedirect() method to ResourceResponseBase so this is reusable? Or a isRedirectResponse(response) function is some Webcore header?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:67
> +    if (auto callback = WTFMove(m_callback))

Why are we calling the completion callback with success here and not in didFinish()?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:64
> +    RefPtr<WebCore::ResourceLoader> m_loader;

It looks like m_loader cannot currently be null. If so, it should probably be a Ref<> and we should get rid of all the null checks.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:90
> +    auto fetch = connection.startFetch(*this, loader, loader.identifier(), WTFMove(callback));

This is a risky pattern. It is common for networking code to fail synchronously, in which case, you'd try to remove the object from m_ongoingFetchTasks (and fail silently) and then add it to m_ongoingFetchTasks even though it is already finished.

> Source/WebKit/WebProcess/WebProcess.h:443
> +    std::optional<ServiceWorkerContextManager> m_serviceWorkerManager;

Why optional and not unique_ptr?

[youenn fablet]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 324178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324178&action=review
> 
> r=me with comments.
> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:80
> > +    void scheduleLoadFromNetworkProcess(WebCore::ResourceLoader&, const WebCore::ResourceRequest&, const WebResourceLoader::TrackingParameters&, bool shouldClearReferrerOnHTTPSToHTTPRedirect, Seconds);
> 
> I think we should provide the name of the last parameter as it is
> non-obvious.

OK
 
> > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:53
> > +    if (!(response.httpStatusCode() <= 300 || response.httpStatusCode() >= 400 || response.httpStatusCode() == 304 || response.httpStatusCode() == 305 || response.httpStatusCode() == 306)) {
> 
> Can we add a isRedirect() method to ResourceResponseBase so this is
> reusable? Or a isRedirectResponse(response) function is some Webcore header?

Let's add it when implementing redirections, since this check might be made differently.

> > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:67
> > +    if (auto callback = WTFMove(m_callback))
> 
> Why are we calling the completion callback with success here and not in
> didFinish()?

We know that we will follow the SW path since we have a response that will go to the ResourceLoader.
So SW handle fetch is returning a non null response, hence why calling the completion handler.
SW may later on return an error if body has an issue.

> > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:64
> > +    RefPtr<WebCore::ResourceLoader> m_loader;
> 
> It looks like m_loader cannot currently be null. If so, it should probably
> be a Ref<> and we should get rid of all the null checks.

OK, I put it as a RefPtr so that we can clean the loader in case of cancellation.
But we are cleaning the whole fetch object so it can probably be a Ref.

> > Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:90
> > +    auto fetch = connection.startFetch(*this, loader, loader.identifier(), WTFMove(callback));
> 
> This is a risky pattern. It is common for networking code to fail
> synchronously, in which case, you'd try to remove the object from
> m_ongoingFetchTasks (and fail silently) and then add it to
> m_ongoingFetchTasks even though it is already finished.

I would guess in that case that fetch would be null, in which case it would be a RefPtr and we would not add it to the map.

> > Source/WebKit/WebProcess/WebProcess.h:443
> > +    std::optional<ServiceWorkerContextManager> m_serviceWorkerManager;
> 
> Why optional and not unique_ptr?

It removes the need for an additional memory allocation.

[youenn fablet]

Created attachment 324192 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 324192 [details]
Patch for landing

Clearing flags on attachment: 324192

Committed r223650: <https://trac.webkit.org/changeset/223650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35066424>

[Matt Lewis]

This revision cause multiple API Failures on all platforms. On Debug platforms it caused additional Assertion failures in API tests.

Debug build:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/403
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/403/steps/run-api-tests/logs/stdio

Release build:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/536
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/536/steps/run-api-tests/logs/stdio

Talked to Chris and this doe not seem like an easy fix.

[Chris Dumez]

(In reply to Matt Lewis from comment #22)
> This revision cause multiple API Failures on all platforms. On Debug
> platforms it caused additional Assertion failures in API tests.
> 
> Debug build:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/403
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/403/steps/run-api-
> tests/logs/stdio
> 
> Release build:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/536
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/536/steps/run-api-
> tests/logs/stdio
> 
> Talked to Chris and this doe not seem like an easy fix.

Reverted in <https://trac.webkit.org/changeset/223692>.

[youenn fablet]

Created attachment 324278 [details]
Patch

[Chris Dumez]

Comment on attachment 324278 [details]
Patch

Clearing flags on attachment: 324278

Committed r223718: <https://trac.webkit.org/changeset/223718>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

The new patch caused an api failure in WebKit.WebsiteDataStoreCustomPaths on all platforms.
I was able to reproduce the issue:

FAIL WebKit.WebsiteDataStoreCustomPaths

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

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/433/steps/run-api-tests/logs/stdio
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/433

Could this just need to have the expectations updated?

[youenn fablet]

(In reply to Matt Lewis from comment #27)
> The new patch caused an api failure in WebKit.WebsiteDataStoreCustomPaths on
> all platforms.
> I was able to reproduce the issue:
> 
> FAIL WebKit.WebsiteDataStoreCustomPaths
> 
> /Volumes/Data/slave/highsierra-debug/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/WebsiteDataStoreCustomPaths.mm:219
> Value of: [WKWebsiteDataStore _defaultDataStoreExists]
>   Actual: true
> Expected: false
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/433/steps/run-api-tests/
> logs/stdio
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/433
> 
> Could this just need to have the expectations updated?

Filed https://bugs.webkit.org/show_bug.cgi?id=178596.
I will update the expectation and reopen the bug for further investigation.