Fix race condition for non-open/create operations happening after a doom.

net/disk_cache/entry_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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ skipping 2856 matching lines @@
   EXPECT_EQ(net::ERR_IO_PENDING,
             entry2->ReadData(0, 0, buffer1.get(), kSize1, cb.callback()));
   EXPECT_EQ(kSize1, cb.GetResult(net::ERR_IO_PENDING));
 
   // Check that we are not leaking.
   EXPECT_TRUE(
       static_cast<disk_cache::SimpleEntryImpl*>(entry2)->HasOneRef());
   entry2->Close();
 }
 
-// This test is flaky because of the race of Create followed by a Doom.
-// See test SimpleCacheCreateDoomRace.
-TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheOptimistic5) {
+TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic5) {
   // Test sequence:
   // Create, Doom, Write, Read, Close.
   SetSimpleCacheMode();
   InitCache();
   disk_cache::Entry* null = NULL;
   const char key[] = "the first key";
 
   net::TestCompletionCallback cb;
   const int kSize1 = 10;
   scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
@@ skipping 48 matching lines @@
   entry->Doom();
   entry->Doom();
 
   // This Read must not be optimistic, since we don't support that yet.
   EXPECT_EQ(net::ERR_IO_PENDING,
             entry->ReadData(0, 0, buffer1_read.get(), kSize1, cb.callback()));
   EXPECT_EQ(kSize1, cb.GetResult(net::ERR_IO_PENDING));
   EXPECT_EQ(0, memcmp(buffer1->data(), buffer1_read->data(), kSize1));
 
   entry->Doom();
-
-  // Check that we are not leaking.
-  EXPECT_TRUE(
-      static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef());

[Randy Smith (Not in Mondays), 2013/09/03 18:48:19]

Why could/will this have an extra ref?

[Philippe, 2013/09/04 09:35:23]

The doom above is adding an extra reference to the entry instance. This extra
reference is removed when the doom completes. So the reference count at this
point (if Doom() didn't complete which I can reproduce consistently on my
workstation) is 2.

 }
 
 // Confirm that IO buffers are not referenced by the Simple Cache after a write
 // completes.
 TEST_F(DiskCacheEntryTest, SimpleCacheOptimisticWriteReleases) {
   SetSimpleCacheMode();
   InitCache();
 
   const char key[] = "the first key";
   disk_cache::Entry* entry = NULL;
@@ skipping 18 matching lines @@
 
   // Finally, we should perform an optimistic write and confirm that all
   // references to the IO buffer have been released.
   EXPECT_EQ(
       kWriteSize,
       entry->WriteData(
           1, 0, buffer1.get(), kWriteSize, net::CompletionCallback(), false));
   EXPECT_TRUE(buffer1->HasOneRef());
 }
 
-TEST_F(DiskCacheEntryTest, DISABLED_SimpleCacheCreateDoomRace) {
+TEST_F(DiskCacheEntryTest, SimpleCacheCreateDoomRace) {
   // Test sequence:
   // Create, Doom, Write, Close, Check files are not on disk anymore.
   SetSimpleCacheMode();
   InitCache();
   disk_cache::Entry* null = NULL;
   const char key[] = "the first key";
 
   net::TestCompletionCallback cb;
   const int kSize1 = 10;
   scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
   CacheTestFillBuffer(buffer1->data(), kSize1, false);
   disk_cache::Entry* entry = NULL;
 
   ASSERT_EQ(net::OK,
             cache_->CreateEntry(key, &entry, net::CompletionCallback()));
   EXPECT_NE(null, entry);
 
   cache_->DoomEntry(key, cb.callback());
   EXPECT_EQ(net::OK, cb.GetResult(net::ERR_IO_PENDING));
 
-  // Lets do a Write so we block until all operations are done, so we can check
-  // the HasOneRef() below. This call can't be optimistic and we are checking

[Philippe, 2013/08/28 14:05:52]

Why can't this be optimistic? It is optimistic currently.

-  // that here.
   EXPECT_EQ(
-      net::ERR_IO_PENDING,
+      kSize1,
       entry->WriteData(0, 0, buffer1.get(), kSize1, cb.callback(), false));
-  EXPECT_EQ(kSize1, cb.GetResult(net::ERR_IO_PENDING));
 
-  // Check that we are not leaking.
-  EXPECT_TRUE(
-      static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef());
   entry->Close();
 
   // Finish running the pending tasks so that we fully complete the close
   // operation and destroy the entry object.
   base::MessageLoop::current()->RunUntilIdle();
 
   for (int i = 0; i < disk_cache::kSimpleEntryFileCount; ++i) {
     base::FilePath entry_file_path = cache_path_.AppendASCII(
         disk_cache::simple_util::GetFilenameFromKeyAndIndex(key, i));
     base::PlatformFileInfo info;
@@ skipping 364 matching lines @@
 
   // Check that we are not leaking.
   ASSERT_NE(entry, null);
   EXPECT_TRUE(
       static_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef());
   entry->Close();
   entry = NULL;
 }
 
 #endif  // defined(OS_POSIX)