Provisional load may get committed before receiving the decidePolicyForNavigationResp...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Feb 2018 01:08:28 +0000 (01:08 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Feb 2018 01:08:28 +0000 (01:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182720
<rdar://problem/37515204>

Reviewed by Alex Christensen.

Source/WebCore:

Wait for the policy response from the client after receiving a resource response,
before sending the NetworkResourceLoader::ContinueDidReceiveResponse IPC back to
the NetworkProcess. Otherwise, the network process may start sending us data and
we may end up committing the provisional load before receiving the policy decision
fron the client.

Change is covered by new API test.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::responseReceived):
* loader/NetscapePlugInStreamLoader.cpp:
(WebCore::NetscapePlugInStreamLoader::didReceiveResponse):
* loader/NetscapePlugInStreamLoader.h:
* loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::deliverResponseAndData):
(WebCore::ResourceLoader::loadDataURL):
(WebCore::ResourceLoader::didReceiveResponse):
(WebCore::ResourceLoader::didReceiveResponseAsync):
* loader/ResourceLoader.h:
* loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::didReceiveResponse):
(WebCore::SubresourceLoader::didReceiveResponsePolicy):
(WebCore::SubresourceLoader::willCancel):
* loader/SubresourceLoader.h:
* loader/ios/PreviewLoader.mm:
(-[WebPreviewLoader _sendDidReceiveResponseIfNecessary]):

Source/WebKit:

* WebProcess/Network/WebResourceLoader.cpp:
(WebKit::WebResourceLoader::didReceiveResponse):
* WebProcess/Storage/ServiceWorkerClientFetch.cpp:
(WebKit::ServiceWorkerClientFetch::didReceiveResponse):
* WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:
(WebKit::WebURLSchemeTaskProxy::didReceiveResponse):

Source/WTF:

Add convenience CompletionHandlerCallingScope class which calls its CompletionHandler
when destroyed, and provides a release() methods to manually call the completionHandler.

* wtf/CompletionHandler.h:
(WTF::CompletionHandlerCallingScope::CompletionHandlerCallingScope):
(WTF::CompletionHandlerCallingScope::~CompletionHandlerCallingScope):
(WTF::CompletionHandlerCallingScope::CompletionHandler<void):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm:
(-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
(TestWebKitAPI::TEST):

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

17 files changed:
Source/WTF/ChangeLog
Source/WTF/wtf/CompletionHandler.h
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/NetscapePlugInStreamLoader.cpp
Source/WebCore/loader/NetscapePlugInStreamLoader.h
Source/WebCore/loader/ResourceLoader.cpp
Source/WebCore/loader/ResourceLoader.h
Source/WebCore/loader/SubresourceLoader.cpp
Source/WebCore/loader/SubresourceLoader.h
Source/WebCore/loader/ios/PreviewLoader.mm
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/Network/WebResourceLoader.cpp
Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp
Source/WebKit/WebProcess/WebPage/WebURLSchemeTaskProxy.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm

index ba33e71..5dfdf25 100644 (file)
@@ -1,3 +1,19 @@
+2018-02-20  Chris Dumez  <cdumez@apple.com>
+
+        Provisional load may get committed before receiving the decidePolicyForNavigationResponse response
+        https://bugs.webkit.org/show_bug.cgi?id=182720
+        <rdar://problem/37515204>
+
+        Reviewed by Alex Christensen.
+
+        Add convenience CompletionHandlerCallingScope class which calls its CompletionHandler
+        when destroyed, and provides a release() methods to manually call the completionHandler.
+
+        * wtf/CompletionHandler.h:
+        (WTF::CompletionHandlerCallingScope::CompletionHandlerCallingScope):
+        (WTF::CompletionHandlerCallingScope::~CompletionHandlerCallingScope):
+        (WTF::CompletionHandlerCallingScope::CompletionHandler<void):
+
 2018-02-20  Tim Horton  <timothy_horton@apple.com>
 
         Always inline soft linking functions to work around a clang bug
index a553c73..e02faf1 100644 (file)
@@ -64,6 +64,28 @@ private:
     mutable WTF::Function<Out(In...)> m_function;
 };
 
