Bug 64483 – Disable Caches in Safari's Develop menu does not disable caches.

• Component: Page Loading
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

I have a mac with safari 5.0.5 installed

Scenario:

1. build recent webkit
2. run-safari
3. open web inspector's network panel
4. set Disable Caches in Develop menu  
5. go to webkit.org
6. refresh
7. observe 304 statuses of css stylesheets 

I can also see that the first refresh after setting Disable Caches option shows 200 OK, which probably means that Disable Caches clears cache instead of disabling it.

[Alexey Proskuryakov]

<rdar://problem/9770590>

[Darin Adler]

OK, here are the two bugs I found:

1) Setting the cache model to CacheModelDocumentViewer does not turn off MemoryCache. It turns off caching of dead resources, but it sets a non-zero value for the total size of the memory cache, which means that live resources are indeed cached. This policy is implemented in the calculateCacheSizes function and the +[WebView _setCacheModel:]  method. I tried setting cacheTotalCapacity to 0 and indeed this fixed the original reported symptom. It might be bad, however, for clients using CacheModelDocumentViewer for something other than Safari’s Disable Caches. Performance might be suprisingly bad when the memory cache is entirely turned off. Presumably this regression dates back to when we Antti created the MemoryCache.

2) Setting the cache model to CacheModelDocumentViewer does not turn off the NSURLCache disk cache when NETWORK_CACHE is disabled. This policy is in the NetworkProcess::platformSetCacheModel function and the +[WebView _setCacheModel:]  method. Neither will ever shrink disk capacity, only make it larger. Comment says, “Don't shrink a big disk cache, since that would cause churn.” I don’t understand the thinking behind this. This has always been the case <http://trac.webkit.org/changeset/25430>. Safari attempts to work around this by calling -[NSURLCache setDiskCapacity:] directly but this has not had any effect on WebKit networking since we moved it from the UI process to the web process and then later to the network process.

[Darin Adler]

Created attachment 274376 [details]
Patch

[Darin Adler]

Here’s my patch:

I didn't add a test yet.

This won’t fix the problem with Safari until a new version of Safari comes out that uses the new SPI. If we decide this is important, we could put an app-specific hack in to make this do the right thing in old versions of Safari.

I'm not sure this covers all the code paths. There are comments that seem to indicate some loads bypass FrameLoader::addExtraFieldsToRequest.

[Darin Adler]

This doesn't disable the page cache's caching of back/forward history. Maybe it should. Any other caches it should disable?

[Antti Koivisto]

Disabling page cache seems desirable too.

[Antti Koivisto]

Comment on attachment 274376 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518
> +void WKPreferencesSetAllCachesDisabled(WKPreferencesRef preferencesRef, bool flag)
> +{
> +    toImpl(preferencesRef)->setAllCachesDisabled(flag);
> +}
> +
> +bool WKPreferencesGetAllCachesDisabled(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->allCachesDisabled();
> +}

If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all.

[Darin Adler]

Comment on attachment 274376 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518
>> +}
> 
> If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all.

I asked Sam today what to do, and he told me to do this. Safari is currently using the C SPI, and I don’t think it can mix the C SPI with the Objective-C API/SPI. Same said it would be trivial to add the Objective-C API or SPI for this later and so I should not worry about it now.

[Darin Adler]

Comment on attachment 274376 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/C/WKPreferences.cpp:1518
>>> +}
>> 
>> If I understand correctly we don't really want to add more C API. We should add ObjC SPI to WKPreferencesPrivate. I'm not sure we need these C functions at all.
> 
> I asked Sam today what to do, and he told me to do this. Safari is currently using the C SPI, and I don’t think it can mix the C SPI with the Objective-C API/SPI. Same said it would be trivial to add the Objective-C API or SPI for this later and so I should not worry about it now.

Sam said, not "Same said".

[Antti Koivisto]

Comment on attachment 274376 [details]
Patch

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

> Source/WebCore/page/Settings.in:255
> +allCachesDisabled initial=false

We of course have many caches that this setting is not meant to disable. I suppose it is reasonable to assume that "all caches" here refers to resource caches specifically. Still I wonder if there would be a better name, "resourceCachesDisabled" or something.

[Antti Koivisto]

> I asked Sam today what to do, and he told me to do this. Safari is currently
> using the C SPI, and I don’t think it can mix the C SPI with the Objective-C
> API/SPI. Same said it would be trivial to add the Objective-C API or SPI for
> this later and so I should not worry about it now.

Ok. I assumed we can mix SPIs.

[Darin Adler]

Comment on attachment 274376 [details]
Patch

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

>> Source/WebCore/page/Settings.in:255
>> +allCachesDisabled initial=false
> 
> We of course have many caches that this setting is not meant to disable. I suppose it is reasonable to assume that "all caches" here refers to resource caches specifically. Still I wonder if there would be a better name, "resourceCachesDisabled" or something.

I’ll make a new version of this patch that covers the page cache too. The page cache caches resources, but it also caches more than that. I guess the name should be resourceCachingDisabled.

[Darin Adler]

Created attachment 274388 [details]
Patch

[Darin Adler]

Committed r198393: <http://trac.webkit.org/changeset/198393>

[Darin Adler]

Anders asked me to move this from WKPreferences to WKPage.

[Darin Adler]

Created attachment 274501 [details]
Patch

[Darin Adler]

Here’s a new patch that moves this off of preferences.

[Darin Adler]

Created attachment 274502 [details]
Patch

[Antti Koivisto]

Comment on attachment 274502 [details]
Patch

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

> Source/WebCore/page/Page.h:518
> +    bool isResourceCachingDisabled() const { return m_resourceCachingDisabled; }
> +    void setResourceCachingDisabled(bool disabled) { m_resourceCachingDisabled = disabled; }

Why do we prefer Page here? If this is not appropriate part of Settings then what is?

[Darin Adler]

Anders, could you answer Antti’s question?

[Antti Koivisto]

Comment on attachment 274502 [details]
Patch

In any case no need to hold this.

[WebKit Commit Bot]

Comment on attachment 274502 [details]
Patch

Clearing flags on attachment: 274502

Committed r198476: <http://trac.webkit.org/changeset/198476>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.