Closed Bug 1066970 Opened 5 years ago Closed 4 years ago

Indication of amount of content cache does not update when click [Clear Now] button preferences

Categories

(Firefox :: Preferences, defect, minor)

32 Branch
defect
Not set
minor

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox32 --- wontfix
firefox33 + wontfix
firefox34 + wontfix
firefox35 + wontfix
firefox36 + wontfix
firefox37 --- affected
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: alice0775, Assigned: mayhemer)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

[Tracking Requested - why for this release]: regression since Firefox32


Steps To Reproduce:
1. Visit several web pages to accumulate content cache
2. Open about:preferences#advanced and select Network tab
   ---- You can see amount of content cache usage
3. Click [Clear Now] button

Actual Results:
Indication of amount of content cache does not update.

Expected Results:
Indication of amount of content cache should be 0 byte.
Blocks: cache2enable
Honza, can you look into this?
Flags: needinfo?(honzab.moz)
Sure
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
Except if the fix is low risk and arrive soon, it is too late for 33.
Duplicate of this bug: 1083682
Severity: normal → minor
OS: Windows 7 → All
Hardware: x86_64 → All
Jason - Is there someone else who can take a look at this while Honza is out?
Flags: needinfo?(jduell.mcbugs)
Michal, can you take a look at this?

One thing I notice is that if I wait a few seconds, then click the button again, I get an updated value.

I assume we need to just set the value to 0 immediately, even though the actual cache deleteDir operator won't complete for some amount of time.
Assignee: honzab.moz → michal.novotny
Flags: needinfo?(jduell.mcbugs) → needinfo?(michal.novotny)
Jason - This bug hasn't made any progress. Can you please try again for 35?

I'm marking 34 as wontfix because we're late in the cycle.
Flags: needinfo?(jduell.mcbugs)
Honza, do you know if making this work is something for you, or Michal?
Flags: needinfo?(honzab.moz)
I think I can take this.  I (re)wrote the UI parts here.
Flags: needinfo?(honzab.moz)
Assignee: michal.novotny → honzab.moz
Status: NEW → ASSIGNED
Duplicate of this bug: 1101431
The problem here is that we display information provided by the cache index which is updated (cleared) asynchronously.  updateActualCacheSize() called right after cacheService.clear() call will report >0 size (the size before clearing).

There are two ways:
1. on successful exit from cacheService.clear() artificially report 0
2. switch index to a state (probably a new one) making it work like in non-READY state that switches back to READY after we call CacheIndex::RemoveAll() (from CacheFileIOManager::EvictAllInternal()).

