Bug 155187 – Speculative disk cache resource revalidations are sometimes wasted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Speculative disk cache resource revalidations are sometimes wasted. We sometimes successfully revalidate a resource but the NetworkResourceLoader then either:
1. Fails to reuse the speculatively validated entry
2. Reuses the speculatively validated entry but validates it again

[Chris Dumez]

rdar://problem/25032905

[Chris Dumez]

Created attachment 273323 [details]
Patch

[Chris Dumez]

Created attachment 273339 [details]
Patch

[Chris Dumez]

We don't currently have an easy way to write a test for speculative revalidation but I will work on adding test support for this in a follow-up patch.

[Chris Dumez]

Created attachment 273371 [details]
Patch

[Antti Koivisto]

Comment on attachment 273371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273371&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:193
> +    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);

Not from this patch but it is bit strange that we use these source values as needsValidation bit. This would be clearer if the authoritative data was an actual bool member, response source could be updated as a side effect.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp:90
> +#if ENABLE(CACHE_PARTITIONING)

Should really kill these ifdefs at some point. Platforms that don't want partition should just use a fixed partition name.

[Chris Dumez]

Comment on attachment 273371 [details]
Patch

Clearing flags on attachment: 273371

Committed r197879: <http://trac.webkit.org/changeset/197879>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 273371 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273371&action=review

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.cpp:193
>> +    m_response.setSource(value ? WebCore::ResourceResponse::Source::DiskCacheAfterValidation : WebCore::ResourceResponse::Source::DiskCache);
> 
> Not from this patch but it is bit strange that we use these source values as needsValidation bit. This would be clearer if the authoritative data was an actual bool member, response source could be updated as a side effect.

I agree, I'll take a look at this in a follow-up.