Http Cache: Deactivate entries without having the key of the

entry to delete.

The disk cache may fail to provide the key for a given entry
(if there is a disk error, for instance), so we fall back
to enumerate the active entries to delete this one.

BUG=9952
TEST=unittest

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

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

3 files changed, 77 insertions, 6 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index e0b19e5..5035fb9 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -1733,13 +1733,16 @@ HttpCache::ActiveEntry* HttpCache::ActivateEntry(
 #endif
 // Avoid optimizing local_entry out of the code.
 void HttpCache::DeactivateEntry(ActiveEntry* entry) {
+  std::string key = entry->disk_entry->GetKey();
+  if (key.empty())
+    return SlowDeactivateEntry(entry);
+
   // TODO(rvargas): remove this code and go back to DCHECKS once we find out
   // why are we crashing. I'm just trying to gather more info for bug 3931.
   ActiveEntry local_entry = *entry;
   size_t readers_size = local_entry.readers.size();
   size_t pending_size = local_entry.pending_queue.size();
 
-  std::string key = entry->disk_entry->GetKey();
   ActiveEntriesMap::iterator it = active_entries_.find(key);
   if (it == active_entries_.end() || it->second != entry ||
       local_entry.will_process_pending_queue || local_entry.doomed ||
@@ -1767,6 +1770,18 @@ void HttpCache::DeactivateEntry(ActiveEntry* entry) {
 #pragma optimize("", on)
 #endif
 
+// We don't know this entry's key so we have to find it without it.
+void HttpCache::SlowDeactivateEntry(ActiveEntry* entry) {
+  for (ActiveEntriesMap::iterator it = active_entries_.begin();
+       it != active_entries_.end(); ++it) {
+    if (it->second == entry) {
+      active_entries_.erase(it);
+      delete entry;
+      break;
+    }
+  }
+}
+
 int HttpCache::AddTransactionToEntry(ActiveEntry* entry, Transaction* trans) {
   DCHECK(entry);
 
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 034ade1..1a76180 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -146,6 +146,7 @@ class HttpCache : public HttpTransactionFactory {
   ActiveEntry* FindActiveEntry(const std::string& key);
   ActiveEntry* ActivateEntry(const std::string& key, disk_cache::Entry*);
   void DeactivateEntry(ActiveEntry* entry);
+  void SlowDeactivateEntry(ActiveEntry* entry);
   ActiveEntry* OpenEntry(const std::string& key);
   ActiveEntry* CreateEntry(const std::string& cache_key);
   void DestroyEntry(ActiveEntry* entry);
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 7678a5f..3f9e667 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -30,11 +30,11 @@ class MockDiskEntry : public disk_cache::Entry,
                       public base::RefCounted<MockDiskEntry> {
  public:
   MockDiskEntry()
-      : test_mode_(0), doomed_(false), sparse_(false) {
+      : test_mode_(0), doomed_(false), sparse_(false), fail_requests_(false) {
   }
 
   explicit MockDiskEntry(const std::string& key)
-      : key_(key), doomed_(false), sparse_(false) {
+      : key_(key), doomed_(false), sparse_(false), fail_requests_(false) {
     //
     // 'key' is prefixed with an identifier if it corresponds to a cached POST.
     // Skip past that to locate the actual URL.
@@ -70,6 +70,8 @@ class MockDiskEntry : public disk_cache::Entry,
   }
 
   virtual std::string GetKey() const {
+    if (fail_requests_)
+      return std::string();
     return key_;
   }
 
@@ -90,6 +92,9 @@ class MockDiskEntry : public disk_cache::Entry,
                        net::CompletionCallback* callback) {
     DCHECK(index >= 0 && index < 2);
 
+    if (fail_requests_)
+      return net::ERR_CACHE_READ_FAILURE;
+
     if (offset < 0 || offset > static_cast<int>(data_[index].size()))
       return net::ERR_FAILED;
     if (static_cast<size_t>(offset) == data_[index].size())
@@ -110,6 +115,9 @@ class MockDiskEntry : public disk_cache::Entry,
     DCHECK(index >= 0 && index < 2);
     DCHECK(truncate);
 
+    if (fail_requests_)
+      return net::ERR_CACHE_READ_FAILURE;
+
     if (offset < 0 || offset > static_cast<int>(data_[index].size()))
       return net::ERR_FAILED;
 
@@ -126,6 +134,9 @@ class MockDiskEntry : public disk_cache::Entry,
     if (offset < 0)
       return net::ERR_FAILED;
 
+    if (fail_requests_)
+      return net::ERR_CACHE_READ_FAILURE;
+
     DCHECK(offset < kint32max);
     int real_offset = static_cast<int>(offset);
     if (!buf_len)
@@ -154,6 +165,9 @@ class MockDiskEntry : public disk_cache::Entry,
     if (!buf_len)
       return 0;
 
+    if (fail_requests_)
+      return net::ERR_CACHE_READ_FAILURE;
+
     DCHECK(offset < kint32max);
     int real_offset = static_cast<int>(offset);
 
@@ -170,6 +184,9 @@ class MockDiskEntry : public disk_cache::Entry,
     if (offset < 0)
       return net::ERR_FAILED;
 
+    if (fail_requests_)
+      return net::ERR_CACHE_READ_FAILURE;
+
     *start = offset;
     DCHECK(offset < kint32max);
     int real_offset = static_cast<int>(offset);
@@ -193,6 +210,9 @@ class MockDiskEntry : public disk_cache::Entry,
     return count;
   }
 
+  // Fail most subsequent requests.
+  void set_fail_requests() { fail_requests_ = true; }
+
  private:
   // Unlike the callbacks for MockHttpTransaction, we want this one to run even
   // if the consumer called Close on the MockDiskEntry.  We achieve that by
@@ -210,11 +230,14 @@ class MockDiskEntry : public disk_cache::Entry,
   int test_mode_;
   bool doomed_;
   bool sparse_;
+  bool fail_requests_;
 };
 
 class MockDiskCache : public disk_cache::Backend {
  public:
-  MockDiskCache() : open_count_(0), create_count_(0), fail_requests_(0) {
+  MockDiskCache()
+      : open_count_(0), create_count_(0), fail_requests_(false),
+        soft_failures_(false) {
   }
 
   ~MockDiskCache() {
@@ -246,6 +269,9 @@ class MockDiskCache : public disk_cache::Backend {
     it->second->AddRef();
     *entry = it->second;
 
+    if (soft_failures_)
+      it->second->set_fail_requests();
+
     return true;
   }
 
@@ -266,6 +292,9 @@ class MockDiskCache : public disk_cache::Backend {
     new_entry->AddRef();
     *entry = new_entry;
 
+    if (soft_failures_)
+      new_entry->set_fail_requests();
+
     return true;
   }
 
@@ -310,12 +339,16 @@ class MockDiskCache : public disk_cache::Backend {
   // Fail any subsequent CreateEntry and OpenEntry.
   void set_fail_requests() { fail_requests_ = true; }
 
+  // Return entries that fail some of their requests.
+  void set_soft_failures(bool value) { soft_failures_ = value; }
+
  private:
   typedef base::hash_map<std::string, MockDiskEntry*> EntryMap;
   EntryMap entries_;
   int open_count_;
   int create_count_;
   bool fail_requests_;
+  bool soft_failures_;
 };
 
 class MockHttpCache {
@@ -583,6 +616,26 @@ TEST(HttpCache, SimpleGETNoDiskCache) {
   EXPECT_EQ(0, cache.disk_cache()->create_count());
 }
 
+TEST(HttpCache, SimpleGETWithDiskFailures) {
+  MockHttpCache cache;
+
+  cache.disk_cache()->set_soft_failures(true);
+
+  // Read from the network, and fail to write to the cache.
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // This one should see an empty cache again.
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
 TEST(HttpCache, SimpleGET_LoadOnlyFromCache_Hit) {
   MockHttpCache cache;
 
@@ -847,7 +900,8 @@ TEST(HttpCache, SimpleGET_RacingReaders) {
   c = context_list[1];
   ASSERT_EQ(net::ERR_IO_PENDING, c->result);
   c->result = c->callback.WaitForResult();
-  ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
+  if (c->result == net::OK)
+    ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
 
   // At this point we have one reader, two pending transactions and a task on
   // the queue to move to the next transaction. Now we cancel the request that
@@ -861,7 +915,8 @@ TEST(HttpCache, SimpleGET_RacingReaders) {
     Context* c = context_list[i];
     if (c->result == net::ERR_IO_PENDING)
       c->result = c->callback.WaitForResult();
-    ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
+    if (c->result == net::OK)
+      ReadAndVerifyTransaction(c->trans.get(), kSimpleGET_Transaction);
   }
 
   // We should not have had to re-open the disk entry.