Bug 1074832 - CacheIndex merges journal with pending updates incorrectly, r=honzab

This commit is contained in:
Michal Novotny 2014-10-14 16:33:06 +02:00
parent 1337194399
commit 20087db9e6
2 changed files with 178 additions and 61 deletions

View File

@ -53,7 +53,7 @@ public:
mIndex->AssertOwnsLock();
mHash = aHash;
CacheIndexEntry *entry = FindEntry();
const CacheIndexEntry *entry = FindEntry();
mIndex->mIndexStats.BeforeChange(entry);
if (entry && entry->IsInitialized() && !entry->IsRemoved()) {
mOldRecord = entry->mRec;
@ -66,7 +66,7 @@ public:
{
mIndex->AssertOwnsLock();
CacheIndexEntry *entry = FindEntry();
const CacheIndexEntry *entry = FindEntry();
mIndex->mIndexStats.AfterChange(entry);
if (!entry || !entry->IsInitialized() || entry->IsRemoved()) {
entry = nullptr;
@ -125,9 +125,9 @@ public:
void DoNotSearchInUpdates() { mDoNotSearchInUpdates = true; }
private:
CacheIndexEntry * FindEntry()
const CacheIndexEntry * FindEntry()
{
CacheIndexEntry *entry = nullptr;
const CacheIndexEntry *entry = nullptr;
switch (mIndex->mState) {
case CacheIndex::READING:
@ -520,6 +520,7 @@ CacheIndex::AddEntry(const SHA1Sum::Hash *aHash)
CacheIndexEntry *entry = index->mIndex.GetEntry(*aHash);
bool entryRemoved = entry && entry->IsRemoved();
CacheIndexEntryUpdate *updated = nullptr;
if (index->mState == READY || index->mState == UPDATING ||
index->mState == BUILDING) {
@ -558,7 +559,7 @@ CacheIndex::AddEntry(const SHA1Sum::Hash *aHash)
entry = index->mIndex.PutEntry(*aHash);
}
} else { // WRITING, READING
CacheIndexEntry *updated = index->mPendingUpdates.GetEntry(*aHash);
updated = index->mPendingUpdates.GetEntry(*aHash);
bool updatedRemoved = updated && updated->IsRemoved();
if ((updated && !updatedRemoved) ||
@ -578,12 +579,17 @@ CacheIndex::AddEntry(const SHA1Sum::Hash *aHash)
}
updated = index->mPendingUpdates.PutEntry(*aHash);
entry = updated;
}
entry->InitNew();
entry->MarkDirty();
entry->MarkFresh();
if (updated) {
updated->InitNew();
updated->MarkDirty();
updated->MarkFresh();
} else {
entry->InitNew();
entry->MarkDirty();
entry->MarkFresh();
}
}
if (updateIfNonFreshEntriesExist &&
@ -652,7 +658,7 @@ CacheIndex::EnsureEntryExists(const SHA1Sum::Hash *aHash)
}
entry->MarkFresh();
} else { // WRITING, READING
CacheIndexEntry *updated = index->mPendingUpdates.GetEntry(*aHash);
CacheIndexEntryUpdate *updated = index->mPendingUpdates.GetEntry(*aHash);
bool updatedRemoved = updated && updated->IsRemoved();
if (updatedRemoved ||
@ -732,6 +738,7 @@ CacheIndex::InitEntry(const SHA1Sum::Hash *aHash,
CacheIndexEntryAutoManage entryMng(aHash, index);
CacheIndexEntry *entry = index->mIndex.GetEntry(*aHash);
CacheIndexEntryUpdate *updated = nullptr;
bool reinitEntry = false;
if (entry && entry->IsRemoved()) {
@ -753,7 +760,7 @@ CacheIndex::InitEntry(const SHA1Sum::Hash *aHash,
}
}
} else {
CacheIndexEntry *updated = index->mPendingUpdates.GetEntry(*aHash);
updated = index->mPendingUpdates.GetEntry(*aHash);
DebugOnly<bool> removed = updated && updated->IsRemoved();
MOZ_ASSERT(updated || !removed);
@ -770,7 +777,6 @@ CacheIndex::InitEntry(const SHA1Sum::Hash *aHash,
return NS_OK;
}
}
entry = updated;
} else {
MOZ_ASSERT(entry->IsFresh());
@ -786,18 +792,28 @@ CacheIndex::InitEntry(const SHA1Sum::Hash *aHash,
// make a copy of a read-only entry
updated = index->mPendingUpdates.PutEntry(*aHash);
*updated = *entry;
entry = updated;
}
}
if (reinitEntry) {
// There is a collision and we are going to rewrite this entry. Initialize
// it as a new entry.
entry->InitNew();
entry->MarkFresh();
if (updated) {
updated->InitNew();
updated->MarkFresh();
} else {
entry->InitNew();
entry->MarkFresh();
}
}
if (updated) {
updated->Init(aAppId, aAnonymous, aInBrowser);
updated->MarkDirty();
} else {
entry->Init(aAppId, aAnonymous, aInBrowser);
entry->MarkDirty();
}
entry->Init(aAppId, aAnonymous, aInBrowser);
entry->MarkDirty();
}
index->StartUpdatingIndexIfNeeded();
@ -865,7 +881,7 @@ CacheIndex::RemoveEntry(const SHA1Sum::Hash *aHash)
}
}
} else { // WRITING, READING
CacheIndexEntry *updated = index->mPendingUpdates.GetEntry(*aHash);
CacheIndexEntryUpdate *updated = index->mPendingUpdates.GetEntry(*aHash);
bool updatedRemoved = updated && updated->IsRemoved();
if (updatedRemoved ||
@ -945,42 +961,58 @@ CacheIndex::UpdateEntry(const SHA1Sum::Hash *aHash,
if (!HasEntryChanged(entry, aFrecency, aExpirationTime, aSize)) {
return NS_OK;
}
MOZ_ASSERT(entry->IsFresh());
MOZ_ASSERT(entry->IsInitialized());
entry->MarkDirty();
if (aFrecency) {
entry->SetFrecency(*aFrecency);
}
if (aExpirationTime) {
entry->SetExpirationTime(*aExpirationTime);
}
if (aSize) {
entry->SetFileSize(*aSize);
}
} else {
CacheIndexEntry *updated = index->mPendingUpdates.GetEntry(*aHash);
CacheIndexEntryUpdate *updated = index->mPendingUpdates.GetEntry(*aHash);
DebugOnly<bool> removed = updated && updated->IsRemoved();
MOZ_ASSERT(updated || !removed);
MOZ_ASSERT(updated || entry);
if (!updated) {
if (entry &&
HasEntryChanged(entry, aFrecency, aExpirationTime, aSize)) {
// make a copy of a read-only entry
updated = index->mPendingUpdates.PutEntry(*aHash);
*updated = *entry;
entry = updated;
} else {
if (!entry) {
LOG(("CacheIndex::UpdateEntry() - Entry was found neither in mIndex "
"nor in mPendingUpdates!"));
NS_WARNING(("CacheIndex::UpdateEntry() - Entry was found neither in "
"mIndex nor in mPendingUpdates!"));
return NS_ERROR_NOT_AVAILABLE;
}
} else {
entry = updated;
// make a copy of a read-only entry
updated = index->mPendingUpdates.PutEntry(*aHash);
*updated = *entry;
}
}
MOZ_ASSERT(entry->IsFresh());
MOZ_ASSERT(entry->IsInitialized());
entry->MarkDirty();
MOZ_ASSERT(updated->IsFresh());
MOZ_ASSERT(updated->IsInitialized());
updated->MarkDirty();
if (aFrecency) {
entry->SetFrecency(*aFrecency);
}
if (aFrecency) {
updated->SetFrecency(*aFrecency);
}
if (aExpirationTime) {
entry->SetExpirationTime(*aExpirationTime);
}
if (aExpirationTime) {
updated->SetExpirationTime(*aExpirationTime);
}
if (aSize) {
entry->SetFileSize(*aSize);
if (aSize) {
updated->SetFileSize(*aSize);
}
}
}
@ -1096,7 +1128,7 @@ CacheIndex::HasEntry(const nsACString &aKey, EntryStatus *_retval)
sum.update(aKey.BeginReading(), aKey.Length());
sum.finish(hash);
CacheIndexEntry *entry = nullptr;
const CacheIndexEntry *entry = nullptr;
switch (index->mState) {
case READING:
@ -1481,7 +1513,7 @@ CacheIndex::ProcessPendingOperations()
// static
PLDHashOperator
CacheIndex::UpdateEntryInIndex(CacheIndexEntry *aEntry, void* aClosure)
CacheIndex::UpdateEntryInIndex(CacheIndexEntryUpdate *aEntry, void* aClosure)
{
CacheIndex *index = static_cast<CacheIndex *>(aClosure);
@ -1516,8 +1548,16 @@ CacheIndex::UpdateEntryInIndex(CacheIndexEntry *aEntry, void* aClosure)
return PL_DHASH_REMOVE;
}
entry = index->mIndex.PutEntry(*aEntry->Hash());
*entry = *aEntry;
if (entry) {
// Some information in mIndex can be newer than in mPendingUpdates (see bug
// 1074832). This will copy just those values that were really updated.
aEntry->ApplyUpdate(entry);
} else {
// There is no entry in mIndex, copy all information from mPendingUpdates
// to mIndex.
entry = index->mIndex.PutEntry(*aEntry->Hash());
*entry = *aEntry;
}
return PL_DHASH_REMOVE;
}

View File

@ -166,32 +166,32 @@ public:
}
}
const SHA1Sum::Hash * Hash() { return &mRec->mHash; }
const SHA1Sum::Hash * Hash() const { return &mRec->mHash; }
bool IsInitialized() { return !!(mRec->mFlags & kInitializedMask); }
bool IsInitialized() const { return !!(mRec->mFlags & kInitializedMask); }
uint32_t AppId() { return mRec->mAppId; }
bool Anonymous() { return !!(mRec->mFlags & kAnonymousMask); }
bool InBrowser() { return !!(mRec->mFlags & kInBrowserMask); }
uint32_t AppId() const { return mRec->mAppId; }
bool Anonymous() const { return !!(mRec->mFlags & kAnonymousMask); }
bool InBrowser() const { return !!(mRec->mFlags & kInBrowserMask); }
bool IsRemoved() { return !!(mRec->mFlags & kRemovedMask); }
bool IsRemoved() const { return !!(mRec->mFlags & kRemovedMask); }
void MarkRemoved() { mRec->mFlags |= kRemovedMask; }
bool IsDirty() { return !!(mRec->mFlags & kDirtyMask); }
bool IsDirty() const { return !!(mRec->mFlags & kDirtyMask); }
void MarkDirty() { mRec->mFlags |= kDirtyMask; }
void ClearDirty() { mRec->mFlags &= ~kDirtyMask; }
bool IsFresh() { return !!(mRec->mFlags & kFreshMask); }
bool IsFresh() const { return !!(mRec->mFlags & kFreshMask); }
void MarkFresh() { mRec->mFlags |= kFreshMask; }
void SetFrecency(uint32_t aFrecency) { mRec->mFrecency = aFrecency; }
uint32_t GetFrecency() { return mRec->mFrecency; }
uint32_t GetFrecency() const { return mRec->mFrecency; }
void SetExpirationTime(uint32_t aExpirationTime)
{
mRec->mExpirationTime = aExpirationTime;
}
uint32_t GetExpirationTime() { return mRec->mExpirationTime; }
uint32_t GetExpirationTime() const { return mRec->mExpirationTime; }
// Sets filesize in kilobytes.
void SetFileSize(uint32_t aFileSize)
@ -205,12 +205,12 @@ public:
mRec->mFlags |= aFileSize;
}
// Returns filesize in kilobytes.
uint32_t GetFileSize() { return GetFileSize(mRec); }
uint32_t GetFileSize() const { return GetFileSize(mRec); }
static uint32_t GetFileSize(CacheIndexRecord *aRec)
{
return aRec->mFlags & kFileSizeMask;
}
bool IsFileEmpty() { return GetFileSize() == 0; }
bool IsFileEmpty() const { return GetFileSize() == 0; }
void WriteToBuf(void *aBuf)
{
@ -246,7 +246,7 @@ public:
mRec->mFlags = NetworkEndian::readUint32(&src->mFlags);
}
void Log() {
void Log() const {
LOG(("CacheIndexEntry::Log() [this=%p, hash=%08x%08x%08x%08x%08x, fresh=%u,"
" initialized=%u, removed=%u, dirty=%u, anonymous=%u, inBrowser=%u, "
"appId=%u, frecency=%u, expirationTime=%u, size=%u]",
@ -280,6 +280,7 @@ public:
}
private:
friend class CacheIndexEntryUpdate;
friend class CacheIndex;
friend class CacheIndexEntryAutoManage;
@ -308,6 +309,82 @@ private:
nsAutoPtr<CacheIndexRecord> mRec;
};
class CacheIndexEntryUpdate : public CacheIndexEntry
{
public:
explicit CacheIndexEntryUpdate(CacheIndexEntry::KeyTypePointer aKey)
: CacheIndexEntry(aKey)
, mUpdateFlags(0)
{
MOZ_COUNT_CTOR(CacheIndexEntryUpdate);
LOG(("CacheIndexEntryUpdate::CacheIndexEntryUpdate()"));
}
~CacheIndexEntryUpdate()
{
MOZ_COUNT_DTOR(CacheIndexEntryUpdate);
LOG(("CacheIndexEntryUpdate::~CacheIndexEntryUpdate()"));
}
CacheIndexEntryUpdate& operator=(const CacheIndexEntry& aOther)
{
MOZ_ASSERT(memcmp(&mRec->mHash, &aOther.mRec->mHash,
sizeof(SHA1Sum::Hash)) == 0);
mUpdateFlags = 0;
*(static_cast<CacheIndexEntry *>(this)) = aOther;
return *this;
}
void InitNew()
{
mUpdateFlags = 0;
CacheIndexEntry::InitNew();
}
void SetFrecency(uint32_t aFrecency)
{
mUpdateFlags |= kFrecencyUpdatedMask;
CacheIndexEntry::SetFrecency(aFrecency);
}
void SetExpirationTime(uint32_t aExpirationTime)
{
mUpdateFlags |= kExpirationUpdatedMask;
CacheIndexEntry::SetExpirationTime(aExpirationTime);
}
void SetFileSize(uint32_t aFileSize)
{
mUpdateFlags |= kFileSizeUpdatedMask;
CacheIndexEntry::SetFileSize(aFileSize);
}
void ApplyUpdate(CacheIndexEntry *aDst) {
MOZ_ASSERT(memcmp(&mRec->mHash, &aDst->mRec->mHash,
sizeof(SHA1Sum::Hash)) == 0);
if (mUpdateFlags & kFrecencyUpdatedMask) {
aDst->mRec->mFrecency = mRec->mFrecency;
}
if (mUpdateFlags & kExpirationUpdatedMask) {
aDst->mRec->mExpirationTime = mRec->mExpirationTime;
}
aDst->mRec->mAppId = mRec->mAppId;
if (mUpdateFlags & kFileSizeUpdatedMask) {
aDst->mRec->mFlags = mRec->mFlags;
} else {
// Copy all flags except file size.
aDst->mRec->mFlags &= kFileSizeMask;
aDst->mRec->mFlags |= (mRec->mFlags & ~kFileSizeMask);
}
}
private:
static const uint32_t kFrecencyUpdatedMask = 0x00000001;
static const uint32_t kExpirationUpdatedMask = 0x00000002;
static const uint32_t kFileSizeUpdatedMask = 0x00000004;
uint32_t mUpdateFlags;
};
class CacheIndexStats
{
public:
@ -397,7 +474,7 @@ public:
return mSize;
}
void BeforeChange(CacheIndexEntry *aEntry) {
void BeforeChange(const CacheIndexEntry *aEntry) {
#ifdef DEBUG_STATS
if (!mDisableLogging) {
LOG(("CacheIndexStats::BeforeChange()"));
@ -441,7 +518,7 @@ public:
}
}
void AfterChange(CacheIndexEntry *aEntry) {
void AfterChange(const CacheIndexEntry *aEntry) {
MOZ_ASSERT(mStateLogged, "CacheIndexStats::AfterChange() - state not "
"logged!");
#ifdef DEBUG
@ -642,7 +719,7 @@ private:
// Merge all pending operations from mPendingUpdates into mIndex.
void ProcessPendingOperations();
static PLDHashOperator UpdateEntryInIndex(CacheIndexEntry *aEntry,
static PLDHashOperator UpdateEntryInIndex(CacheIndexEntryUpdate *aEntry,
void* aClosure);
// Following methods perform writing of the index file.
@ -929,7 +1006,7 @@ private:
// We cannot add, remove or change any entry in mIndex in states READING and
// WRITING. We track all changes in mPendingUpdates during these states.
nsTHashtable<CacheIndexEntry> mPendingUpdates;
nsTHashtable<CacheIndexEntryUpdate> mPendingUpdates;
// Contains information statistics for mIndex + mPendingUpdates.
CacheIndexStats mIndexStats;