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.
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
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 953287f..5812f13 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -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
index 0000000..081cc8d
--- /dev/null
+++ b/LayoutTests/http/wpt/service-workers/fetch-timeout-worker.js
@@ -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
index 0000000..8022b2d
--- /dev/null
+++ b/LayoutTests/http/wpt/service-workers/fetch-timeout.https-expected.txt
@@ -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
index 0000000..4663e4e
--- /dev/null
+++ b/LayoutTests/http/wpt/service-workers/fetch-timeout.https.html
@@ -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>
diff --git a/LayoutTests/http/wpt/service-workers/resources/lengthy-pass.py b/LayoutTests/http/wpt/service-workers/resources/lengthy-pass.py
index 04ef57a..14dd790 100644
--- a/LayoutTests/http/wpt/service-workers/resources/lengthy-pass.py
+++ b/LayoutTests/http/wpt/service-workers/resources/lengthy-pass.py
@@ -2,6 +2,9 @@
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()
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 5e1a11b..a9aa697f 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -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
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
index 0dcb3b3..834b569 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp
@@ -59,6 +59,7 @@
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::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::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);
}
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h
index 6add0d0..d8b9d6f 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.h
@@ -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 @@
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 @@
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 @@
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)
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
index e36bb12..85d9e95 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp
@@ -157,7 +157,7 @@
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 @@
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
diff --git a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
index 57bb4f3..e30f341 100644
--- a/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
+++ b/Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h
@@ -103,7 +103,7 @@
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
