Disk cache: Keep a map of all open entries.

We still have a few crashes when for some reason we believe
an entry is not dirty and we follow the pointer stored by
its rankings node only to crash while accessing the memory.
I have no explanation to why the dirty id matches the current
one (a page boundary issue maybe?), but having a map with all
open entries solves the issue of having to follow pointers
from disk.

BUG=15596, b/1120346
TEST=unittests

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

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

7 files changed, 95 insertions, 40 deletions

diff --git a/net/disk_cache/backend_impl.cc b/net/disk_cache/backend_impl.cc
index d41efe8..3cd3cc2 100644
--- a/net/disk_cache/backend_impl.cc
+++ b/net/disk_cache/backend_impl.cc
@@ -426,6 +426,9 @@ bool BackendImpl::CreateEntry(const std::string& key, Entry** entry) {
     return false;
   }
 
+  // We are not failing the operation; let's add this to the map.
+  open_entries_[entry_address.value()] = cache_entry;
+
   if (parent.get())
     parent->SetNextAddress(entry_address);
 
@@ -720,11 +723,26 @@ void BackendImpl::RemoveEntry(EntryImpl* entry) {
   DecreaseNumEntries();
 }
 
-void BackendImpl::CacheEntryDestroyed() {
+void BackendImpl::CacheEntryDestroyed(Addr address) {
+  EntriesMap::iterator it = open_entries_.find(address.value());
+  if (it != open_entries_.end())
+    open_entries_.erase(it);
   DecreaseNumRefs();
 }
 
-int32 BackendImpl::GetCurrentEntryId() {
+bool BackendImpl::IsOpen(CacheRankingsBlock* rankings) const {
+  DCHECK(rankings->HasData());
+  EntriesMap::const_iterator it =
+      open_entries_.find(rankings->Data()->contents);
+  if (it != open_entries_.end()) {
+    // We have this entry in memory.
+    return rankings->Data()->pointer == it->second;
+  }
+
+  return false;
+}
+
+int32 BackendImpl::GetCurrentEntryId() const {
   return data_->header.this_id;
 }
 
@@ -1035,6 +1053,16 @@ void BackendImpl::PrepareForRestart() {
 }
 
 int BackendImpl::NewEntry(Addr address, EntryImpl** entry, bool* dirty) {
+  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;
+  }
+
   scoped_refptr<EntryImpl> cache_entry(new EntryImpl(this, address));
   IncreaseNumRefs();
   *entry = NULL;
@@ -1064,6 +1092,10 @@ int BackendImpl::NewEntry(Addr address, EntryImpl** entry, bool* dirty) {
   if (!rankings_.SanityCheck(cache_entry->rankings(), false))
     return ERR_INVALID_LINKS;
 
+  // We only add clean entries to the map.
+  if (!*dirty)
+    open_entries_[address.value()] = cache_entry;
+
   cache_entry.swap(entry);
   return 0;
 }
@@ -1119,11 +1151,17 @@ EntryImpl* BackendImpl::MatchEntry(const std::string& key, uint32 hash,
     }
 
     if (cache_entry->IsSameEntry(key, hash)) {
-      cache_entry = EntryImpl::Update(cache_entry);
+      if (!cache_entry->Update()) {
+        cache_entry->Release();
+        cache_entry = NULL;
+      }
       found = true;
       break;
     }
-    cache_entry = EntryImpl::Update(cache_entry);
+    if (!cache_entry->Update()) {
+      cache_entry->Release();
+      cache_entry = NULL;
+    }
     if (parent_entry)
       parent_entry->Release();
     parent_entry = cache_entry;
@@ -1279,9 +1317,11 @@ EntryImpl* BackendImpl::GetEnumeratedEntry(CacheRankingsBlock* next) {
 
     return NULL;
   }
+  if (!entry->Update())
+    return NULL;
 
   entry.swap(&temp);
-  return EntryImpl::Update(temp);  // Update returns an adref'd entry.
+  return temp;
 }
 
 bool BackendImpl::ResurrectEntry(EntryImpl* deleted_entry, Entry** entry) {
diff --git a/net/disk_cache/backend_impl.h b/net/disk_cache/backend_impl.h
index 0aae9de..84a10c1 100644
--- a/net/disk_cache/backend_impl.h
+++ b/net/disk_cache/backend_impl.h
@@ -7,6 +7,7 @@
 #ifndef NET_DISK_CACHE_BACKEND_IMPL_H_
 #define NET_DISK_CACHE_BACKEND_IMPL_H_
 
+#include "base/hash_tables.h"
 #include "base/timer.h"
 #include "net/disk_cache/block_files.h"
 #include "net/disk_cache/disk_cache.h"
@@ -99,10 +100,15 @@ class BackendImpl : public Backend {
   void RemoveEntry(EntryImpl* entry);
 
   // This method must be called whenever an entry is released for the last time.
-  void CacheEntryDestroyed();
+  // |address| is the cache address of the entry.
+  void CacheEntryDestroyed(Addr address);
+
+  // Returns true if the data stored by the provided |rankings| points to an
+  // open entry, false otherwise.
+  bool IsOpen(CacheRankingsBlock* rankings) const;
 
   // Returns the id being used on this run of the cache.
-  int32 GetCurrentEntryId();
+  int32 GetCurrentEntryId() const;
 
   // Returns the maximum size for a file to reside on the cache.
   int MaxFileSize() const;
@@ -169,6 +175,8 @@ class BackendImpl : public Backend {
   bool OpenPrevEntry(void** iter, Entry** prev_entry);
 
  private:
+  typedef base::hash_map<CacheAddr, EntryImpl*> EntriesMap;
+
   // Creates a new backing file for the cache index.
   bool CreateBackingStore(disk_cache::File* file);
   bool InitBackingStore(bool* file_created);
@@ -241,6 +249,7 @@ class BackendImpl : public Backend {
   uint32 mask_;  // Binary mask to map a hash to the hash table.
   int32 max_size_;  // Maximum data size for this instance.
   Eviction eviction_;  // Handler of the eviction algorithm.
+  EntriesMap open_entries_;  // Map of open entries.
   int num_refs_;  // Number of referenced cache entries.
   int max_refs_;  // Max number of referenced cache entries.
   int num_pending_io_;  // Number of pending IO operations.
diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index bd510c0..8364f5e 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -57,7 +57,7 @@ class DiskCacheBackendTest : public DiskCacheTestWithCache {
   void BackendRecoverInsert();
   void BackendRecoverRemove();
   void BackendInvalidEntry2();
-  void BackendNotMarkedButDirty();
+  void BackendNotMarkedButDirty(const std::wstring& name);
   void BackendDoomAll();
   void BackendDoomAll2();
   void BackendInvalidRankings();
@@ -974,8 +974,8 @@ TEST_F(DiskCacheBackendTest, NewEvictionInvalidEntry2) {
 }
 
 // We want to be able to deal with abnormal dirty entries.
-void DiskCacheBackendTest::BackendNotMarkedButDirty() {
-  ASSERT_TRUE(CopyTestCache(L"dirty_entry"));
+void DiskCacheBackendTest::BackendNotMarkedButDirty(const std::wstring& name) {
+  ASSERT_TRUE(CopyTestCache(name));
   DisableFirstCleanup();
   InitCache();
 
@@ -986,12 +986,21 @@ void DiskCacheBackendTest::BackendNotMarkedButDirty() {
 }
 
 TEST_F(DiskCacheBackendTest, NotMarkedButDirty) {
-  BackendNotMarkedButDirty();
+  BackendNotMarkedButDirty(L"dirty_entry");
 }
 
 TEST_F(DiskCacheBackendTest, NewEvictionNotMarkedButDirty) {
   SetNewEviction();
-  BackendNotMarkedButDirty();
+  BackendNotMarkedButDirty(L"dirty_entry");
+}
+
+TEST_F(DiskCacheBackendTest, NotMarkedButDirty2) {
+  BackendNotMarkedButDirty(L"dirty_entry2");
+}
+
+TEST_F(DiskCacheBackendTest, NewEvictionNotMarkedButDirty2) {
+  SetNewEviction();
+  BackendNotMarkedButDirty(L"dirty_entry2");
 }
 
 // We want to be able to deal with messed up entries on disk.
diff --git a/net/disk_cache/entry_impl.cc b/net/disk_cache/entry_impl.cc
index 0c8ee87..4c17cbb 100644
--- a/net/disk_cache/entry_impl.cc
+++ b/net/disk_cache/entry_impl.cc
@@ -128,7 +128,7 @@ EntryImpl::~EntryImpl() {
     }
   }
 
-  backend_->CacheEntryDestroyed();
+  backend_->CacheEntryDestroyed(entry_.address());
 }
 
 void EntryImpl::Doom() {
@@ -506,34 +506,27 @@ bool EntryImpl::LoadNodeAddress() {
   return node_.Load();
 }
 
-EntryImpl* EntryImpl::Update(EntryImpl* entry) {
-  DCHECK(entry->rankings()->HasData());
+bool EntryImpl::Update() {
+  DCHECK(node_.HasData());
 
-  RankingsNode* rankings = entry->rankings()->Data();
+  RankingsNode* rankings = node_.Data();
   if (rankings->pointer) {
-    // Already in memory. Prevent clearing the dirty flag on the destructor.
-    rankings->dirty = 0;
-    EntryImpl* real_node = reinterpret_cast<EntryImpl*>(rankings->pointer);
-    real_node->AddRef();
-    entry->Release();
-    return real_node;
+    // Nothing to do here, the entry was in memory.
+    DCHECK(rankings->pointer == this);
   } else {
-    rankings->dirty = entry->backend_->GetCurrentEntryId();
-    rankings->pointer = entry;
-    if (!entry->rankings()->Store()) {
-      entry->Release();
-      return NULL;
-    }
-    return entry;
+    rankings->dirty = backend_->GetCurrentEntryId();
+    rankings->pointer = this;
+    if (!node_.Store())
+      return false;
   }
+  return true;
 }
 
 bool EntryImpl::IsDirty(int32 current_id) {
   DCHECK(node_.HasData());
   // We are checking if the entry is valid or not. If there is a pointer here,
-  // |dirty| has to be the id of the cache that is using the entry (the one
-  // that created the pointer), 0 is not a valid id.
-  if (node_.Data()->pointer && !node_.Data()->dirty)
+  // we should not be checking the entry.
+  if (node_.Data()->pointer)
     return true;
 
   return node_.Data()->dirty && current_id != node_.Data()->dirty;
diff --git a/net/disk_cache/entry_impl.h b/net/disk_cache/entry_impl.h
index 89c9573..ad254e0 100644
--- a/net/disk_cache/entry_impl.h
+++ b/net/disk_cache/entry_impl.h
@@ -76,10 +76,10 @@ class EntryImpl : public Entry, public base::RefCounted<EntryImpl> {
   // Reloads the rankings node information.
   bool LoadNodeAddress();
 
-  // Reloads the data for this entry. If there is already an object in memory
-  // for the entry, the returned value is a pointer to that entry, otherwise
-  // it is the passed in entry. On failure returns NULL.
-  static EntryImpl* Update(EntryImpl* entry);
+  // Updates the stored data to reflect the run-time information for this entry.
+  // Returns false if the data could not be updated. The purpose of this method
+  // 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);
diff --git a/net/disk_cache/eviction.cc b/net/disk_cache/eviction.cc
index c29d730..14ffce9 100644
--- a/net/disk_cache/eviction.cc
+++ b/net/disk_cache/eviction.cc
@@ -182,7 +182,8 @@ bool Eviction::EvictEntry(CacheRankingsBlock* node, bool empty) {
   }
 
   if (node->Data()->pointer) {
-    entry = EntryImpl::Update(entry);
+    // We ignore the failure; we're removing the entry anyway.
+    entry->Update();
   }
   ReportTrimTimes(entry);
   if (empty || !new_eviction_) {
@@ -400,7 +401,8 @@ bool Eviction::RemoveDeletedNode(CacheRankingsBlock* node) {
   }
 
   if (node->Data()->pointer) {
-    entry = EntryImpl::Update(entry);
+    // We ignore the failure; we're removing the entry anyway.
+    entry->Update();
   }
   entry->entry()->Data()->state = ENTRY_DOOMED;
   entry->Doom();
diff --git a/net/disk_cache/rankings.cc b/net/disk_cache/rankings.cc
index 5e511bb..f0dee9b 100644
--- a/net/disk_cache/rankings.cc
+++ b/net/disk_cache/rankings.cc
@@ -223,14 +223,16 @@ bool Rankings::GetRanking(CacheRankingsBlock* rankings) {
 
   backend_->OnEvent(Stats::OPEN_RANKINGS);
 
-  if (backend_->GetCurrentEntryId() != rankings->Data()->dirty) {
+  if (backend_->GetCurrentEntryId() != rankings->Data()->dirty ||
+      !backend_->IsOpen(rankings)) {
     // 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
     // from a regular open/create path.
     rankings->Data()->pointer = NULL;
+    rankings->Data()->dirty = backend_->GetCurrentEntryId() - 1;
     if (!rankings->Data()->dirty)
-      rankings->Data()->dirty = backend_->GetCurrentEntryId() - 1;
+      rankings->Data()->dirty--;
     return true;
   }