+class CompletionHandlerCallingScope {
+public:
+    CompletionHandlerCallingScope(CompletionHandler<void()>&& completionHandler)
+        : m_completionHandler(WTFMove(completionHandler))
+    { }
+
+    ~CompletionHandlerCallingScope()
+    {
+        if (m_completionHandler)
+            m_completionHandler();
+    }
+
+    CompletionHandlerCallingScope(CompletionHandlerCallingScope&&) = default;
+    CompletionHandlerCallingScope& operator=(CompletionHandlerCallingScope&&) = default;
+
+    CompletionHandler<void()> release() { return WTFMove(m_completionHandler); }
+
+private:
+    CompletionHandler<void()> m_completionHandler;
+};
+
 } // namespace WTF
 
 using WTF::CompletionHandler;
+using WTF::CompletionHandlerCallingScope;
index 301225a..f437c75 100644 (file)
@@ -1,5 +1,40 @@
 2018-02-20  Chris Dumez  <cdumez@apple.com>
 
+        Provisional load may get committed before receiving the decidePolicyForNavigationResponse response
+        https://bugs.webkit.org/show_bug.cgi?id=182720
+        <rdar://problem/37515204>
+
+        Reviewed by Alex Christensen.
+
+        Wait for the policy response from the client after receiving a resource response,
+        before sending the NetworkResourceLoader::ContinueDidReceiveResponse IPC back to
+        the NetworkProcess. Otherwise, the network process may start sending us data and
+        we may end up committing the provisional load before receiving the policy decision
+        fron the client.
+
+        Change is covered by new API test.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::responseReceived):
+        * loader/NetscapePlugInStreamLoader.cpp:
+        (WebCore::NetscapePlugInStreamLoader::didReceiveResponse):
+        * loader/NetscapePlugInStreamLoader.h:
+        * loader/ResourceLoader.cpp:
+        (WebCore::ResourceLoader::deliverResponseAndData):
+        (WebCore::ResourceLoader::loadDataURL):
+        (WebCore::ResourceLoader::didReceiveResponse):
+        (WebCore::ResourceLoader::didReceiveResponseAsync):
+        * loader/ResourceLoader.h:
+        * loader/SubresourceLoader.cpp:
+        (WebCore::SubresourceLoader::didReceiveResponse):
+        (WebCore::SubresourceLoader::didReceiveResponsePolicy):
+        (WebCore::SubresourceLoader::willCancel):
+        * loader/SubresourceLoader.h:
+        * loader/ios/PreviewLoader.mm:
+        (-[WebPreviewLoader _sendDidReceiveResponseIfNecessary]):
+
+2018-02-20  Chris Dumez  <cdumez@apple.com>
+
         Crash under JSC::JSCell::toNumber(JSC::ExecState*)
         https://bugs.webkit.org/show_bug.cgi?id=182984
         <rdar://problem/37694346>
index b9dcd9a..3d9d68f 100644 (file)
@@ -783,7 +783,12 @@ void DocumentLoader::responseReceived(const ResourceResponse& response)
     }
 #endif
 
+    if (auto* mainResourceLoader = this->mainResourceLoader())
+        mainResourceLoader->markInAsyncResponsePolicyCheck();
     frameLoader()->checkContentPolicy(m_response, [this, protectedThis = makeRef(*this)](PolicyAction policy) {
+        if (auto* mainResourceLoader = this->mainResourceLoader())
+            mainResourceLoader->didReceiveResponsePolicy();
+
         continueAfterContentPolicy(policy);
     });
 }
index edf099d..4c776e2 100644 (file)
@@ -97,9 +97,10 @@ void NetscapePlugInStreamLoader::willSendRequest(ResourceRequest&& request, cons
     });
 }
 
-void NetscapePlugInStreamLoader::didReceiveResponse(const ResourceResponse& response)
+void NetscapePlugInStreamLoader::didReceiveResponse(const ResourceResponse& response, CompletionHandler<void()>&& policyCompletionHandler)
 {
     Ref<NetscapePlugInStreamLoader> protectedThis(*this);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(policyCompletionHandler));
 
     m_client->didReceiveResponse(this, response);
 
