Changeset 288949 in webkit

Timestamp:

Feb 2, 2022 2:15:07 AM (6 months ago)

Author:

youenn@apple.com

Message:

ServiceWorkerNavigationPreloader should only be used once
​https://bugs.webkit.org/show_bug.cgi?id=235882
<rdar://88226432>

Reviewed by Chris Dumez.

Source/WebKit:

In case service worker preload is being used and related service worker context crashes (or service worker context sends bad messages),
We can end up in a bad state where we will ask the preload twice for the same response (once for good, and the next one as we go to didNotHandle case).
To prevent this, we add checks in loadResponseFromPreloader and loadBodyFromPreloader.
As part of this investigation, I found out that ServiceWorkerNavigationPreloader is not correctly handling the case of preload responses coming from cache.
In particular, no body will be given since we return early in waitForBody in case the preload network load is null.
Prevent this by making sure waitForBody calls the response completion handler if available, even if the preload network load is null.
And update the response body callback before executing the response completion handler to make sure data received synchronously from the preload is given to the service worker fetch task.

Test: http/wpt/service-workers/fetch-service-worker-preload-cache.https.html

• NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
• NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:

LayoutTests:

• http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt: Added.
• http/wpt/service-workers/fetch-service-worker-preload-cache.https.html: Added.
• http/wpt/service-workers/resources/fetch-service-worker-preload-script.py:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt (added)
• LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https.html (added)
• LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp (modified) (2 diffs)
• Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-02-02  Youenn Fablet  <youenn@apple.com>
+
+        ServiceWorkerNavigationPreloader should only be used once
+        https://bugs.webkit.org/show_bug.cgi?id=235882
+        <rdar://88226432>
+
+        Reviewed by Chris Dumez.
+
+        * http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt: Added.
+        * http/wpt/service-workers/fetch-service-worker-preload-cache.https.html: Added.
+        * http/wpt/service-workers/resources/fetch-service-worker-preload-script.py:
+
 2022-02-01  Alan Bujtas  <zalan@apple.com>
 

trunk/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py

@@ -31 +31 @@
     if not value:
         value = b"nothing"
-    response.headers.set(b"Cache-Control", b"no-cache")
+
+    if b"cache" in request.GET:
+        response.headers.set(b"Cache-Control", b"max-age=31536000")
+    else:
+        response.headers.set(b"Cache-Control", b"no-cache")
 
     if b"download" in request.GET:

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2022-02-02  Youenn Fablet  <youenn@apple.com>
+
+        ServiceWorkerNavigationPreloader should only be used once
+        https://bugs.webkit.org/show_bug.cgi?id=235882
+        <rdar://88226432>
+
+        Reviewed by Chris Dumez.
+
+        In case service worker preload is being used and related service worker context crashes (or service worker context sends bad messages),
+        We can end up in a bad state where we will ask the preload twice for the same response (once for good, and the next one as we go to didNotHandle case).
+        To prevent this, we add checks in loadResponseFromPreloader and loadBodyFromPreloader.
+        As part of this investigation, I found out that ServiceWorkerNavigationPreloader is not correctly handling the case of preload responses coming from cache.
+        In particular, no body will be given since we return early in waitForBody in case the preload network load is null.
+        Prevent this by making sure waitForBody calls the response completion handler if available, even if the preload network load is null.
+        And update the response body callback before executing the response completion handler to make sure data received synchronously from the preload is given to the service worker fetch task.
+
+        Test: http/wpt/service-workers/fetch-service-worker-preload-cache.https.html
+
+        * NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
+        * NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:
+
 2022-02-01  Myles C. Maxfield  <mmaxfield@apple.com>
 

trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp

@@ -364 +364 @@
     SWFETCH_RELEASE_LOG("loadResponseFromPreloader");
 
-    ASSERT(!m_isLoadingFromPreloader);
+    if (m_isLoadingFromPreloader)
+        return;
+
     m_isLoadingFromPreloader = true;
 
@@ -393 +395 @@
 
     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));
+        return;
+    }
+
     m_preloader->waitForBody([weakThis = WeakPtr { *this }, this](auto&& chunk, int length) {
         if (!weakThis)

trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp

@@ -232 +232 @@
 void ServiceWorkerNavigationPreloader::waitForBody(BodyCallback&& callback)
 {
-    if (!m_error.isNull()) {
+    if (!m_error.isNull() || !m_responseCompletionHandler) {
         callback({ }, 0);
         return;
@@ -238 +238 @@
 
     ASSERT(!m_response.isNull());
-    ASSERT(m_responseCompletionHandler || !m_networkLoad);
-    if (!m_networkLoad) {
-        callback({ }, 0);
-        return;
-    }
-    if (m_responseCompletionHandler)
-        m_responseCompletionHandler(PolicyAction::Use);
     m_bodyCallback = WTFMove(callback);
+    m_responseCompletionHandler(PolicyAction::Use);
 }