Fix bug in MemEntryImpl, which caused browser crash during clearing

net/disk_cache/backend_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/file_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/platform_thread.h"
@@ skipping 32 matching lines @@
   void BackendInvalidEntry();
   void BackendInvalidEntryRead();
   void BackendInvalidEntryWithLoad();
   void BackendTrimInvalidEntry();
   void BackendTrimInvalidEntry2();
   void BackendEnumerations();
   void BackendEnumerations2();
   void BackendInvalidEntryEnumeration();
   void BackendFixEnumerators();
   void BackendDoomRecent();
+
+  // Adds 3 sparse entries. |doomed_start| and |doomed_end| if not NULL,
+  // will be filled with times, used by DoomEntriesSince and DoomEntriesBetween
+  // to doom last two entries.
+  void InitSparseCache(base::Time* doomed_start, base::Time* doomed_end);
+
   void BackendDoomBetween();
   void BackendTransaction(const std::string& name, int num_entries, bool load);
   void BackendRecoverInsert();
   void BackendRecoverRemove();
   void BackendRecoverWithEviction();
   void BackendInvalidEntry2();
   void BackendInvalidEntry3();
   void BackendInvalidEntry7();
   void BackendInvalidEntry8();
   void BackendInvalidEntry9(bool eviction);
@@ skipping 1275 matching lines @@
 TEST_F(DiskCacheBackendTest, NewEvictionDoomRecent) {
   SetNewEviction();
   BackendDoomRecent();
 }
 
 TEST_F(DiskCacheBackendTest, MemoryOnlyDoomRecent) {
   SetMemoryOnlyMode();
   BackendDoomRecent();
 }
 
+void DiskCacheBackendTest::InitSparseCache(base::Time* doomed_start,
+                                           base::Time* doomed_end) {
+  InitCache();
+
+  const int kSize = 50;
+  // This must be greater then MemEntryImpl::kMaxSparseEntrySize.
+  const int kOffset = 10 + 1024 * 1024;
+
+  disk_cache::Entry* entry0 = NULL;
+  disk_cache::Entry* entry1 = NULL;
+  disk_cache::Entry* entry2 = NULL;
+
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kSize));
+  CacheTestFillBuffer(buffer->data(), kSize, false);
+
+  // entry0 is the only entry whish left after
+  // DoomEntriesSince and DoomEntriesBetween.
+  ASSERT_EQ(net::OK, CreateEntry("zeroth", &entry0));
+  ASSERT_EQ(kSize, WriteSparseData(entry0, 0, buffer.get(), kSize));
+  ASSERT_EQ(kSize,
+            WriteSparseData(entry0, kOffset  + kSize, buffer.get(), kSize));
+  entry0->Close();
+
+  AddDelay();
+  if (doomed_start)
+    *doomed_start = base::Time::Now();
+
+  // Order in rankings list:
+  // first_part1, first_part2, second_part1, second_part2
+  ASSERT_EQ(net::OK, CreateEntry("first", &entry1));
+  ASSERT_EQ(kSize, WriteSparseData(entry1, 0, buffer.get(), kSize));
+  ASSERT_EQ(kSize,
+            WriteSparseData(entry1, kOffset  + kSize, buffer.get(), kSize));
+  entry1->Close();
+
+  ASSERT_EQ(net::OK, CreateEntry("second", &entry2));
+  ASSERT_EQ(kSize, WriteSparseData(entry2, 0, buffer.get(), kSize));
+  ASSERT_EQ(kSize,
+            WriteSparseData(entry2, kOffset + kSize, buffer.get(), kSize));
+  entry2->Close();
+
+  // Order in rankings list:
+  // third_part1, fourth_part1, third_part2, fourth_part2

[rvargas (doing something else), 2013/03/21 22:01:57]

The second time stamp should be somewhere in the middle, to make sure that
deleting from time1 to time2 is not the same as deleting from time1.

It would be interesting to see how that works in practice given the mix between
parent and children in this part of the test.

[Slava Chigrin, 2013/03/22 13:45:02]

Done.

