|Summary:||Regression(r188698): http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html is very flaky|
|Product:||WebKit||Reporter:||Alexey Proskuryakov <ap>|
|Component:||WebKit Misc.||Assignee:||Chris Dumez <cdumez>|
|Severity:||Normal||CC:||cdumez, cgarcia, commit-queue, koivisto, thorton|
Description Alexey Proskuryakov 2015-08-19 17:48:11 PDT
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
Comment 1 Chris Dumez 2015-08-20 12:56:58 PDT
I can reproduce the flakiness locally. Looking at this.
Comment 2 Chris Dumez 2015-08-20 13:53:34 PDT
Committed r188698: <http://trac.webkit.org/changeset/188698>
Comment 3 Chris Dumez 2015-08-20 15:35:28 PDT
Looks like this became flaky due to <http://trac.webkit.org/changeset/188640>. I am still working to understand why.
Comment 4 Chris Dumez 2015-08-20 15:44:26 PDT
Reopening to investigate the real cause of the regression.
Comment 5 Chris Dumez 2015-08-20 16:34:21 PDT
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.
Comment 7 Antti Koivisto 2015-08-21 03:33:00 PDT
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 8 Chris Dumez 2015-08-21 09:19:00 PDT
Comment on attachment 259580 [details] Patch Clearing flags on attachment: 259580 Committed r188755: <http://trac.webkit.org/changeset/188755>
Comment 9 Chris Dumez 2015-08-21 09:19:04 PDT
All reviewed patches have been landed. Closing bug.
Comment 10 Tim Horton 2015-08-21 11:33:58 PDT
(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.