WebKit.RestoreSessionStateContainingScrollRestorationDefault API test is failing...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Mar 2018 18:33:48 +0000 (18:33 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Mar 2018 18:33:48 +0000 (18:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183679

Reviewed by Alex Christensen.

Source/WebCore:

Update CachedRawResource::didAddClient() to not send data until we've received
the policy decision for the response.

No new tests, covered by new API test.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::handleSubstituteDataLoadNow):
(WebCore::DocumentLoader::responseReceived):
* loader/DocumentLoader.h:
* loader/DocumentThreadableLoader.cpp:
(WebCore::DocumentThreadableLoader::responseReceived):
* loader/DocumentThreadableLoader.h:
* loader/MediaResourceLoader.cpp:
(WebCore::MediaResource::responseReceived):
* loader/MediaResourceLoader.h:
* loader/appcache/ApplicationCacheResourceLoader.cpp:
(WebCore::ApplicationCacheResourceLoader::responseReceived):
* loader/appcache/ApplicationCacheResourceLoader.h:
* loader/cache/CachedRawResource.cpp:
(WebCore::CachedRawResource::didAddClient):
(WebCore::CachedRawResource::responseReceived):
* loader/cache/CachedRawResourceClient.h:
(WebCore::CachedRawResourceClient::responseReceived):
* loader/cache/KeepaliveRequestTracker.cpp:
(WebCore::KeepaliveRequestTracker::responseReceived):
* loader/cache/KeepaliveRequestTracker.h:
* platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.h:
* platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:
(WebCore::WebCoreAVFResourceLoader::responseReceived):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKit/RestoreSessionState.cpp:
(TestWebKitAPI::decidePolicyForNavigationAction):
(TestWebKitAPI::decidePolicyForResponse):
(TestWebKitAPI::TEST):

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

19 files changed:
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/DocumentLoader.h
Source/WebCore/loader/DocumentThreadableLoader.cpp
Source/WebCore/loader/DocumentThreadableLoader.h
Source/WebCore/loader/MediaResourceLoader.cpp
Source/WebCore/loader/MediaResourceLoader.h
Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.cpp
Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.h
Source/WebCore/loader/cache/CachedRawResource.cpp
Source/WebCore/loader/cache/CachedRawResourceClient.h
Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp
Source/WebCore/loader/cache/KeepaliveRequestTracker.h
Source/WebCore/platform/graphics/avfoundation/cf/WebCoreAVCFResourceLoader.cpp
Source/WebCore/platform/graphics/avfoundation/cf/WebCoreAVCFResourceLoader.h
Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.h
Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit/RestoreSessionState.cpp

index f3da54b..ed48028 100644 (file)
@@ -1,3 +1,40 @@
+2018-03-16  Chris Dumez  <cdumez@apple.com>
+
+        WebKit.RestoreSessionStateContainingScrollRestorationDefault API test is failing with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183679
+
+        Reviewed by Alex Christensen.
+
+        Update CachedRawResource::didAddClient() to not send data until we've received
+        the policy decision for the response.
+
+        No new tests, covered by new API test.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::handleSubstituteDataLoadNow):
+        (WebCore::DocumentLoader::responseReceived):
+        * loader/DocumentLoader.h:
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::responseReceived):
+        * loader/DocumentThreadableLoader.h:
+        * loader/MediaResourceLoader.cpp:
+        (WebCore::MediaResource::responseReceived):
+        * loader/MediaResourceLoader.h:
+        * loader/appcache/ApplicationCacheResourceLoader.cpp:
+        (WebCore::ApplicationCacheResourceLoader::responseReceived):
+        * loader/appcache/ApplicationCacheResourceLoader.h:
+        * loader/cache/CachedRawResource.cpp:
+        (WebCore::CachedRawResource::didAddClient):
+        (WebCore::CachedRawResource::responseReceived):
+        * loader/cache/CachedRawResourceClient.h:
+        (WebCore::CachedRawResourceClient::responseReceived):
+        * loader/cache/KeepaliveRequestTracker.cpp:
+        (WebCore::KeepaliveRequestTracker::responseReceived):
+        * loader/cache/KeepaliveRequestTracker.h:
+        * platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.h:
+        * platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:
+        (WebCore::WebCoreAVFResourceLoader::responseReceived):
+
 2018-03-16  Youenn Fablet  <youenn@apple.com>
 
         Name Service Worker threads differently from regular Worker threads
