Bug 156048 – We sometimes fail to remove outdated entry from the disk cache after revalidation and when the resource is no longer cacheable

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Nolan Lawson]

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.

[Nolan Lawson]

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

[Nolan Lawson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/25514480>

[Chris Dumez]

Disabling disk cache speculative validation does not fix the problem so this is not a regression from this new feature.

[Chris Dumez]

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.

[Chris Dumez]

(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.

[Chris Dumez]

This is a regression from:
http://trac.webkit.org/changeset/190320

[Chris Dumez]

I will try and look into this soon.

[Chris Dumez]

@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.

[Antti Koivisto]

Nice find. Sounds like easy fix.

[Chris Dumez]

Created attachment 275586 [details]
Patch

[Chris Dumez]

@Nolan Lawson: Thanks a lot for the bug report and especially for the test case. It helped a lot.

[Antti Koivisto]

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).

[WebKit Commit Bot]

Comment on attachment 275586 [details]
Patch

Clearing flags on attachment: 275586

Committed r199061: <http://trac.webkit.org/changeset/199061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.