55405 - SetSizeEntry(..) called during shutdown could lead to corrupted cache.

Status: RESOLVED FIXED

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

Description

[neeti]

Currently, we write dowm the number of entries, and the number of storage bytes 
in use to the database only during shutdown.

Comment 1

[neeti]

The SetSizeEntry(..) method is called in the destructer of nsNetDiskCache, which  
is called only during shutdown. If there is a leak, or if the app crashes, then 
these values are never written out to the database, leading to cache corruption.

I think we should call SetSizeEntry at the end of each url load, so the cache is  
always in sync.

My fix is to call SetSizeEntry in nsCachedNetData::Release(..).

Comment 2

[neeti]

Attachment: patch for the bug

Comment 3

[Gagan]

neeti: once you have the review-set this to rtm+

Comment 4

[neeti]

I am attaching a new patch which instead of updating the database for every url 
load, detects a corrupt cache (Which could be due to a leak or due to a crash) 
at startup and does a DbRecovery. 

Comment 5

[neeti]

Attachment: patch to detect and clean cache corruption at startup.

Comment 6

[Suresh Duddi (gone)]

Attachment: Added xpcom shutdown observer to earlier patch from neeti.

Comment 7

[neeti]

Attachment: patch using AssignWithConversion instead of NS_LITERAL_STRING

Comment 8

[neeti]

I have attached another patch, using AssignWithConversion instead of 
NS_LITERAL_STRING, since it does not compile with NS_LITERAL_STRING.

Rick,

We need a super review on this patch.

Thanks,
Neeti

Comment 9

[rpotts (gone)]

hey neeti,

this patch looks fine.  The only question I have is why the NetDiskCache needs 
to be an nsWeakReference?  It doesn't look like your changes take advantage of 
that...

(sr=rpotts)

-- rick

Comment 10

[Suresh Duddi (gone)]

ObserverService will store a weakref to an observer if it implemented it. That
way the observer service doesn't have a owning reference to the object
increasing its lifetime, and the object and observerservice can get destroyed in
any order and all will be fine. That is why we dont even try to RemoveObserver too.

This is the std way for doing observers. :-)

Setting rtm+ as this bug has been reviewed and super reviewed.

Comment 11

[neeti]

Attachment: latest patch to return error from DBRecovery if cache.db file in use

Comment 12

[Suresh Duddi (gone)]

r=dp That is for being paranoid and preventing an infinite loop if the db file 
is not writeable (no permission and stuff). We disable the cache in that 
senario.

Comment 13

[neeti]

I attached a patch to return an error from RemoveAll(...) which is called by 
DBRecovery(..), if the cache.db file exists and it could not be deleted (it 
could be in use). This prevents an infinite loop condition, where we could end 
up calling DBRecovery(..) again and again.

Rick, could you please do a super review for this?

Thanks,
Neeti

Comment 14

[neeti]

Attachment: patch with updated cache version

Comment 15

[neeti]

I added a one line change to nsNetDiskCache.h to update the cache version. This 
will blow away all old corrupt caches, so everyone starts with a clean new 
cache.

Comment 16

[rpotts (gone)]

(sr=rpotts)

Comment 17

[Phil Peterson]

This all sounds like good stuff, but do we have evidence that this is really
needed? It sounds like we're talking about hypothetical bugs here, and without a
real bug, it's hard to verify that the fix was effective. Marking [rtm need
info] until the cost of shipping without this is clearer.

Comment 18

[neeti]

I have noticed the leak of nsNetDiskCache on many instances. Just browsing on 
the mozilla site causes these leaks. Putterman has also seen these leaks. Also, 
anytime the broser crashes,these values are never written out to the cache. For 
the cache to work correctly, we need an accurate values of the number of entries 
and number of storage bytes in use, since the cache logic relies heavily on 
these numbers. The cost of shipping without this is we have all these above 
cases where we would corrupt the cache, which would lead to all sorts of other 
cache corruption bugs, including crashers. See bug 54072. This fix would 
minimize the cases,where we have the corrupt cache, and in the cases where we 
get a corrupt cache (due to a crash), it would clean up the corrupted cache.

Comment 19

[scottputterman]

This bug isn't hypothetical. When I was testing cache crashes I continually hit
this case. It wasn't hard.  When this happens the db gets completely out of
synch with what it really contains (because it thinks it has 10 items when it
really has 100, for example) and eventually either led to the crash in 54072 or
made the cache unuseable because other error checking prevented new items from
getting added.

Comment 20

[Gagan]

PDT: As putterman pointed out this is a common crash scenario and a much needed
fix. Recommending we accept this. 

Comment 21

[Peter Trudelle]

PDT needs info: JAR thought cache corruption was no longer fatal, i.e., cache
was self-healing.  Is this true?  

Comment 22

[neeti]

This is the fix for self-healing of the cache. Without this fix, the cache would 
not function properly, or would become useless, because other error checking 
would prevent new items from being added. See bug 50559. 

Renominating again for rtm++.

Comment 23

[neeti]

Cache corruptness could also cause the disk size to grow without bound, because 
the actual size of the database could be out of sync with what it really 
contains. See bug 42606.

Comment 24

[Peter Trudelle]

PDT says rtm++, land on branch ASAP

Comment 25

[neeti]

Fix checked in on branch and trunk.

Comment 26

[neeti]

Marking it fixed

Comment 27

[Tom Everingham]

verified on branch:
WinNT4 10/30
Linux rh6 10/30
Mac os9 10/30

needs verified on trunk