Connect SimpleCache Backend active_entries_ more closely to Entry lifetime.

net/disk_cache/simple/simple_backend_impl.cc

Index: net/disk_cache/simple/simple_backend_impl.cc
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index 76335a754a2509386b85a267245fa761f5c4965b..6a15fd63038fb6a346f5eb7d24fa340f246e8547 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -195,6 +195,26 @@ void RecordIndexLoad(net::CacheType cache_type,
 
 }  // 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,
@@ -251,10 +271,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 +528,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));

[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()) {
@@ -531,7 +546,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,14 +655,16 @@ 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();
+    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