index c850fc3..327c581 100644 (file)
@@ -461,7 +461,7 @@ void DocumentLoader::handleSubstituteDataLoadNow()
     if (response.url().isEmpty())
         response = ResourceResponse(m_request.url(), m_substituteData.mimeType(), m_substituteData.content()->size(), m_substituteData.textEncoding());
 
-    responseReceived(response);
+    responseReceived(response, nullptr);
 }
 
 void DocumentLoader::startDataLoadTimer()
@@ -716,14 +716,16 @@ void DocumentLoader::stopLoadingAfterXFrameOptionsOrContentSecurityPolicyDenied(
         cancelMainResourceLoad(frameLoader->cancelledError(m_request));
 }
 
-void DocumentLoader::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void DocumentLoader::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, m_mainResource == &resource);
-    responseReceived(response);
+    responseReceived(response, WTFMove(completionHandler));
 }
 
-void DocumentLoader::responseReceived(const ResourceResponse& response)
+void DocumentLoader::responseReceived(const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
+
 #if ENABLE(CONTENT_FILTERING)
     if (m_contentFilter && !m_contentFilter->continueAfterResponseReceived(response))
         return;
@@ -809,10 +811,12 @@ void DocumentLoader::responseReceived(const ResourceResponse& response)
     RefPtr<SubresourceLoader> mainResourceLoader = this->mainResourceLoader();
     if (mainResourceLoader)
         mainResourceLoader->markInAsyncResponsePolicyCheck();
-    frameLoader()->checkContentPolicy(m_response, [this, protectedThis = makeRef(*this), mainResourceLoader = WTFMove(mainResourceLoader)](PolicyAction policy) {
+    frameLoader()->checkContentPolicy(m_response, [this, protectedThis = makeRef(*this), mainResourceLoader = WTFMove(mainResourceLoader), completionHandler = completionHandlerCaller.release()](PolicyAction policy) {
         continueAfterContentPolicy(policy);
         if (mainResourceLoader)
             mainResourceLoader->didReceiveResponsePolicy();
+        if (completionHandler)
+            completionHandler();
     });
 }
 
index 677aef9..70a1a3e 100644 (file)
@@ -355,11 +355,11 @@ private:
     void finishedLoading();
     void mainReceivedError(const ResourceError&);
     WEBCORE_EXPORT void redirectReceived(CachedResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) override;
-    WEBCORE_EXPORT void responseReceived(CachedResource&, const ResourceResponse&) override;
+    WEBCORE_EXPORT void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) override;
     WEBCORE_EXPORT void dataReceived(CachedResource&, const char* data, int length) override;
     WEBCORE_EXPORT void notifyFinished(CachedResource&) override;
 
-    void responseReceived(const ResourceResponse&);
+    void responseReceived(const ResourceResponse&, CompletionHandler<void()>&&);
     void dataReceived(const char* data, int length);
 
     bool maybeLoadEmpty();
index 4f4be1a..e2197f9 100644 (file)
@@ -308,10 +308,13 @@ void DocumentThreadableLoader::dataSent(CachedResource& resource, unsigned long
     m_client->didSendData(bytesSent, totalBytesToBeSent);
 }
 
-void DocumentThreadableLoader::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void DocumentThreadableLoader::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, &resource == m_resource);
     didReceiveResponse(m_resource->identifier(), response);
+
+    if (completionHandler)
+        completionHandler();
 }
 
 void DocumentThreadableLoader::didReceiveResponse(unsigned long identifier, const ResourceResponse& response)
