Bug 148205 – Regression(r188698): http/tests/cache/disk-cache/disk-cache-revalidation-new-expire-header.html is very flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Alexey Proskuryakov]

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

[Chris Dumez]

I can reproduce the flakiness locally. Looking at this.

[Chris Dumez]

Committed r188698: <http://trac.webkit.org/changeset/188698>

[Chris Dumez]

Looks like this became flaky due to <http://trac.webkit.org/changeset/188640>. I am still working to understand why.

[Chris Dumez]

Reopening to investigate the real cause of the regression.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 259580 [details]
Patch

[Antti Koivisto]

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.

[Chris Dumez]

Comment on attachment 259580 [details]
Patch

Clearing flags on attachment: 259580

Committed r188755: <http://trac.webkit.org/changeset/188755>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

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