Bug 148205

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>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, koivisto, thorton
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=148137
Attachments:
Description Flags
Patch none

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 6 Chris Dumez 2015-08-20 20:31:56 PDT
Created attachment 259580 [details]
Patch
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.