+  disk_cache::Entry* entry3 = NULL;
+  disk_cache::Entry* entry4 = NULL;
+  ASSERT_EQ(net::OK, CreateEntry("third", &entry3));
+  ASSERT_EQ(kSize, WriteSparseData(entry3, 0, buffer.get(), kSize));
+  ASSERT_EQ(net::OK, CreateEntry("fourth", &entry4));
+  ASSERT_EQ(kSize, WriteSparseData(entry4, 0, buffer.get(), kSize));
+  ASSERT_EQ(kSize,
+            WriteSparseData(entry3, kOffset + kSize, buffer.get(), kSize));
+  ASSERT_EQ(kSize,
+            WriteSparseData(entry4, kOffset + kSize, buffer.get(), kSize));
+  entry3->Close();
+  entry4->Close();
+
+  AddDelay();
+  if (doomed_end)
+    *doomed_end = base::Time::Now();
+}
+
+TEST_F(DiskCacheBackendTest, MemoryOnlyDoomEntriesSinceSparse) {

[rvargas (doing something else), 2013/03/21 22:01:57]

It would be nice to also add tests for the regular backend (just the simple one,
not AppCache or the others). Copy/paste sounds good.

[Slava Chigrin, 2013/03/22 13:45:02]

I have added tests for BackendImpl here. I noticed that GetEntryCount() for it
counts also child entries, whereas MemBackendImpl - only parent. Is it expected
behavior? I  multiplied expected results by 3 in tests for BackendImpl to pass
the tests.

These new test fail sometimes. I investigated this issue - that is because of
last used time of sparse entry updated when Close() operation completed on
EntryImpl (since it destroys SparseControl, which calls
SparseControl::CloseChild, which performs Write operation).

Since Close() operation is asynchronous for disk backend entries, there are
seems no way to guarantee that sparse entry will have last used time in
particular time range, which we need for tests.

I added FLAKY_ prefix for these tests, is this Ok? I can increase possibility
that test will pass by adding more AddDelay() calls, but still we'll not have
100% guarantee that test will pass. 

I can not find any ways to fix this without adding Callback parameter to Close()
operation, but  seems it will cause huge refactoring.

On 2013/03/21 22:01:57, rvargas wrote:

[rvargas (doing something else), 2013/03/23 02:08:31]

yes... the disk backend allows enumeration (and inspection) of sparse children
so it reports the total number of entries. The memory backend doesn't enumerate
sparse children so it doesn't count them either when reporting the number of
entries.
Take a look at DiskCacheBackendTest::BackendDoomBetween(). We use
FlushQueueForTest() whenever we have to make sure that Close() does something
before moving on with the test. I should have mentioned that before.

+  SetMemoryOnlyMode();
+  base::Time start;
+  InitSparseCache(&start, NULL);
+  DoomEntriesSince(start);
+  EXPECT_EQ(1, cache_->GetEntryCount());
+}
+
+TEST_F(DiskCacheBackendTest, MemoryOnlyDoomAllSparse) {
+  SetMemoryOnlyMode();
+  InitSparseCache(NULL, NULL);
+  EXPECT_EQ(net::OK, DoomAllEntries());
+  EXPECT_EQ(0, cache_->GetEntryCount());
+}
+
 void DiskCacheBackendTest::BackendDoomBetween() {
   InitCache();
 
   disk_cache::Entry *entry;
   ASSERT_EQ(net::OK, CreateEntry("first", &entry));
   entry->Close();
   FlushQueueForTest();
 
   AddDelay();
   Time middle_start = Time::Now();
@@ skipping 37 matching lines @@
 TEST_F(DiskCacheBackendTest, NewEvictionDoomBetween) {
   SetNewEviction();
   BackendDoomBetween();
 }
 
 TEST_F(DiskCacheBackendTest, MemoryOnlyDoomBetween) {
   SetMemoryOnlyMode();
   BackendDoomBetween();
 }
 
+TEST_F(DiskCacheBackendTest, MemoryOnlyDoomEntriesBetweenSparse) {
+  SetMemoryOnlyMode();
+  base::Time start, end;
+  InitSparseCache(&start, &end);
+  DoomEntriesBetween(start, end);
+  EXPECT_EQ(1, cache_->GetEntryCount());
+}
+
 void DiskCacheBackendTest::BackendTransaction(const std::string& name,
                                               int num_entries, bool load) {
   success_ = false;
   ASSERT_TRUE(CopyTestCache(name));
   DisableFirstCleanup();
 
   uint32 mask;
   if (load) {
     mask = 0xf;
     SetMaxSize(0x100000);
@@ skipping 1269 matching lines @@
   // Ping the oldest entry.
   cache_->OnExternalCacheHit("key0");
 
   TrimForTest(false);
 
   // Make sure the older key remains.
   EXPECT_EQ(1, cache_->GetEntryCount());
   ASSERT_EQ(net::OK, OpenEntry("key0", &entry));
   entry->Close();
 }