Unlink corrupt SimpleCache index files immediately after load.

net/disk_cache/simple/simple_index_file_unittest.cc

Index: net/disk_cache/simple/simple_index_file_unittest.cc
diff --git a/net/disk_cache/simple/simple_index_file_unittest.cc b/net/disk_cache/simple/simple_index_file_unittest.cc
index 76ecd91726c99f90d3091d1da0897d74e228ff34..716c62f132e02ec5f54cf7443335d59595038380 100644
--- a/net/disk_cache/simple/simple_index_file_unittest.cc
+++ b/net/disk_cache/simple/simple_index_file_unittest.cc
@@ -51,6 +51,23 @@ TEST(IndexMetadataTest, Serialize) {
   EXPECT_TRUE(new_index_metadata.CheckIndexMetadata());
 }
 
+class TestSimpleIndexFile : public SimpleIndexFile {

[Randy Smith (Not in Mondays), 2013/06/19 21:06:00]

The pattern I've seen more often is friending the test class (in this case
SimpleIndexFileTest) directly, and including delegates in it for the private
member functions on the original that you want to use.  I don't object to this,
but I found it a bit weird, and it took me a little time to figure out why you
were doing it.  If you keep this version, put in a comment explaining what's
going on.

[gavinp, 2013/06/19 23:21:25]

Done.