index 21c8ab6..c2bbc4d 100644 (file)
@@ -81,7 +81,7 @@ namespace WebCore {
 
         // CachedRawResourceClient
         void dataSent(CachedResource&, unsigned long long bytesSent, unsigned long long totalBytesToBeSent) override;
-        void responseReceived(CachedResource&, const ResourceResponse&) override;
+        void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) override;
         void dataReceived(CachedResource&, const char* data, int dataLength) override;
         void redirectReceived(CachedResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) override;
         void finishedTimingForWorkerLoad(CachedResource&, const ResourceTiming&) override;
index 3812b82..1482643 100644 (file)
@@ -139,9 +139,10 @@ void MediaResource::setDefersLoading(bool defersLoading)
         m_resource->setDefersLoading(defersLoading);
 }
 
-void MediaResource::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void MediaResource::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, &resource == m_resource);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
 
     if (!m_loader->document())
         return;
index a31df18..349277e 100644 (file)
@@ -82,7 +82,7 @@ public:
     bool didPassAccessControlCheck() const override { return m_didPassAccessControlCheck; }
 
     // CachedRawResourceClient
-    void responseReceived(CachedResource&, const ResourceResponse&) override;
+    void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) override;
     void redirectReceived(CachedResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) override;
     bool shouldCacheResponse(CachedResource&, const ResourceResponse&) override;
     void dataSent(CachedResource&, unsigned long long, unsigned long long) override;
index 8dff94d..864cd0e 100644 (file)
@@ -75,9 +75,10 @@ void ApplicationCacheResourceLoader::cancel(Error error)
     }
 }
 
-void ApplicationCacheResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void ApplicationCacheResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, &resource == m_resource);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
 
     if (response.httpStatusCode() == 404 || response.httpStatusCode() == 410) {
         cancel(Error::NotFound);
index 430a73e..72b2975 100644 (file)
@@ -54,7 +54,7 @@ private:
     explicit ApplicationCacheResourceLoader(unsigned, CachedResourceHandle<CachedRawResource>&&, CompletionHandler<void(ResourceOrError&&)>&&);
 
     // CachedRawResourceClient
-    void responseReceived(CachedResource&, const ResourceResponse&) final;
+    void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) final;
     void dataReceived(CachedResource&, const char* data, int dataLength) final;
     void redirectReceived(CachedResource&, ResourceRequest&&, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&&) final;
     void notifyFinished(CachedResource&) final;
index e7dbfcd..07f9d46 100644 (file)
@@ -158,6 +158,16 @@ void CachedRawResource::didAddClient(CachedResourceClient& c)
     iterateRedirects(CachedResourceHandle<CachedRawResource>(this), client, WTFMove(redirectsInReverseOrder), [this, protectedThis = CachedResourceHandle<CachedRawResource>(this), client = &client] (ResourceRequest&&) mutable {
         if (!hasClient(*client))
             return;
+        auto responseProcessedHandler = [this, protectedThis = WTFMove(protectedThis), client] {
+            if (!hasClient(*client))
+                return;
+            if (m_data)
+                client->dataReceived(*this, m_data->data(), m_data->size());
+            if (!hasClient(*client))
+                return;
+            CachedResource::didAddClient(*client);
+        };
+
         if (!m_response.isNull()) {
             ResourceResponse response(m_response);
             if (validationCompleting())
@@ -166,15 +176,9 @@ void CachedRawResource::didAddClient(CachedResourceClient& c)
                 ASSERT(!validationInProgress());
                 response.setSource(ResourceResponse::Source::MemoryCache);
             }
-            client->responseReceived(*this, response);
-        }
-        if (!hasClient(*client))
-            return;
-        if (m_data)
-            client->dataReceived(*this, m_data->data(), m_data->size());
-        if (!hasClient(*client))
-            return;
-        CachedResource::didAddClient(*client);
+            client->responseReceived(*this, response, WTFMove(responseProcessedHandler));
+        } else
+            responseProcessedHandler();
     });
 }
 
