FetchEvent should not start its navigation preload response load if t…

…he preload was already used https://bugs.webkit.org/show_bug.cgi?id=245788 rdar://98144044 Reviewed by Alex Christensen. Creating the navigation preload promise was scheduling a load which is hitting the network if the preload is no longer in network process. This case happens if the navigation preload promise is actually used in respondWith. To circumvent this issue, we do not trigger the load if the preload is used in respondWith. Add logs to capture the fact that a load is not getting its expected preload. Drive-by fixes to actually use the current preload request and not the loader original request. This might change in case of redirections. Covered by newly added test which mirrors what we were testing for non-exposed navigation preloads. * LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https-expected.txt: Added. * LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https.html: Added. * LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-worker.js: (event.event.request.url.includes): * LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py: (main): * Source/WebCore/Modules/fetch/FetchResponse.cpp: (WebCore::FetchResponse::markAsUsedForPreload): (WebCore::FetchResponse::markAsDisturbed): Deleted. * Source/WebCore/Modules/fetch/FetchResponse.h: * Source/WebCore/workers/service/FetchEvent.cpp: (WebCore::FetchEvent::navigationPreloadIsReady): * Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp: (WebCore::ServiceWorkerFetch::processResponse): * Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp: (WebKit::ServiceWorkerFetchTask::fromNavigationPreloader): (WebKit::ServiceWorkerFetchTask::ServiceWorkerFetchTask): (WebKit::ServiceWorkerFetchTask::loadBodyFromPreloader): Canonical link: https://commits.webkit.org/254992@main
WebKit · Sep 29, 2022 · 757903c6fe5a2e68d46e63e48391f49df66a6a03 · 757903c
1 parent de85914
commit 757903c6fe5a2e68d46e63e48391f49df66a6a03
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 7 deletions.
diff --git a/...Tests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https-expected.txt b/...Tests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https-expected.txt
@@ -0,0 +1,5 @@
+
+
+PASS Setup activating worker
+PASS Service worker preloadResponse does not trigger an additional load
+
diff --git a/LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https.html b/LayoutTests/http/wpt/service-workers/fetch-service-worker-navigation-preload.https.html
@@ -0,0 +1,68 @@
+<!doctype html>
+<html>
+<head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/common/utils.js"></script>
+<script src="resources/routines.js"></script>
+</head>
+<body>
+<script>
+var activeWorker;
+var frame;
+const channel = new MessageChannel();
+const uuid = token();
+const url = "/WebKit/service-workers/resources/fetch-service-worker-preload-script.py?useNavigationPreloadPromise=true&token=" + uuid;
+
+function waitUntilActivating()
+{
+    return new Promise(resolve => {
+        channel.port2.onmessage = (event) => {
+            if (event.data === "activating")
+                resolve();
+        };
+    });
+}
+
+function triggerActivation()
+{
+    activeWorker.postMessage("activate");
+}
+
+promise_test(async (test) => {
+    if (window.testRunner) {
+        testRunner.setUseSeparateServiceWorkerProcess(true);
+        await fetch("").then(() => { }, () => { });
+    }
+
+    let registration = await navigator.serviceWorker.register("/WebKit/service-workers/fetch-service-worker-preload-worker.js", { scope : url });
+    if (!registration.installing) {
+        registration.unregister();
+        registration = await navigator.serviceWorker.register("/WebKit/service-workers/fetch-service-worker-preload-worker.js", { scope : url });
+    }
+
+    activeWorker = registration.installing;
+    activeWorker.postMessage({ port: channel.port1 }, [channel.port1]);
+
+    await waitUntilActivating();
+
+    if (registration.navigationPreload)
+        await registration.navigationPreload.enable();
+
+    triggerActivation();
+}, "Setup activating worker");
+
+promise_test(async (test) => {
+    await fetch(url + "&value=use-preload2", { method: 'POST' });
+
+
+    const frame = await withIframe(url);
+    assert_equals(frame.contentWindow.value, "use-preload2");
+
+    // We should have only one GET fetch to url: the service worker preload
+    const response = await fetch(url + "&count=True");
+    assert_equals(await response.text(), "1");
+}, "Service worker preloadResponse does not trigger an additional load");
+</script>
+</body>
+</html>
diff --git a/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-worker.js b/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-worker.js
@@ -25,5 +25,10 @@ onactivate = (event) => {
 self.addEventListener('fetch', (event) => {
     if (event.request.url.includes("no-fetch-event-handling"))
         return;
+
+    if (event.request.url.includes("useNavigationPreloadPromise") && event.preloadResponse) {
+        event.respondWith(event.preloadResponse);
+        return;
+    }
     event.respondWith(fetch(event.request));
 });
diff --git a/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py b/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py
@@ -12,6 +12,7 @@ def main(request, response):
         request.server.stash.take(testId)
         request.server.stash.put(testId, request.GET[b'value'])
         response.headers.set(b"Content-Type", b"text/ascii")
+        request.server.stash.put(countId, 0)
         return b"updated to " + request.GET[b'value']
 
     if request.GET.first(b"count", False):

diff --git a/Source/WebCore/Modules/fetch/FetchResponse.cpp b/Source/WebCore/Modules/fetch/FetchResponse.cpp
@@ -450,10 +450,11 @@ FetchResponse::ResponseData FetchResponse::consumeBody()
     return body().take();
 }
 
