The test started to fail a lot this afternoon: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache%2Fdisk-cache%2Fdisk-cache-revalidation-new-expire-header.html
I can reproduce the flakiness locally. Looking at this.
Committed r188698: <http://trac.webkit.org/changeset/188698>
Looks like this became flaky due to <http://trac.webkit.org/changeset/188640>. I am still working to understand why.
Reopening to investigate the real cause of the regression.
So it looks like what is happening is that we get a 304 response in NetworkResourceLoader::didReceiveResponseAsync() / NetworkResourceLoader::didFinishLoading() but m_cacheEntryForValidation is null. As a result, we will call store() with that 304 response instead of updating an existing cache entry. Since 304 is not cacheable, makeCacheDecision() will refuse to cache and we will delete the existing entry in the cache (because we think it is no longer cacheable). My assumption is that those are 304 responses to revalidation's triggered by our MemoryCache instead of the disk cache. This is why m_cacheEntryForValidation is null. I think that even though the resource was revalidated by the memory cache, we should probably still update it in the disk cache if it exists.
Created attachment 259580 [details] Patch
Comment on attachment 259580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259580&action=review It is encouraging that tests caught this subtle bug. > Source/WebKit2/ChangeLog:14 > + Longer term, we should probably update the entry in the disk cache (if > + it exists) when it is revalidated by the memory cache. Currently, Yeah.
Comment on attachment 259580 [details] Patch Clearing flags on attachment: 259580 Committed r188755: <http://trac.webkit.org/changeset/188755>
All reviewed patches have been landed. Closing bug.
(In reply to comment #7) > Comment on attachment 259580 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259580&action=review > > It is encouraging that tests caught this subtle bug. Yes! That is awesome.