Bug 71509 - WebKit memory cache doesn't respect Vary header
Summary: WebKit memory cache doesn't respect Vary header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 158518
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-03 14:18 PDT by Andrew Berry
Modified: 2016-06-08 06:36 PDT (History)
17 users (show)

See Also:


Attachments
patch (20.38 KB, patch)
2016-06-06 08:42 PDT, Antti Koivisto
cdumez: review+
Details | Formatted Diff | Diff
alternative patch (24.44 KB, patch)
2016-06-06 11:50 PDT, Antti Koivisto
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (864.77 KB, application/zip)
2016-06-06 13:53 PDT, Build Bot
no flags Details
vary header implementation for memory cache (24.59 KB, patch)
2016-06-06 13:59 PDT, Antti Koivisto
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Berry 2011-11-03 14:18:00 PDT
I'm running into a problem where Webkit doesn't appear to be respecting the Vary: Cookie header when Cache-Control is set. For example:

HTTP/1.1 200 OK
Server: Apache/2.2.3 (CentOS)
X-Powered-By: PHP/5.2.17
Cache-Control: public, max-age=300
Last-Modified: Thu, 03 Nov 2011 21:09:15 +0000
Expires: Sun, 11 Mar 1984 12:00:00 GMT
Vary: Cookie
ETag: "1320354555"
Content-Type: text/html; charset=utf-8
Content-Length: 23080
Date: Thu, 03 Nov 2011 21:09:15 GMT
X-Varnish: 1635548510
Age: 0
Via: 1.1 varnish
Connection: keep-alive
X-Varnish-Cache: MISS

This is causing pages previously loaded to only hit the server if a manual reload is done, or if 300 seconds pass.

To reproduce:

1. View a few pages as an anonymous user where the above Cache-Control and Vary headers are set.
2. Log into the site, such that a cookie is set the headers are now something like:

Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0

3. Browse back to one of the pages you previously loaded. Webkit won't make an outbound HTTP request even though the cookie has changed.

I've verified this in Webkit nightlies in both OS X and Windows, and haven't run into this problem in any other browsers.
Comment 1 Robert Brown 2011-11-17 13:15:28 PST
I am having the same issue, specifically with Safari 5.1.1 (7534.51.22) for both MacOS and Windows. Chrome does not exhibit this behavior.

I can reproduce as follows.

1) Visit a webpage, /myurl. Returned headers are as follows:
Cache-Control: public, max-age=3600
Expires: Sun, 11 Mar 1984 12:00:00 GMT
Vary: Cookie,Accept-Encoding

Note that there are no cookies presented to the client at this time.

2) Authenticate to the site. A single session cookie is created for the client browser. During the authentication process, the user is redirected to a different URL. 

IE, during this step a POST is made to the same URL, and it receives a 302 Found to redirect to a different URL. I have verified at this point that the session cookie has been properly created.

3) Navigate back to the original page, /myurl. Even though a cookie is present in the browser and the URL had Vary:Cookie set, the browser serves up a cached webpage. I have run a network sniffer and verified that it does not make a connection back to the server for a new page.

I did verify that if I modified the original Cache-Control header to no-cache, this does not happen. So it definitely has to do with Cache-Control and Vary.

