54470 - Calling nsINetDataDiskCache::RemoveAll(...) leads to disk cache corruption.

Status: VERIFIED FIXED

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

Description

[rpotts (gone)]

It appears that RemoveAll(...) should *never* be called.  Doing so throws away 
the underlying cache records, but leaves the nsCachedNetData entries in the 
cache manager.

These "orphaned" entries will cause the disk cache to think it is corrupt when 
Evict() is called, because the recordID associated with the entry no longer 
exists (it was removed by RemoveAll()).

Since the disk cache cannot locate the record in GetCachedNetDataByID(...) it 
calls DBRecovery().  Unfortunately, DBRecovery() calls RemoveAll() which just 
makes things worse :-)

Currently, RemoveAll() is called in two situations:
1. When the disk cache is cleared via the prefs panel.
2. Inside of DBRecovery(...)

I believe that the RemoveAll() functionality should be moved up into the cache 
manager.  Then the nsCachedNetData entries can be removed along with the cache 
records.

Comment 1

[rpotts (gone)]

As an aside...  Once RemoveAll() is called, if a URL is revisited, it will *not* 
reuse the DORMANT nsCachedNetData entry in the cache manager since the record ID 
returned from nsINetDataCache::GetCachedNetData(...) will *not* match the record 
ID in the DORMANT entry.  The IDs do not match because the disk cache will have 
created a new entry for the URL (with a new recordID).
Since the DORMANT entry is not use a new entry is created by the cache mgr for 
the same URL :-(

This means that the orphaned DORMANT records will remain (like land mines) 
pushing the cache manager closer to calling Evict() :-)

And I thought there was an international treaty against land mines!!

-- rick

Comment 2

[rpotts (gone)]

just adding some random keywords...

Comment 3

[Suresh Duddi (gone)]

I will do
- remove DBRecovery() if GetRecordID() fails
- propogate error up
- delete entry if this happens

Rick saz he will attack this from the top.

Comment 4

[Suresh Duddi (gone)]

Plusing for rtm.

Comment 5

[Suresh Duddi (gone)]

Attachment: Patch to delete cache entry with no record associated with it.

Comment 6

[Suresh Duddi (gone)]

Attachment: Improved patch. Keeping DBRecovery() for the real corruption case.

Comment 7

[rpotts (gone)]

Attachment: Patch to nsCacheManager::Clear(...)

Comment 8

[rpotts (gone)]

I've attached a patch which causes nsCacheManager::Clear(...) to delete both the 
underlying cache record and the nsCachedNetData entry held by the 
nsReplacementPolicy...

Basically, the fix is to call nsReplacementPolicy::DeleteAtleastOneEntry(...) 
with a target occupancy of zero...  Rather than just telling the underlying 
cache to remove all its records.

-- rick

Comment 9

[Scott MacGregor]

looks okay from here..sr=mscott

Comment 10

[Michael La Guardia]

Marking rtm++.  Please check this in as soon as possible.

Comment 11

[Suresh Duddi (gone)]

We decided that my patch will go into branch and tip. Rick's patch will go only
in the tip.

I am building now. Once I test with the latest tree, I will check it in.

Comment 12

[Suresh Duddi (gone)]

Fix checked in.

For testing purpose, rick maybe you should file a separate bug for the other
part and then, we can mark this bug fixed and leave it rtm++.

Comment 13

[Jim Roskind]

Is this now all checked in (trunk and branch)? If so, can we mark it fixed?

Comment 14

[Suresh Duddi (gone)]

Half the patch was checked into the branch. The portion checked into the truck 
deals with the clear cache button in the pref. Since that is isolated, and the 
other part had to do with cache corruption, I am marking this fixed.

Rick if you want to open another bug for the clear cache button and mark it 
fixed feel free. I dont think it is required.

Comment 15

[neeti]

Rick,

I think we should check in the patch for the clear cache button in the prefs 
panel, in the branch too, as this will eliminate DBRecovery being called when 
the clear button is used. DBRecovery throws away the underlying cache records, 
but leaves the nsCachedNetData entries in the cache manager. When the 
nsCachedNetData entry tries to access its mRecord, it contains some freed up 
value and we crash. See bug 52818 and bug 55616.

Comment 16

[Tom Everingham]

development issues - closing based on engineers comments