Let's implement Handle Fetch up to the creation of the fetch event
Created attachment 324142 [details] Patch
Created attachment 324146 [details] Rebasing
Created attachment 324150 [details] GTK fix try
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.
Created attachment 324154 [details] GTK fix try
Created attachment 324169 [details] Patch
(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.
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?
Created attachment 324174 [details] Patch
> > 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.
(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()
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
(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.
> 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.
Created attachment 324178 [details] Patch
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?
(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.
Created attachment 324192 [details] Patch for landing
Comment on attachment 324192 [details] Patch for landing Clearing flags on attachment: 324192 Committed r223650: <https://trac.webkit.org/changeset/223650>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35066424>
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.
(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>.
Created attachment 324278 [details] Patch
Comment on attachment 324278 [details] Patch Clearing flags on attachment: 324278 Committed r223718: <https://trac.webkit.org/changeset/223718>
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?
(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.