Disk cache should support Vary: Cookie
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2015 16:09:06 +0000 (16:09 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Mar 2015 16:09:06 +0000 (16:09 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142770
Source/WebKit2:

rdar://problem/19764945

Reviewed by Anders Carlsson.

Cookies are not part of the original request but are added by the networking layer when submitting the request.
Fetch them explicitly when resolving Vary: Cookie.

The implementation is not perfect as it fetches the cookie for the cache entry when saving a Vary:Cookie response,
not when making the request. In principle the cookie may have changed in-between. This should be enough to handle
reasonable cases though. Fetching cookies for every request might be too expensive for this rarely used feature.

* NetworkProcess/cache/NetworkCache.cpp:
(WebKit::NetworkCache::headerValueForVary):
(WebKit::NetworkCache::encodeStorageEntry):
(WebKit::NetworkCache::verifyVaryingRequestHeaders):

LayoutTests:

Reviewed by Anders Carlsson.

* http/tests/cache/disk-cache-vary-cookie-expected.txt: Added.
* http/tests/cache/disk-cache-vary-cookie.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/disk-cache-vary-cookie-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cache/disk-cache-vary-cookie.html [new file with mode: 0644]
LayoutTests/platform/mac-wk1/TestExpectations
LayoutTests/platform/win/TestExpectations
Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp

index e9da67e..b0422f9 100644 (file)
@@ -1,3 +1,13 @@
+2015-03-17  Antti Koivisto  <antti@apple.com>
+
+        Disk cache should support Vary: Cookie
+        https://bugs.webkit.org/show_bug.cgi?id=142770
+
+        Reviewed by Anders Carlsson.
+
+        * http/tests/cache/disk-cache-vary-cookie-expected.txt: Added.
+        * http/tests/cache/disk-cache-vary-cookie.html: Added.
+
 2015-03-16  Ryosuke Niwa  <rniwa@webkit.org>
 
         Enable ES6 classes by default
diff --git a/LayoutTests/http/tests/cache/disk-cache-vary-cookie-expected.txt b/LayoutTests/http/tests/cache/disk-cache-vary-cookie-expected.txt
new file mode 100644 (file)
index 0000000..8ee540c
--- /dev/null
@@ -0,0 +1,37 @@
+Test that Vary: Cookie in response is handled by the disk cache.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Setting cookie and loading
+response headers: {"Cache-control":"max-age=100"}
+response source: Network
+
+response headers: {"Vary":"Cookie","Cache-control":"max-age=100"}
+response source: Network
+
+Loading again
+response headers: {"Cache-control":"max-age=100"}
+response source: Disk cache
+
+response headers: {"Vary":"Cookie","Cache-control":"max-age=100"}
+response source: Disk cache
+
+Changing cookie and loading
+response headers: {"Cache-control":"max-age=100"}
+response source: Disk cache
+
+response headers: {"Vary":"Cookie","Cache-control":"max-age=100"}
+response source: Network
+
+Loading again
+response headers: {"Cache-control":"max-age=100"}
+response source: Disk cache
+
+response headers: {"Vary":"Cookie","Cache-control":"max-age=100"}
+response source: Disk cache
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cache/disk-cache-vary-cookie.html b/LayoutTests/http/tests/cache/disk-cache-vary-cookie.html
new file mode 100644 (file)
index 0000000..1361e7c
--- /dev/null
@@ -0,0 +1,38 @@
+<script src="/js-test-resources/js-test-pre.js"></script>
+<script src="resources/cache-test.js"></script>
+<body>
+<script>
+
+var tests =
+[
+  { responseHeaders: {'Cache-control': 'max-age=100'} },
+  { responseHeaders: {'Vary': 'Cookie', 'Cache-control': 'max-age=100'} },
+];
+
+description("Test that Vary: Cookie in response is handled by the disk cache.");
+
+debug("Setting cookie and loading");
+document.cookie = "cookie=value";
+loadResources(tests, function () {
+    printResults(tests);
+    internals.clearMemoryCache();
+    debug("Loading again");
+    loadResources(tests, function () {
+        printResults(tests);
+        internals.clearMemoryCache();
+        debug("Changing cookie and loading");
+        document.cookie = "cookie=othervalue";
+        loadResources(tests, function () {
+            printResults(tests);
+            internals.clearMemoryCache()
+            debug("Loading again");
+            loadResources(tests, function () {
+               printResults(tests);
+               finishJSTest();
+            });
+        });
+    });
+});
+
+</script>
+<script src="/js-test-resources/js-test-post.js"></script>
index ebb28cb..ea19d2f 100644 (file)
@@ -94,6 +94,7 @@ compositing/iframes/overlapped-nested-iframes.html [ Pass Failure ]
 # Disk cache is WK2 only
 http/tests/cache/disk-cache-validation.html
 http/tests/cache/disk-cache-disable.html
+http/tests/cache/disk-cache-vary-cookie.html
 
 ### END OF (2) Failures without bug reports
 ########################################
index 7f3ff28..a9c993d 100644 (file)
@@ -2222,6 +2222,7 @@ webkit.org/b/60206 http/tests/navigation/response204.html [ Failure ]
 # Disk cache is WK2 only
 http/tests/cache/disk-cache-validation.html
 http/tests/cache/disk-cache-disable.html
+http/tests/cache/disk-cache-vary-cookie.html
 
 # The following are unreviewed:
 http/tests/cache/content-type-ignored-during-revalidation.html [ Failure ]
index 9eaa675..cf5e157 100644 (file)
@@ -1,3 +1,23 @@
+2015-03-17  Antti Koivisto  <antti@apple.com>
+
+        Disk cache should support Vary: Cookie
+        https://bugs.webkit.org/show_bug.cgi?id=142770
+        rdar://problem/19764945
+
+        Reviewed by Anders Carlsson.
+
+        Cookies are not part of the original request but are added by the networking layer when submitting the request.
+        Fetch them explicitly when resolving Vary: Cookie.
+
+        The implementation is not perfect as it fetches the cookie for the cache entry when saving a Vary:Cookie response,
+        not when making the request. In principle the cookie may have changed in-between. This should be enough to handle
+        reasonable cases though. Fetching cookies for every request might be too expensive for this rarely used feature.
+
+        * NetworkProcess/cache/NetworkCache.cpp:
+        (WebKit::NetworkCache::headerValueForVary):
+        (WebKit::NetworkCache::encodeStorageEntry):
+        (WebKit::NetworkCache::verifyVaryingRequestHeaders):
+
 2015-03-17  Zan Dobersek  <zdobersek@igalia.com>
 
         [WK2] Use C++ lambdas in IPC::Connection
index ffd574f..571641e 100644 (file)
@@ -38,6 +38,8 @@
 #include <WebCore/CacheValidation.h>
 #include <WebCore/FileSystem.h>
 #include <WebCore/HTTPHeaderNames.h>
+#include <WebCore/NetworkStorageSession.h>
+#include <WebCore/PlatformCookieJar.h>
 #include <WebCore/ResourceResponse.h>
 #include <WebCore/SharedBuffer.h>
 #include <wtf/NeverDestroyed.h>
@@ -97,6 +99,17 @@ static Key makeCacheKey(const WebCore::ResourceRequest& request)
     return { request.httpMethod(), partition, request.url().string()  };
 }
 
+static String headerValueForVary(const WebCore::ResourceRequest& request, const String& headerName)
+{
+    // Explicit handling for cookies is needed because they are added magically by the networking layer.
+    // FIXME: The value might have changed between making the request and retrieving the cookie here.
+    // We could fetch the cookie when making the request but that seems overkill as the case is very rare and it
+    // is a blocking operation. This should be sufficient to cover reasonable cases.
+    if (headerName == httpHeaderNameString(WebCore::HTTPHeaderName::Cookie))
+        return WebCore::cookieRequestHeaderFieldValue(WebCore::NetworkStorageSession::defaultStorageSession(), request.firstPartyForCookies(), request.url());
+    return request.httpHeaderField(headerName);
+}
+
 static Storage::Entry encodeStorageEntry(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, PassRefPtr<WebCore::SharedBuffer> responseData)
 {
     Encoder encoder;
@@ -114,7 +127,8 @@ static Storage::Entry encodeStorageEntry(const WebCore::ResourceRequest& request
         Vector<std::pair<String, String>> varyingRequestHeaders;
         for (auto& varyHeaderName : varyingHeaderNames) {
             String headerName = varyHeaderName.stripWhiteSpace();
-            varyingRequestHeaders.append(std::make_pair(headerName, request.httpHeaderField(headerName)));
+            String headerValue = headerValueForVary(request, headerName);
+            varyingRequestHeaders.append(std::make_pair(headerName, headerValue));
         }
         encoder << varyingRequestHeaders;
     }
@@ -135,8 +149,8 @@ static bool verifyVaryingRequestHeaders(const Vector<std::pair<String, String>>&
         // FIXME: Vary: * in response would ideally trigger a cache delete instead of a store.
         if (varyingRequestHeader.first == "*")
             return false;
-        String requestValue = request.httpHeaderField(varyingRequestHeader.first);
-        if (requestValue != varyingRequestHeader.second)
+        String headerValue = headerValueForVary(request, varyingRequestHeader.first);
+        if (headerValue != varyingRequestHeader.second)
             return false;
     }
     return true;