Connect SimpleCache Backend active_entries_ more closely to Entry lifetime.

net/disk_cache/simple/simple_backend_impl.cc

 // Copyright (c) 2013 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 "net/disk_cache/simple/simple_backend_impl.h"
 
 #include <algorithm>
 #include <cstdlib>
 #include <functional>
 
@@ skipping 177 matching lines @@
   if (result == net::OK) {
     SIMPLE_CACHE_UMA(TIMES, "CreationToIndex", cache_type, creation_to_index);
   } else {
     SIMPLE_CACHE_UMA(TIMES,
                      "CreationToIndexFail", cache_type, creation_to_index);
   }
 }
 
 }  // namespace
 
+// Removes an entry from the Backend's |active_entries_| when the entry is done.
+// Provided to each Entry as soon as it's added to |active_entries_|.
+class SimpleBackendImpl::EntryDisconnector
+    : public SimpleEntryImpl::BackendDisconnector {
+ public:
+  EntryDisconnector(uint64 entry_hash,
+                    SimpleBackendImpl* backend)
+      : entry_hash_(entry_hash),
+        backend_(backend->AsWeakPtr()) {}
+
+  virtual ~EntryDisconnector() {
+    if (backend_)

[Deprecated (see juliatuttle), 2014/08/22 19:26:37]

It makes me uncomfortable that we've lost the "make sure the entry we remove
from the hash is the one that just disappeared" check. Can we reintroduce it
somehow without making things too gross? Even just a CHECK would be nice, since
we believe it should always be the right one.

[gavinp, 2014/08/25 13:18:27]

DCHECK added. It's not clear what we're losing though, I guess you mean versus
earlier versions of this patch?

+      backend_->active_entries_.erase(entry_hash_);
+  }
+
+ private:
+  uint64 entry_hash_;
+  base::WeakPtr<SimpleBackendImpl> backend_;
+};
+
 SimpleBackendImpl::SimpleBackendImpl(
     const FilePath& path,
     int max_bytes,
     net::CacheType cache_type,
     const scoped_refptr<base::SingleThreadTaskRunner>& cache_thread,
     net::NetLog* net_log)
     : path_(path),
       cache_type_(cache_type),
       cache_thread_(cache_thread),
       orig_max_size_(max_bytes),
@@ skipping 36 matching lines @@
 
 bool SimpleBackendImpl::SetMaxSize(int max_bytes) {
   orig_max_size_ = max_bytes;
   return index_->SetMaxSize(max_bytes);
 }
 
 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));
   entries_pending_doom_.insert(
       std::make_pair(entry_hash, std::vector<Closure>()));
 }
 
 void SimpleBackendImpl::OnDoomComplete(uint64 entry_hash) {
   // TODO(ttuttle): Revert to DCHECK once http://crbug.com/317138 is fixed.
   CHECK_EQ(1u, entries_pending_doom_.count(entry_hash));
@@ skipping 237 matching lines @@
     DCHECK(result.max_size);
   }
   return result;
 }
 
 scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
     const uint64 entry_hash,
     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));

[Deprecated (see juliatuttle), 2014/08/22 19:26:38]

Ooh, that's cute, using ::value_type. Nice.

   EntryMap::iterator& it = insert_result.first;
-  if (insert_result.second)
-    DCHECK(!it->second.get());
-  if (!it->second.get()) {
-    SimpleEntryImpl* entry = new SimpleEntryImpl(
+  if (insert_result.second) {
+    it->second = new SimpleEntryImpl(

[Deprecated (see juliatuttle), 2014/08/22 19:26:37]

I'd prefer if you had this named entry still and then assigned it to it->second,
but it's not a big deal.

[gavinp, 2014/08/25 13:18:26]

Done.

         cache_type_, path_, entry_hash, entry_operations_mode_, this, net_log_);
-    entry->SetKey(key);
-    it->second = entry->AsWeakPtr();
+    scoped_ptr<EntryDisconnector>
+        entry_disconnector(new EntryDisconnector(entry_hash, this));
+    it->second->SetBackendDisconnector(
+        entry_disconnector.PassAs<SimpleEntryImpl::BackendDisconnector>());
+    it->second->SetKey(key);

[Deprecated (see juliatuttle), 2014/08/22 19:26:38]

Similarly, for no good reason, I'd prefer SetKey before SetBackendDisconnector.

[gavinp, 2014/08/25 13:18:26]

Done.

   }
-  DCHECK(it->second.get());

[Deprecated (see juliatuttle), 2014/08/22 19:26:38]

Why not change to DCHECK(it->second)?

[gavinp, 2014/08/25 13:18:26]

Done.

   // It's possible, but unlikely, that we have an entry hash collision with a
   // currently active entry.
   if (key != it->second->key()) {
     it->second->Doom();
     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,
                                          Entry** entry,
                                          const CompletionCallback& callback) {
   base::hash_map<uint64, std::vector<Closure> >::iterator it =
       entries_pending_doom_.find(entry_hash);
   if (it != entries_pending_doom_.end()) {
     Callback<int(const net::CompletionCallback&)> operation =
         base::Bind(&SimpleBackendImpl::OpenEntryFromHash,
@@ skipping 88 matching lines @@
     Entry** entry,
     scoped_refptr<SimpleEntryImpl> simple_entry,
     const CompletionCallback& callback,
     int error_code) {
   if (error_code != net::OK) {
     callback.Run(error_code);
     return;
   }
   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();
+    scoped_ptr<EntryDisconnector>

[Deprecated (see juliatuttle), 2014/08/22 19:26:37]

Can we abstract out "add this entry to the map using this iterator and also set
a disconnector on it" to a separate method?

[gavinp, 2014/08/25 13:18:27]

I considered it. That method would have weird arguments, have a confusing name,
and it would replace very few lines of code. Worse yet, the two places I do this
have slightly different conventions; one of them creates the object when insert
succeeds, the other uses an object that already exists.

I did clean up both of these sites by creating a static creation method in the
disconnector class. Does that help?

+        entry_disconnector(new EntryDisconnector(hash, this));
+    it->second->SetBackendDisconnector(

[Deprecated (see juliatuttle), 2014/08/22 19:26:37]

I'd like a comment here that explicitly points out why we only set the
Disconnector on this branch. The logic of how we prevent deleting the wrong
entry seems very implicit, and that worries me.

[gavinp, 2014/08/25 13:18:27]

I've beefed up the comment on this branch.

+        entry_disconnector.PassAs<SimpleEntryImpl::BackendDisconnector>());
     callback.Run(error_code);
   } 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.
     simple_entry->Close();
     it->second->OpenEntry(entry, callback);
   }
 }
 
@@ skipping 42 matching lines @@
                    this));
   callback.Run(result);
 }
 
 void SimpleBackendImpl::FlushWorkerPoolForTesting() {
   if (g_sequenced_worker_pool)
     g_sequenced_worker_pool->FlushForTesting();
 }
 
 }  // namespace disk_cache