I can reproduce this 100% of the time, and I'd be happy to provide a test login to my site if you have an interest in viewing the details. I also have network sniffer captures and ngrep output.
Comment 2 Robert Brown 2011-11-17 13:19:50 PST
FWIW, I also tested this with the latest Webkit build, 5.1.1 7534.51.22, r100547 and this behavior still happens. So it does not seem like it has been fixed along the way.
Comment 3 Robert Brown 2011-11-17 17:15:39 PST
(In reply to comment #2)
> FWIW, I also tested this with the latest Webkit build, 5.1.1 7534.51.22, r100547 and this behavior still happens. So it does not seem like it has been fixed along the way.

If anyone else is trying to work around this with Varnish, here's what I did. I'm stripping out the Cache-Control header to ensure that Webkit does not improperly cache it. I only have one page that presents a real problem, hence the && req.url.

sub vcl_fetch {
  if (req.http.user-agent ~ "Safari" && req.url == "/myurl") {
    unset beresp.http.Cache-Control;
    set beresp.http.Cache-Control = "no-cache";
  }
}
Comment 4 Andrew Berry 2011-11-18 14:25:10 PST
I'm pretty sure that you don't need to unset the cache-control header first. At least, it's working fine for me.

Also remember to separate out your cache bins. Otherwise, if Safari hits a page first, all clients will be presented with the no-cache directive.

Finally, Chrome contains the Safari keyword in it's user agent as well. I'm sure there are other Webkit derivatives that have Safari in the UA, but I'm erring on the side of caution with them.

In sub vcl_fetch:
  # Force Safari to always check the server as it doesn't respect Vary: cookie.
  # See https://bugs.webkit.org/show_bug.cgi?id=71509
  if (req.http.user-agent ~ "Safari" && !req.http.user-agent ~ "Chrome") {
    set beresp.http.cache-control = "max-age: 0";
  }

In sub vcl_hash:
  # Ensure we have a separate bin for Safari as it has a different cache-conrol
  # header.
  if (req.http.user-agent ~ "Safari" && !req.http.user-agent ~ "Chrome") {
    set req.hash += "safari-disable-cache-control";
  }
Comment 5 bill 2012-03-21 11:19:45 PDT
The root of the problem seems to be that safari doesn't send any cookies in the request.

I'm able to duplicate the problem but when I hit the other page that i've previously hit without being logged in,   Safari sends a request without any cookies.  This causes the proxy (varnish in our case) to send me a cache hit since there are no cookies and it doesn't think I have a session.

from safari:
Request URL:https://www.pdup.allplayers.com/group_search
Request Method:GET
Status Code:200 OK
Request Headers
Accept:text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Referer:https://www.pdup.allplayers.com/group_search
User-Agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/534.54.16 (KHTML, like Gecko) Version/5.1.4 Safari/534.54.16

This is after login so there are a number of Cookies that have been set on this domain.

Here with same scenerio what I got on chrome (which worked fine):
Request URL:https://www.pdup.allplayers.com/group_search
Request Method:GET
Status Code:200 OK
Request Headers
Accept:text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Charset:ISO-8859-1,utf-8;q=0.7,*;q=0.3
Accept-Encoding:gzip,deflate,sdch
Accept-Language:en-US,en;q=0.8
Connection:keep-alive
Cookie:SESSe74701e02960fa02cd447c1478f7abe1=ee4235eac16a7104698da11cb0ee1231; CHOCOLATECHIPSSLb4db41620f5fb578b70bec49d4025279=0KDzOgVQrwzDuGc%2FaqzD%2FgSMFuUAlD7lOn8oL2E07HwhG%2BlyfKjNejZSXg9Z2QCJPGtL08bz3s1wwPts2RvzxxcZe6yCOZ5QhpSXTfvcrauNnfZVGp1nyKAh5q%2Fa651iB6QnByNXSw26THXReIVHBWyOcAuAAg85oq4ac9LPijuF3hdIBOI3fvBdjTfjrrKmRovgDtnjjtxeEPuSy7LjRduAP5v%2BeAGtgGVMFxCG07mhSdt0gl84Zgl1zJdQEjN%2FViHR3HzBzU5Jf23oWMAjNoAoOf76OW7qmRXJIIEyJiIdImdJd%2BbjeF54wA7jbf2Ye0oueYehoBFVqcUzggCJE8RZoSQxGdPDnH6OMRsmlC0U0F%2BfuE5pwwRV%2B9%2Fv3%2BqS; __utma=181778935.695123550.1332169001.1332180843.1332184029.3; __utmc=181778935; __utmz=181778935.1332169001.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none); SESSc911711ef8706eaf3847304b167bf69d=7c0aba40cd2cfac4336206df312018e3; CHOCOLATECHIPSSL1ad866034ea2b30f49d3f9fc345b7170=j7J%2BBjfnwH%2BHSmRSI3HAoTLI4C4x3OA4oDCz6Jk2TSh1ekZzojRnz09dD0h9qtSpMZjDdNJHB4YnGc6yBQIqE9WTAH0g5ycNH%2BUWnavg8uUdb%2BvU96nok1NruqPElke6NuBFntubUXwW4g5EPiChLSMAbPfnwZxfSMJHKmhApiEeSZG3Y%2FS%2B8FDLXKFuuflIH8uQ63yYRUQ8bGHUjH6n5X7i5jH46%2F%2FO%2FN9GBShL38tltcIT4TQF3OVzVRPvO7ClKxUaJlcZNiFjzrZH%2FQZZEkbgby0QF%2B9Z0FVBkE1N1ZyTyrAtzWnXno8WpQLky9KW1aHbrZDpZZECYh3g%2BsXaNJEQqgyyXcrWMrr2z16gBz7%2BstRDD9Q8FlFJD0ESd9pl2pEHizLEvUvgG9%2BPu6fX0w%3D%3D; __utma=24126587.930127860.1332169084.1332184202.1332350831.4; __utmb=24126587.11.10.1332350831; __utmc=24126587; __utmz=24126587.1332169084.1.1.utmcsr=(direct)|utmccn=(direct)|utmcmd=(none)
Host:www.pdup.allplayers.com
If-Modified-Since:Wed, 21 Mar 2012 17:27:35 +0000
If-None-Match:"1332350855"
Referer:https://www.pdup.allplayers.com/g/_gear_wildcats/registration_complete
User-Agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.79 Safari/535.11
Comment 6 bill 2012-03-21 11:46:24 PDT
the default.vcl shown in comment #4 

does resolve the issue on our server setup.  It essentially makes it so that safari always says give me a new page don't allow it to be cached.  I guess the inspect element looking at headers on safari don't show the full header, as it must be sending cookies even though they are not shown on the inspect element web kit, but are shown on chrome.

