321833 - Clearing the cache prevents disk cache from working

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Juha-Matti Tilli]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8) Gecko/20051204 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8) Gecko/20051204 Firefox/1.5

After Firefox is started, about:cache works as expected. When the cache is cleared, about:cache stops working. Restarting Firefox solves the problem.

Reproducible: Always

Steps to Reproduce:
1. Try opening about:cache. It should work.
2. Clear the cache from Edit -> Preferences -> Privacy -> Cache -> Clear Cache Now
3. Try opening about:cache again.

Actual Results:  
Nothing happens.

Expected Results:  
about:cache should work.

I tried disabling all installed extensions, but it didn't solve the problem.

Comment 1

[Joe Wells]

I see the exact same problem.  Furthermore, the first time the cache
is cleared (from the preferences panel), the "Cache" profile
subdirectory is removed (also emptied) and replaced by a directory
named "Cache.Trash" containing a directory named "Trash".  The "Cache"
profile subdirectory never returns, and all about:cache links (i.e.,
"about:cache" or "about:cache?anything") never start working again.
The second time the cache is cleared, the "Cache.Trash" directory
disappears (and never reappears).

All problems are fixed by restarting Firefox.

I'm using the Gentoo ebuild www-client/mozilla-firefox-1.5-r9.

Comment 2

[Darin Fisher]

Yeah, I've noticed this problem too.

Comment 3

[Darin Fisher]

This bug is caused by the Firefox preferences code.  It deletes the Cache directory out from under the Cache service before calling the Cache's EvictEntries method.  On UNIX systems, this causes all of the Cache files to be unlinked.  It doesn't happen on Windows for many of the cache files because they cannot be removed when they are in use.  Then, the nsDiskCacheDevice gets greatly confused because the Cache folder is suddenly not present.  We should fix the disk cache device to be more fault tolerant, but we should also fix the very broken code in browser/base/content/sanitize.js.

Comment 4

[Darin Fisher]

It looks like this bug was caused by the patch for bug 284086 :-(

Comment 5

[Darin Fisher]

This does much more than just disable about:cache.  It basically renders the disk cache useless.  I think this bug is something we should fix for FF 2 for sure (and maybe even in a minor update to 1.5).

Comment 6

[Darin Fisher]

The code features this comment:

// Here we play dirty, trying to brutally wipe out all the cache files
// even if the disk cache device has gone away (if it is still with us,
// our removal attempt will fail because the directory is locked,
// and we fall back to the "nice" way below)

Uhm, guys... there is no directory locking.  Sigh :-(

Comment 7

[Darin Fisher]

It turns out that bug 296256 is caused by the fact that cleanup of the Cache.Trash folder happens lazily on a background thread.  So, when we evict on shutdown (actually, in this case from profile-change-teardown), the cleanup thread never runs, or at least never runs to completion.

I haven't decided how best to solve this yet.  One idea is to modify the EvictEntries method to take a boolean parameter that indicates whether or not the eviction may happen lazily.  A simpler solution is to discard the lazy eviction code and just run eviction synchronously on the calling thread all the time.  That may not be too bad actually.

Comment 8

[Darin Fisher]

Attachment: v1 patch

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 209049 [details] [diff] [review]
v1 patch

I really can't see a point in time where the lazy cleanup really is useful.  I'm sure there's some cases, but I really can't think of any (its also after 3 AM)

Comment 10

[Alfred Kayser]

There are four locations where DeleteDir is called:

** 1 **
nsCacheService.cpp
317 nsresult
318 nsCacheProfilePrefObserver::ReadPrefs(nsIPrefBranch* branch)
353                     // We no longer store the cache directory in the main
354                     // profile directory, so we should cleanup the old one.
359                             DeleteDir(profDir, PR_FALSE);
-> Doing the deleting of the 'old' cache location as a separate thread is nice and safe

** 2** 
nsDiskCacheService.cpp
815 nsresult
816 nsDiskCacheDevice::OpenDiskCache()
835         // move "corrupt" caches to trash
837             rv = DeleteDir(mCacheDirectory, PR_TRUE);
-> This is safe to do as a separate thread, preventing startup time impact

** 3 **
855         // delete any trash files leftover from a previous run
857         GetTrashDir(mCacheDirectory, &trashDir);
861                 DeleteDir(trashDir, PR_FALSE);
-> This is also safe to do as a separate thread, preventing startup time impact

** 4 **
869 nsresult
870 nsDiskCacheDevice::ClearDiskCache()
871 {
875     nsresult rv = Shutdown_Private(PR_FALSE);  // false: don't bother flushing
879     rv = DeleteDir(mCacheDirectory, PR_TRUE);
883     return Init();
884 }
Moving the cache to Cache.Trash at clear cache all
This function is only used in EvictEntries when clientID is null (clear all...)
-> This is the where 'laziness' is not desired!! 

This confirms darin's comment: the Eviction should not be lazy, but the others can be lazy and prevent delays in startup caused by removal of 'old' stuff. 

So, to conclude, I would not change DeleteDir itself (keeping the laziness), but instead make EvictEntries/ClearDiskCache delete the directory itself directly. 
(see also bug 103851 where the laziness was introduced). 
EvictEntries could even be reverted to not delete directories, but evict all entries using the VisitRecords (note the 'if (clientID == nsnull) Trim' after the VisitRecords is currently deadcode, because of the DeleteDir above.
This would prevent issues with bound entries.

Comment 11

[Darin Fisher]

Alfred, thanks for the suggestions.  Perhaps it would make sense to pass a boolean to the DeleteDir function that indicates whether the files should be deleted now or lazily.  Then, we can change that according to the callsite.  OK, I'll revise the patch.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #9)
> I really can't see a point in time where the lazy cleanup really is useful. 
> I'm sure there's some cases, but I really can't think of any (its also after 3
> AM)

- when Necko detects that the cache is corrupt on startup and deletes it
- when hitting the "Clear Cache" button in the UI (so as not to hang the UI)

Comment 13

[Darin Fisher]

I think the best solution here is to keep the lazy deleting, and just do a synchronous delete of the cache.trash folder on shutdown.  That'll give us the best of both worlds ;-)

Comment 14

[Darin Fisher]

Attachment: v1.1 patch

Comment 15

[Alfred Kayser]

Comment on attachment 209423 [details] [diff] [review]
v1.1 patch

Looks ok to me. 
Personally I'm in favor of doing a 'sync'ed deletion on 'ClearDiskCache', for both the sanitize at shutdown, and the 'clear cache' from the UI.

Comment 16

[Darin Fisher]

> Personally I'm in favor of doing a 'sync'ed deletion on 'ClearDiskCache', for
> both the sanitize at shutdown, and the 'clear cache' from the UI.

I wanted to avoid sync clear from the 'clear cache' UI because it has a tendency to hang the UI while the hard disk thrashes about.

Comment 17

[Darin Fisher]

fixed-on-trunk

Comment 18

[Darin Fisher]

fixed1.8.1

Comment 19

[Daniel Veditz [:dveditz]]

This really doesn't seem like that common a case, and is recoverable by restarting Firefox. Do we really have to have this in a 1.5.0.x release?

Comment 20

[Darin Fisher]

> This really doesn't seem like that common a case, and is recoverable by
> restarting Firefox. Do we really have to have this in a 1.5.0.x release?

Agreed, it is not that critical.  Problems resulting from this bug are most significant on non-Windows systems anyways.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 209423 [details] [diff] [review]
v1.1 patch

not approved for 1.8.0 branch