604880 - Doom cache-entry if writing data and/or metadata fails

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

This comes from bug #588804, comment #60. If something bad happens when writing or flushing data or metadata for a cache-entry the entry should be doomed and the cache should be cleaned up.

Comment 1

[Bjarne (:bjarne)]

Assigned, added dependency and requesting blocking2.0

Comment 2

[Michal Novotny [:michal]]

Attachment: fix

New patch fixing one more problem. If writing of metadata fails then the eviction rank in record and cachemap could get out of sync, which causes an assertion in nsDiskCacheMap::DeleteRecord().

Comment 3

[Bjarne (:bjarne)]

Comment on attachment 483876 [details] [diff] [review]
fix

Looks good. The added comment in WriteDiskCacheEntry() is a little confusing - please be more specific about which failure is acceptable. Also: have you checked the other two cache-devices for similar issues?

I guess it would be hard to write a testcase for this but if possible, please give it a shot.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v3

(In reply to comment #3)
> Looks good. The added comment in WriteDiskCacheEntry() is a little confusing -
> please be more specific about which failure is acceptable. Also: have you
> checked the other two cache-devices for similar issues?

It isn't acceptable, we just need to fail after the call to UpdateRecord(). I've changed the comment a bit. I checked both and didn't find similar problem there.

> I guess it would be hard to write a testcase for this but if possible, please
> give it a shot.

Actually it shouldn't be too hard to write the test now, but after fixing bug #604897 it wouldn't work anymore.

Comment 5

[Bjarne (:bjarne)]

Comment on attachment 483999 [details] [diff] [review]
patch v3

Looks fine, assuming the other devices are not subject tho similar issues.

>         // flush buffer to block files
>         if (mStreamEnd > 0) {
>             rv = cacheMap->WriteDataCacheBlocks(mBinding, mBuffer, mBufEnd);
>             if (NS_FAILED(rv)) {
>                 NS_WARNING("WriteDataCacheBlocks() failed.");
>-                return rv;   // XXX doom cache entry?
>-                
>+                nsCacheService::DoomEntry(mBinding->mCacheEntry);
>+                mBufDirty = PR_FALSE;
>+                return rv;
>             }
>         }
> 
>         mBufDirty = PR_FALSE;

Clear |mBufDirty| before the if-block?  r=bjarne with this.

Comment 6

[Johnny Stenback (:jst)]

Blocking.

Comment 7

[Michal Novotny [:michal]]

Attachment: patch v4

Comment 8

[Jason Duell]

Comment on attachment 484356 [details] [diff] [review]
patch v4

I'm not a superreviewer. Reassign to bz or biesi?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484356 [details] [diff] [review]
patch v4

sr=me

Comment 10

[Johnny Stenback (:jst)]

Checked in.

http://hg.mozilla.org/mozilla-central/rev/5ae431772486