@@ -107,21 +108,21 @@ void NetscapePlugInStreamLoader::didReceiveResponse(const ResourceResponse& resp
     if (!m_client)
         return;
 
-    ResourceLoader::didReceiveResponse(response);
+    ResourceLoader::didReceiveResponse(response, [this, protectedThis = WTFMove(protectedThis), response, completionHandlerCaller = WTFMove(completionHandlerCaller)]() mutable {
+        // Don't continue if the stream is cancelled
+        if (!m_client)
+            return;
 
-    // Don't continue if the stream is cancelled
-    if (!m_client)
-        return;
+        if (!response.isHTTP())
+            return;
 
-    if (!response.isHTTP())
-        return;
-    
-    if (m_client->wantsAllStreams())
-        return;
+        if (m_client->wantsAllStreams())
+            return;
 
-    // Status code can be null when serving from a Web archive.
-    if (response.httpStatusCode() && (response.httpStatusCode() < 100 || response.httpStatusCode() >= 400))
-        cancel(frameLoader()->client().fileDoesNotExistError(response));
+        // Status code can be null when serving from a Web archive.
+        if (response.httpStatusCode() && (response.httpStatusCode() < 100 || response.httpStatusCode() >= 400))
+            cancel(frameLoader()->client().fileDoesNotExistError(response));
+    });
 }
 
 void NetscapePlugInStreamLoader::didReceiveData(const char* data, unsigned length, long long encodedDataLength, DataPayloadType dataPayloadType)
index 600792e..00bbb9e 100644 (file)
@@ -60,7 +60,7 @@ private:
     void init(ResourceRequest&&, CompletionHandler<void(bool)>&&) override;
 
     void willSendRequest(ResourceRequest&&, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&& callback) override;
-    void didReceiveResponse(const ResourceResponse&) override;
+    void didReceiveResponse(const ResourceResponse&, CompletionHandler<void()>&& policyCompletionHandler) override;
     void didReceiveData(const char*, unsigned, long long encodedDataLength, DataPayloadType) override;
     void didReceiveBuffer(Ref<SharedBuffer>&&, long long encodedDataLength, DataPayloadType) override;
     void didFinishLoading(const NetworkLoadMetrics&) override;
