Closed Bug 321833 Opened 18 years ago Closed 18 years ago

Clearing the cache prevents disk cache from working

Categories

(Core :: Networking: Cache, defect)

1.8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: juhis, Assigned: darin.moz)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → darin
Component: General → Networking: Cache
Product: Firefox → Core
QA Contact: general → networking.cache
Version: unspecified → 1.8 Branch
Yeah, I've noticed this problem too.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: FreeBSD → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
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.
It looks like this bug was caused by the patch for bug 284086 :-(
Blocks: 284086
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).
Flags: blocking1.8.1?
Flags: blocking1.8.0.2?
Summary: Clearing the cache prevents about:cache from working → Clearing the cache prevents disk cache from working
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 :-(
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.
Blocks: 296256
Attached patch v1 patch (obsolete) — Splinter Review
Attachment #209049 - Flags: review?(alfredkayser)
Attachment #209049 - Flags: review?(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)
Attachment #209049 - Flags: review?(mconnor) → review+
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.
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.
(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)
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 ;-)
Attached patch v1.1 patchSplinter Review
Attachment #209049 - Attachment is obsolete: true
Attachment #209423 - Flags: review?(alfredkayser)
Attachment #209049 - Flags: review?(alfredkayser)
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.
Attachment #209423 - Flags: review?(alfredkayser) → review+
> 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.
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #209423 - Flags: approval1.8.1?
Attachment #209423 - Flags: approval1.8.0.2?
Attachment #209423 - Flags: approval1.8.1? → branch-1.8.1+
fixed1.8.1
Keywords: fixed1.8.1
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?
> 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 on attachment 209423 [details] [diff] [review]
v1.1 patch

not approved for 1.8.0 branch
Attachment #209423 - Flags: approval1.8.0.2? → approval1.8.0.2-
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2-
You need to log in before you can comment on or make changes to this bug.