DoomPartialEntry should not attempt to doom an already doomed entry.

net/http/http_cache_unittest.cc

Index: net/http/http_cache_unittest.cc
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index b01427a7a6164b85c0ede119fb4c1da12d1bc7d7..01b461c476c893a83187c0f28a917f8c03cb2952 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -147,7 +147,7 @@ void TestLoadTimingCachedResponse(const LoadTimingInfo& load_timing_info) {
   EXPECT_TRUE(load_timing_info.receive_headers_end.is_null());
 }
 
-void DeferNetworkStart(bool* defer) {
+void DeferCallback(bool* defer) {
   *defer = true;
 }
 
@@ -1478,6 +1478,75 @@ TEST(HttpCache, RangeGET_ParallelValidationNoMatch) {
   EXPECT_EQ(5, cache.disk_cache()->create_count());
 }
 
+// Tests that if a transaction is dooming the entry and the entry was doomed by
+// another transaction that was not part of the entry, there should not be a
+// assertion hit. (crbug.com/736993)

[jkarlin, 2017/07/07 20:03:15]

After F2F we discussed that this test doesn't actually reproduce the assertion
from the bug. There are two related bug behaviors and we should test both.

Scenario 1: Dooming the wrong entry

1. Transaction one is about to read headers
2. Transaction two dooms and creates a new entry
3. Transaction one dooms (the HC looks up and deletes the new entry!)


Scenario 2: Nothing to doom

1. Transaction one is about to read headers
2. Transaction two dooms and hasn't created its new entry yet
3. Transaction one dooms, the HC doesn't know what to do because it can't find
the entry and DCHECKS

We should test both of these.

[shivanisha, 2017/07/10 18:51:27]

Added 2 tests. Without the fix, the first dooms the wrong entry and second
crashes with the same stack trace as reported.

+TEST(HttpCache, RangeGET_ParallelValidationNoMatchDoomEntry) {
+  MockHttpCache cache;
+
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
+  MockHttpRequest request(transaction);
+
+  MockTransaction dooming_transaction(kRangeGET_TransactionOK);
+  dooming_transaction.load_flags |= LOAD_BYPASS_CACHE;
+  MockHttpRequest dooming_request(dooming_transaction);
+
+  std::vector<std::unique_ptr<Context>> context_list;
+  const int kNumTransactions = 3;
+
+  for (int i = 0; i < kNumTransactions; ++i) {
+    context_list.push_back(base::MakeUnique<Context>());
+    auto& c = context_list[i];
+
+    c->result = cache.CreateTransaction(&c->trans);
+    ASSERT_THAT(c->result, IsOk());
+    EXPECT_EQ(LOAD_STATE_IDLE, c->trans->GetLoadState());
+
+    MockHttpRequest* this_request = &request;
+
+    if (i == 2)
+      this_request = &dooming_request;
+
+    if (i == 1) {
+      cache.disk_cache()->SetBeforeCacheOperationCallback(
+          kRangeGET_TransactionOK.url, base::Bind(&DeferCallback));
+    }
+
+    c->result = c->trans->Start(this_request, c->callback.callback(),
+                                NetLogWithSource());
+
+    // Continue the transactions. 2nd will pause at the cache reading state and
+    // 3rd transaction will doom the entry.
+    base::RunLoop().RunUntilIdle();
+  }
+
+  EXPECT_TRUE(
+      cache.disk_cache()->IsDiskEntryDoomed(kRangeGET_TransactionOK.url));
+
+  // Resume cache read by 1st transaction which will lead to dooming the entry
+  // as well since the entry cannot be validated. This double dooming should not
+  // lead to an assertion.
+  cache.disk_cache()->ResumeDoomedEntryCacheOperation(
+      kRangeGET_TransactionOK.url);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(3, cache.disk_cache()->create_count());
+
+  for (auto& context : context_list) {
+    EXPECT_EQ(LOAD_STATE_IDLE, context->trans->GetLoadState());
+  }
+
+  for (auto& c : context_list) {
+    ReadAndVerifyTransaction(c->trans.get(), kRangeGET_TransactionOK);
+  }
+
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(3, cache.disk_cache()->create_count());
+}
+
 // Tests parallel validation on range requests with non-overlapping ranges.
 TEST(HttpCache, RangeGET_ParallelValidationDifferentRanges) {
   MockHttpCache cache;
@@ -2096,7 +2165,7 @@ TEST(HttpCache, SimpleGET_ParallelValidationCancelReader) {
     MockHttpRequest* this_request = &request;
     if (i == 3) {
       this_request = &validate_request;
-      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferNetworkStart));
+      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferCallback));
     }
 
     c->result = c->trans->Start(this_request, c->callback.callback(),
@@ -2194,7 +2263,7 @@ TEST(HttpCache, SimpleGET_ParallelValidationCancelWriter) {
     MockHttpRequest* this_request = &request;
     if (i == 2) {
       this_request = &validate_request;
-      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferNetworkStart));
+      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferCallback));
     }
 
     c->result = c->trans->Start(this_request, c->callback.callback(),
@@ -2336,7 +2405,7 @@ TEST(HttpCache, SimpleGET_ParallelValidationStopCaching) {
     MockHttpRequest* this_request = &request;
     if (i == 2) {
       this_request = &validate_request;
-      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferNetworkStart));
+      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferCallback));
     }
 
     c->result = c->trans->Start(this_request, c->callback.callback(),
@@ -2406,7 +2475,7 @@ TEST(HttpCache, SimpleGET_ParallelValidationCancelHeaders) {
     ASSERT_THAT(c->result, IsOk());
 
     if (i == 0)
-      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferNetworkStart));
+      c->trans->SetBeforeNetworkStartCallback(base::Bind(&DeferCallback));
 
     c->result =
         c->trans->Start(&request, c->callback.callback(), NetLogWithSource());