index 8a01218..957b1b2 100644 (file)
@@ -169,21 +169,20 @@ void ResourceLoader::init(ResourceRequest&& clientRequest, CompletionHandler<voi
 
 void ResourceLoader::deliverResponseAndData(const ResourceResponse& response, RefPtr<SharedBuffer>&& buffer)
 {
-    Ref<ResourceLoader> protectedThis(*this);
-
-    didReceiveResponse(response);
-    if (reachedTerminalState())
-        return;
-
-    if (buffer) {
-        unsigned size = buffer->size();
-        didReceiveBuffer(buffer.releaseNonNull(), size, DataPayloadWholeResource);
+    didReceiveResponse(response, [this, protectedThis = makeRef(*this), buffer = WTFMove(buffer)]() mutable {
         if (reachedTerminalState())
             return;
-    }
 
-    NetworkLoadMetrics emptyMetrics;
-    didFinishLoading(emptyMetrics);
+        if (buffer) {
+            unsigned size = buffer->size();
+            didReceiveBuffer(buffer.releaseNonNull(), size, DataPayloadWholeResource);
+            if (reachedTerminalState())
+                return;
+        }
+
+        NetworkLoadMetrics emptyMetrics;
+        didFinishLoading(emptyMetrics);
+    });
 }
 
 void ResourceLoader::start()
@@ -251,14 +250,14 @@ void ResourceLoader::loadDataURL()
     if (auto* scheduledPairs = m_frame->page()->scheduledRunLoopPairs())
         scheduleContext.scheduledPairs = *scheduledPairs;
 #endif
-    DataURLDecoder::decode(url, scheduleContext, [protectedThis = makeRef(*this), url](auto decodeResult) {
-        if (protectedThis->reachedTerminalState())
+    DataURLDecoder::decode(url, scheduleContext, [this, protectedThis = makeRef(*this), url](auto decodeResult) mutable {
+        if (this->reachedTerminalState())
             return;
         if (!decodeResult) {
             protectedThis->didFail(ResourceError(errorDomainWebKitInternal, 0, url, "Data URL decoding failed"));
             return;
         }
-        if (protectedThis->wasCancelled())
+        if (this->wasCancelled())
             return;
         auto& result = decodeResult.value();
         auto dataSize = result.data ? result.data->size() : 0;
@@ -268,15 +267,15 @@ void ResourceLoader::loadDataURL()
         dataResponse.setHTTPStatusText(ASCIILiteral("OK"));
         dataResponse.setHTTPHeaderField(HTTPHeaderName::ContentType, result.contentType);
         dataResponse.setSource(ResourceResponse::Source::Network);
-        protectedThis->didReceiveResponse(dataResponse);
+        this->didReceiveResponse(dataResponse, [this, protectedThis = WTFMove(protectedThis), dataSize, data = result.data.releaseNonNull()]() mutable {
+            if (!this->reachedTerminalState() && dataSize)
+                this->didReceiveBuffer(WTFMove(data), dataSize, DataPayloadWholeResource);
 
-        if (!protectedThis->reachedTerminalState() && dataSize)
-            protectedThis->didReceiveBuffer(result.data.releaseNonNull(), dataSize, DataPayloadWholeResource);
-
-        if (!protectedThis->reachedTerminalState()) {
-            NetworkLoadMetrics emptyMetrics;
-            protectedThis->didFinishLoading(emptyMetrics);
-        }
+            if (!this->reachedTerminalState()) {
+                NetworkLoadMetrics emptyMetrics;
+                this->didFinishLoading(emptyMetrics);
+            }
+        });
     });
 }
 
@@ -459,9 +458,10 @@ void ResourceLoader::didBlockAuthenticationChallenge()
     FrameLoader::reportAuthenticationChallengeBlocked(m_frame.get(), m_request.url(), ASCIILiteral("it is a cross-origin request"));
 }
 
-void ResourceLoader::didReceiveResponse(const ResourceResponse& r)
+void ResourceLoader::didReceiveResponse(const ResourceResponse& r, CompletionHandler<void()>&& policyCompletionHandler)
 {
     ASSERT(!m_reachedTerminalState);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(policyCompletionHandler));
 
     // Protect this in this delegate method since the additional processing can do
     // anything including possibly derefing this; one example of this is Radar 3266216.
@@ -660,8 +660,7 @@ void ResourceLoader::didReceiveResponseAsync(ResourceHandle*, ResourceResponse&&
         completionHandler();
         return;
     }
-    didReceiveResponse(response);
-    completionHandler();
+    didReceiveResponse(response, WTFMove(completionHandler));
 }
 
 void ResourceLoader::didReceiveData(ResourceHandle*, const char* data, unsigned length, int encodedDataLength)
index b84d672..d45a0e3 100644 (file)
@@ -101,7 +101,7 @@ public:
 
     virtual void willSendRequest(ResourceRequest&&, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&& callback);
     virtual void didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent);
-    virtual void didReceiveResponse(const ResourceResponse&);
+    virtual void didReceiveResponse(const ResourceResponse&, CompletionHandler<void()>&& policyCompletionHandler);
     virtual void didReceiveData(const char*, unsigned, long long encodedDataLength, DataPayloadType);
     virtual void didReceiveBuffer(Ref<SharedBuffer>&&, long long encodedDataLength, DataPayloadType);
     virtual void didFinishLoading(const NetworkLoadMetrics&);
