http/tests/workers/service/service-worker-download.https.html times out with async...
authorajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2018 22:58:54 +0000 (22:58 +0000)
committerajuma@chromium.org <ajuma@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Mar 2018 22:58:54 +0000 (22:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183479

Reviewed by Youenn Fablet.

Source/WebKit:

Ensure that ServiceWorkerFetchClient::m_isCheckingResponse is set before code that depends on it
executes. This bit was set by code that's posted to the runloop using 'callOnMainThread' in
ServiceWorkerFetchClient::didReceiveResponse. But when didReceiveResponse is executing, tasks for
handling didReceiveData, didFail, or didFinish may already have been posted to the runloop, and in
that case would execute before m_isCheckingResponse gets set, and then incorrectly fail to
early-out. Fix this by directly setting m_isCheckingResponse in didReceiveResponse.

* WebProcess/Storage/ServiceWorkerClientFetch.cpp:
(WebKit::ServiceWorkerClientFetch::start):
(WebKit::ServiceWorkerClientFetch::didReceiveResponse):

LayoutTests:

Add layout test coverage.

* http/tests/workers/service/service-worker-download-async-delegates.https-expected.txt: Added.
* http/tests/workers/service/service-worker-download-async-delegates.https.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https.html [new file with mode: 0644]
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp

index 72133e0..88712b4 100644 (file)
@@ -1,3 +1,15 @@
+2018-03-12  Ali Juma  <ajuma@chromium.org>
+
+        http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183479
+
+        Reviewed by Youenn Fablet.
+
+        Add layout test coverage.
+
+        * http/tests/workers/service/service-worker-download-async-delegates.https-expected.txt: Added.
+        * http/tests/workers/service/service-worker-download-async-delegates.https.html: Added.
+
 2018-03-12  Chris Dumez  <cdumez@apple.com>
 
         http/tests/security/frame-loading-via-document-write-async-delegates.html fails with async delegates
diff --git a/LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https-expected.txt b/LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https-expected.txt
new file mode 100644 (file)
index 0000000..814299c
--- /dev/null
@@ -0,0 +1,4 @@
+Download started.
+Downloading URL with suggested filename "download-binary.php"
+Download completed.
+
diff --git a/LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https.html b/LayoutTests/http/tests/workers/service/service-worker-download-async-delegates.https.html
new file mode 100644 (file)
index 0000000..0393eb9
--- /dev/null
@@ -0,0 +1,24 @@
+<html>
+<head>
+<script src="resources/sw-test-pre.js"></script>
+</head>
+<body>
+<script>
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    testRunner.setShouldLogDownloadCallbacks(true);
+    testRunner.waitUntilDownloadFinished();
+    testRunner.setShouldDownloadUndisplayableMIMETypes(true);
+    if (testRunner.setShouldDecideResponsePolicyAfterDelay)
+        testRunner.setShouldDecideResponsePolicyAfterDelay(true);
+}
+
+async function doTest()
+{
+    await interceptedFrame("resources/service-worker-download-worker.js", "/workers/service/resources/");
+    window.location = "/workers/service/resources/download-binary.php";
+}
+doTest();
+</script>
+</body>
+</html>
index dc9a6c0..451b9b4 100644 (file)
@@ -1,3 +1,21 @@
+2018-03-12  Ali Juma  <ajuma@chromium.org>
+
+        http/tests/workers/service/service-worker-download.https.html times out with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183479
+
+        Reviewed by Youenn Fablet.
+
+        Ensure that ServiceWorkerFetchClient::m_isCheckingResponse is set before code that depends on it
+        executes. This bit was set by code that's posted to the runloop using 'callOnMainThread' in
+        ServiceWorkerFetchClient::didReceiveResponse. But when didReceiveResponse is executing, tasks for
+        handling didReceiveData, didFail, or didFinish may already have been posted to the runloop, and in
+        that case would execute before m_isCheckingResponse gets set, and then incorrectly fail to 
+        early-out. Fix this by directly setting m_isCheckingResponse in didReceiveResponse.
+
+        * WebProcess/Storage/ServiceWorkerClientFetch.cpp:
+        (WebKit::ServiceWorkerClientFetch::start):
+        (WebKit::ServiceWorkerClientFetch::didReceiveResponse):
+
 2018-03-12  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         REGRESSION(r211643): Dismissing WKActionSheet should not also dismiss its presenting view controller
index 2808f87..eaf715f 100644 (file)
@@ -68,6 +68,9 @@ void ServiceWorkerClientFetch::start()
 
     auto referrer = request.httpReferrer();
 
+    m_didFail = false;
+    m_didFinish = false;
+
     // We are intercepting fetch calls after going through the HTTP layer, which may add some specific headers.
     cleanHTTPRequestHeadersForAccessControl(request, options.httpHeadersToKeep);
 
@@ -100,11 +103,15 @@ std::optional<ResourceError> ServiceWorkerClientFetch::validateResponse(const Re
 
 void ServiceWorkerClientFetch::didReceiveResponse(ResourceResponse&& response)
 {
+    m_isCheckingResponse = true;
     callOnMainThread([this, protectedThis = makeRef(*this), response = WTFMove(response)]() mutable {
-        if (!m_loader)
+        if (!m_loader) {
+            m_isCheckingResponse = false;
             return;
+        }
 
         if (auto error = validateResponse(response)) {
+            m_isCheckingResponse = false;
             m_loader->didFail(error.value());
             ASSERT(!m_loader);
             if (auto callback = WTFMove(m_callback))
@@ -114,6 +121,8 @@ void ServiceWorkerClientFetch::didReceiveResponse(ResourceResponse&& response)
         response.setSource(ResourceResponse::Source::ServiceWorker);
 
         if (response.isRedirection() && response.httpHeaderFields().contains(HTTPHeaderName::Location)) {
+            m_isCheckingResponse = false;
+            continueLoadingAfterCheckingResponse();
             m_redirectionStatus = RedirectionStatus::Receiving;
             m_loader->willSendRequest(m_loader->request().redirectedRequest(response, m_shouldClearReferrerOnHTTPSToHTTPRedirect), response, [protectedThis = makeRef(*this), this](ResourceRequest&& request) {
                 if (request.isNull() || !m_callback)
@@ -142,7 +151,6 @@ void ServiceWorkerClientFetch::didReceiveResponse(ResourceResponse&& response)
         if (response.url().isNull())
             response.setURL(m_loader->request().url());
 
-        m_isCheckingResponse = true;
         m_loader->didReceiveResponse(response, [this, protectedThis = WTFMove(protectedThis)] {
             m_isCheckingResponse = false;
             continueLoadingAfterCheckingResponse();