Connect SimpleCache Backend active_entries_ more closely to Entry lifetime.

Entries remove themselves from their backend's |active_entries_| at two
distinct points in their lifetime; after a successful doom and on
destruction.

Entries are also added to |active_entries_| in one of two possible places;
early in opening/creating an entry from its key, and during iteration, after
successfully opening an entry from its hash alone.

We weren't carefully tracking this relationship, and so entries previously
would double remove themselves from the backend, which could create
races that could cause entries to not open. For instance, creating an entry,
dooming it, closing it, then creating a new entry with the same key could
race, and the doom's completion would remove the closing entry from
|active_entries_|, allowing a subsequent OpenEntry() to fail because it
found an half baked entry.

To avoid exposing the state of the backend to the entry, this patch
introduces an abstraction to Entry to allow observation of destruction. This
abstraction is then used to register the backend as entries are placed in
|active_entries_|.

R=jkarlin@chromium.org,ttuttle@chromium.org,pasko@chromium.org
BUG=317138,404676

Review URL: https://codereview.chromium.org/478573003

Cr-Commit-Position: refs/heads/master@{#291728}

net/disk_cache/entry_unittest.cc

@@ -3179,6 +3179,38 @@ TEST_F(DiskCacheEntryTest, SimpleCacheDoomCreateDoom) {
   // This test passes if it doesn't crash.
 }
 
+TEST_F(DiskCacheEntryTest, SimpleCacheDoomCloseCreateCloseOpen) {
+  // Test sequence: Create, Doom, Close, Create, Close, Open.
+  SetSimpleCacheMode();
+  InitCache();
+
+  disk_cache::Entry* null = NULL;
+
+  const char key[] = "this is a key";
+
+  disk_cache::Entry* entry1 = NULL;
+  ASSERT_EQ(net::OK, CreateEntry(key, &entry1));
+  ScopedEntryPtr entry1_closer(entry1);
+  EXPECT_NE(null, entry1);
+
+  entry1->Doom();
+  entry1_closer.reset();
+  entry1 = NULL;
+
+  disk_cache::Entry* entry2 = NULL;
+  ASSERT_EQ(net::OK, CreateEntry(key, &entry2));
+  ScopedEntryPtr entry2_closer(entry2);
+  EXPECT_NE(null, entry2);
+
+  entry2_closer.reset();
+  entry2 = NULL;
+
+  disk_cache::Entry* entry3 = NULL;
+  ASSERT_EQ(net::OK, OpenEntry(key, &entry3));
+  ScopedEntryPtr entry3_closer(entry3);
+  EXPECT_NE(null, entry3);
+}
+
 // Checks that an optimistic Create would fail later on a racing Open.
 TEST_F(DiskCacheEntryTest, SimpleCacheOptimisticCreateFailsOnOpen) {
   SetSimpleCacheMode();

net/disk_cache/simple/simple_backend_impl.cc

@@ -195,6 +195,34 @@ void RecordIndexLoad(net::CacheType cache_type,
 
 }  // namespace
 
+class SimpleBackendImpl::ActiveEntryProxy
+    : public SimpleEntryImpl::ActiveEntryProxy {
+ public:
+  virtual ~ActiveEntryProxy() {
+    if (backend_) {
+      DCHECK_EQ(1U, backend_->active_entries_.count(entry_hash_));
+      backend_->active_entries_.erase(entry_hash_);
+    }
+  }
+
+  static scoped_ptr<SimpleEntryImpl::ActiveEntryProxy> Create(
+      int64 entry_hash,
+      SimpleBackendImpl* backend) {
+    scoped_ptr<SimpleEntryImpl::ActiveEntryProxy>
+        proxy(new ActiveEntryProxy(entry_hash, backend));
+    return proxy.Pass();
+  }
+
+ private:
+  ActiveEntryProxy(uint64 entry_hash,
+                   SimpleBackendImpl* backend)
+      : entry_hash_(entry_hash),
+        backend_(backend->AsWeakPtr()) {}
+
+  uint64 entry_hash_;
+  base::WeakPtr<SimpleBackendImpl> backend_;
+};
+
 SimpleBackendImpl::SimpleBackendImpl(
     const FilePath& path,
     int max_bytes,
@@ -251,10 +279,6 @@ int SimpleBackendImpl::GetMaxFileSize() const {
   return index_->max_size() / kMaxFileRatio;
 }
 
-void SimpleBackendImpl::OnDeactivated(const SimpleEntryImpl* entry) {
-  active_entries_.erase(entry->entry_hash());
-}
-
 void SimpleBackendImpl::OnDoomStart(uint64 entry_hash) {
   // TODO(ttuttle): Revert to DCHECK once http://crbug.com/317138 is fixed.
   CHECK_EQ(0u, entries_pending_doom_.count(entry_hash));
@@ -512,18 +536,17 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
     const std::string& key) {
   DCHECK_EQ(entry_hash, simple_util::GetEntryHashKey(key));
   std::pair<EntryMap::iterator, bool> insert_result =
-      active_entries_.insert(std::make_pair(entry_hash,
-                                            base::WeakPtr<SimpleEntryImpl>()));
+      active_entries_.insert(EntryMap::value_type(entry_hash, NULL));
   EntryMap::iterator& it = insert_result.first;
-  if (insert_result.second)
-    DCHECK(!it->second.get());
-  if (!it->second.get()) {
-    SimpleEntryImpl* entry = new SimpleEntryImpl(
-        cache_type_, path_, entry_hash, entry_operations_mode_, this, net_log_);
+  const bool did_insert = insert_result.second;
+  if (did_insert) {
+    SimpleEntryImpl* entry = it->second =
+        new SimpleEntryImpl(cache_type_, path_, entry_hash,
+                            entry_operations_mode_,this, net_log_);
     entry->SetKey(key);
-    it->second = entry->AsWeakPtr();
+    entry->SetActiveEntryProxy(ActiveEntryProxy::Create(entry_hash, this));
   }
-  DCHECK(it->second.get());
+  DCHECK(it->second);
   // It's possible, but unlikely, that we have an entry hash collision with a
   // currently active entry.
   if (key != it->second->key()) {
@@ -531,7 +554,7 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
     DCHECK_EQ(0U, active_entries_.count(entry_hash));
     return CreateOrFindActiveEntry(entry_hash, key);
   }
-  return make_scoped_refptr(it->second.get());
+  return make_scoped_refptr(it->second);
 }
 
 int SimpleBackendImpl::OpenEntryFromHash(uint64 entry_hash,
@@ -640,19 +663,19 @@ void SimpleBackendImpl::OnEntryOpenedFromHash(
   }
   DCHECK(*entry);
   std::pair<EntryMap::iterator, bool> insert_result =
-      active_entries_.insert(std::make_pair(hash,
-                                            base::WeakPtr<SimpleEntryImpl>()));
+      active_entries_.insert(EntryMap::value_type(hash, simple_entry));
   EntryMap::iterator& it = insert_result.first;
   const bool did_insert = insert_result.second;
   if (did_insert) {
-    // There is no active entry corresponding to this hash. The entry created
-    // is put in the map of active entries and returned to the caller.
-    it->second = simple_entry->AsWeakPtr();
-    callback.Run(error_code);
+    // There was no active entry corresponding to this hash. We've already put
+    // the entry opened from hash in the |active_entries_|. We now provide the
+    // proxy object to the entry.
+    it->second->SetActiveEntryProxy(ActiveEntryProxy::Create(hash, this));
+    callback.Run(net::OK);
   } else {
-    // The entry was made active with the key while the creation from hash
-    // occurred. The entry created from hash needs to be closed, and the one
-    // coming from the key returned to the caller.
+    // The entry was made active while we waiting for the open from hash to
+    // finish. The entry created from hash needs to be closed, and the one
+    // in |active_entries_| can be returned to the caller.
     simple_entry->Close();
     it->second->OpenEntry(entry, callback);
   }

net/disk_cache/simple/simple_backend_impl.h

@@ -66,10 +66,6 @@ class NET_EXPORT_PRIVATE SimpleBackendImpl : public Backend,
   // Returns the maximum file size permitted in this backend.
   int GetMaxFileSize() const;
 
-  // Removes |entry| from the |active_entries_| set, forcing future Open/Create
-  // operations to construct a new object.
-  void OnDeactivated(const SimpleEntryImpl* entry);
-
   // Flush our SequencedWorkerPool.
   static void FlushWorkerPoolForTesting();
 
@@ -109,11 +105,14 @@ class NET_EXPORT_PRIVATE SimpleBackendImpl : public Backend,
   virtual void OnExternalCacheHit(const std::string& key) OVERRIDE;
 
  private:
-  typedef base::hash_map<uint64, base::WeakPtr<SimpleEntryImpl> > EntryMap;
+  typedef base::hash_map<uint64, SimpleEntryImpl*> EntryMap;
 
   typedef base::Callback<void(base::Time mtime, uint64 max_size, int result)>
       InitializeIndexCallback;
 
+  class ActiveEntryProxy;
+  friend class ActiveEntryProxy;
+
   // Return value of InitCacheStructureOnDisk().
   struct DiskStatResult {
     base::Time cache_dir_mtime;

net/disk_cache/simple/simple_entry_impl.cc

@@ -161,6 +161,8 @@ class SimpleEntryImpl::ScopedOperationRunner {
   SimpleEntryImpl* const entry_;
 };
 
+SimpleEntryImpl::ActiveEntryProxy::~ActiveEntryProxy() {}
+
 SimpleEntryImpl::SimpleEntryImpl(net::CacheType cache_type,
                                  const FilePath& path,
                                  const uint64 entry_hash,
@@ -196,6 +198,12 @@ SimpleEntryImpl::SimpleEntryImpl(net::CacheType cache_type,
       CreateNetLogSimpleEntryConstructionCallback(this));
 }
 
+void SimpleEntryImpl::SetActiveEntryProxy(
+    scoped_ptr<ActiveEntryProxy> active_entry_proxy) {
+  DCHECK(!active_entry_proxy_);
+  active_entry_proxy_.reset(active_entry_proxy.release());
+}
+
 int SimpleEntryImpl::OpenEntry(Entry** out_entry,
                                const CompletionCallback& callback) {
   DCHECK(backend_.get());
@@ -526,7 +534,6 @@ SimpleEntryImpl::~SimpleEntryImpl() {
   DCHECK_EQ(0U, pending_operations_.size());
   DCHECK(state_ == STATE_UNINITIALIZED || state_ == STATE_FAILURE);
   DCHECK(!synchronous_entry_);
-  RemoveSelfFromBackend();
   net_log_.EndEvent(net::NetLog::TYPE_SIMPLE_CACHE_ENTRY);
 }
 
@@ -568,18 +575,12 @@ void SimpleEntryImpl::ReturnEntryToCaller(Entry** out_entry) {
   *out_entry = this;
 }
 
-void SimpleEntryImpl::RemoveSelfFromBackend() {
-  if (!backend_.get())
-    return;
-  backend_->OnDeactivated(this);
-}
-
 void SimpleEntryImpl::MarkAsDoomed() {
   doomed_ = true;
   if (!backend_.get())
     return;
   backend_->index()->Remove(entry_hash_);
-  RemoveSelfFromBackend();
+  active_entry_proxy_.reset();
 }
 
 void SimpleEntryImpl::RunNextOperationIfNeeded() {

net/disk_cache/simple/simple_entry_impl.h

@@ -11,7 +11,6 @@
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "net/base/cache_type.h"
 #include "net/base/net_export.h"
@@ -40,8 +39,7 @@ struct SimpleEntryCreationResults;
 // disk cache. It proxies for the SimpleSynchronousEntry, which performs IO
 // on the worker thread.
 class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
-    public base::RefCounted<SimpleEntryImpl>,
-    public base::SupportsWeakPtr<SimpleEntryImpl> {
+    public base::RefCounted<SimpleEntryImpl> {
   friend class base::RefCounted<SimpleEntryImpl>;
  public:
   enum OperationsMode {
@@ -49,6 +47,14 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
     OPTIMISTIC_OPERATIONS,
   };
 
+  // The Backend provides an |ActiveEntryProxy| instance to this entry when it
+  // is active, meaning it's the canonical entry for this |entry_hash_|. The
+  // entry can make itself inactive by deleting its proxy.
+  class ActiveEntryProxy {
+   public:
+    virtual ~ActiveEntryProxy() = 0;
+  };
+
   SimpleEntryImpl(net::CacheType cache_type,
                   const base::FilePath& path,
                   uint64 entry_hash,
@@ -56,6 +62,9 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
                   SimpleBackendImpl* backend,
                   net::NetLog* net_log);
 
+  void SetActiveEntryProxy(
+      scoped_ptr<ActiveEntryProxy> active_entry_proxy);
+
   // Adds another reader/writer to this entry, if possible, returning |this| to
   // |entry|.
   int OpenEntry(Entry** entry, const CompletionCallback& callback);
@@ -150,10 +159,6 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
   // count.
   void ReturnEntryToCaller(Entry** out_entry);
 
-  // Ensures that |this| is no longer referenced by our |backend_|, this
-  // guarantees that this entry cannot have OpenEntry/CreateEntry called again.
-  void RemoveSelfFromBackend();
-
   // An error occured, and the SimpleSynchronousEntry should have Doomed
   // us at this point. We need to remove |this| from the Backend and the
   // index.
@@ -297,6 +302,8 @@ class NET_EXPORT_PRIVATE SimpleEntryImpl : public Entry,
                   int length,
                   int stream_index);
 
+  scoped_ptr<ActiveEntryProxy> active_entry_proxy_;
+
   // All nonstatic SimpleEntryImpl methods should always be called on the IO
   // thread, in all cases. |io_thread_checker_| documents and enforces this.
   base::ThreadChecker io_thread_checker_;