Blockfile cache: fix long-standing sparse + evict reentrancy problem

Thanks to nedwilliamson@ (on gmail) for an alternative perspective
plus a reduction to make fixing this much easier.

Bug: 826626, 518908, 537063, 802886
Change-Id: Ibfa01416f9a8e7f7b361e4f93b4b6b134728b85f
Reviewed-on: https://chromium-review.googlesource.com/985052Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547103}

net/disk_cache/backend_unittest.cc

@@ -4311,3 +4311,35 @@ TEST_F(DiskCacheBackendTest, SimpleFdLimit) {
   histogram_tester.ExpectBucketCount("SimpleCache.FileDescriptorLimiterAction",
                                      disk_cache::FD_LIMIT_FAIL_REOPEN_FILE, 0);
 }
+
+TEST_F(DiskCacheBackendTest, SparseEvict) {
+  const int kMaxSize = 512;
+
+  SetMaxSize(kMaxSize);
+  InitCache();
+
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(64));
+
+  disk_cache::Entry* entry0 = nullptr;
+  ASSERT_THAT(CreateEntry("http://www.0.com/", &entry0), IsOk());
+
+  disk_cache::Entry* entry1 = nullptr;
+  ASSERT_THAT(CreateEntry("http://www.1.com/", &entry1), IsOk());
+
+  disk_cache::Entry* entry2 = nullptr;
+  // This strange looking domain name affects cache trim order
+  // due to hashing
+  ASSERT_THAT(CreateEntry("http://www.15360.com/", &entry2), IsOk());
+
+  // Write sparse data to put us over the eviction threshold
+  ASSERT_EQ(64, WriteSparseData(entry0, 0, buffer.get(), 64));
+  ASSERT_EQ(1, WriteSparseData(entry0, 67108923, buffer.get(), 1));
+  ASSERT_EQ(1, WriteSparseData(entry1, 53, buffer.get(), 1));
+  ASSERT_EQ(1, WriteSparseData(entry2, 0, buffer.get(), 1));
+
+  // Closing these in a special order should not lead to buggy reentrant
+  // eviction.
+  entry1->Close();
+  entry2->Close();
+  entry0->Close();
+}

net/disk_cache/blockfile/backend_impl.cc

@@ -168,6 +168,7 @@ BackendImpl::BackendImpl(
       new_eviction_(false),
       first_timer_(true),
       user_load_(false),
+      consider_evicting_at_op_end_(false),
       net_log_(net_log),
       done_(base::WaitableEvent::ResetPolicy::MANUAL,
             base::WaitableEvent::InitialState::NOT_SIGNALED),
@@ -195,6 +196,7 @@ BackendImpl::BackendImpl(
       new_eviction_(false),
       first_timer_(true),
       user_load_(false),
+      consider_evicting_at_op_end_(false),
       net_log_(net_log),
       done_(base::WaitableEvent::ResetPolicy::MANUAL,
             base::WaitableEvent::InitialState::NOT_SIGNALED),
@@ -913,9 +915,16 @@ void BackendImpl::OnEntryDestroyBegin(Addr address) {
 
 void BackendImpl::OnEntryDestroyEnd() {
   DecreaseNumRefs();
-  if (data_->header.num_bytes > max_size_ && !read_only_ &&
-      (up_ticks_ > kTrimDelay || user_flags_ & kNoRandom))
-    eviction_.TrimCache(false);
+  consider_evicting_at_op_end_ = true;
+}
+
+void BackendImpl::OnSyncBackendOpComplete() {
+  if (consider_evicting_at_op_end_) {
+    if (data_->header.num_bytes > max_size_ && !read_only_ &&
+        (up_ticks_ > kTrimDelay || user_flags_ & kNoRandom))
+      eviction_.TrimCache(false);
+    consider_evicting_at_op_end_ = false;
+  }
 }
 
 EntryImpl* BackendImpl::GetOpenEntry(CacheRankingsBlock* rankings) const {

net/disk_cache/blockfile/backend_impl.h

@@ -89,6 +89,9 @@ class NET_EXPORT_PRIVATE BackendImpl : public Backend {
   void SyncEndEnumeration(std::unique_ptr<Rankings::Iterator> iterator);
   void SyncOnExternalCacheHit(const std::string& key);
 
+  // Called at end of any backend operation on the background thread.
+  void OnSyncBackendOpComplete();
+
   // Open or create an entry for the given |key| or |iter|.
   scoped_refptr<EntryImpl> OpenEntryImpl(const std::string& key);
   scoped_refptr<EntryImpl> CreateEntryImpl(const std::string& key);
@@ -418,6 +421,9 @@ class NET_EXPORT_PRIVATE BackendImpl : public Backend {
   bool first_timer_;  // True if the timer has not been called.
   bool user_load_;  // True if we see a high load coming from the caller.
 
+  // True if we should consider doing eviction at end of current operation.
+  bool consider_evicting_at_op_end_;
+
   net::NetLog* net_log_;
 
   Stats stats_;  // Usage statistics.

net/disk_cache/blockfile/in_flight_backend_io.cc

@@ -310,6 +310,7 @@ void BackendIO::ExecuteBackendOperation() {
   }
   DCHECK_NE(net::ERR_IO_PENDING, result_);
   NotifyController();
+  backend_->OnSyncBackendOpComplete();
 }
 
 // Runs on the background thread.