13 files changed, 162 insertions, 40 deletions

diff --git a/net/data/cache_tests/dirty_entry2/contents.txt b/net/data/cache_tests/dirty_entry2/contents.txt
new file mode 100644
index 0000000..cde34ce
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/contents.txt
@@ -0,0 +1,67 @@
+Index header:
+num_entries: 2
+num_bytes: 27
+this_id: 1
+table_len: 64k
+
+head: 0x90000001
+tail: 0x90000000
+
+Address: 0xa0010002
+Address: 0xa0010003
+
+-------------------------------
+
+entry:
+Address: 0xa0010002
+hash: 0x687d1422
+next: 0
+rankings_node: 0x90000000
+key_len: 13
+long_key: 0
+data_size: 0's
+data_addr: 0's
+key: "the first key"
+
+rankings:
+Address: 0x90000000
+next: 0x90000000
+prev: 0x90000001
+contents: 0xa0010002
+dirty: 0
+pointer: 0
+
+-------------------------------
+
+entry:
+Address: 0xa0010003
+hash: 0x63909ecb
+next: 0
+rankings_node: 0x90000001
+key_len: 14
+long_key: 0
+data_size: 0's
+data_addr: 0's
+key: "some other key"
+
+rankings:
+Address: 0x90000001
+next: 0x90000000
+prev: 0x90000001
+contents: 0xa0010003
+dirty: 2                  <- Next id!.
+pointer: 0x0169dc48       <- Invalid.
+
+================================
+
+Generated with:
+
+disk_cache::Entry *entry;
+ASSERT_TRUE(cache_->CreateEntry("the first key", &entry));
+entry->Close();
+
+ASSERT_TRUE(cache_->CreateEntry("some other key", &entry));
+entry->Close();                                              <---- Edit value*
+
+* Edit the value with the debugger before it is saved to disk (break on
+the destructor of EntryImpl and skip the line that clears "pointer")
\ No newline at end of file
diff --git a/net/data/cache_tests/dirty_entry2/data_0 b/net/data/cache_tests/dirty_entry2/data_0
new file mode 100644
index 0000000..5ba0deb
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/data_0
diff --git a/net/data/cache_tests/dirty_entry2/data_1 b/net/data/cache_tests/dirty_entry2/data_1
new file mode 100644
index 0000000..446ac45
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/data_1
diff --git a/net/data/cache_tests/dirty_entry2/data_2 b/net/data/cache_tests/dirty_entry2/data_2
new file mode 100644
index 0000000..c7e2eb9
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/data_2
diff --git a/net/data/cache_tests/dirty_entry2/data_3 b/net/data/cache_tests/dirty_entry2/data_3
new file mode 100644
index 0000000..5eec973
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/data_3
diff --git a/net/data/cache_tests/dirty_entry2/index b/net/data/cache_tests/dirty_entry2/index
new file mode 100644
index 0000000..ba2b6ca
--- /dev/null
+++ b/net/data/cache_tests/dirty_entry2/index
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;
   }