CacheIndex merges journal with pending updates incorrectly

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Posted 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+
Posted 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.