Summary: | We sometimes fail to remove outdated entry from the disk cache after revalidation and when the resource is no longer cacheable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nolan Lawson <nolan> | ||||||
Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, beidson, cdumez, cgarcia, commit-queue, jan, koivisto, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.11 | ||||||||
Attachments: |
|
Description
Nolan Lawson
2016-03-30 17:17:04 PDT
Clarification: by "server provides a cached response" I meant "server provides a cacheable response." I'll also note that this could be considered a CouchDB bug rather than a WebKit bug, because if the client never gets the 404, they would never be informed that the content had changed, since the ETag never changed. And AFAICT the spec doesn't specify whether 404 should invalidate the ETag: http://tools.ietf.org/html/rfc7232#section-2.3 FWIW, I've filed a bug on CouchDB: https://issues.apache.org/jira/browse/COUCHDB-2978 I leave it up to your discretion as to whether you consider this a bug from WebKit's perspective. Disabling disk cache speculative validation does not fix the problem so this is not a regression from this new feature. Seems like a disk cache bug. We should probably delete the entry we have in the disk cache in case of failed revalidation, if the new response is not cacheable. (In reply to comment #5) > Seems like a disk cache bug. We should probably delete the entry we have in > the disk cache in case of failed revalidation, if the new response is not > cacheable. Actually, it looks like we already do so at disk cache level. However, what I see is the memory cache making revalidation requests. This is a regression from: http://trac.webkit.org/changeset/190320 I will try and look into this soon. @Antti: So I found the bug. The issue is that Storage::removeFromPendingWriteOperations(const Key&) only removes the *first* pending write operation with the key instead of *ALL* of them. So even though, the disk cache try to remove outdated entries from the cache, it can fail if there are several pending writes for this entry. Nice find. Sounds like easy fix. Created attachment 275586 [details]
Patch
@Nolan Lawson: Thanks a lot for the bug report and especially for the test case. It helped a lot. Comment on attachment 275586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275586&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:529 > + auto found = m_pendingWriteOperations.findIf([&key](const std::unique_ptr<WriteOperation>& operation) { Another option would be to eliminate duplicates from m_pendingWriteOperations by turning it into ListHashSet or something. It is bit silly we may end up writing same entry multiple times (though it didn't feel common enough to optimize just for itself). Comment on attachment 275586 [details] Patch Clearing flags on attachment: 275586 Committed r199061: <http://trac.webkit.org/changeset/199061> All reviewed patches have been landed. Closing bug. |