329260 - Disk cache keeps emptying itself

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect, P1)

Description

[dolphinling]

Latest nightly trunk build (linux) 2006-03-03

The disk cache seems to keep emptying itself, losing items from one page load to the next.

An easy way to see this is to load, e.g., http://images.google.com/images?q=mozilla in one tab and about:cache in another. Reload the google tab and then watch about:cache. Then repeat a few times. The number of cached items is never constant when the google tab finishes loading, sometimes even dropping to 0.

Compare this to a build from last month (I tested 02-08) where, when the google tab finishes loading, the number of items reported in about:cache is always cosntant (45).

So this is a regression sometime between feb 8 and march 3. I'll narrow it down farther.

Comment 1

[dolphinling]

Regressed between 2006-02-28 and 2006-03-01

Comment 2

[dolphinling]

http://bonsai.mozilla.org/cvsquery.cgi?branch=HEAD&date=explicit&mindate=2006-02-28&maxdate=2006-03-01

Looks like either bug 189570 or bug 321456; both dealt with cache.

Comment 3

[Philip Withnall (unavailable)]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a1) Gecko/20060303 Firefox/1.6a1 ID:0000000000
Works fine for me, with the number of cached items staying at a level 80 (I've got three Bugzilla tabs open as well as Google and about:cache).

Comment 4

[Sven Grull]

Confirming with SeaMonkey 20060303 on Windows XP
Still working: SeaMonkey 20060227

Comment 5

[Frank Wein [:mcsmurf]]

Possibly related: When loading the URL mentioned in Comment 0 and then reloading it, i get these two assertions
###!!! ASSERTION: Flush() failed: 'NS_SUCCEEDED(rv)', file h:/mozilla/tree-main/
mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp, line 465
###!!! ASSERTION: deleting dirty buffer: 'mBufDirty == PR_FALSE', file h:/mozill
a/tree-main/mozilla/netwerk/cache/src/nsDiskCacheStreams.cpp, line 750
many times (not only on that specific URL but on many pages, which are probably in disk cache).

Comment 6

[dolphinling]

Confirming based on comments.

Comment 7

[Darin Fisher]

Yup, I'm also able to see this bug.  It seems like it might be related to bug 189570.

-> me for investigation

Comment 8

[Darin Fisher]

I'm seeing a lot of HTTP channels being canceled with error 0x80540005, which is an Image Lib error code: NS_IMAGELIB_ERROR_FAILURE

Comment 9

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

hm, it does that when it finds the URL in the memory cache, iirc

Comment 10

[Darin Fisher]

Actually, I'm starting to become more suspicious of bug 321456.  I think there might be a miscalculation someplace where it is off by a factor of 1024 (not sure).

Comment 11

[Darin Fisher]

Attachment: v1 patch

This patch is larger than it needs to be.  Most of the patch is just me adding a bunch of log lines to the cache code (debug only).

Main changes otherwise:
1) Only the first call to nsIRequest::cancel matters.  Don't change the status of the channel again.
2) Set mOPenedCacheForWriting earlier, when we first start to configure mCacheEntry inside InitCacheEntry.
3) nsDiskCacheDevice::OnDataSizeChange does not understand how to deal with negative deltas properly, which occurs when a cache entry is being replaced.

Comment 12

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

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

cache/src/nsCacheEntryDescriptor.cpp
+nsCacheEntryDescriptor::GetMetaDataElement(const char *key, char **result)
 {
+    NS_ENSURE_ARG_POINTER(key && result);

string is the return value... I really don't think it needs to be nullchecked (JS can't pass null here)

cache/src/nsDiskCacheDevice.cpp
 nsDiskCacheDevice::EvictDiskCacheEntries(PRUint32  targetCapacity)
+    NS_ASSERTION(targetCapacity > 0, "oops");

When the pref for the size is set to 0 we don't reach this code, right? (because DiskCacheEnabled() returns false)

protocol/http/src/nsHttpHandler.cpp
-    LOG(("nsHttpHandler::AddStandardRequestHeaders\n"));

out of curiousity, is there a special reason for this?

Comment 13

[Darin Fisher]

> cache/src/nsCacheEntryDescriptor.cpp
> +nsCacheEntryDescriptor::GetMetaDataElement(const char *key, char **result)
>  {
> +    NS_ENSURE_ARG_POINTER(key && result);
> 
> string is the return value... I really don't think it needs to be nullchecked
> (JS can't pass null here)

I was just duplicating the existing code path, but you are correct.  That should not need to be null checked.


> cache/src/nsDiskCacheDevice.cpp
>  nsDiskCacheDevice::EvictDiskCacheEntries(PRUint32  targetCapacity)
> +    NS_ASSERTION(targetCapacity > 0, "oops");
> 
> When the pref for the size is set to 0 we don't reach this code, right?
> (because DiskCacheEnabled() returns false)

Right.


> protocol/http/src/nsHttpHandler.cpp
> -    LOG(("nsHttpHandler::AddStandardRequestHeaders\n"));
> 
> out of curiousity, is there a special reason for this?

Just general cleanup.  That logging statement adds little information.  I resisted the temptation to clean up more of the HTTP logging.  I plan on doing that as part of the nsBaseChannel patch for HTTP.

Comment 14

[Simon Howes]

I experience the same bug in Mac OS X.  however, trunk builds without Cairo enabled do not have this bug.  Is this bug only related to Cairo enabled?

I submitted a similar bug, but stating only with Cairo enabled:  https://bugzilla.mozilla.org/show_bug.cgi?id=329683

Comment 15

[Darin Fisher]

fixed-on-trunk

I think we probably want to apply the change to mOpenedCacheForWriting to the 1.8 branch as well.

Comment 16

[Simon Howes]

*** Bug 329683 has been marked as a duplicate of this bug. ***

Comment 17

[Darin Fisher]

Actually, after further investigation I don't think we need to make any changes for gecko 1.8.1