Michal, how does the solution #2 sound to you?  It's also clearer, the artificial report might be disturbed or even broken when the callback (since this is async) is already waiting and is fired after we report the artificial 0.
Flags: needinfo?(michal.novotny)
Flags: needinfo?(michal.novotny)
Attached patch solution1, v1 [for reference] (obsolete) — Splinter Review
Clearing the whole cache should be quite fast, so I don't think we should introduce a new state for this. Why is updateActualCacheSize() called right after calling the clear() method? Shouldn't we wait for "cacheservice:empty-cache" notification? Btw, we might need to move call to CacheIndex::RemoveAll() before posting the notification in CacheFileIOManager::EvictAllInternal() to make it work properly.
Flags: needinfo?(michal.novotny)
(In reply to Michal Novotny (:michal) from comment #13)
> Clearing the whole cache should be quite fast, so I don't think we should
> introduce a new state for this. Why is updateActualCacheSize() called right
> after calling the clear() method? 

There is actually no serious reason except that it might take some time before we drop UI to zero (on a slower HW).

> Shouldn't we wait for
> "cacheservice:empty-cache" notification? 

Same problem - there may be a delay (there is a potential) that would look like nothing has happened.

> Btw, we might need to move call to
> CacheIndex::RemoveAll() before posting the notification in
> CacheFileIOManager::EvictAllInternal() to make it work properly.

Not sure I follow 100% correctly - where do you suggest to put CacheIndex::RemoveAll() exactly?  If it's going to be called before we exit from cacheService.clear(), then it would resolve this bug immediately.  Seems then like you should do it tho.

My idea (probably off the table now, but still) with a new "being deleted" state would cause the following:
- cache is scheduled to be cleared
- we try to obtain the size from the index
- index is in "being deleted" state and hence defers the callback on the queue
- based on that the UI will change to "Calculating..."
- when CacheIndex::RemoveAll() is actually called, we reset the state to READY and that would also fire the callback and now UI would update to a 'proper' 0
Flags: needinfo?(michal.novotny)
A few of use on our work team have been using developer edition. Myself and others I have been clicking the Clear Now button several times since the UI doesn't update. When the UI does update the only change is "0 bytes of disk space." UI feedback is very subtle and hard to notice possibly consider enhancing this so it is easier to tell the cache is really cleared.
(In reply to Honza Bambas (:mayhemer) from comment #14)
> > Btw, we might need to move call to
> > CacheIndex::RemoveAll() before posting the notification in
> > CacheFileIOManager::EvictAllInternal() to make it work properly.
> 
> Not sure I follow 100% correctly - where do you suggest to put
> CacheIndex::RemoveAll() exactly?  If it's going to be called before we exit
> from cacheService.clear(), then it would resolve this bug immediately. 
> Seems then like you should do it tho.

I just meant that even if UI would wait for "cacheservice:empty-cache" notification, it could still happen that the index would return the non-zero value since we first post the notification and then call CacheIndex::RemoveAll().


> My idea (probably off the table now, but still) with a new "being deleted"
> state would cause the following:
> - cache is scheduled to be cleared
> - we try to obtain the size from the index
> - index is in "being deleted" state and hence defers the callback on the
> queue
> - based on that the UI will change to "Calculating..."
> - when CacheIndex::RemoveAll() is actually called, we reset the state to
> READY and that would also fire the callback and now UI would update to a
> 'proper' 0

First, what you describe is IMO a state of the cache and not the state of the index. Anyway, I'm against introducing a new state to the index since it would complicate more already quite complex code. But we could create a simple flag for this, so the state won't change. We would check this flag in CacheIndex::AsyncGetDiskConsumption() together with the state.
Flags: needinfo?(michal.novotny)
Flags: needinfo?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #11)
> The problem here is that we display information provided by the cache index
> which is updated (cleared) asynchronously.  updateActualCacheSize() called
> right after cacheService.clear() call will report >0 size (the size before
> clearing).

Not being entirely familiar with these APIs or their implementation, I would expect asyncGetDiskConsumption to not return until after cache eviction is complete, if there is one pending when it is called.

If changing that API behavior is not feasible for whatever reason, then the UI should be using a separate notification to detect when the cache has been cleared (sounds like cacheservice:empty-cache already exists for that).

The UI should respond immediately to the button click by going into an "indeterminate" state (we can re-use the actualDiskCacheSizeCalculated string for this, I suppose), and then update to the correct state once the clearing is complete.
Glad to see progress here but we're now pretty late in the 35 cycle so this is a wontfix once more and hopefully there will be something for early 36 betas that can get uplifted and tested out by the larger user base.
It is too late for 36 and we released a few versions with this. Not sure it is worth tracking anymore.
Still seeing this behavior in Firefox Nightly 47.0a1 and if you follow my STR here, you'll see that there is a mismatch between the total in about:preferences#advanced and about:cache:

1) Install Firefox Nightly (or create a new profile)
2) Browse around the web
3) Navigate to about:preferences > Advanced > Network
4) Notice where it says XX MB for web content cache (in my case, 230 MB)
5) Click Clear Now
6) Notice it still displays the original amount used
7) Navigate to about:cache and notice it says 886 KB in use (in my case)
8) Go back to the preference page and click Clear Now again.
9) Amount updated but still doesn't match about:cache. In my case, it says 2.1MB in about:preferences and 1KB in about:cache

If I navigate to C:\Users\Brian\AppData\Local\Mozilla\Firefox\Profiles\x.default and pull up the disk usage for the cache2 directory, I see where the 2.1MB is coming from in about:preferences as the total disk usage for the cache2 directory is approximately 2.1MB. I looked around the source until my eyes bled, but couldn't see where it was getting the incorrect value from, but then again, I'm not a very proficient coder.
Seeing this in 44.0.2 stable, on Linux and Windows platforms. Most of the time the "Clear Now" button has to be pressed a second time for the cache size to be reported as 0 . Sometimes pressing Clear Now once will reduce the size number reported but will not actually cause it to display 0 until a second time pressing the button.
This is a very annoying problem, clearing a full cache on a 5400RPM HDD can take a pretty decent chunk of time. At the very least there needs to be some sort of UI feedback that it's actually doing something.
Attached patch v1Splinter Review
Simple and effective.
Attachment #8525691 - Attachment is obsolete: true
Attachment #8738120 - Flags: review?(michal.novotny)
Attachment #8738120 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/395327b197cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
See Also: → 1267195
Too late for 47 uplift but the fix is in 48, yay.
Hi Alice0775, Kyle, could you please verify this issue is fixed as expected on a Nightly build? Thanks! This might help me decide whether to uplift this to Beta or not.
Flags: needinfo?(repinski23)
Flags: needinfo?(alice0775)
Comment on attachment 8738120 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: new HTTP cache + cache pinning
[User impact if declined]: people falsely believe the HTTP cache is not cleared when pressing the [Clear Now] button in preferences
[Describe test coverage new/current, TreeHerder]: sits in m-a for a long time, automated tests N/A
[Risks and why]: very low, the patch is safe (no lifetime/referencing changes or similar)
[String/UUID change made/needed]: none
Attachment #8738120 - Flags: approval-mozilla-beta?
Comment on attachment 8738120 [details] [diff] [review]
v1

Given that this is an old regression and that we are entering RC week in 2 days, it's safer not to uplift this to Beta47. I also don't have verification of the fix. The good news is this long standing regression will be fixed in Fx48!
Attachment #8738120 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
No longer reproduce on Nightly49.0a1
Flags: needinfo?(alice0775)
Flags: needinfo?(repinski23)
(In reply to Alice0775 White from comment #33)
> No longer reproduce on Nightly49.0a1

Awesome! Thank you for all your due diligence :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.