Closed
Bug 321833
Opened 18 years ago
Closed 18 years ago
Clearing the cache prevents disk cache from working
Categories
(Core :: Networking: Cache, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: juhis, Assigned: darin.moz)
References
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 1 obsolete file)
11.14 KB,
patch
|
alfredkayser
:
review+
benjamin
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2-
|
Details | Diff | Splinter Review |
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.
Updated•18 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•18 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•18 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•18 years ago
|
||
It looks like this bug was caused by the patch for bug 284086 :-(
Blocks: 284086
Assignee | ||
Comment 5•18 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•18 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•18 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•18 years ago
|
||
Attachment #209049 -
Flags: review?(alfredkayser)
Assignee | ||
Updated•18 years ago
|
Attachment #209049 -
Flags: review?(mconnor)
Comment 9•18 years ago
|
||
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•18 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•18 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.
Comment 12•18 years ago
|
||
(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•18 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•18 years ago
|
||
Attachment #209049 -
Attachment is obsolete: true
Attachment #209423 -
Flags: review?(alfredkayser)
Attachment #209049 -
Flags: review?(alfredkayser)
Comment 15•18 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•18 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•18 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #209423 -
Flags: approval1.8.1?
Attachment #209423 -
Flags: approval1.8.0.2?
Updated•18 years ago
|
Attachment #209423 -
Flags: approval1.8.1? → branch-1.8.1+
Comment 19•18 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?
Assignee | ||
Comment 20•18 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 21•18 years ago
|
||
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-
Updated•18 years ago
|
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.
Description
•