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

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Alice0775 White]

[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.

Comment 1

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

Honza, can you look into this?

Comment 2

[Honza Bambas (:mayhemer)]

Sure

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Except if the fix is low risk and arrive soon, it is too late for 33.

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Jason - Is there someone else who can take a look at this while Honza is out?

Comment 6

[Jason Duell]

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.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 8

[Jason Duell]

Honza, do you know if making this work is something for you, or Michal?

Comment 9

[Honza Bambas (:mayhemer)]

I think I can take this.  I (re)wrote the UI parts here.

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: solution1, v1 [for reference]

Comment 13

[Michal Novotny [:michal]]

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.

Comment 14

[Honza Bambas (:mayhemer)]

(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

Comment 15

[Mark Bokil]

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.

Comment 16

[Michal Novotny [:michal]]

(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.

Comment 17

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

(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.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

It is too late for 36 and we released a few versions with this. Not sure it is worth tracking anymore.

Comment 22

[Brian Carpenter [:geeknik]]

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.

Comment 23

[u564464]

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.

Comment 24

[Kyle Repinski]

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.

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: v1

Simple and effective.

Comment 26

[Honza Bambas (:mayhemer)]

Covered by https://treeherder.mozilla.org/#/jobs?repo=try&revision=0aaf0751f4a3d7cfe35e6db40b890a0777523e91

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/395327b197cc

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/395327b197cc

Comment 29

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Too late for 47 uplift but the fix is in 48, yay.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 31

[Honza Bambas (:mayhemer)]

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

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 33

[Alice0775 White]

No longer reproduce on Nightly49.0a1

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Alice0775 White from comment #33)
> No longer reproduce on Nightly49.0a1

Awesome! Thank you for all your due diligence :)