thanks   Andrew Berry for the tips.
Comment 7 Silvia Pfeiffer 2014-11-09 15:59:58 PST
Note there is a similar issue with byte range requests: https://bugs.webkit.org/show_bug.cgi?id=82672
Comment 8 Alexey Proskuryakov 2014-11-09 16:11:27 PST
Yes, these are both deficiencies of WebCore memory cache. Unfortunately, there is no way to disable WebCore memory cache, but keep the more correct CFNetwork cache.
Comment 9 Antti Koivisto 2015-06-15 05:36:21 PDT
Could someone provide an actual repro URL for this bug? The only repro I have seen starts with "1. Install Drupal core."
Comment 10 Andrew Berry 2015-07-02 11:00:29 PDT
This might be fixed now - I can't reproduce this on Safari 8.0.7. Can anyone else?
Comment 11 ruv 2015-08-23 09:14:34 PDT
(In reply to comment #10)
> This might be fixed now - I can't reproduce this on Safari 8.0.7.

Some of our users have faced this issue using Safari 8.0.7.
Could you please check the bug via the following testcase:
https://iafastro.directory/iac/testcase/cache-vary/

This script measures time to check whether the page was loaded from a cache after cookie is changed.
Comment 12 Antti Koivisto 2015-08-23 12:01:56 PDT
Safari 9.0 on El Capitan passes the test so I think this has been fixed by the new cache code.
Comment 13 ruv 2015-08-23 17:07:31 PDT
(In reply to comment #12)
Antti, could you please refresh the page and run the test again by press Auto-test button?
I have added comparison the current and the sent cookie to make the test more accurate.
Comment 14 Antti Koivisto 2015-08-24 05:33:23 PDT
Ok, the updated test case still fails.
Comment 15 Antti Koivisto 2015-08-24 05:33:50 PDT
Thanks for the test!
Comment 16 Damien McKenna 2016-02-27 07:43:48 PST
FYI Safari 9.0.3 on OSX 10.11.3 passes the test.
Comment 17 Antti Koivisto 2016-06-06 08:42:39 PDT
Created attachment 280597 [details]
patch
Comment 18 Chris Dumez 2016-06-06 08:47:21 PDT
Comment on attachment 280597 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280597&action=review

> Source/WebCore/loader/cache/CachedResource.cpp:762
> +        // If Vary header is present don't use the memory cache entry. Instead fall back to disk cache that knows how to handle it.

This comment seems a bit odd. In case of memory cache revalidation, we bypass the disk cache.
Comment 19 Chris Dumez 2016-06-06 08:53:56 PDT
Comment on attachment 280597 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280597&action=review

R=me although I wish we simply implemented basic Vary support in the memory cache.

>> Source/WebCore/loader/cache/CachedResource.cpp:762
>> +        // If Vary header is present don't use the memory cache entry. Instead fall back to disk cache that knows how to handle it.
> 
> This comment seems a bit odd. In case of memory cache revalidation, we bypass the disk cache.

Oh, I see that you worked around this by tweaking canUseCacheValidator() above. Ok.
Comment 20 Sam Weinig 2016-06-06 08:57:49 PDT
How much additional complex it would it be to implement full vary support?
Comment 21 Antti Koivisto 2016-06-06 09:04:09 PDT
> R=me although I wish we simply implemented basic Vary support in the memory
> cache.

There is not really "basic" Vary support, it needs to be fully implemented. I don't think it is worth the complexity to move the logic to WebCore. Similarly I don't think memory cache should handle redirects (as it currently does).
Comment 22 Antti Koivisto 2016-06-06 09:15:00 PDT
(In reply to comment #20)
> How much additional complex it would it be to implement full vary support?

It would require moving the logic from network cache code in WK2 to WebCore. Memory cache entries would need to contain the varying header values. 

The biggest problem is "Vary:Cookie" (which also the most common use) as cookie header is not visible in ResourceRequest and gets added magically behind the scenes by CFNetwork. Getting the value from web process involves synchronous IPC to network process and synchronous cookie storage read. Making this not a performance risk is tricky.
Comment 23 Radar WebKit Bug Importer 2016-06-06 09:32:44 PDT
<rdar://problem/26651033>
Comment 24 Antti Koivisto 2016-06-06 11:50:48 PDT
Created attachment 280613 [details]
alternative patch

Here is a full implementation. It is not too bad. We only need to fetch cookies when we actually see Vary:Cookie so maybe that is not a real issue.
Comment 25 Build Bot 2016-06-06 13:52:59 PDT
Comment on attachment 280613 [details]
alternative patch

Attachment 280613 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1454339

New failing tests:
http/tests/navigation/redirect-to-random-url-versus-memory-cache.html
Comment 26 Build Bot 2016-06-06 13:53:03 PDT
Created attachment 280627 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 27 Antti Koivisto 2016-06-06 13:59:07 PDT
Created attachment 280630 [details]
vary header implementation for memory cache
Comment 28 Ryosuke Niwa 2016-06-06 16:10:16 PDT
Comment on attachment 280630 [details]
vary header implementation for memory cache

View in context: https://bugs.webkit.org/attachment.cgi?id=280630&action=review

> LayoutTests/ChangeLog:12
> +2016-06-06  Antti Koivisto  <antti@apple.com>

Double change logs.
Comment 29 Antti Koivisto 2016-06-08 06:36:10 PDT
https://trac.webkit.org/r201805