index 54397e0..094f4b9 100644 (file)
@@ -288,11 +288,13 @@ bool SubresourceLoader::shouldCreatePreviewLoaderForResponse(const ResourceRespo
 
 #endif
 
-void SubresourceLoader::didReceiveResponse(const ResourceResponse& response)
+void SubresourceLoader::didReceiveResponse(const ResourceResponse& response, CompletionHandler<void()>&& policyCompletionHandler)
 {
     ASSERT(!response.isNull());
     ASSERT(m_state == Initialized);
 
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(policyCompletionHandler));
+
 #if USE(QUICK_LOOK)
     if (shouldCreatePreviewLoaderForResponse(response)) {
         m_previewLoader = PreviewLoader::create(*this, response);
@@ -335,7 +337,7 @@ void SubresourceLoader::didReceiveResponse(const ResourceResponse& response)
             if (m_frame && m_frame->page())
                 m_frame->page()->diagnosticLoggingClient().logDiagnosticMessageWithResult(DiagnosticLoggingKeys::cachedResourceRevalidationKey(), emptyString(), DiagnosticLoggingResultPass, ShouldSample::Yes);
             if (!reachedTerminalState())
-                ResourceLoader::didReceiveResponse(revalidationResponse);
+                ResourceLoader::didReceiveResponse(revalidationResponse, [completionHandlerCaller = WTFMove(completionHandlerCaller)] { });
             return;
         }
         // Did not get 304 response, continue as a regular resource load.
@@ -356,36 +358,49 @@ void SubresourceLoader::didReceiveResponse(const ResourceResponse& response)
     if (reachedTerminalState())
         return;
 
-    ResourceLoader::didReceiveResponse(response);
-    if (reachedTerminalState())
-        return;
+    bool isResponseMultipart = response.isMultipart();
+    ResourceLoader::didReceiveResponse(response, [this, protectedThis = WTFMove(protectedThis), isResponseMultipart, completionHandlerCaller = WTFMove(completionHandlerCaller)]() mutable {
+        if (reachedTerminalState())
+            return;
 
-    // FIXME: Main resources have a different set of rules for multipart than images do.
-    // Hopefully we can merge those 2 paths.
-    if (response.isMultipart() && m_resource->type() != CachedResource::MainResource) {
-        m_loadingMultipartContent = true;
+        // FIXME: Main resources have a different set of rules for multipart than images do.
+        // Hopefully we can merge those 2 paths.
+        if (isResponseMultipart && m_resource->type() != CachedResource::MainResource) {
+            m_loadingMultipartContent = true;
 
-        // We don't count multiParts in a CachedResourceLoader's request count
-        m_requestCountTracker = std::nullopt;
-        if (!m_resource->isImage()) {
-            cancel();
-            return;
+            // We don't count multiParts in a CachedResourceLoader's request count
+            m_requestCountTracker = std::nullopt;
+            if (!m_resource->isImage()) {
+                cancel();
+                return;
+            }
         }
-    }
 
-    auto* buffer = resourceData();
-    if (m_loadingMultipartContent && buffer && buffer->size()) {
-        // The resource data will change as the next part is loaded, so we need to make a copy.
-        m_resource->finishLoading(buffer->copy().ptr());
-        clearResourceData();
-        // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.
-        // After the first multipart section is complete, signal to delegates that this load is "finished"
-        NetworkLoadMetrics emptyMetrics;
-        m_documentLoader->subresourceLoaderFinishedLoadingOnePart(this);
-        didFinishLoadingOnePart(emptyMetrics);
-    }
+        auto* buffer = resourceData();
+        if (m_loadingMultipartContent && buffer && buffer->size()) {
+            // The resource data will change as the next part is loaded, so we need to make a copy.
+            m_resource->finishLoading(buffer->copy().ptr());
+            clearResourceData();
+            // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.
+            // After the first multipart section is complete, signal to delegates that this load is "finished"
+            NetworkLoadMetrics emptyMetrics;
+            m_documentLoader->subresourceLoaderFinishedLoadingOnePart(this);
+            didFinishLoadingOnePart(emptyMetrics);
+        }
 
-    checkForHTTPStatusCodeError();
+        checkForHTTPStatusCodeError();
+
+        if (m_inAsyncResponsePolicyCheck)
+            m_policyForResponseCompletionHandler = completionHandlerCaller.release();
+    });
+}
+
+void SubresourceLoader::didReceiveResponsePolicy()
+{
+    ASSERT(m_inAsyncResponsePolicyCheck);
+    m_inAsyncResponsePolicyCheck = false;
+    if (auto completionHandler = WTFMove(m_policyForResponseCompletionHandler))
+        completionHandler();
 }
 
 void SubresourceLoader::didReceiveData(const char* data, unsigned length, long long encodedDataLength, DataPayloadType dataPayloadType)
@@ -659,6 +674,9 @@ void SubresourceLoader::willCancel(const ResourceError& error)
     ASSERT(!reachedTerminalState());
     LOG(ResourceLoading, "Cancelled load of '%s'.\n", m_resource->url().string().latin1().data());
 
+    if (auto policyForResponseCompletionHandler = WTFMove(m_policyForResponseCompletionHandler))
+        policyForResponseCompletionHandler();
+
     Ref<SubresourceLoader> protectedThis(*this);
 #if PLATFORM(IOS)
     m_state = m_state == Uninitialized ? CancelledWhileInitializing : Finishing;
index 1216b0b..08dfdfd 100644 (file)
@@ -30,6 +30,7 @@
 
 #include "FrameLoaderTypes.h"
 #include "ResourceLoader.h"
+#include <wtf/CompletionHandler.h>
 #include <wtf/text/WTFString.h>
  
 namespace WebCore {
@@ -60,6 +61,9 @@ public:
 
     unsigned redirectCount() const { return m_redirectCount; }
 
+    void markInAsyncResponsePolicyCheck() { m_inAsyncResponsePolicyCheck = true; }
+    void didReceiveResponsePolicy();
+
 private:
     SubresourceLoader(Frame&, CachedResource&, const ResourceLoaderOptions&);
 
@@ -67,7 +71,7 @@ private:
 
     void willSendRequestInternal(ResourceRequest&&, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&&) override;
     void didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent) override;
-    void didReceiveResponse(const ResourceResponse&) override;
+    void didReceiveResponse(const ResourceResponse&, CompletionHandler<void()>&& policyCompletionHandler) override;
     void didReceiveData(const char*, unsigned, long long encodedDataLength, DataPayloadType) override;
     void didReceiveBuffer(Ref<SharedBuffer>&&, long long encodedDataLength, DataPayloadType) override;
     void didFinishLoading(const NetworkLoadMetrics&) override;
@@ -124,8 +128,10 @@ private:
     SubresourceLoaderState m_state;
     std::optional<RequestCountTracker> m_requestCountTracker;
     RefPtr<SecurityOrigin> m_origin;
+    CompletionHandler<void()> m_policyForResponseCompletionHandler;
     unsigned m_redirectCount { 0 };
     bool m_loadingMultipartContent { false };
+    bool m_inAsyncResponsePolicyCheck { false };
 };
 
 }
