Allow CachedResourceLoader clients to opt out of the MemoryCache.

• Component: Page Loading
• Status: RESOLVED FIXED

[Jer Noble]

Allow CachedResourceLoader clients to opt out of the MemoryCache.

[Jer Noble]

Created attachment 270003 [details]
Patch

[Jer Noble]

Created attachment 270033 [details]
Patch

[Jer Noble]

Created attachment 270040 [details]
Patch

[Darin Adler]

Comment on attachment 270040 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1160
> -        if (resource->errorOccurred())
> +        if (resource->errorOccurred() && resource->preloadResult() == CachedResource::PreloadNotReferenced)

I don’t see anything in the change log that explains this change.

[Alex Christensen]

Comment on attachment 270040 [details]
Patch

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

> Source/WebCore/loader/ResourceLoaderOptions.h:98
> +    ResourceLoaderOptions(SendCallbackPolicy sendLoadCallbacks, ContentSniffingPolicy sniffContent, DataBufferingPolicy dataBufferingPolicy, StoredCredentials allowCredentials, ClientCredentialPolicy credentialPolicy, CredentialRequest credentialRequest, SecurityCheckPolicy securityCheck, RequestOriginPolicy requestOriginPolicy, CertificateInfoPolicy certificateInfoPolicy, ContentSecurityPolicyImposition contentSecurityPolicyImposition, DefersLoadingPolicy defersLoadingPolicy, CachingPolicy cachingPolicy)

Since this is the last parameter, would it be a good idea to use a default value here so we don't have to worry about adding CachingPolicy::AllowCaching everywhere we create a ResourceLoaderOptions?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:224
> +    if (request.allowsCaching()) {

requestUserCSSStyleSheet should always allow caching, right?  Shouldn't this just be an assert?

[Jer Noble]

(In reply to comment #4)
> Comment on attachment 270040 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270040&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:1160
> > -        if (resource->errorOccurred())
> > +        if (resource->errorOccurred() && resource->preloadResult() == CachedResource::PreloadNotReferenced)
> 
> I don’t see anything in the change log that explains this change.

That was inadvertent; I'll remove it before landing.

[Jer Noble]

(In reply to comment #5)
> Comment on attachment 270040 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270040&action=review
> 
> > Source/WebCore/loader/ResourceLoaderOptions.h:98
> > +    ResourceLoaderOptions(SendCallbackPolicy sendLoadCallbacks, ContentSniffingPolicy sniffContent, DataBufferingPolicy dataBufferingPolicy, StoredCredentials allowCredentials, ClientCredentialPolicy credentialPolicy, CredentialRequest credentialRequest, SecurityCheckPolicy securityCheck, RequestOriginPolicy requestOriginPolicy, CertificateInfoPolicy certificateInfoPolicy, ContentSecurityPolicyImposition contentSecurityPolicyImposition, DefersLoadingPolicy defersLoadingPolicy, CachingPolicy cachingPolicy)
> 
> Since this is the last parameter, would it be a good idea to use a default
> value here so we don't have to worry about adding
> CachingPolicy::AllowCaching everywhere we create a ResourceLoaderOptions?

Most of the policy arguments could probably use default values, and CachingPolicy is no different. We should probably come up with a better way of initializing a ResourceLoaderOptions with defaults-except-for-one-policy, but I don't think just adding a default to the cachingPolicy parameter helps here.

> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:224
> > +    if (request.allowsCaching()) {
> 
> requestUserCSSStyleSheet should always allow caching, right?  Shouldn't this
> just be an assert?

Sure!

[Alex Christensen]

After talking with Darin, we're not sure it's a good idea to not check the cache at all for media loads.  Maybe this policy should only prevent data from being put in the cache, but if it has already been put in the cache some other way (someone pre-loading a small video with xhr, for example) then we should use it from the cache.  If that's true, this patch should be rewritten before landing.

[Jer Noble]

Committed r195770: <http://trac.webkit.org/changeset/195770>

[Jer Noble]

I explained to Alex offline why allowing pre-loading via XHR of media data is a bad idea. TL;DR, it will seriously mess with the "canplaythrough" calculations, authors will get the preload amount wrong, and we provide another way to pre-load entire media data for small videos (XHR -> blob -> blob URL -> video).

[Alex Christensen]

(In reply to comment #10)
> I explained to Alex offline why allowing pre-loading via XHR of media data
> is a bad idea. TL;DR, it will seriously mess with the "canplaythrough"
> calculations, authors will get the preload amount wrong, and we provide
> another way to pre-load entire media data for small videos (XHR -> blob ->
> blob URL -> video).
So this is a new kind of loading that never caches anything and never checks the cache for anything.  This is why I think long-term we should refactor the loading code instead of tacking things on like this.

[Csaba Osztrogonác]

(In reply to comment #9)
> Committed r195770: <http://trac.webkit.org/changeset/195770>

It broke the Windows build.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #9)
> > Committed r195770: <http://trac.webkit.org/changeset/195770>
> 
> It broke the Windows build.

... as the EWS noticed it ...

[Alexey Proskuryakov]

This caused a use-after-free, detected by ASan after landing. It seems to be isolated to one test and not causing much instability on the bots, so I will not be rolling out immediately.

Please let me know if this can be fixed by Monday - if it's tricky, let's roll out after all.

Filed bug 153727 to track the regression.

[Alex Christensen]

http://trac.webkit.org/changeset/195816 fixed this.

[youenn fablet]

I am trying to relate this flag with fetch cache mode.
How is it different/similar to fetch cache mode being set to "no-store"?

I am not exactly sure of the purpose of this flag (performances, security?).
Why removing it from the memory cache and not from other HTTP caches (network process, http library)?

[Alex Christensen]

(In reply to comment #16)
> I am trying to relate this flag with fetch cache mode.
> How is it different/similar to fetch cache mode being set to "no-store"?
It's similar.  The memory cache is not in any spec, though.
> 
> I am not exactly sure of the purpose of this flag (performances, security?).
> Why removing it from the memory cache and not from other HTTP caches
> (network process, http library)?
It was removed from the memory cache here and from the disk cache in http://trac.webkit.org/changeset/200578 because we are using AVFoundation's cache instead right now.