+ public:
+  using SimpleIndexFile::Deserialize;
+  using SimpleIndexFile::IsIndexFileStale;
+  using SimpleIndexFile::kIndexFileName;
+  using SimpleIndexFile::LoadFromDisk;
+  using SimpleIndexFile::Serialize;
+
+  explicit TestSimpleIndexFile(const base::FilePath& index_file_directory)
+      : SimpleIndexFile(base::MessageLoopProxy::current(),
+                        base::MessageLoopProxy::current(),
+                        index_file_directory) {
+  }
+  virtual ~TestSimpleIndexFile() {
+  }
+};
+
 class SimpleIndexFileTest : public testing::Test {
  public:
   bool CompareTwoEntryMetadata(const EntryMetadata& a, const EntryMetadata& b) {
@@ -58,15 +75,6 @@ class SimpleIndexFileTest : public testing::Test {
            a.entry_size_ == b.entry_size_;
   }
 
-  void IndexCompletionCallback(
-      scoped_ptr<SimpleIndex::EntrySet> index_file_entries,
-      bool force_index_flush) {
-    EXPECT_FALSE(callback_result_);
-    callback_result_.reset(
-        new IndexCompletionCallbackResult(index_file_entries.Pass(),
-                                          force_index_flush));
-  }
-
  protected:
   struct IndexCompletionCallbackResult {
     IndexCompletionCallbackResult(
@@ -80,11 +88,25 @@ class SimpleIndexFileTest : public testing::Test {
     const bool force_index_flush;
   };
 
+  SimpleIndexFile::IndexCompletionCallback GetCallback() {

[gavinp, 2013/06/19 17:17:31]

rdsmith: This fixes the funny scoping you complained about in an earlier review.

[Randy Smith (Not in Mondays), 2013/06/19 21:06:00]

Amusing.  Now that I understand what's going on, I'd also be good with the
original with a comment.  But this is fine too.

[gavinp, 2013/06/19 23:21:25]

I prefer this; it makes the tests more succinct but leaves readability intact in
my opinion. I'll leave it as is, with no comment. Fair?

[Randy Smith (Not in Mondays), 2013/06/20 12:44:30]

SG; no worries.

+    return base::Bind(&SimpleIndexFileTest::IndexCompletionCallback,
+                      base::Unretained(this));
+  }
+
   IndexCompletionCallbackResult* callback_result() {
     return callback_result_.get();
   }
 
  private:
+  void IndexCompletionCallback(
+      scoped_ptr<SimpleIndex::EntrySet> index_file_entries,
+      bool force_index_flush) {
+    EXPECT_FALSE(callback_result_);
+    callback_result_.reset(
+        new IndexCompletionCallbackResult(index_file_entries.Pass(),
+                                          force_index_flush));
+  }
+
   scoped_ptr<IndexCompletionCallbackResult> callback_result_;
 };
 
@@ -103,13 +125,13 @@ TEST_F(SimpleIndexFileTest, Serialize) {
     SimpleIndex::InsertInEntrySet(hash, metadata_entries[i], &entries);
   }
 
-  scoped_ptr<Pickle> pickle = SimpleIndexFile::Serialize(
+  scoped_ptr<Pickle> pickle = TestSimpleIndexFile::Serialize(
       index_metadata, entries);
   EXPECT_TRUE(pickle.get() != NULL);
 
-  scoped_ptr<SimpleIndex::EntrySet> new_entries = SimpleIndexFile::Deserialize(
-      reinterpret_cast<const char*>(pickle->data()),
-      pickle->size());
+  scoped_ptr<SimpleIndex::EntrySet> new_entries =
+      TestSimpleIndexFile::Deserialize(static_cast<const char*>(pickle->data()),
+                                       pickle->size());
   EXPECT_TRUE(new_entries.get() != NULL);
   EXPECT_EQ(entries.size(), new_entries->size());
 
@@ -127,57 +149,96 @@ TEST_F(SimpleIndexFileTest, IsIndexFileStale) {
   const std::string kIndexFileName = "simple-index";
   const base::FilePath index_path =
       temp_dir.path().AppendASCII(kIndexFileName);
-  EXPECT_TRUE(SimpleIndexFile::IsIndexFileStale(index_path));
+  EXPECT_TRUE(TestSimpleIndexFile::IsIndexFileStale(index_path));
   const std::string kDummyData = "nothing to be seen here";
   EXPECT_EQ(static_cast<int>(kDummyData.size()),
             file_util::WriteFile(index_path,
                                  kDummyData.data(),
                                  kDummyData.size()));
-  EXPECT_FALSE(SimpleIndexFile::IsIndexFileStale(index_path));
+  EXPECT_FALSE(TestSimpleIndexFile::IsIndexFileStale(index_path));
 
   const base::Time past_time = base::Time::Now() -
       base::TimeDelta::FromSeconds(10);
   EXPECT_TRUE(file_util::TouchFile(index_path, past_time, past_time));
   EXPECT_TRUE(file_util::TouchFile(temp_dir.path(), past_time, past_time));
-  EXPECT_FALSE(SimpleIndexFile::IsIndexFileStale(index_path));
+  EXPECT_FALSE(TestSimpleIndexFile::IsIndexFileStale(index_path));
 
   EXPECT_EQ(static_cast<int>(kDummyData.size()),
             file_util::WriteFile(temp_dir.path().AppendASCII("other_file"),
                                  kDummyData.data(),
                                  kDummyData.size()));
 
-  EXPECT_TRUE(SimpleIndexFile::IsIndexFileStale(index_path));
+  EXPECT_TRUE(TestSimpleIndexFile::IsIndexFileStale(index_path));
 }
 
-TEST_F(SimpleIndexFileTest, IsIndexFileCorrupt) {
+TEST_F(SimpleIndexFileTest, WriteThenLoadIndex) {
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
   const base::FilePath index_path =
-      temp_dir.path().AppendASCII(SimpleIndexFile::kIndexFileName);
-  EXPECT_TRUE(SimpleIndexFile::IsIndexFileStale(index_path));
+      temp_dir.path().AppendASCII(TestSimpleIndexFile::kIndexFileName);
+  EXPECT_TRUE(TestSimpleIndexFile::IsIndexFileStale(index_path));
+
+  SimpleIndex::EntrySet entries;
+  static const uint64 kHashes[] = { 11, 22, 33 };
+  static const size_t kNumHashes = arraysize(kHashes);
+  EntryMetadata metadata_entries[kNumHashes];
+  for (size_t i = 0; i < kNumHashes; ++i) {
+    uint64 hash = kHashes[i];
+    metadata_entries[i] =
+        EntryMetadata(Time::FromInternalValue(hash), hash);
+    SimpleIndex::InsertInEntrySet(hash, metadata_entries[i], &entries);
+  }
+
+  const uint64 kCacheSize = 456U;
+  {
+    TestSimpleIndexFile simple_index_file(temp_dir.path());
+    simple_index_file.WriteToDisk(entries, kCacheSize,
+                                  base::TimeTicks(), false);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(file_util::PathExists(index_path));
+  }
+
+  TestSimpleIndexFile simple_index_file(temp_dir.path());
+  simple_index_file.LoadIndexEntries(base::MessageLoopProxy::current(),
+                                     GetCallback());
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(file_util::PathExists(index_path));
+  ASSERT_TRUE(callback_result());
+  EXPECT_FALSE(callback_result()->force_index_flush);
+  const SimpleIndex::EntrySet* read_entries =
+      callback_result()->index_file_entries.get();
+  ASSERT_TRUE(read_entries);
+
+  EXPECT_EQ(kNumHashes, read_entries->size());
+  for (size_t i = 0; i < kNumHashes; ++i)
+    EXPECT_EQ(1U, read_entries->count(kHashes[i]));
+}
+
+TEST_F(SimpleIndexFileTest, LoadCorruptIndex) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  const base::FilePath index_path =
+      temp_dir.path().AppendASCII(TestSimpleIndexFile::kIndexFileName);
+  EXPECT_TRUE(TestSimpleIndexFile::IsIndexFileStale(index_path));
   const std::string kDummyData = "nothing to be seen here";
   EXPECT_EQ(static_cast<int>(kDummyData.size()),
             file_util::WriteFile(index_path,
                                  kDummyData.data(),
                                  kDummyData.size()));
-  EXPECT_FALSE(SimpleIndexFile::IsIndexFileStale(index_path));
-
-  SimpleIndexFile simple_index_file(base::MessageLoopProxy::current(),
-                                    base::MessageLoopProxy::current(),
-                                    temp_dir.path());
-
-  SimpleIndexFile::IndexCompletionCallback callback =
-      base::Bind(&SimpleIndexFileTest::IndexCompletionCallback,
-                 base::Unretained(this));
+  EXPECT_FALSE(TestSimpleIndexFile::IsIndexFileStale(index_path));
 
+  TestSimpleIndexFile simple_index_file(temp_dir.path());
   simple_index_file.LoadIndexEntries(base::MessageLoopProxy::current(),
-                                     callback);
+                                     GetCallback());
   base::RunLoop().RunUntilIdle();
 
+  EXPECT_FALSE(file_util::PathExists(index_path));
   ASSERT_TRUE(callback_result());
-  EXPECT_TRUE(callback_result()->index_file_entries);
   EXPECT_TRUE(callback_result()->force_index_flush);
+  EXPECT_TRUE(callback_result()->index_file_entries);
 }
 
 }  // namespace disk_cache