@@ -215,7 +219,7 @@ void CachedRawResource::responseReceived(const ResourceResponse& response)
     CachedResource::responseReceived(response);
     CachedResourceClientWalker<CachedRawResourceClient> w(m_clients);
     while (CachedRawResourceClient* c = w.next())
-        c->responseReceived(*this, m_response);
+        c->responseReceived(*this, m_response, nullptr);
 }
 
 bool CachedRawResource::shouldCacheResponse(const ResourceResponse& response)
index 758b7ad..547c689 100644 (file)
@@ -39,7 +39,12 @@ public:
     CachedResourceClientType resourceClientType() const override { return expectedType(); }
 
     virtual void dataSent(CachedResource&, unsigned long long /* bytesSent */, unsigned long long /* totalBytesToBeSent */) { }
-    virtual void responseReceived(CachedResource&, const ResourceResponse&) { }
+    virtual void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&& completionHandler)
+    {
+        if (completionHandler)
+            completionHandler();
+    }
+
     virtual bool shouldCacheResponse(CachedResource&, const ResourceResponse&) { return true; }
     virtual void dataReceived(CachedResource&, const char* /* data */, int /* length */) { }
     virtual void redirectReceived(CachedResource&, ResourceRequest&& request, const ResourceResponse&, CompletionHandler<void(ResourceRequest&&)>&& completionHandler) { completionHandler(WTFMove(request)); }
index 55beb7e..00c5505 100644 (file)
@@ -66,11 +66,14 @@ void KeepaliveRequestTracker::registerRequest(CachedResource& resource)
     resource.addClient(*this);
 }
 
-void KeepaliveRequestTracker::responseReceived(CachedResource& resource, const ResourceResponse&)
+void KeepaliveRequestTracker::responseReceived(CachedResource& resource, const ResourceResponse&, CompletionHandler<void()>&& completionHandler)
 {
     // Per Fetch specification, allocated quota should be returned before the promise is resolved,
     // which is when the response is received.
     unregisterRequest(resource);
+
+    if (completionHandler)
+        completionHandler();
 }
 
 void KeepaliveRequestTracker::notifyFinished(CachedResource& resource)
index 47da4ed..0c333db 100644 (file)
@@ -38,7 +38,7 @@ public:
     bool tryRegisterRequest(CachedResource&);
 
     // CachedRawResourceClient.
-    void responseReceived(CachedResource&, const ResourceResponse&) final;
+    void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) final;
     void notifyFinished(CachedResource&) final;
 
 private:
index 52eb744..9f92281 100644 (file)
@@ -111,9 +111,10 @@ void WebCoreAVCFResourceLoader::invalidate()
     });
 }
 
-void WebCoreAVCFResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void WebCoreAVCFResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, &resource == m_resource);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
 
     int status = response.httpStatusCode();
     if (status && (status < 200 || status > 299)) {
index dae06e1..1135bbb 100644 (file)
@@ -56,7 +56,7 @@ public:
 
 private:
     // CachedRawResourceClient
-    void responseReceived(CachedResource&, const ResourceResponse&) override;
+    void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) override;
     void dataReceived(CachedResource&, const char*, int) override;
     void notifyFinished(CachedResource&) override;
 
index f5e3740..eccdbd3 100644 (file)
@@ -56,7 +56,7 @@ public:
 
 private:
     // CachedResourceClient
-    void responseReceived(CachedResource&, const ResourceResponse&) override;
+    void responseReceived(CachedResource&, const ResourceResponse&, CompletionHandler<void()>&&) override;
     void dataReceived(CachedResource&, const char*, int) override;
     void notifyFinished(CachedResource&) override;
 
index 971c831..16b07e6 100644 (file)
@@ -108,9 +108,10 @@ void WebCoreAVFResourceLoader::invalidate()
     });
 }
 