-void FetchResponse::markAsDisturbed()
+void FetchResponse::markAsUsedForPreload()
 {
     ASSERT(!m_isDisturbed);
     m_isDisturbed = true;
+    m_isUsedForPreload = true;
 }
 
 void FetchResponse::consumeBodyReceivedByChunk(ConsumeDataByChunkCallback&& callback)

diff --git a/Source/WebCore/Modules/fetch/FetchResponse.h b/Source/WebCore/Modules/fetch/FetchResponse.h
@@ -119,7 +119,8 @@ class FetchResponse final : public FetchBodyOwner {
 
     void setIsNavigationPreload(bool isNavigationPreload) { m_isNavigationPreload = isNavigationPreload; }
     bool isAvailableNavigationPreload() const { return m_isNavigationPreload && m_bodyLoader && !m_bodyLoader->hasLoader() && !hasReadableStreamBody(); }
-    void markAsDisturbed();
+    void markAsUsedForPreload();
+    bool isUsedForPreload() const { return m_isUsedForPreload; }
 
 private:
     FetchResponse(ScriptExecutionContext*, std::optional<FetchBody>&&, Ref<FetchHeaders>&&, ResourceResponse&&);
@@ -177,6 +178,7 @@ class FetchResponse final : public FetchBodyOwner {
     NetworkLoadMetrics m_networkLoadMetrics;
     bool m_hasInitializedInternalResponse { false };
     bool m_isNavigationPreload { false };
+    bool m_isUsedForPreload { false };
 };
 
 } // namespace WebCore
diff --git a/Source/WebCore/workers/service/FetchEvent.cpp b/Source/WebCore/workers/service/FetchEvent.cpp
@@ -192,7 +192,8 @@ void FetchEvent::navigationPreloadIsReady(ResourceResponse&& response)
 
     // We postpone the load to leave some time for the service worker to use the preload before loading it.
     context->postTask([fetchResponse = WTFMove(fetchResponse), request = WTFMove(request)](auto& context) {
-        fetchResponse->startLoader(context, request.get(), cachedResourceRequestInitiators().navigation);
+        if (!fetchResponse->isUsedForPreload())
+            fetchResponse->startLoader(context, request.get(), cachedResourceRequestInitiators().navigation);
     });
 }
 

diff --git a/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp b/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp
@@ -101,7 +101,7 @@ static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, s
 
     if (response->isAvailableNavigationPreload()) {
         client->usePreload();
-        response->markAsDisturbed();
+        response->markAsUsedForPreload();
         return;
     }
 

diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
@@ -59,8 +59,10 @@ std::unique_ptr<ServiceWorkerFetchTask> ServiceWorkerFetchTask::fromNavigationPr
         return nullptr;
 
     auto task = session ? session->navigationPreloaderTaskFromFetchIdentifier(*loader.parameters().navigationPreloadIdentifier) : nullptr;
-    if (!task || !task->m_preloader || task->m_isLoadingFromPreloader)
+    if (!task || !task->m_preloader || task->m_isLoadingFromPreloader) {
+        RELEASE_LOG_ERROR(ServiceWorker, "Unable to retrieve preloader, load will go to the network");
         return nullptr;
+    }
 
     auto preload = std::exchange(task->m_preloader, { });
     return makeUnique<ServiceWorkerFetchTask>(swServerConnection, loader, WTFMove(preload));
@@ -96,7 +98,8 @@ ServiceWorkerFetchTask::ServiceWorkerFetchTask(WebSWServerConnection& swServerCo
         m_timeoutTimer->startOneShot(loader.connectionToWebProcess().networkProcess().serviceWorkerFetchTimeout());
     }
 
-    bool shouldDoNavigationPreload = session && isNavigationRequest(loader.parameters().options.destination) && loader.originalRequest().httpMethod() == "GET"_s;
+    bool shouldDoNavigationPreload = session && isNavigationRequest(loader.parameters().options.destination) && m_currentRequest.httpMethod() == "GET"_s;
+
     if (shouldDoNavigationPreload && (!isWorkerReady || registration.navigationPreloadState().enabled)) {
         NetworkLoadParameters parameters = loader.parameters();
         parameters.request = m_currentRequest;
@@ -441,7 +444,7 @@ void ServiceWorkerFetchTask::loadBodyFromPreloader()
     ASSERT(m_isLoadingFromPreloader);
     if (!m_preloader) {
         SWFETCH_RELEASE_LOG_ERROR("loadBodyFromPreloader preloader is null");
-        didFail(ResourceError(errorDomainWebKitInternal, 0, m_loader.originalRequest().url(), "Request canceled from preloader"_s, ResourceError::Type::Cancellation));
+        didFail(ResourceError(errorDomainWebKitInternal, 0, m_currentRequest.url(), "Request canceled from preloader"_s, ResourceError::Type::Cancellation));
         return;
     }