index 592b0ca..9c31260 100644 (file)
@@ -130,7 +130,7 @@ static PreviewLoaderClient& emptyClient()
     _resourceLoader->documentLoader()->setPreviewConverter(WTFMove(_converter));
 
     _hasSentDidReceiveResponse = YES;
-    _resourceLoader->didReceiveResponse(response);
+    _resourceLoader->didReceiveResponse(response, nullptr);
 }
 
 - (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data lengthReceived:(long long)lengthReceived
index d9fd306..6aba890 100644 (file)
@@ -1,3 +1,18 @@
+2018-02-20  Chris Dumez  <cdumez@apple.com>
+
+        Provisional load may get committed before receiving the decidePolicyForNavigationResponse response
+        https://bugs.webkit.org/show_bug.cgi?id=182720
+        <rdar://problem/37515204>
+
+        Reviewed by Alex Christensen.
+
+        * WebProcess/Network/WebResourceLoader.cpp:
+        (WebKit::WebResourceLoader::didReceiveResponse):
+        * WebProcess/Storage/ServiceWorkerClientFetch.cpp:
+        (WebKit::ServiceWorkerClientFetch::didReceiveResponse):
+        * WebProcess/WebPage/WebURLSchemeTaskProxy.cpp:
+        (WebKit::WebURLSchemeTaskProxy::didReceiveResponse):
+
 2018-02-20  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r228829.
index 6a6d88f..1b42615 100644 (file)
@@ -107,19 +107,21 @@ void WebResourceLoader::didReceiveResponse(const ResourceResponse& response, boo
     LOG(Network, "(WebProcess) WebResourceLoader::didReceiveResponse for '%s'. Status %d.", m_coreLoader->url().string().latin1().data(), response.httpStatusCode());
     RELEASE_LOG_IF_ALLOWED("didReceiveResponse: (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ", status = %d)", m_trackingParameters.pageID, m_trackingParameters.frameID, m_trackingParameters.resourceID, response.httpStatusCode());
 
-    Ref<WebResourceLoader> protect(*this);
+    Ref<WebResourceLoader> protectedThis(*this);
 
     if (m_coreLoader->documentLoader()->applicationCacheHost().maybeLoadFallbackForResponse(m_coreLoader.get(), response))
         return;
 
-    m_coreLoader->didReceiveResponse(response);
-
-    // If m_coreLoader becomes null as a result of the didReceiveResponse callback, we can't use the send function(). 
-    if (!m_coreLoader)
-        return;
+    CompletionHandler<void()> policyDecisionCompletionHandler;
+    if (needsContinueDidReceiveResponseMessage) {
+        policyDecisionCompletionHandler = [this, protectedThis = WTFMove(protectedThis)] {
+            // If m_coreLoader becomes null as a result of the didReceiveResponse callback, we can't use the send function().
+            if (m_coreLoader)
+                send(Messages::NetworkResourceLoader::ContinueDidReceiveResponse());
+        };
+    }
 
-    if (needsContinueDidReceiveResponseMessage)
-        send(Messages::NetworkResourceLoader::ContinueDidReceiveResponse());
+    m_coreLoader->didReceiveResponse(response, WTFMove(policyDecisionCompletionHandler));
 }
 
 void WebResourceLoader::didReceiveData(const IPC::DataReference& data, int64_t encodedDataLength)
index 380c57e..1f72a30 100644 (file)
@@ -141,9 +141,10 @@ void ServiceWorkerClientFetch::didReceiveResponse(ResourceResponse&& response)
         if (response.url().isNull())
             response.setURL(m_loader->request().url());
 
-        m_loader->didReceiveResponse(response);
-        if (auto callback = WTFMove(m_callback))
-            callback(Result::Succeeded);
+        m_loader->didReceiveResponse(response, [this, protectedThis = WTFMove(protectedThis)] {
+            if (auto callback = WTFMove(m_callback))
+                callback(Result::Succeeded);
+        });
     });
 }
 