-void WebCoreAVFResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response)
+void WebCoreAVFResourceLoader::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT_UNUSED(resource, &resource == m_resource);
+    CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
 
     int status = response.httpStatusCode();
     if (status && (status < 200 || status > 299)) {
index e5684b4..6161664 100644 (file)
@@ -1,3 +1,17 @@
+2018-03-16  Chris Dumez  <cdumez@apple.com>
+
+        WebKit.RestoreSessionStateContainingScrollRestorationDefault API test is failing with async policy delegates
+        https://bugs.webkit.org/show_bug.cgi?id=183679
+
+        Reviewed by Alex Christensen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKit/RestoreSessionState.cpp:
+        (TestWebKitAPI::decidePolicyForNavigationAction):
+        (TestWebKitAPI::decidePolicyForResponse):
+        (TestWebKitAPI::TEST):
+
 2018-03-16  Zalan Bujtas  <zalan@apple.com>
 
         [LayoutReloaded] Introduce Display.Box
index 825201e..fc7942a 100644 (file)
@@ -32,6 +32,7 @@
 #include "PlatformWebView.h"
 #include "Test.h"
 #include <WebKit/WKSessionStateRef.h>
+#include <wtf/RunLoop.h>
 
 namespace TestWebKitAPI {
 
@@ -42,6 +43,22 @@ static void didFinishLoadForFrame(WKPageRef, WKFrameRef, WKTypeRef, const void*)
     didFinishLoad = true;
 }
 
+static void decidePolicyForNavigationAction(WKPageRef page, WKFrameRef frame, WKFrameNavigationType navigationType, WKEventModifiers modifiers, WKEventMouseButton mouseButton, WKFrameRef originatingFrame, WKURLRequestRef request, WKFramePolicyListenerRef listener, WKTypeRef userData, const void* clientInfo)
+{
+    WKRetainPtr<WKFramePolicyListenerRef> retainedListener(listener);
+    RunLoop::main().dispatch([retainedListener = WTFMove(retainedListener)] {
+        WKFramePolicyListenerUse(retainedListener.get());
+    });
+}
+
+static void decidePolicyForResponse(WKPageRef page, WKFrameRef frame, WKURLResponseRef response, WKURLRequestRef request, bool canShowMIMEType, WKFramePolicyListenerRef listener, WKTypeRef userData, const void* clientInfo)
+{
+    WKRetainPtr<WKFramePolicyListenerRef> retainedListener(listener);
+    RunLoop::main().dispatch([retainedListener = WTFMove(retainedListener)] {
+        WKFramePolicyListenerUse(retainedListener.get());
+    });
+}
+
 static void setPageLoaderClient(WKPageRef page)
 {
     WKPageLoaderClientV0 loaderClient;
@@ -84,6 +101,31 @@ TEST(WebKit, RestoreSessionStateContainingScrollRestorationDefault)
     EXPECT_JS_EQ(webView.page(), "history.scrollRestoration", "auto");
 }
 
+TEST(WebKit, RestoreSessionStateContainingScrollRestorationDefaultWithAsyncPolicyDelegates)
+{
+    WKRetainPtr<WKContextRef> context(AdoptWK, WKContextCreate());
+
+    PlatformWebView webView(context.get());
+    setPageLoaderClient(webView.page());
+
+    WKPagePolicyClientV1 policyClient;
+    memset(&policyClient, 0, sizeof(policyClient));
+    policyClient.base.version = 1;
+    policyClient.decidePolicyForNavigationAction = decidePolicyForNavigationAction;
+    policyClient.decidePolicyForResponse = decidePolicyForResponse;
+    WKPageSetPagePolicyClient(webView.page(), &policyClient.base);
+
+    WKRetainPtr<WKDataRef> data = createSessionStateData(context.get());
+    EXPECT_NOT_NULL(data);
+
+    auto sessionState = adoptWK(WKSessionStateCreateFromData(data.get()));
+    WKPageRestoreFromSessionState(webView.page(), sessionState.get());
+
+    Util::run(&didFinishLoad);
+
+    EXPECT_JS_EQ(webView.page(), "history.scrollRestoration", "auto");
+}
+
 } // namespace TestWebKitAPI
 
 #endif