Disk cache: More fixes to avoid corruption.

* We now keep dirty entries in the map of open entries
so that we handle better crashes when we are working with
dirty entries.

* EntryImpl now remembers if the entry was dirty on disk.

* When we find a dirty entry while doing enumerations we
now go through the regular path to delete the entry
(InternalDoomEntry), and let MatchEntry delete the entry.
The main problem with the old code is that it was possible
to delete the entry without first removing it from the index,
so a crash at that time would leave references to free addresses.

* Now we correctly consider the case of not finding an entry
when looking for its parent as an error.

BUG=62085
TEST=net_unittests

Review URL: http://codereview.chromium.org/6538006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75685 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 37 insertions, 66 deletions

diff --git a/net/disk_cache/backend_impl.cc b/net/disk_cache/backend_impl.cc
index 7fc1639..58d75ad 100644
--- a/net/disk_cache/backend_impl.cc
+++ b/net/disk_cache/backend_impl.cc
@@ -894,8 +894,7 @@ void BackendImpl::UpdateRank(EntryImpl* entry, bool modified) {
 void BackendImpl::RecoveredEntry(CacheRankingsBlock* rankings) {
   Addr address(rankings->Data()->contents);
   EntryImpl* cache_entry = NULL;
-  bool dirty;
-  if (NewEntry(address, &cache_entry, &dirty))
+  if (NewEntry(address, &cache_entry))
     return;
 
   uint32 hash = cache_entry->GetHash();
@@ -918,8 +917,15 @@ void BackendImpl::InternalDoomEntry(EntryImpl* entry) {
 
   Trace("Doom entry 0x%p", entry);
 
-  eviction_.OnDoomEntry(entry);
-  entry->InternalDoom();
+  if (!entry->doomed()) {
+    // We may have doomed this entry from within MatchEntry.
+    eviction_.OnDoomEntry(entry);
+    entry->InternalDoom();
+    if (!new_eviction_) {
+      DecreaseNumEntries();
+    }
+    stats_.OnEvent(Stats::DOOM_ENTRY);
+  }
 
   if (parent_entry) {
     parent_entry->SetNextAddress(Addr(child));
@@ -927,12 +933,6 @@ void BackendImpl::InternalDoomEntry(EntryImpl* entry) {
   } else if (!error) {
     data_->table[hash & mask_] = child;
   }
-
-  if (!new_eviction_) {
-    DecreaseNumEntries();
-  }
-
-  stats_.OnEvent(Stats::DOOM_ENTRY);
 }
 
 // An entry may be linked on the DELETED list for a while after being doomed.
@@ -1064,6 +1064,8 @@ bool BackendImpl::ShouldReportAgain() {
 
 void BackendImpl::FirstEviction() {
   DCHECK(data_->header.create_time);
+  if (!GetEntryCount())
+    return;  // This is just for unit tests.
 
   Time create_time = Time::FromInternalValue(data_->header.create_time);
   CACHE_UMA(AGE, "FillupAge", 0, create_time);
@@ -1517,14 +1519,13 @@ void BackendImpl::PrepareForRestart() {
   restarted_ = true;
 }
 
-int BackendImpl::NewEntry(Addr address, EntryImpl** entry, bool* dirty) {
+int BackendImpl::NewEntry(Addr address, EntryImpl** entry) {
   EntriesMap::iterator it = open_entries_.find(address.value());
   if (it != open_entries_.end()) {
     // Easy job. This entry is already in memory.
     EntryImpl* this_entry = it->second;
     this_entry->AddRef();
     *entry = this_entry;
-    *dirty = false;
     return 0;
   }
 
@@ -1555,22 +1556,19 @@ int BackendImpl::NewEntry(Addr address, EntryImpl** entry, bool* dirty) {
   if (!cache_entry->LoadNodeAddress())
     return ERR_READ_FAILURE;
 
-  *dirty = cache_entry->IsDirty(GetCurrentEntryId());
-
   // Prevent overwriting the dirty flag on the destructor.
-  cache_entry->ClearDirtyFlag();
+  cache_entry->SetDirtyFlag(GetCurrentEntryId());
 
   if (!rankings_.SanityCheck(cache_entry->rankings(), false))
     return ERR_INVALID_LINKS;
 
-  if (*dirty) {
+  if (cache_entry->dirty()) {
     Trace("Dirty entry 0x%p 0x%x", reinterpret_cast<void*>(cache_entry.get()),
           address.value());
-  } else {
-    // We only add clean entries to the map.
-    open_entries_[address.value()] = cache_entry;
   }
 
+  open_entries_[address.value()] = cache_entry;
+
   cache_entry->BeginLogging(net_log_, false);
   cache_entry.swap(entry);
   return 0;
@@ -1605,11 +1603,10 @@ EntryImpl* BackendImpl::MatchEntry(const std::string& key, uint32 hash,
       break;
     }
 
-    bool dirty;
-    int error = NewEntry(address, &tmp, &dirty);
+    int error = NewEntry(address, &tmp);
     cache_entry.swap(&tmp);
 
-    if (error || dirty) {
+    if (error || cache_entry->dirty()) {
       // This entry is dirty on disk (it was not properly closed): we cannot
       // trust it.
       Addr child(0);
@@ -1623,6 +1620,9 @@ EntryImpl* BackendImpl::MatchEntry(const std::string& key, uint32 hash,
         data_->table[hash & mask_] = child.value();
       }
 
+      Trace("MatchEntry dirty %d 0x%x 0x%x", find_parent, entry_addr.value(),
+            address.value());
+
       if (!error) {
         // It is important to call DestroyInvalidEntry after removing this
         // entry from the table.
@@ -1663,6 +1663,11 @@ EntryImpl* BackendImpl::MatchEntry(const std::string& key, uint32 hash,
   if (parent_entry && (!find_parent || !found))
     parent_entry = NULL;
 
+  if (find_parent && entry_addr.is_initialized() && !cache_entry) {
+    *match_error = true;
+    parent_entry = NULL;
+  }
+
   if (cache_entry && (find_parent || !found))
     cache_entry = NULL;
 
@@ -1775,13 +1780,16 @@ EntryImpl* BackendImpl::GetEnumeratedEntry(CacheRankingsBlock* next) {
     return NULL;
 
   EntryImpl* entry;
-  bool dirty;
-  if (NewEntry(Addr(next->Data()->contents), &entry, &dirty))
+  if (NewEntry(Addr(next->Data()->contents), &entry)) {
+    // TODO(rvargas) bug 73102: We should remove this node from the list, and
+    // maybe do a better cleanup.
     return NULL;
+  }
 
-  if (dirty) {
-    // We cannot trust this entry. This code also releases the reference.
-    DestroyInvalidEntryFromEnumeration(entry);
+  if (entry->dirty()) {
+    // We cannot trust this entry.
+    InternalDoomEntry(entry);
+    entry->Release();
     return NULL;
   }
 
@@ -1837,42 +1845,6 @@ void BackendImpl::DestroyInvalidEntry(EntryImpl* entry) {
   stats_.OnEvent(Stats::INVALID_ENTRY);
 }
 
-// This is kind of ugly. The entry may or may not be part of the cache index
-// table, and it may even have corrupt fields. If we just doom it, we may end up
-// deleting it twice (if all fields are right, and when looking up the parent of
-// chained entries wee see this one... and we delete it because it is dirty). If
-// we ignore it, we may leave it here forever. So we're going to attempt to
-// delete it through the provided object, without touching the index table
-// (because we cannot jus call MatchEntry()), and also attempt to delete it from
-// the table through the key: this may find a new entry (too bad), or an entry
-// that was just deleted and consider it a very corrupt entry.
-void BackendImpl::DestroyInvalidEntryFromEnumeration(EntryImpl* entry) {
-  std::string key = entry->GetKey();
-  entry->SetPointerForInvalidEntry(GetCurrentEntryId());
-  CacheAddr next_entry = entry->GetNextAddress();
-  if (!next_entry) {
-    DestroyInvalidEntry(entry);
-    entry->Release();
-  }
-  SyncDoomEntry(key);
-
-  if (!next_entry)
-    return;
-
-  // We have a chained entry so instead of destroying first this entry and then
-  // anything with this key, we just called DoomEntry() first. If that call
-  // deleted everything, |entry| has invalid data. Let's see if there is
-  // something else to do. We started with just a rankings node (we come from
-  // an enumeration), so that one may still be there.
-  CacheRankingsBlock* rankings = entry->rankings();
-  rankings->Load();
-  if (rankings->Data()->contents) {
-    // We still have something. Clean this up.
-    DestroyInvalidEntry(entry);
-  }
-  entry->Release();
-}
-
 void BackendImpl::AddStorageSize(int32 bytes) {
   data_->header.num_bytes += bytes;
   DCHECK_GE(data_->header.num_bytes, 0);
@@ -2088,15 +2060,14 @@ int BackendImpl::CheckAllEntries() {
     if (!address.is_initialized())
       continue;
     for (;;) {
-      bool dirty;
       EntryImpl* tmp;
-      int ret = NewEntry(address, &tmp, &dirty);
+      int ret = NewEntry(address, &tmp);
       if (ret)
         return ret;
       scoped_refptr<EntryImpl> cache_entry;
       cache_entry.swap(&tmp);
 
-      if (dirty)
+      if (cache_entry->dirty())
         num_dirty++;
       else if (CheckEntry(cache_entry.get()))
         num_entries++;