DoomPartialEntry should not attempt to doom an already doomed entry.

net/http/http_cache_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_cache.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 129 matching lines @@
   // same value.
   EXPECT_FALSE(load_timing_info.send_start.is_null());
   EXPECT_EQ(load_timing_info.send_start, load_timing_info.send_end);
 
   // Set by URLRequest / URLRequestHttpJob, at a higher level.
   EXPECT_TRUE(load_timing_info.request_start_time.is_null());
   EXPECT_TRUE(load_timing_info.request_start.is_null());
   EXPECT_TRUE(load_timing_info.receive_headers_end.is_null());
 }
 
-void DeferNetworkStart(bool* defer) {
+void DeferCallback(bool* defer) {
   *defer = true;
 }
 
 class DeleteCacheCompletionCallback : public TestCompletionCallbackBase {
  public:
   explicit DeleteCacheCompletionCallback(MockHttpCache* cache)
       : cache_(cache),
         callback_(base::Bind(&DeleteCacheCompletionCallback::OnComplete,
                              base::Unretained(this))) {
   }
@@ skipping 1310 matching lines @@
       c->result = c->callback.WaitForResult();
 
     ReadAndVerifyTransaction(c->trans.get(), kRangeGET_TransactionOK);
   }
 
   EXPECT_EQ(5, cache.network_layer()->transaction_count());
   EXPECT_EQ(0, cache.disk_cache()->open_count());
   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;
 
   ScopedMockTransaction transaction(kRangeGET_TransactionOK);
 
   std::vector<std::unique_ptr<Context>> context_list;
   const int kNumTransactions = 2;
 
   for (int i = 0; i < kNumTransactions; ++i) {
@@ skipping 598 matching lines @@
   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());
 
     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(),
                                 NetLogWithSource());
   }
 
   // Allow all requests to move from the Create queue to the active entry.
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
@@ skipping 77 matching lines @@
   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());
 
     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(),
                                 NetLogWithSource());
   }
 
   // Allow all requests to move from the Create queue to the active entry.
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
@@ skipping 121 matching lines @@
   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());
 
     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(),
                                 NetLogWithSource());
   }
 
   // Allow all requests to move from the Create queue to the active entry.
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
@@ skipping 49 matching lines @@
   std::vector<std::unique_ptr<Context>> context_list;
 
   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());
 
     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());
   }
 
   base::RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(cache.IsHeadersTransactionPresent(kSimpleGET_Transaction.url));
   EXPECT_EQ(1, cache.GetCountAddToEntryQueue(kSimpleGET_Transaction.url));
 
@@ skipping 6884 matching lines @@
   ASSERT_TRUE(attrs->GetDictionary(
       base::trace_event::MemoryAllocatorDump::kNameSize, &size_attrs));
   std::string size;
   ASSERT_TRUE(size_attrs->GetString("value", &size));
   int actual_size = 0;
   ASSERT_TRUE(base::HexStringToInt(size, &actual_size));
   ASSERT_LT(0, actual_size);
 }
 
 }  // namespace net