Http cache: Don't attempt to doom the same entry multiple times

and make sure that side effects of adding the truncation flag are
considered by the cache.

BUG=125159
TEST=net_unittests
Review URL: https://chromiumcodereview.appspot.com/10382089

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

4 files changed, 108 insertions, 3 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index 363b321..41bd1c0 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -860,6 +860,9 @@ void HttpCache::DoneWithEntry(ActiveEntry* entry, Transaction* trans,
       // This is a successful operation in the sense that we want to keep the
       // entry.
       success = trans->AddTruncatedFlag();
+      // The previous operation may have deleted the entry.
+      if (!trans->entry())
+        return;
     }
     DoneWritingToEntry(entry, success);
   } else {
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index f6a8efc..527118a 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -1054,8 +1054,10 @@ int HttpCache::Transaction::DoUpdateCachedResponse() {
   response_.request_time = new_response_->request_time;
 
   if (response_.headers->HasHeaderValue("cache-control", "no-store")) {
-    int ret = cache_->DoomEntry(cache_key_, NULL);
-    DCHECK_EQ(OK, ret);
+    if (!entry_->doomed) {
+      int ret = cache_->DoomEntry(cache_key_, NULL);
+      DCHECK_EQ(OK, ret);
+    }
   } else {
     // If we are already reading, we already updated the headers for this
     // request; doing it again will change Content-Length.
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 531601e..9592723 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -85,9 +85,12 @@ class HttpCache::Transaction : public HttpTransaction {
 
   // This transaction is being deleted and we are not done writing to the cache.
   // We need to indicate that the response data was truncated.  Returns true on
-  // success.
+  // success. Keep in mind that this operation may have side effects, such as
+  // deleting the active entry.
   bool AddTruncatedFlag();
 
+  HttpCache::ActiveEntry* entry() { return entry_; }
+
   // Returns the LoadState of the writer transaction of a given ActiveEntry. In
   // other words, returns the LoadState of this transaction without asking the
   // http cache, because this transaction should be the one currently writing
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index a780f2d..dbf0c0f 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -3971,6 +3971,103 @@ TEST(HttpCache, GET_IncompleteResource) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+// Tests the handling of no-store when revalidating a truncated entry.
+TEST(HttpCache, GET_IncompleteResource_NoStore) {
+  MockHttpCache cache;
+  AddMockTransaction(&kRangeGET_TransactionOK);
+
+  std::string raw_headers("HTTP/1.1 200 OK\n"
+                          "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
+                          "ETag: \"foo\"\n"
+                          "Accept-Ranges: bytes\n"
+                          "Content-Length: 80\n");
+  CreateTruncatedEntry(raw_headers, &cache);
+  RemoveMockTransaction(&kRangeGET_TransactionOK);
+
+  // Now make a regular request.
+  MockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.request_headers = EXTRA_HEADER;
+  std::string response_headers(transaction.response_headers);
+  response_headers += ("Cache-Control: no-store\n");
+  transaction.response_headers = response_headers.c_str();
+  transaction.data = "rg: 00-09 rg: 10-19 rg: 20-29 rg: 30-39 rg: 40-49 "
+                     "rg: 50-59 rg: 60-69 rg: 70-79 ";
+  AddMockTransaction(&transaction);
+
+  std::string headers;
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  // We update the headers with the ones received while revalidating.
+  std::string expected_headers(
+      "HTTP/1.1 200 OK\n"
+      "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
+      "Accept-Ranges: bytes\n"
+      "Cache-Control: no-store\n"
+      "ETag: \"foo\"\n"
+      "Content-Length: 80\n");
+
+  EXPECT_EQ(expected_headers, headers);
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Verify that the disk entry was deleted.
+  disk_cache::Entry* entry;
+  EXPECT_FALSE(cache.OpenBackendEntry(kRangeGET_TransactionOK.url, &entry));
+  RemoveMockTransaction(&transaction);
+}
+
+// Tests cancelling a request after the server sent no-store.
+TEST(HttpCache, GET_IncompleteResource_Cancel) {
+  MockHttpCache cache;
+  AddMockTransaction(&kRangeGET_TransactionOK);
+
+  std::string raw_headers("HTTP/1.1 200 OK\n"
+                          "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
+                          "ETag: \"foo\"\n"
+                          "Accept-Ranges: bytes\n"
+                          "Content-Length: 80\n");
+  CreateTruncatedEntry(raw_headers, &cache);
+  RemoveMockTransaction(&kRangeGET_TransactionOK);
+
+  // Now make a regular request.
+  MockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.request_headers = EXTRA_HEADER;
+  std::string response_headers(transaction.response_headers);
+  response_headers += ("Cache-Control: no-store\n");
+  transaction.response_headers = response_headers.c_str();
+  transaction.data = "rg: 00-09 rg: 10-19 rg: 20-29 rg: 30-39 rg: 40-49 "
+                     "rg: 50-59 rg: 60-69 rg: 70-79 ";
+  AddMockTransaction(&transaction);
+
+  MockHttpRequest request(transaction);
+  Context* c = new Context();
+
+  int rv = cache.http_cache()->CreateTransaction(&c->trans);
+  EXPECT_EQ(net::OK, rv);
+
+  rv = c->trans->Start(&request, c->callback.callback(), net::BoundNetLog());
+  EXPECT_EQ(net::OK, c->callback.GetResult(rv));
+
+  // Make sure that the entry has some data stored.
+  scoped_refptr<net::IOBufferWithSize> buf(new net::IOBufferWithSize(5));
+  rv = c->trans->Read(buf, buf->size(), c->callback.callback());
+  EXPECT_EQ(5, c->callback.GetResult(rv));
+
+  // Cancel the request.
+  delete c;
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Verify that the disk entry was deleted.
+  disk_cache::Entry* entry;
+  EXPECT_FALSE(cache.OpenBackendEntry(kRangeGET_TransactionOK.url, &entry));
+  MessageLoop::current()->RunAllPending();
+  RemoveMockTransaction(&transaction);
+}
+
 // Tests that we delete truncated entries if the server changes its mind midway.
 TEST(HttpCache, GET_IncompleteResource2) {
   MockHttpCache cache;