| Summary: | [Cocoa] Treat Epoch as invalid value for "Last-Modified" header | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
| Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | ap, barraclough, buildbot, cgarcia, commit-queue, koivisto, rniwa, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | Other | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Chris Dumez
2015-08-18 21:08:40 PDT
Created attachment 259350 [details]
Patch
Comment on attachment 259350 [details] Patch Attachment 259350 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/74299 New failing tests: http/tests/cache/disk-cache/disk-cache-404-status-code.html http/tests/cache/disk-cache/disk-cache-204-status-code.html http/tests/cache/disk-cache/disk-cache-307-status-code.html Created attachment 259355 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Is this what other browsers do? Comment on attachment 259350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259350&action=review > Source/WebCore/platform/network/CacheValidation.cpp:134 > + const std::chrono::system_clock::time_point epoch; You can leave out std::chrono. This change will affect memory cache too. I wonder if this change (if we do it) should be disk cache specific. Created attachment 259374 [details]
Patch
(In reply to comment #6) > Comment on attachment 259350 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259350&action=review > > > Source/WebCore/platform/network/CacheValidation.cpp:134 > > + const std::chrono::system_clock::time_point epoch; > > You can leave out std::chromo. Done. > > This change will affect memory cache too. I wonder if this change (if we do > it) should be disk cache specific. 1. I haven't checked yet how other browsers behave in this case. I don't even know if all browser even implement *heuristic* freshness. I will try and get data though. 2. Only considering Epoch as an invalid value when computing freshness seems like a safe / logical change. Do we really *ever* want a resource to not be revalidated for ~4.5 years simply due to heuristic freshness and having Epoch as Last-Modified header? 3. I found many examples out there for this happening: a. rdar://problem/22330837 b. Several of our layout tests were returning an invalid value for "Last-Modified". Our networking code was translating this to Epoch and those resources were getting cached because of this. My patch made this problem apparent and I had to fix our test infrastructure. c. https://issues.apache.org/jira/browse/MRM-1854 d. https://groups.google.com/forum/#!topic/golang-codereviews/-CO9KJ75d20 e. https://core.trac.wordpress.org/ticket/23021 f. https://wordpress.org/support/topic/the-plugin-returns-header-last-modified-thu-01-jan-1970-000000-gmt g. https://mail.zope.org/pipermail/zope/2004-August/152629.html 4. User impact is pretty bad when this happens: We keep serving stale content from the cache, without ever revalidating. 5. I think the behavior of our memory cache should be consistent with the one of the disk cache so I do not want to make this change specific to the disk cache. I think it makes sense for both of them. I think everyone implements heuristic freshness, it is a pretty basic feature. For example chrome implementation can be seen here (HttpResponseHeaders::GetFreshnessLifetime): https://chromium.googlesource.com/chromium/chromium/+/master/net/http/http_response_headers.cc and I don't see hack like this there. You should be able to demonstrate that other browsers have this sort of behavior. Otherwise this seems like a pure site bug we shouldn't be trying to work around. If the issue is that invalid Last-Modified values are misparsed then the fix should be in the parsing code (see below ResourceResponseBase::lastModified()). Besides our parsing bug (which maps invalid input to Epoch), I am not disagreeing this is a site bug. However, there are examples of this bug out there (including the one from the radar) and the user experience in this case is really bad. Is it worth not fixing this simply for caching efficiency purposes? And again, do we *ever* want a resource to not be revalidated for over 4 year? I don't think we should implement behaviors that make our caching less efficient if other browsers manage without. It seems to me that the first thing to do would be fix the parsing bug. Wouldn't that solve most of these problems? (In reply to comment #12) > I don't think we should implement behaviors that make our caching less > efficient if other browsers manage without. > > It seems to me that the first thing to do would be fix the parsing bug. > Wouldn't that solve most of these problems? It would fix our layout tests. It would NOT fix the radar. (In reply to comment #12) > I don't think we should implement behaviors that make our caching less > efficient if other browsers manage without. > > It seems to me that the first thing to do would be fix the parsing bug. > Wouldn't that solve most of these problems? Also, this is a problem in CFNetwork. From WebKit's point of view, we are getting "Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT" from [NSHTTPURLResponse allHeaderFields] (verified when running LayoutTests/http/tests/cache/disk-cache/disk-cache-204-status-code.html). So right now, there is no way for WebKit to distinguish invalid date from Epoch from an invalid date header because CFNetwork seems to use Epoch to represent invalid dates. I'm assuming that the vast majority of websites do not return resources with a last modified date of Epoch. So the majority of websites should not be affected by any decision we make here. The question is what should we do for the small minority of sites that do happen to return Epoch as the last modified date for some resources. Fundamentally the heuristic allowed by the spec is about common sense – the principal that we should base our estimate for how long we think it is reasonable to cache a resource should be proportional to the period of timer which we know it to have gone unchanged. I think it makes sense to apply the spec rigidly here if either (a) we think the logic of the spec still applies, and it actually still actually makes sense, or (b) we think that logical or not, websites are deliberately and explicitly setting this value in order to achieve a desired outcome. I don't think either of these things is true. (a) When we get a resource back with a last modified date of Epoch, I don't think we really believe that resource has actually existed for 45 years, and is likely going to be unchanged for a proportionate amount of time, and (b) I don't think servers are returning these responses deliberately with the explicit desired goal that we should cache them for 4.5 years. Preponderance of evidence seems to indicate that it's just a server side bug that they're returning these values. I don't think it makes sense to apply a lawyerly approach here. Just because the spec says we can cache, doesn't mean we should - and the fact that it both appears nonsensical to cache, and that doing so has observably negatively impacted customer experience seems like a strong argument not to do so. I don't think the idea of fixing the bug where it is rather than working around in another layer is particularly lawyerly. Web sites surely are not deliberately setting Last-Modified to epoch. However there may be sites that do that by accident and get the best caching behavior by accident too. Vast majority of resources never change. The behavior is clearly web compatible if other browsers manage without this sort of workaround. Comment on attachment 259374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259374&action=review > Source/WebCore/ChangeLog:12 > + Ignore "Last-Modified" header when computing heuristic freshness if it > + is Epoch. Some HTTP servers wrongly return Epoch as "Last-Modified" > + header instead of omitting the header which results in cached resources > + with a giant lifetime. As far as I understand this is a workaround for a CFNetwork bug and should be described as such. Having Last-Modified value far in the past is quirky but valid way to get long cache lifetime. > Source/WebCore/platform/network/CacheValidation.cpp:135 > + // We ignore the "Last-Modified" header if it is Epoch as some servers return Epoch instead of omitting the header. > auto lastModified = response.lastModified(); > - if (lastModified) > + const system_clock::time_point epoch; > + if (lastModified && lastModified.value() != epoch) FIXME should explain that this is a workaround for a CFNetwork bug instead. The code change should be in ResourceResponseBase::lastModified(), that is return nullopt for epoch since we can't tell it apart from unparseable value. As far as I can tell popehat.com where this was seen behaves exactly as we do in Chrome (always loads from cache). It seems like a pure site bug. Do we really need this workaround for the CFNetwork bug? We have presumably had it forever. Created attachment 259472 [details]
Patch
Comment on attachment 259374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259374&action=review >> Source/WebCore/ChangeLog:12 >> + with a giant lifetime. > > As far as I understand this is a workaround for a CFNetwork bug and should be described as such. Having Last-Modified value far in the past is quirky but valid way to get long cache lifetime. I updated the change log. >> Source/WebCore/platform/network/CacheValidation.cpp:135 >> + if (lastModified && lastModified.value() != epoch) > > FIXME should explain that this is a workaround for a CFNetwork bug instead. > > The code change should be in ResourceResponseBase::lastModified(), that is return nullopt for epoch since we can't tell it apart from unparseable value. I moved this code to ResourceResponseBase::lastModified() and make it Cocoa only as it works around a CFNetwork bug. (In reply to comment #19) > Do we really need this workaround for the CFNetwork bug? We have presumably > had it forever. We now know about the bug and rely on the LastModified value for our new WebKit Disk Cache. Comment on attachment 259472 [details] Patch Clearing flags on attachment: 259472 Committed r188690: <http://trac.webkit.org/changeset/188690> All reviewed patches have been landed. Closing bug. (In reply to comment #17) > Comment on attachment 259374 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259374&action=review > > > Source/WebCore/ChangeLog:12 > > + Ignore "Last-Modified" header when computing heuristic freshness if it > > + is Epoch. Some HTTP servers wrongly return Epoch as "Last-Modified" > > + header instead of omitting the header which results in cached resources > > + with a giant lifetime. > > As far as I understand this is a workaround for a CFNetwork bug and should > be described as such. Having Last-Modified value far in the past is quirky > but valid way to get long cache lifetime. I don't think it's a CFNetwork bug (CFNetwork also probably converts invalid dates to Epoch though), I think it's also present in servers. The test introduced in this bug has always failed for GTK+, that doesn't use CFNetwork. So, it should be apache the one returning Epoch. I'm pretty sure libsoup doesn't process all the response headers, they are just parsed as key value pairs, and passed to WebKit. And I've checked that what we get in WebKit is Thu, 01 Jan 1970 00:00:00 GMT with that test when Last-Modified header contains "invalid". > > Source/WebCore/platform/network/CacheValidation.cpp:135 > > + // We ignore the "Last-Modified" header if it is Epoch as some servers return Epoch instead of omitting the header. > > auto lastModified = response.lastModified(); > > - if (lastModified) > > + const system_clock::time_point epoch; > > + if (lastModified && lastModified.value() != epoch) > > FIXME should explain that this is a workaround for a CFNetwork bug instead. > > The code change should be in ResourceResponseBase::lastModified(), that is > return nullopt for epoch since we can't tell it apart from unparseable value. Doing the same thing to soup fixes the test for us, which would be expected for the case when we use Epoch explicitly, but it also fixes the case of using an invalid header. |