Clearing the cache prevents disk cache from working

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Networking: Cache
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Juha-Matti Tilli, Assigned: Darin Fisher)

Tracking

({fixed1.8.1})

1.8 Branch
mozilla1.9alpha1
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

11.14 KB, patch
Alfred Kayser
: review+
Benjamin Smedberg
: approval-branch-1.8.1+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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

12 years ago
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.

Updated

12 years ago
Assignee: nobody → darin
Component: General → Networking: Cache
Product: Firefox → Core
QA Contact: general → networking.cache
Version: unspecified → 1.8 Branch
(Assignee)

Comment 2

12 years ago
Yeah, I've noticed this problem too.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: FreeBSD → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 3

12 years ago
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.
(Assignee)

Comment 4

12 years ago
It looks like this bug was caused by the patch for bug 284086 :-(
Blocks: 284086
(Assignee)

Comment 5

12 years ago
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
(Assignee)

Comment 6

12 years ago
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 :-(
(Assignee)

Comment 7

12 years ago
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
(Assignee)

Comment 8

12 years ago
Created attachment 209049 [details] [diff] [review]
v1 patch
Attachment #209049 - Flags: review?(alfredkayser)
(Assignee)

Updated

12 years ago
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+

Comment 10

12 years ago
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.
(Assignee)

Comment 11

12 years ago
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)
(Assignee)

Comment 13

12 years ago
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 ;-)
(Assignee)

Comment 14

12 years ago
Created attachment 209423 [details] [diff] [review]
v1.1 patch
Attachment #209049 - Attachment is obsolete: true
Attachment #209423 - Flags: review?(alfredkayser)
Attachment #209049 - Flags: review?(alfredkayser)

Comment 15

12 years ago
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+
(Assignee)

Comment 16

12 years ago
> 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.
(Assignee)

Comment 17

12 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #209423 - Flags: approval1.8.1?
Attachment #209423 - Flags: approval1.8.0.2?

Updated

12 years ago
Attachment #209423 - Flags: approval1.8.1? → branch-1.8.1+
(Assignee)

Comment 18

12 years ago
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?
(Assignee)

Comment 20

12 years ago
> 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.