522425 - Network > Clear Now button does not clear cache if cache size is set to 0

Status: NEW

(Firefox :: Settings UI, defect)

Description

[Alan Baxter]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

Not wanting a disk cache anymore, I set the cache size to 0 and clicked OK.  I noticed the cache had not been cleared yet, so I went to Options again and clicked Network > Clear Now, but that didn't clear the cache either.

Reproducible: Always

Steps to Reproduce:
1. Set Options > Advanced > Network to non-zero value.
2. Create some disk cache by browsing.
3. Set Options > Advanced > Network to 0 MB of cache and click OK.
4. Click Options > Advanced > Network > Clear Now
Actual Results:  
The cache is not cleared.

Expected Results:  
The cache is cleared.

Bug also present in current Namoroka and Minefield nightlies:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20091014 Namoroka/3.6b1pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091014 Minefield/3.7a1pre

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3

I experience the same behavior.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Does setting the cache size to 0 not end up deleting the entire cache the next time you start up Firefox?

Blocking for now, though based on what the actual end state is, we might punt this off to a future release.

Comment 3

[Alan Baxter]

(In reply to comment #2)
> Does setting the cache size to 0 not end up deleting the entire cache the next
> time you start up Firefox?

It does not delete the entire cache even after a restart.  There are still about a dozen or so files left in the Cache directory.  To clear the entire cache I have to set the space allowed for the cache to a non-zero amount and click Clear Now.  Then and only then is the entire cache removed.  Just tested in Namoroka Alpha 1 and the most recent nightly: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b2pre) Gecko/20091026 Namoroka/3.6b2pre

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Simple fix... I'm going to look at the history of nsCacheService.cpp to see if this has always been the case which so far appears to be the case.

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 - only clear the disk cache

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 409041 [details] [diff] [review]
patch rev2 - only clear the disk cache

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js
>--- a/browser/components/preferences/advanced.js
>+++ b/browser/components/preferences/advanced.js
>@@ -169,12 +169,20 @@ var gAdvancedPane = {
> 
>   /**
>    * Converts the cache size as specified in UI (in MB) to KB and returns that
>-   * value.
>+   * value and clears the disk cache when it has been set to 0.
>    */
>   writeCacheSize: function ()
>   {
>     var cacheSize = document.getElementById("cacheSize");
>     var intValue = parseInt(cacheSize.value, 10);
>+    if (intValue == 0) {
It might be a good idea to change this check to the following to be consistent with the check that follows it.
if (isNaN(intValue) || intValue == 0) {

>+      // Clear the disk cache since it has been set to 0.
>+      var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
>+                           	       .getService(Components.interfaces.nsICacheService);
>+      try {
>+        cacheService.evictEntries(Components.interfaces.nsICache.STORE_ON_DISK);
>+      } catch(ex) {}
>+    }
>     return isNaN(intValue) ? 0 : intValue * 1024;
>   },
>

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, please pick another reviewer; Gavin's slammed with Fennec atm.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
> It might be a good idea to change this check to the following to be consistent
> with the check that follows it.
> if (isNaN(intValue) || intValue == 0) {

isNaN is probably a leftover from former days. Since we have a number box, it's not needed. This code should use .valueNumber instead of .value.

As far as the actual fix is concerned, it doesn't seem quite right to me. On Windows, pref changes need to be confirmed with the OK button, otherwise they don't apply. So I don't think the cache should be cleared right away when the user modifies the value.

Comment 9

[Robert Strong (they/them - no direct email)]

I was thinking along the same lines but couldn't think of a good solution for this. I considered changing the behavior of the backend so Clear Now always cleared the disk cache but I think that is just as bad as this patch since setting the cache to zero should just clear it.

What do you think about only clearing the cache when zero and setting the pref value when the cacheSize element receives blur? It isn't perfect but I think it is better.

Comment 10

[Robert Strong (they/them - no direct email)]

An after 3.6 solution might be having a checkbox for disabling the disk cache instead of using 0 MB as being the trigger to disable it.

Comment 11

[Dão Gottwald [:dao]]

Blur will happen even if you hit the Cancel button, so I don't think this is substantially better. I think fixing the Clear Now button would be better for now... certainly not a complete fix, but at least no new unexpected behavior.

Comment 12

[Robert Strong (they/them - no direct email)]

hmmm... I really don't like the idea of making clear now be the only way to clear the cache when the user sets the disk cache to 0 since I believe the expectation of setting it to 0 is to not store the cache on disk and the only way they would know their cache is still on the disk would be to inspect the file system. Also, making clear now clear after the disk cache is set to 0 would require either setting the disk cache to a greater than zero value, calling evictEntries, and resetting it back to zero or changing the backend code to create the disk device when it isn't present, evicting the entries, and deleting the disk device which is more likely to introduce unexpected behavior than using blur. btw: if the implementation were to use blur it would of course respect cancel and not set the value.

I'll see how well making clear works via the js method described above.

Comment 13

[Dão Gottwald [:dao]]

Maybe nsCacheService::SetDiskCacheCapacity, which is called by the browser.cache.disk.capacity observer, should call evictEntries?

Comment 14

[Robert Strong (they/them - no direct email)]

Wouldn't that also affect the case where instantApply is true?

Comment 15

[Dão Gottwald [:dao]]

Sure, I think that's expected behavior if instantApply is true.

Comment 16

[Robert Strong (they/them - no direct email)]

note: I highly suspect that bug 329260 caused this.

Comment 17

[Robert Strong (they/them - no direct email)]

Bug 395627 is also of note and is hit in a debug build when setting the cache size to 0.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #13)
> Maybe nsCacheService::SetDiskCacheCapacity, which is called by the
> browser.cache.disk.capacity observer, should call evictEntries?

This sounds ideal, indeed.

Comment 19

[Robert Strong (they/them - no direct email)]

I agree though I don't think this type of change is appropriate for 1.9.2 so after talking with beltzner and johnathan I'm targeting this for 1.9.3.

Comment 20

[alexey.shytikov]

I have the same behavior, but even more strange.

When I press 'Clear now' button for the first time, it does nothing, but on second time, it actually clears the cache. So I need to press button twice.

Always.

Firefox 33.1 / Windows 8.1