Closed Bug 1074832 Opened 10 years ago Closed 10 years ago

CacheIndex merges journal with pending updates incorrectly

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: michal, Assigned: michal)

Details

Attachments

(1 file, 2 obsolete files)

Once all index data is read successfully from the disk, we merge first mTmpJournal with mIndex and then mPendingUpdates with mIndex. The problem is that the entries in mPendingUpdates are created as a copy of entries in mIndex and we don't track what data in CacheIndexRecord was updated. In case we won't update all data in the record we can end up with an obsolete information with a "fresh" flag and such entries are ignored during update process, so we will keep the incorrect data until shutdown or until someone reloads the resource.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8502287 - Flags: review?(honzab.moz)
Comment on attachment 8502287 [details] [diff] [review]
patch v1

Review of attachment 8502287 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: netwerk/cache2/CacheIndex.cpp
@@ +985,5 @@
>        MOZ_ASSERT(updated || entry);
>  
>        if (!updated) {
> +        if (!entry) {
> +          // LOG

finish the log or remove the comment

::: netwerk/cache2/CacheIndex.h
@@ +316,5 @@
> +    : CacheIndexEntry(aKey)
> +  {
> +    MOZ_COUNT_CTOR(CacheIndexEntryUpdate);
> +    LOG(("CacheIndexEntryUpdate::CacheIndexEntryUpdate()"));
> +    mUpdateFlags = 0;

maybe rather use initializer

@@ +473,5 @@
>      MOZ_ASSERT(!mStateLogged, "CacheIndexStats::Size() - state logged!");
>      return mSize;
>    }
>  
> +  void BeforeChange(const CacheIndexEntry *aEntry) {

I slightly more prefer to write const after the subject you want constify.  It's according to c++ (before is just a C backward compatibility exception) and makes the code a bit more readable. Up to you.
Attachment #8502287 - Flags: review?(honzab.moz) → review+
Attached patch patch v2 (obsolete) — Splinter Review
https://hg.mozilla.org/integration/mozilla-inbound/rev/f156971f0f4d
Attachment #8502287 - Attachment is obsolete: true
Attachment #8504756 - Flags: review+
Attached patch patch v3Splinter Review
Fixed wrong initialization of mUpdateFlags in CacheIndexEntryUpdate::InitNew()

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f6709b2d5145
Attachment #8504756 - Attachment is obsolete: true
Attachment #8512713 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9c7cc05533b3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.