index 7dbba87..7563355 100644 (file)
@@ -93,7 +93,7 @@ void WebURLSchemeTaskProxy::didReceiveResponse(const ResourceResponse& response)
     if (!hasLoader())
         return;
 
-    m_coreLoader->didReceiveResponse(response);
+    m_coreLoader->didReceiveResponse(response, nullptr);
 }
 
 void WebURLSchemeTaskProxy::didReceiveData(size_t size, const uint8_t* data)
index f1e1193..1ba80a0 100644 (file)
@@ -1,3 +1,17 @@
+2018-02-20  Chris Dumez  <cdumez@apple.com>
+
+        Provisional load may get committed before receiving the decidePolicyForNavigationResponse response
+        https://bugs.webkit.org/show_bug.cgi?id=182720
+        <rdar://problem/37515204>
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/AsyncPolicyForNavigationResponse.mm:
+        (-[TestAsyncNavigationDelegate webView:decidePolicyForNavigationResponse:decisionHandler:]):
+        (TestWebKitAPI::TEST):
+
 2018-02-20  Nan Wang  <n_wang@apple.com>
 
         AX: AOM: Dispatch accessibleclick event
index 5b6156a..56bd8f4 100644 (file)
@@ -31,6 +31,7 @@
 #import <wtf/RetainPtr.h>
 
 #if WK_API_ENABLED
+static bool shouldAccept = true;
 static bool navigationComplete = false;
 static bool navigationFailed = false;
 
@@ -66,7 +67,7 @@ static bool navigationFailed = false;
     int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
     dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
     dispatch_after(when, dispatch_get_main_queue(), ^{
-        decisionHandler(WKNavigationResponsePolicyAllow);
+        decisionHandler(shouldAccept ? WKNavigationResponsePolicyAllow : WKNavigationResponsePolicyCancel);
     });
 }
 @end
@@ -82,12 +83,33 @@ TEST(WebKit, RespondToPolicyForNavigationResponseAsynchronously)
     [webView setNavigationDelegate:delegate.get()];
     [webView setUIDelegate:delegate.get()];
 
+    shouldAccept = true;
+    navigationFailed = false;
+    navigationComplete = false;
     [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     TestWebKitAPI::Util::run(&navigationComplete);
 
     EXPECT_FALSE(navigationFailed);
 }
 
+TEST(WebKit, PolicyForNavigationResponseCancelAsynchronously)
+{
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto webView = adoptNS([[WKWebView alloc] init]);
+    auto delegate = adoptNS([[TestAsyncNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+    [webView setUIDelegate:delegate.get()];
+
+    shouldAccept = false;
+    navigationFailed = false;
+    navigationComplete = false;
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    TestWebKitAPI::Util::run(&navigationComplete);
+
+    EXPECT_TRUE(navigationFailed);
+}
+
 } // namespace TestWebKitAPI
 
 #endif // WK_API_ENABLED