Do not timeout a load intercepted by service worker that receives a response
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Oct 2019 19:52:04 +0000 (19:52 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Oct 2019 19:52:04 +0000 (19:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202787

Reviewed by Chris Dumez.

Source/WebKit:

Stop making ServiceWorkerFetchTask ref counted since it is not needed and
can potentially make ServiceWorkerFetchTask oulive its WebSWServerToContextConnection member.

Stop the ServiceWorkerFetchTask timeout timer whenever receiving a response so that the load will not timeout in that case.
This ensures that a load that is starting in a service worker will not be failing.
Instead the load will go to network process.

Removed m_didReachTerminalState which is not needed as WebSWServerToContextConnection unregisters the ServiceWorkerFetchTask
as an IPC listener for all terminating messages.

* NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
(WebKit::ServiceWorkerFetchTask::didReceiveRedirectResponse):
(WebKit::ServiceWorkerFetchTask::didReceiveResponse):
(WebKit::ServiceWorkerFetchTask::didReceiveData):
(WebKit::ServiceWorkerFetchTask::didReceiveFormData):
(WebKit::ServiceWorkerFetchTask::didFinish):
(WebKit::ServiceWorkerFetchTask::didFail):
(WebKit::ServiceWorkerFetchTask::didNotHandle):
(WebKit::ServiceWorkerFetchTask::timeoutTimerFired):
* NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:
* NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:
(WebKit::WebSWServerToContextConnection::startFetch):
(WebKit::WebSWServerToContextConnection::fetchTaskTimedOut):
Use a Vector instead of a HasSet for performance reasons.
Update according fetch map using unique_ptr instead of Ref<>.
* NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h:

LayoutTests:

* http/wpt/service-workers/fetch-timeout-worker.js: Added.
(async.doTest):
* http/wpt/service-workers/fetch-timeout.https-expected.txt: Added.
* http/wpt/service-workers/fetch-timeout.https.html: Added.
* http/wpt/service-workers/resources/lengthy-pass.py:
(main):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@250985 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/wpt/service-workers/fetch-timeout-worker.js [new file with mode: 0644]
LayoutTests/http/wpt/service-workers/fetch-timeout.https-expected.txt [new file with mode: 0644]
LayoutTests/http/wpt/service-workers/fetch-timeout.https.html [new file with mode: 0644]
LayoutTests/http/wpt/service-workers/resources/lengthy-pass.py
Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h
Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h

index 953287f..5812f13 100644 (file)
@@ -1,3 +1,17 @@
+2019-10-10  Youenn Fablet  <youenn@apple.com>
+
+        Do not timeout a load intercepted by service worker that receives a response
+        https://bugs.webkit.org/show_bug.cgi?id=202787
+
+        Reviewed by Chris Dumez.
+
+        * http/wpt/service-workers/fetch-timeout-worker.js: Added.
+        (async.doTest):
+        * http/wpt/service-workers/fetch-timeout.https-expected.txt: Added.
+        * http/wpt/service-workers/fetch-timeout.https.html: Added.
+        * http/wpt/service-workers/resources/lengthy-pass.py:
+        (main):
+
 2019-10-10  Myles C. Maxfield  <mmaxfield@apple.com>
 
         FontFaceSet's ready promise is not always resolved
diff --git a/LayoutTests/http/wpt/service-workers/fetch-timeout-worker.js b/LayoutTests/http/wpt/service-workers/fetch-timeout-worker.js
new file mode 100644 (file)
index 0000000..081cc8d
--- /dev/null
@@ -0,0 +1,6 @@
+async function doTest(event)
+{
+    event.respondWith(fetch(event.request));
+}
+
+self.addEventListener("fetch", doTest);
diff --git a/LayoutTests/http/wpt/service-workers/fetch-timeout.https-expected.txt b/LayoutTests/http/wpt/service-workers/fetch-timeout.https-expected.txt
new file mode 100644 (file)
index 0000000..8022b2d
--- /dev/null
@@ -0,0 +1,4 @@
+
+PASS Setup worker 
+PASS Make sure a load that makes progress does not time out 
+
diff --git a/LayoutTests/http/wpt/service-workers/fetch-timeout.https.html b/LayoutTests/http/wpt/service-workers/fetch-timeout.https.html
new file mode 100644 (file)
index 0000000..4663e4e
--- /dev/null
@@ -0,0 +1,39 @@
+<html>
+<head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/service-workers/service-worker/resources/test-helpers.sub.js"></script>
+</head>
+<body>
+<script>
+var scope = "resources";
+var activeWorker;
+
+promise_test(async (test) => {
+    var registration = await navigator.serviceWorker.register("fetch-timeout-worker.js", { scope : scope });
+    activeWorker = registration.active;
+    if (activeWorker)
+        return;
+    activeWorker = registration.installing;
+    return new Promise(resolve => {
+        activeWorker.addEventListener('statechange', () => {
+            if (activeWorker.state === "activated")
+                resolve();
+        });
+    });
+}, "Setup worker");
+
+promise_test(async (test) => {
+    const iframe = await with_iframe(scope);
+
+    if (window.testRunner)
+        testRunner.setServiceWorkerFetchTimeout(1);
+
+     const response = await iframe.contentWindow.fetch("/WebKit/service-workers/resources/lengthy-pass.py?delay=0.5");
+     const text = await response.text();
+     assert_equals(text, "document.body.innerHTML = 'PASS'");
+     iframe.remove(); 
+}, "Make sure a load that makes progress does not time out");
+</script>
+</body>
+</html>
index 04ef57a..14dd790 100644 (file)
@@ -2,6 +2,9 @@ import time
 
 def main(request, response):
     delay = 0.05
+    headers = []
+    if "delay" in request.GET:
+        delay = float(request.GET.first("delay"))
     response.headers.set("Content-type", "text/javascript")
     response.headers.append("Access-Control-Allow-Origin", "*")
     response.write_status_headers()
index 5e1a11b..a9aa697 100644 (file)
@@ -1,3 +1,37 @@
+2019-10-10  Youenn Fablet  <youenn@apple.com>
+
+        Do not timeout a load intercepted by service worker that receives a response
+        https://bugs.webkit.org/show_bug.cgi?id=202787
+
+        Reviewed by Chris Dumez.
+
+        Stop making ServiceWorkerFetchTask ref counted since it is not needed and
+        can potentially make ServiceWorkerFetchTask oulive its WebSWServerToContextConnection member.
+
+        Stop the ServiceWorkerFetchTask timeout timer whenever receiving a response so that the load will not timeout in that case.
+        This ensures that a load that is starting in a service worker will not be failing.
+        Instead the load will go to network process.
+
+        Removed m_didReachTerminalState which is not needed as WebSWServerToContextConnection unregisters the ServiceWorkerFetchTask
+        as an IPC listener for all terminating messages.
+
+        * NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
+        (WebKit::ServiceWorkerFetchTask::didReceiveRedirectResponse):
+        (WebKit::ServiceWorkerFetchTask::didReceiveResponse):
+        (WebKit::ServiceWorkerFetchTask::didReceiveData):
+        (WebKit::ServiceWorkerFetchTask::didReceiveFormData):
+        (WebKit::ServiceWorkerFetchTask::didFinish):
+        (WebKit::ServiceWorkerFetchTask::didFail):
+        (WebKit::ServiceWorkerFetchTask::didNotHandle):
+        (WebKit::ServiceWorkerFetchTask::timeoutTimerFired):
+        * NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h:
+        * NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:
+        (WebKit::WebSWServerToContextConnection::startFetch):
+        (WebKit::WebSWServerToContextConnection::fetchTaskTimedOut):
+        Use a Vector instead of a HasSet for performance reasons.
+        Update according fetch map using unique_ptr instead of Ref<>.
+        * NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h:
+
 2019-10-10  Rob Buis  <rbuis@igalia.com>
 
         SpeculativeLoad should use CompletionHandler
index 0dcb3b3..834b569 100644 (file)
@@ -59,6 +59,7 @@ ServiceWorkerFetchTask::ServiceWorkerFetchTask(PAL::SessionID sessionID, WebSWSe
 void ServiceWorkerFetchTask::didReceiveRedirectResponse(const ResourceResponse& response)
 {
     RELEASE_LOG_IF_ALLOWED("didReceiveRedirectResponse: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
+    m_timeoutTimer.stop();
     m_wasHandled = true;
     if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveRedirectResponse { response }, m_identifier.fetchIdentifier);
@@ -67,6 +68,7 @@ void ServiceWorkerFetchTask::didReceiveRedirectResponse(const ResourceResponse&
 void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response, bool needsContinueDidReceiveResponseMessage)
 {
     RELEASE_LOG_IF_ALLOWED("didReceiveResponse: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
+    m_timeoutTimer.stop();
     m_wasHandled = true;
     if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveResponse { response, needsContinueDidReceiveResponseMessage }, m_identifier.fetchIdentifier);
@@ -74,51 +76,47 @@ void ServiceWorkerFetchTask::didReceiveResponse(const ResourceResponse& response
 
 void ServiceWorkerFetchTask::didReceiveData(const IPC::DataReference& data, int64_t encodedDataLength)
 {
+    ASSERT(!m_timeoutTimer.isActive());
     if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveData { data, encodedDataLength }, m_identifier.fetchIdentifier);
 }
 
 void ServiceWorkerFetchTask::didReceiveFormData(const IPC::FormDataReference& formData)
 {
+    ASSERT(!m_timeoutTimer.isActive());
     if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidReceiveFormData { formData }, m_identifier.fetchIdentifier);
 }
 
 void ServiceWorkerFetchTask::didFinish()
 {
+    ASSERT(!m_timeoutTimer.isActive());
     RELEASE_LOG_IF_ALLOWED("didFinishFetch: fetchIdentifier: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
     m_timeoutTimer.stop();
-    if (!m_didReachTerminalState && m_connection)
+    if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidFinish { }, m_identifier.fetchIdentifier);
-    m_didReachTerminalState = true;
 }
 
 void ServiceWorkerFetchTask::didFail(const ResourceError& error)
 {
     RELEASE_LOG_ERROR_IF_ALLOWED("didFailFetch: fetchIdentifier: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
     m_timeoutTimer.stop();
-    if (!m_didReachTerminalState && m_connection)
+    if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidFail { error }, m_identifier.fetchIdentifier);
-    m_didReachTerminalState = true;
 }
 
 void ServiceWorkerFetchTask::didNotHandle()
 {
     RELEASE_LOG_IF_ALLOWED("didNotHandleFetch: fetchIdentifier: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
     m_timeoutTimer.stop();
-    if (!m_didReachTerminalState && m_connection)
+    if (m_connection)
         m_connection->send(Messages::ServiceWorkerClientFetch::DidNotHandle { }, m_identifier.fetchIdentifier);
-    m_didReachTerminalState = true;
 }
 
 void ServiceWorkerFetchTask::timeoutTimerFired()
 {
     RELEASE_LOG_IF_ALLOWED("timeoutTimerFired: fetchIdentifier: %s", m_identifier.fetchIdentifier.loggingString().utf8().data());
-    if (!m_wasHandled)
-        didNotHandle();
-    else
-        didFail({ errorDomainWebKitInternal, 0, { }, "Service Worker fetch timed out"_s });
-
+    didNotHandle();
     m_contextConnection.fetchTaskTimedOut(*this);
 }
 
index 6add0d0..d8b9d6f 100644 (file)
@@ -31,7 +31,6 @@
 #include <WebCore/ServiceWorkerTypes.h>
 #include <WebCore/Timer.h>
 #include <pal/SessionID.h>
-#include <wtf/RefCounted.h>
 
 namespace WebCore {
 class ResourceError;
@@ -50,12 +49,10 @@ namespace WebKit {
 class WebSWServerConnection;
 class WebSWServerToContextConnection;
 
-class ServiceWorkerFetchTask : public RefCounted<ServiceWorkerFetchTask> {
+class ServiceWorkerFetchTask {
+    WTF_MAKE_FAST_ALLOCATED;
 public:
-    template<typename... Args> static Ref<ServiceWorkerFetchTask> create(Args&&... args)
-    {
-        return adoptRef(*new ServiceWorkerFetchTask(std::forward<Args>(args)...));
-    }
+    ServiceWorkerFetchTask(PAL::SessionID, WebSWServerConnection&, WebSWServerToContextConnection&, WebCore::FetchIdentifier, WebCore::ServiceWorkerIdentifier, Seconds timeout);
 
     void didNotHandle();
     void fail(const WebCore::ResourceError& error) { didFail(error); }
@@ -79,8 +76,6 @@ public:
     bool wasHandled() const { return m_wasHandled; }
 
 private:
-    ServiceWorkerFetchTask(PAL::SessionID, WebSWServerConnection&, WebSWServerToContextConnection&, WebCore::FetchIdentifier, WebCore::ServiceWorkerIdentifier, Seconds timeout);
-
     void didReceiveRedirectResponse(const WebCore::ResourceResponse&);
     void didReceiveResponse(const WebCore::ResourceResponse&, bool needsContinueDidReceiveResponseMessage);
     void didReceiveData(const IPC::DataReference&, int64_t encodedDataLength);
@@ -97,7 +92,6 @@ private:
     Seconds m_timeout;
     WebCore::Timer m_timeoutTimer;
     bool m_wasHandled { false };
-    bool m_didReachTerminalState { false };
 };
 
 inline bool operator==(const ServiceWorkerFetchTask::Identifier& a, const ServiceWorkerFetchTask::Identifier& b)
index e36bb12..85d9e95 100644 (file)
@@ -157,7 +157,7 @@ void WebSWServerToContextConnection::startFetch(PAL::SessionID sessionID, WebSWS
     auto serverConnectionIdentifier = contentConnection.identifier();
     auto fetchIdentifier = FetchIdentifier::generate();
 
-    auto result = m_ongoingFetches.add(fetchIdentifier, ServiceWorkerFetchTask::create(sessionID, contentConnection, *this, contentFetchIdentifier, serviceWorkerIdentifier, m_networkProcess->serviceWorkerFetchTimeout()));
+    auto result = m_ongoingFetches.add(fetchIdentifier, makeUnique<ServiceWorkerFetchTask>(sessionID, contentConnection, *this, contentFetchIdentifier, serviceWorkerIdentifier, m_networkProcess->serviceWorkerFetchTimeout()));
 
     ASSERT(!m_ongoingFetchIdentifiers.contains({ serverConnectionIdentifier, contentFetchIdentifier }));
     m_ongoingFetchIdentifiers.add({ serverConnectionIdentifier, contentFetchIdentifier }, fetchIdentifier);
@@ -215,17 +215,17 @@ void WebSWServerToContextConnection::fetchTaskTimedOut(ServiceWorkerFetchTask& t
     ASSERT(m_ongoingFetches.contains(takenIdentifier));
     auto takenTask = m_ongoingFetches.take(takenIdentifier);
     ASSERT(takenTask);
-    ASSERT(takenTask->ptr() == &task);
+    ASSERT(takenTask.get() == &task);
 
     // Gather all other fetches in this service worker
-    HashSet<Ref<ServiceWorkerFetchTask>> otherFetches;
+    Vector<ServiceWorkerFetchTask*> otherFetches;
     for (auto& fetchTask : m_ongoingFetches.values()) {
         if (fetchTask->serviceWorkerIdentifier() == task.serviceWorkerIdentifier())
-            otherFetches.add(fetchTask.copyRef());
+            otherFetches.append(fetchTask.get());
     }
 
     // Signal load failure for them
-    for (auto& fetchTask : otherFetches) {
+    for (auto* fetchTask : otherFetches) {
         if (fetchTask->wasHandled())
             fetchTask->fail({ errorDomainWebKitInternal, 0, { }, "Service Worker context closed"_s });
         else
index 57bb4f3..e30f341 100644 (file)
@@ -103,7 +103,7 @@ private:
     WeakPtr<WebCore::SWServer> m_server;
 
     HashMap<ServiceWorkerFetchTask::Identifier, WebCore::FetchIdentifier> m_ongoingFetchIdentifiers;
-    HashMap<WebCore::FetchIdentifier, Ref<ServiceWorkerFetchTask>> m_ongoingFetches;
+    HashMap<WebCore::FetchIdentifier, std::unique_ptr<ServiceWorkerFetchTask>> m_ongoingFetches;
     bool m_isThrottleable { true };
 }; // class WebSWServerToContextConnection