Cached "Expires" header is not updated upon successful resource revalidation
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Mar 2015 22:47:05 +0000 (22:47 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 30 Mar 2015 22:47:05 +0000 (22:47 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143228
<rdar://problem/20348059>

Reviewed by Antti Koivisto.

Source/WebCore:

Cached "Expires" header was not updated upon successful resource
revalidation. This affected both our disk cache and our memory cache.
This was caused by shouldUpdateHeaderAfterRevalidation() in
CacheValidation.cpp returning false for the "Expires" header.

There is a comment there stating that the list of ignored headers
matches Chromium's net library but that's not the case, at least not
anymore:
http://osxr.org/android/source/external/chromium/net/http/http_response_headers.cc

HTTP servers such as Apache return an "Expires" header in their 304
responses and the "Expires" header is potentially a new one. However,
our caches were ignoring the updated expiration date and kept using the
old one, which meant that the cached resource expired sooner than it
should have.

See the following Apache bugs that explain the issue:
https://bz.apache.org/bugzilla/show_bug.cgi?id=24884
https://bz.apache.org/bugzilla/show_bug.cgi?id=25123

Test: http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html

* platform/network/CacheValidation.cpp:

LayoutTests:

Add layout test to check that a cached response's "Expires" header is
updated from the 304 response's headers upon successful revalidation.

* http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header-expected.txt: Added.
* http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html: Added.
* http/tests/cache/disk-cache/resources/cache-test.js:
(generateTestURL):
(loadResource):
* http/tests/cache/disk-cache/resources/generate-response.cgi:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html [new file with mode: 0644]
LayoutTests/http/tests/cache/disk-cache/resources/cache-test.js
LayoutTests/http/tests/cache/disk-cache/resources/generate-response.cgi
Source/WebCore/ChangeLog
Source/WebCore/platform/network/CacheValidation.cpp

index fcfe9e5..721af56 100644 (file)
@@ -1,3 +1,21 @@
+2015-03-30  Chris Dumez  <cdumez@apple.com>
+
+        Cached "Expires" header is not updated upon successful resource revalidation
+        https://bugs.webkit.org/show_bug.cgi?id=143228
+        <rdar://problem/20348059>
+
+        Reviewed by Antti Koivisto.
+
+        Add layout test to check that a cached response's "Expires" header is
+        updated from the 304 response's headers upon successful revalidation.
+
+        * http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header-expected.txt: Added.
+        * http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html: Added.
+        * http/tests/cache/disk-cache/resources/cache-test.js:
+        (generateTestURL):
+        (loadResource):
+        * http/tests/cache/disk-cache/resources/generate-response.cgi:
+
 2015-03-30  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Regression: Preview for [[null]] shouldn't be []
diff --git a/LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header-expected.txt b/LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header-expected.txt
new file mode 100644 (file)
index 0000000..754f8cd
--- /dev/null
@@ -0,0 +1,19 @@
+Test that the 'Expires' header is updated upon successful validation.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+
+response headers: {"Expires":"now(0)","ETag":"match"}
+response's 'Expires' header is overriden by future date in 304 response
+response source: Disk cache after validation
+
+304 response included an 'Expires' header in the future, so we should not need to revalidate this time.
+response headers: {"Expires":"now(0)","ETag":"match"}
+response's 'Expires' header is overriden by future date in 304 response
+response source: Disk cache
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html b/LayoutTests/http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html
new file mode 100644 (file)
index 0000000..033ca0f
--- /dev/null
@@ -0,0 +1,21 @@
+<script src="/js-test-resources/js-test-pre.js"></script>
+<script src="resources/cache-test.js"></script>
+<body>
+<script>
+
+var tests =
+[
+ { responseHeaders: {'Expires': 'now(0)', 'ETag': 'match' }, expiresInFutureIn304: true },
+];
+
+description("Test that the 'Expires' header is updated upon successful validation.");
+
+debug("");
+
+runTests(tests, function() {
+    debug("304 response included an 'Expires' header in the future, so we should not need to revalidate this time.");
+    runTests(tests);
+});
+
+</script>
+<script src="/js-test-resources/js-test-post.js"></script>
index 314689b..150d499 100644 (file)
@@ -38,12 +38,15 @@ function makeHeaderValue(value)
     return value;
 }
 
-function generateTestURL(test, includeBody)
+function generateTestURL(test, includeBody, expiresInFutureIn304)
 {
     includeBody = typeof includeBody !== 'undefined' ? includeBody : true;
+    expiresInFutureIn304 = typeof expiresInFutureIn304 !== 'undefined' ? expiresInFutureIn304 : false;
     var uniqueTestId = Math.floor((Math.random() * 1000000000000));
-    var cgi_script = "resources/generate-response.cgi?include-body=" + (includeBody ? "1" : "0");
-    var testURL = cgi_script + "&uniqueId=" + uniqueTestId++ + "&Content-type=text/plain";
+    var testURL = "resources/generate-response.cgi?include-body=" + (includeBody ? "1" : "0");
+    if (expiresInFutureIn304)
+        testURL += "&expires-in-future-in-304=1";
+    testURL += "&uniqueId=" + uniqueTestId++ + "&Content-type=text/plain";
     for (var header in test.responseHeaders)
         testURL += '&' + header + '=' + makeHeaderValue(test.responseHeaders[header]);
     return testURL;
@@ -52,7 +55,7 @@ function generateTestURL(test, includeBody)
 function loadResource(test, onload)
 {
     if (!test.url)
-        test.url = generateTestURL(test, test.includeBody);
+        test.url = generateTestURL(test, test.includeBody, test.expiresInFutureIn304);
 
     test.xhr = new XMLHttpRequest();
     test.xhr.onload = onload;
@@ -81,6 +84,8 @@ function printResults(tests)
     for (var i = 0; i < tests.length; ++i) {
         var test = tests[i];
         debug("response headers: " + JSON.stringify(test.responseHeaders));
+        if (test.expiresInFutureIn304)
+            debug("response's 'Expires' header is overriden by future date in 304 response");
         if (test.requestHeaders)
             debug("request headers: " + JSON.stringify(test.requestHeaders));
         responseSource = internals.xhrResponseSource(test.xhr);
index ca05fbf..130d46d 100755 (executable)
@@ -6,17 +6,24 @@ use HTTP::Date;
 my $query = new CGI;
 @names = $query->param;
 my $includeBody = $query->param('include-body') || 0;
+my $expiresInFutureIn304 = $query->param('expires-in-future-in-304') || 0;
 
 my $hasStatusCode = 0;
+my $hasExpiresHeader = 0;
 if ($query->http && $query->http("If-None-Match") eq "match") {
     print "Status: 304\n";
     $hasStatusCode = 1;
+    if ($expiresInFutureIn304) {
+        print "Expires: " . HTTP::Date::time2str(time() + 100) . "\n";
+        $hasExpiresHeader = 1;
+    }
 }
 
 foreach (@names) {
     next if ($_ eq "uniqueId");
     next if ($_ eq "include-body");
     next if ($_ eq "Status" and $hasStatusCode);
+    next if ($_ eq "Expires" and $hasExpiresHeader);
     print $_ . ": " . $query->param($_) . "\n";
 }
 print "\n";
index e7f3dad..6ffb29e 100644 (file)
@@ -1,3 +1,35 @@
+2015-03-30  Chris Dumez  <cdumez@apple.com>
+
+        Cached "Expires" header is not updated upon successful resource revalidation
+        https://bugs.webkit.org/show_bug.cgi?id=143228
+        <rdar://problem/20348059>
+
+        Reviewed by Antti Koivisto.
+
+        Cached "Expires" header was not updated upon successful resource
+        revalidation. This affected both our disk cache and our memory cache.
+        This was caused by shouldUpdateHeaderAfterRevalidation() in
+        CacheValidation.cpp returning false for the "Expires" header.
+
+        There is a comment there stating that the list of ignored headers
+        matches Chromium's net library but that's not the case, at least not
+        anymore:
+        http://osxr.org/android/source/external/chromium/net/http/http_response_headers.cc
+
+        HTTP servers such as Apache return an "Expires" header in their 304
+        responses and the "Expires" header is potentially a new one. However,
+        our caches were ignoring the updated expiration date and kept using the
+        old one, which meant that the cached resource expired sooner than it
+        should have.
+
+        See the following Apache bugs that explain the issue:
+        https://bz.apache.org/bugzilla/show_bug.cgi?id=24884
+        https://bz.apache.org/bugzilla/show_bug.cgi?id=25123
+
+        Test: http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html
+
+        * platform/network/CacheValidation.cpp:
+
 2015-03-30  Antti Koivisto  <antti@apple.com>
 
         Don't cache resources that are very unlikely to be reused
index f8b6633..3d17248 100644 (file)
@@ -39,7 +39,6 @@ const char* const headersToIgnoreAfterRevalidation[] = {
     "allow",
     "connection",
     "etag",
-    "expires",
     "keep-alive",
     "last-modified"
     "proxy-authenticate",