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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Networking: Cache
--
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bjarne, Assigned: michal)

Tracking

Trunk
mozilla2.0b7
Points:
---

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Assigned, added dependency and requesting blocking2.0
Assignee: nobody → michal.novotny
Blocks: 588804
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
Created attachment 483876 [details] [diff] [review]
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().
Attachment #483876 - Flags: review?(bjarne)
(Reporter)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
Created attachment 483999 [details] [diff] [review]
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.
Attachment #483876 - Attachment is obsolete: true
Attachment #483999 - Flags: review?(bjarne)
Attachment #483876 - Flags: review?(bjarne)
(Reporter)

Comment 5

7 years ago
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.
Attachment #483999 - Flags: review?(bjarne) → review+
Blocking.
blocking2.0: ? → beta8+
(Assignee)

Comment 7

7 years ago
Created attachment 484356 [details] [diff] [review]
patch v4
Attachment #483999 - Attachment is obsolete: true
Attachment #484356 - Flags: superreview?(jduell.mcbugs)
Comment on attachment 484356 [details] [diff] [review]
patch v4

I'm not a superreviewer. Reassign to bz or biesi?
Attachment #484356 - Flags: superreview?(jduell.mcbugs) → superreview?
(Assignee)

Updated

7 years ago
Attachment #484356 - Flags: superreview? → superreview?(bzbarsky)
Comment on attachment 484356 [details] [diff] [review]
patch v4

sr=me
Attachment #484356 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Checked in.

http://hg.mozilla.org/mozilla-central/rev/5ae431772486
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: checkin-needed
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7

Updated

7 years ago
Duplicate of this bug: 595184
You need to log in before you can comment on or make changes to this bug.