- Revision
- 288949
- Author
- [email protected]
- Date
- 2022-02-02 02:15:07 -0800 (Wed, 02 Feb 2022)
Log 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:
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (288948 => 288949)
--- trunk/LayoutTests/ChangeLog 2022-02-02 10:07:20 UTC (rev 288948)
+++ trunk/LayoutTests/ChangeLog 2022-02-02 10:15:07 UTC (rev 288949)
@@ -1,3 +1,15 @@
+2022-02-02 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
[LFC][IFC] Using Fontcascade::spaceWidth to subtract the trailing space width may result in incorrect layout
Added: trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt (0 => 288949)
--- trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt (rev 0)
+++ trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https-expected.txt 2022-02-02 10:15:07 UTC (rev 288949)
@@ -0,0 +1,5 @@
+
+
+PASS Setup activating worker
+PASS Service worker load uses preload through calling fetch on the fetch event request
+
Added: trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https.html (0 => 288949)
--- trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https.html (rev 0)
+++ trunk/LayoutTests/http/wpt/service-workers/fetch-service-worker-preload-cache.https.html 2022-02-02 10:15:07 UTC (rev 288949)
@@ -0,0 +1,70 @@
+<!doctype html>
+<html>
+<head>
+<script src=""
+<script src=""
+<script src=""
+<script src=""
+</head>
+<body>
+<script>
+var activeWorker;
+var uuid = token();
+var url = "" + uuid;
+var frame;
+const channel = new MessageChannel();
+
+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]);
+
+ return waitUntilActivating();
+}, "Setup activating worker");
+
+promise_test(async (test) => {
+ fetch(url + "&value=use-preload", { method: 'POST' });
+
+ // Put in the cache.
+ await fetch(url);
+
+ // Load iframe, with activating worker, so only preload will start.
+ const promise = withIframe(url);
+
+ triggerActivation();
+
+ const frame = await promise;
+ assert_equals(frame.contentWindow.value, "use-preload");
+
+ // 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 load uses preload through calling fetch on the fetch event request");
+</script>
+</body>
+</html>
Modified: trunk/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py (288948 => 288949)
--- trunk/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py 2022-02-02 10:07:20 UTC (rev 288948)
+++ trunk/LayoutTests/http/wpt/service-workers/resources/fetch-service-worker-preload-script.py 2022-02-02 10:15:07 UTC (rev 288949)
@@ -30,8 +30,12 @@
value = request.server.stash.take(testId)
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:
response.headers.set(b"Content-Type", b"text/vcard")
return value.decode()
Modified: trunk/Source/WebKit/ChangeLog (288948 => 288949)
--- trunk/Source/WebKit/ChangeLog 2022-02-02 10:07:20 UTC (rev 288948)
+++ trunk/Source/WebKit/ChangeLog 2022-02-02 10:15:07 UTC (rev 288949)
@@ -1,3 +1,24 @@
+2022-02-02 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
Rename FontCascade::fontMetrics() and RenderStyle::fontMetrics() to fontMetricsOfPrimaryFont()
Modified: trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp (288948 => 288949)
--- trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp 2022-02-02 10:07:20 UTC (rev 288948)
+++ trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp 2022-02-02 10:15:07 UTC (rev 288949)
@@ -363,7 +363,9 @@
{
SWFETCH_RELEASE_LOG("loadResponseFromPreloader");
- ASSERT(!m_isLoadingFromPreloader);
+ if (m_isLoadingFromPreloader)
+ return;
+
m_isLoadingFromPreloader = true;
m_preloader->waitForResponse([weakThis = WeakPtr { *this }, this] {
@@ -392,6 +394,12 @@
SWFETCH_RELEASE_LOG("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));
+ return;
+ }
+
m_preloader->waitForBody([weakThis = WeakPtr { *this }, this](auto&& chunk, int length) {
if (!weakThis)
return;
Modified: trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp (288948 => 288949)
--- trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp 2022-02-02 10:07:20 UTC (rev 288948)
+++ trunk/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp 2022-02-02 10:15:07 UTC (rev 288949)
@@ -231,20 +231,14 @@
void ServiceWorkerNavigationPreloader::waitForBody(BodyCallback&& callback)
{
- if (!m_error.isNull()) {
+ if (!m_error.isNull() || !m_responseCompletionHandler) {
callback({ }, 0);
return;
}
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);
}
bool ServiceWorkerNavigationPreloader::convertToDownload(DownloadManager& manager, DownloadID downloadID, const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response)
