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

6 files changed, 75 insertions, 85 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++;
diff --git a/net/disk_cache/backend_impl.h b/net/disk_cache/backend_impl.h
index be491e6..9be5d4a 100644
--- a/net/disk_cache/backend_impl.h
+++ b/net/disk_cache/backend_impl.h
@@ -282,9 +282,9 @@ class BackendImpl : public Backend {
   void RestartCache(bool failure);
   void PrepareForRestart();
 
-  // Creates a new entry object and checks to see if it is dirty. Returns zero
-  // on success, or a disk_cache error on failure.
-  int NewEntry(Addr address, EntryImpl** entry, bool* dirty);
+  // Creates a new entry object. Returns zero on success, or a disk_cache error
+  // on failure.
+  int NewEntry(Addr address, EntryImpl** entry);
 
   // Returns a given entry from the cache. The entry to match is determined by
   // key and hash, and the returned entry may be the matched one or it's parent
@@ -313,7 +313,6 @@ class BackendImpl : public Backend {
   EntryImpl* ResurrectEntry(EntryImpl* deleted_entry);
 
   void DestroyInvalidEntry(EntryImpl* entry);
-  void DestroyInvalidEntryFromEnumeration(EntryImpl* entry);
 
   // Handles the used storage count.
   void AddStorageSize(int32 bytes);
diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index 0a693be..1d7037a 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -1393,6 +1393,21 @@ TEST_F(DiskCacheBackendTest, InvalidEntry5) {
   TrimDeletedListForTest(false);
 }
 
+TEST_F(DiskCacheBackendTest, InvalidEntry6) {
+  ASSERT_TRUE(CopyTestCache("dirty_entry5"));
+  SetMask(0x1);  // 2-entry table.
+  SetMaxSize(0x3000);  // 12 kB.
+  DisableFirstCleanup();
+  InitCache();
+
+  // There is a dirty entry (but marked as clean) at the end, pointing to a
+  // deleted entry through the hash collision list. We should not re-insert the
+  // deleted entry into the index table.
+
+  TrimForTest(false);
+  // The cache should be clean (as detected by CheckCacheIntegrity).
+}
+
 // Tests that we don't hang when there is a loop on the hash collision list.
 // The test cache could be a result of bug 69135.
 TEST_F(DiskCacheBackendTest, BadNextEntry1) {
diff --git a/net/disk_cache/entry_impl.cc b/net/disk_cache/entry_impl.cc
index 57d4d83..085efd2 100644
--- a/net/disk_cache/entry_impl.cc
+++ b/net/disk_cache/entry_impl.cc
@@ -366,10 +366,9 @@ bool EntryImpl::UserBuffer::GrowBuffer(int required, int limit) {
 // ------------------------------------------------------------------------
 
 EntryImpl::EntryImpl(BackendImpl* backend, Addr address, bool read_only)
-    : entry_(NULL, Addr(0)), node_(NULL, Addr(0)), read_only_(read_only) {
+    : entry_(NULL, Addr(0)), node_(NULL, Addr(0)), backend_(backend),
+      doomed_(false), read_only_(read_only), dirty_(false) {
   entry_.LazyInit(backend->File(address), address);
-  doomed_ = false;
-  backend_ = backend;
   for (int i = 0; i < kNumStreams; i++) {
     unreported_size_[i] = 0;
   }
@@ -617,18 +616,15 @@ bool EntryImpl::Update() {
   return true;
 }
 
-bool EntryImpl::IsDirty(int32 current_id) {
+void EntryImpl::SetDirtyFlag(int32 current_id) {
   DCHECK(node_.HasData());
   // We are checking if the entry is valid or not. If there is a pointer here,
   // we should not be checking the entry.
   if (node_.Data()->dummy)
-    return true;
-
-  return node_.Data()->dirty && current_id != node_.Data()->dirty;
-}
+    dirty_ = true;
 
-void EntryImpl::ClearDirtyFlag() {
-  node_.Data()->dirty = 0;
+  if (node_.Data()->dirty && current_id != node_.Data()->dirty)
+    dirty_ = true;
 }
 
 void EntryImpl::SetPointerForInvalidEntry(int32 new_id) {
@@ -882,7 +878,7 @@ EntryImpl::~EntryImpl() {
       int current_id = backend_->GetCurrentEntryId();
       node_.Data()->dirty = current_id == 1 ? -1 : current_id - 1;
       node_.Store();
-    } else if (node_.HasData() && node_.Data()->dirty) {
+    } else if (node_.HasData() && !dirty_) {
       node_.Data()->dirty = 0;
       node_.Store();
     }
diff --git a/net/disk_cache/entry_impl.h b/net/disk_cache/entry_impl.h
index e56fc6b..99e7b4e 100644
--- a/net/disk_cache/entry_impl.h
+++ b/net/disk_cache/entry_impl.h
@@ -87,9 +87,17 @@ class EntryImpl : public Entry, public base::RefCounted<EntryImpl> {
   // is to be able to detect entries that are currently in use.
   bool Update();
 
-  // Returns true if this entry is marked as dirty on disk.
-  bool IsDirty(int32 current_id);
-  void ClearDirtyFlag();
+  bool dirty() {
+    return dirty_;
+  }
+
+  bool doomed() {
+    return doomed_;
+  }
+
+  // Marks this entry as dirty (in memory) if needed. This is intended only for
+  // entries that are being read from disk, to be called during loading.
+  void SetDirtyFlag(int32 current_id);
 
   // Fixes this entry so it can be treated as valid (to delete it).
   void SetPointerForInvalidEntry(int32 new_id);
@@ -234,6 +242,7 @@ class EntryImpl : public Entry, public base::RefCounted<EntryImpl> {
   int unreported_size_[kNumStreams];  // Bytes not reported yet to the backend.
   bool doomed_;               // True if this entry was removed from the cache.
   bool read_only_;            // True if not yet writing.
+  bool dirty_;                // True if we detected that this is a dirty entry.
   scoped_ptr<SparseControl> sparse_;  // Support for sparse entries.
 
   net::BoundNetLog net_log_;
diff --git a/net/disk_cache/rankings.cc b/net/disk_cache/rankings.cc
index b10dac6..3644932 100644
--- a/net/disk_cache/rankings.cc
+++ b/net/disk_cache/rankings.cc
@@ -549,7 +549,7 @@ bool Rankings::GetRanking(CacheRankingsBlock* rankings) {
     return true;
 
   EntryImpl* entry = backend_->GetOpenEntry(rankings);
-  if (backend_->GetCurrentEntryId() != rankings->Data()->dirty || !entry) {
+  if (!entry) {
     // We cannot trust this entry, but we cannot initiate a cleanup from this
     // point (we may be in the middle of a cleanup already). Just get rid of
     // the invalid pointer and continue; the entry will be deleted when detected