Disk cache: Always look for open entries when loading

a rankings node for a read only cache (AppCache)

BUG=137959
TEST=net_unittests
Review URL: https://chromiumcodereview.appspot.com/10795040

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

4 files changed, 56 insertions, 8 deletions

diff --git a/net/disk_cache/backend_impl.cc b/net/disk_cache/backend_impl.cc
index 00970b7..185de7b 100644
--- a/net/disk_cache/backend_impl.cc
+++ b/net/disk_cache/backend_impl.cc
@@ -701,6 +701,8 @@ EntryImpl* BackendImpl::OpenEntryImpl(const std::string& key) {
   eviction_.OnOpenEntry(cache_entry);
   entry_count_++;
 
+  Trace("Open hash 0x%x end: 0x%x", hash,
+        cache_entry->entry()->address().value());
   CACHE_UMA(AGE_MS, "OpenTime", 0, start);
   stats_.OnEvent(Stats::OPEN_HIT);
   SIMPLE_STATS_COUNTER("disk_cache.hit");
@@ -1174,7 +1176,8 @@ void BackendImpl::CriticalError(int error) {
 }
 
 void BackendImpl::ReportError(int error) {
-  STRESS_DCHECK(!error || error == ERR_PREVIOUS_CRASH);
+  STRESS_DCHECK(!error || error == ERR_PREVIOUS_CRASH ||
+                error == ERR_CACHE_CREATED);
 
   // We transmit positive numbers, instead of direct error codes.
   DCHECK_LE(error, 0);
diff --git a/net/disk_cache/backend_impl.h b/net/disk_cache/backend_impl.h
index 1dd33f1..d28dbba 100644
--- a/net/disk_cache/backend_impl.h
+++ b/net/disk_cache/backend_impl.h
@@ -186,6 +186,10 @@ class NET_EXPORT_PRIVATE BackendImpl : public Backend {
     return cache_type_;
   }
 
+  bool read_only() const {
+    return read_only_;
+  }
+
   // Returns a weak pointer to this object.
   base::WeakPtr<BackendImpl> GetWeakPtr();
 
diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc
index fd9cff7..5ff7d93 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -45,6 +45,7 @@ class DiskCacheEntryTest : public DiskCacheTestWithCache {
   void ReuseEntry(int size);
   void InvalidData();
   void DoomNormalEntry();
+  void DoomEntryNextToOpenEntry();
   void DoomedEntry();
   void BasicSparseIO();
   void HugeSparseIO();
@@ -1296,6 +1297,47 @@ TEST_F(DiskCacheEntryTest, MemoryOnlyDoomEntry) {
   DoomNormalEntry();
 }
 
+// Tests dooming an entry that's linked to an open entry.
+void DiskCacheEntryTest::DoomEntryNextToOpenEntry() {
+  InitCache();
+
+  disk_cache::Entry* entry1;
+  disk_cache::Entry* entry2;
+  ASSERT_EQ(net::OK, CreateEntry("fixed", &entry1));
+  entry1->Close();
+  ASSERT_EQ(net::OK, CreateEntry("foo", &entry1));
+  entry1->Close();
+  ASSERT_EQ(net::OK, CreateEntry("bar", &entry1));
+  entry1->Close();
+
+  ASSERT_EQ(net::OK, OpenEntry("foo", &entry1));
+  ASSERT_EQ(net::OK, OpenEntry("bar", &entry2));
+  entry2->Doom();
+  entry2->Close();
+
+  ASSERT_EQ(net::OK, OpenEntry("foo", &entry2));
+  entry2->Doom();
+  entry2->Close();
+  entry1->Close();
+
+  ASSERT_EQ(net::OK, OpenEntry("fixed", &entry1));
+  entry1->Close();
+}
+
+TEST_F(DiskCacheEntryTest, DoomEntryNextToOpenEntry) {
+  DoomEntryNextToOpenEntry();
+}
+
+TEST_F(DiskCacheEntryTest, NewEvictionDoomEntryNextToOpenEntry) {
+  SetNewEviction();
+  DoomEntryNextToOpenEntry();
+}
+
+TEST_F(DiskCacheEntryTest, AppCacheDoomEntryNextToOpenEntry) {
+  SetCacheType(net::APP_CACHE);
+  DoomEntryNextToOpenEntry();
+}
+
 // Verify that basic operations work as expected with doomed entries.
 void DiskCacheEntryTest::DoomedEntry() {
   std::string key("the first key");
diff --git a/net/disk_cache/rankings.cc b/net/disk_cache/rankings.cc
index d8da9f4..e86aa82 100644
--- a/net/disk_cache/rankings.cc
+++ b/net/disk_cache/rankings.cc
@@ -584,16 +584,15 @@ bool Rankings::GetRanking(CacheRankingsBlock* rankings) {
   backend_->OnEvent(Stats::OPEN_RANKINGS);
 
   // Note that if the cache is in read_only mode, open entries are not marked
-  // as dirty, so we can have multiple in-memory obects for the same address.
-  // However, by definition entries should not be mutating the state so the
-  // data at this point should be as good as the one from an entry tracked by
-  // the backend, and that other entry won't overwrite anything done by this
-  // one because it should not have the dirty flag set.
-  if (!rankings->Data()->dirty)
+  // as dirty, except when an entry is doomed. We have to look for open entries.
+  if (!backend_->read_only() && !rankings->Data()->dirty)
     return true;
 
   EntryImpl* entry = backend_->GetOpenEntry(rankings);
   if (!entry) {
+    if (backend_->read_only())
+      return true;
+
     // We cannot trust this entry, but we cannot initiate a cleanup from this
     // point (we may be in the middle of a cleanup already). The entry will be
     // deleted when detected from a regular open/create path.
@@ -845,7 +844,7 @@ int Rankings::CheckList(List list) {
     error += kOneInvalidEntry;
   } else if (rv == ERR_INVALID_ENTRY || rv2 == ERR_INVALID_ENTRY) {
     error += kOneInvalidEntryOneInvalidLink;
-  } else if (rv2 != 0) {
+  } else if (rv2 != ERR_NO_ERROR) {
     error += kTwoInvalidLinks;
   } else {
     error += kOneInvalidLink;