1074832 - CacheIndex merges journal with pending updates incorrectly

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

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.

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

Comment 2

[Honza Bambas (:mayhemer)]

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.

Comment 3

[Michal Novotny [:michal]]

Attachment: patch v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/f156971f0f4d

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/786d392dbb42

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2992000&repo=mozilla-inbound

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v3

Fixed wrong initialization of mUpdateFlags in CacheIndexEntryUpdate::InitNew()

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f6709b2d5145

Comment 6

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7cc05533b3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c7cc05533b3