Created attachment 275241 [details] Reproducible test case This bug appears in WebKit Nightly (9.1 11601.5.17.1 r198834), but not the stable version of Safari (9.1 11601.5.17.1). Steps to repro: 1. Safari performs a GET, server provides a cached response with an ETag 2. Safari requests with if-none-match and the server responds with a 304 3. Safari requests again, but resource has been removed so the server responds with a non-empty 404 4. Safari requests with if-none-match again, so server responds with 304 (this is where the bug occurs!) 5. Every subsequent request is a 304 for the *old* resource rather than the new resource This is not a niche situation - it happens pretty frequently with CouchDB due to CouchDB re-using the revision identifier (_rev) as the ETag. If a resource is removed and re-created with the same _rev, then Safari will not ask for new content and will remain permanently stuck on old content. If step 2 is skipped (i.e. Safari doesn't receive the 304 response), then the bug does not occur. This bug cannot be reproduced in Firefox (which submits if-none-match) or Chrome (which doesn't submit if-none-match). I've attached a test case as a ZIP file. I've also created a Gist: https://gist.github.com/nolanlawson/6cc2a537d390578bc235c00996f5b938. Steps to reproduce are in the _readme.md.
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.
<rdar://problem/25514480>
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.