Bug 153549 - Allow CachedResourceLoader clients to opt out of the MemoryCache.
Summary: Allow CachedResourceLoader clients to opt out of the MemoryCache.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-27 09:43 PST by Jer Noble
Modified: 2016-06-22 14:15 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.84 KB, patch)
2016-01-27 10:26 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (24.64 KB, patch)
2016-01-27 13:33 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (24.60 KB, patch)
2016-01-27 14:29 PST, Jer Noble
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2016-01-27 09:43:48 PST
Allow CachedResourceLoader clients to opt out of the MemoryCache.
Comment 1 Jer Noble 2016-01-27 10:26:27 PST
Created attachment 270003 [details]
Patch
Comment 2 Jer Noble 2016-01-27 13:33:46 PST
Created attachment 270033 [details]
Patch
Comment 3 Jer Noble 2016-01-27 14:29:16 PST
Created attachment 270040 [details]
Patch
Comment 4 Darin Adler 2016-01-27 15:25:51 PST
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.
Comment 5 Alex Christensen 2016-01-27 15:51:13 PST
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?
Comment 6 Jer Noble 2016-01-27 16:41:49 PST
(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.
Comment 7 Jer Noble 2016-01-27 16:44:06 PST
(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!
Comment 8 Alex Christensen 2016-01-27 18:11:57 PST
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.
Comment 9 Jer Noble 2016-01-28 11:53:08 PST
Committed r195770: <http://trac.webkit.org/changeset/195770>
Comment 10 Jer Noble 2016-01-28 16:12:59 PST
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).
Comment 11 Alex Christensen 2016-01-28 16:59:29 PST
(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.
Comment 12 Csaba Osztrogonác 2016-01-28 22:15:12 PST
(In reply to comment #9)
> Committed r195770: <http://trac.webkit.org/changeset/195770>

It broke the Windows build.
Comment 13 Csaba Osztrogonác 2016-01-28 22:16:32 PST
(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 ...
Comment 14 Alexey Proskuryakov 2016-01-31 11:17:45 PST
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.
Comment 15 Alex Christensen 2016-02-09 17:19:12 PST
http://trac.webkit.org/changeset/195816 fixed this.
Comment 16 youenn fablet 2016-06-22 08:47:07 PDT
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)?
Comment 17 Alex Christensen 2016-06-22 14:15:59 PDT
(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.