Connect SimpleCache Backend active_entries_ more closely to Entry lifetime.

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
Committed: https://crrev.com/b02ee6e3d42dc4181251bbf4c4df0e3cc91994f5
Cr-Commit-Position: refs/heads/master@{#291728}

[gavinp, 2014-08-22 16:04:21]

jkarlin, ttuttle: PTAL.

pasko: FYI.

This needs a unit test. jkarlin wrote https://codereview.chromium.org/485753002/
which works. I'll see if I can come up with something narrower and more crashy
before just patching that in.

In the meantime, tuttle, I'd really like to investigate if this is the cause of
the bugbear we've been chasing. It seems likely...

[gavinp, 2014-08-22 17:05:36]

The latest upload adds a test.

As well, I've removed WeakPtrs to SimpleEntryImpl, since we were only using them
for active_entries_, and we always explicitly removed our entry on
destruction.

The double removal has been removed on the entry side, which is cleaner,
and I've added a DCHECK to guard against double removal.

My only concern is how scary it is to have a map of raw pointers hanging around.
I wonder if we should be using a scoped_ptr specialization here, to guarantee
that we always are unreferenced in the backend by destruction.

[gavinp, 2014-08-22 18:13:25]

And... I'm sorry to go all C++ey, however, I determined there was another
complicated path to consider here.

When we iterate a simple cache, entries are constructed from hashes only, and
only added to the backend if, on opening, we discover the entry was not already
open through another path. As a result, there's two ways for an entry to join
active_entries.

To keep the state machine in the Backend from polluting the Entry too much, I
abstracted away a destruction observer, which the Backend provides to the Entry
when it's appropriate. I chose destruction as the place for this to run, because
that avoids double-deletes. As well, we can now store these in a standard
scoped_ptr.

Thoughts?

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

Please enjoy the following mix of substantive suggestions, constructive
criticism, and completely arbitrary style nits.

The major things I want are:
1. Clearer naming of the Disconnector (to make it clear it auto-disconnects).
2. Clearer explanation/control flow for when and how the Disconnector is
attached.
This adds complexity to things, and I want to make sure it's clear to future
readers.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (left):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:526: DCHECK(it->second.get());
Why not change to DCHECK(it->second)?

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:209: if (backend_)
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.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:531:
active_entries_.insert(EntryMap::value_type(entry_hash, NULL));
Ooh, that's cute, using ::value_type. Nice.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:534: it->second = new
SimpleEntryImpl(
I'd prefer if you had this named entry still and then assigned it to it->second,
but it's not a big deal.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:540: it->second->SetKey(key);
Similarly, for no good reason, I'd prefer SetKey before SetBackendDisconnector.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:664: scoped_ptr<EntryDisconnector>
Can we abstract out "add this entry to the map using this iterator and also set
a disconnector on it" to a separate method?

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:666:
it->second->SetBackendDisconnector(
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.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_entry_impl.cc:583: backend_disconnector_.reset();
I'd like a comment here making it clear that resetting the disconnector causes
it to disconnect. (That, or name it auto_disconnector or scoped_disconnector or
something that makes the behavior more inherently clear. Actually, calling it
Connector or Connection or something, since it destroys the connection when it
is destroyed, might make more sense?)

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_entry_impl.h:311: // The |backend_disconnector_| is
among our first methods to insure that the
This is a field, not a method, and isn't destruction order the opposite of
construction order, so shouldn't it be near the *bottom* to be removed early?

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

Thanks for the review, ttuttle. I've significantly cleaned up comments and the
code paths you found confusing, and done a rename that makes this class' role a
lot more clear. How is this?

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (left):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:526: DCHECK(it->second.get());
On 2014/08/22 19:26:38, ttuttle wrote:
> Why not change to DCHECK(it->second)?

Done.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:209: if (backend_)
On 2014/08/22 19:26:37, ttuttle wrote:
> 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.

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

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:534: it->second = new
SimpleEntryImpl(
On 2014/08/22 19:26:37, ttuttle wrote:
> I'd prefer if you had this named entry still and then assigned it to
it->second,
> but it's not a big deal.

Done.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:540: it->second->SetKey(key);
On 2014/08/22 19:26:38, ttuttle wrote:
> Similarly, for no good reason, I'd prefer SetKey before
SetBackendDisconnector.

Done.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:664: scoped_ptr<EntryDisconnector>
On 2014/08/22 19:26:37, ttuttle wrote:
> Can we abstract out "add this entry to the map using this iterator and also
set
> a disconnector on it" to a separate method?

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?

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.cc:666:
it->second->SetBackendDisconnector(
On 2014/08/22 19:26:37, ttuttle wrote:
> 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.

I've beefed up the comment on this branch.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_entry_impl.cc:583: backend_disconnector_.reset();
On 2014/08/22 19:26:38, ttuttle wrote:
> I'd like a comment here making it clear that resetting the disconnector causes
> it to disconnect. (That, or name it auto_disconnector or scoped_disconnector
or
> something that makes the behavior more inherently clear. Actually, calling it
> Connector or Connection or something, since it destroys the connection when it
> is destroyed, might make more sense?)

I like your suggestion we rename this; I have gone a bit farther and renamed the
class, too.

I'm calling it "ActiveEntryProxy" now. This bleeds a bit of the abstraction from
the backend to the entry, but the name makes the flow a lot clearer; the pure
virtual base class abstraction avoids bleeding the design as heavily in to the
code as it was before, which makes getting the special cases right easier.

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/478573003/diff/100001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_entry_impl.h:311: // The |backend_disconnector_| is
among our first methods to insure that the
On 2014/08/22 19:26:38, ttuttle wrote:
> This is a field, not a method, and isn't destruction order the opposite of
> construction order, so shouldn't it be near the *bottom* to be removed early?

Removed this comment. And I think its placement is fine; I'm not moving it. It's
really scary referencing in to in-destruction objects, and putting this early in
destruction seems to encourage that kind of chicanery.

[Deprecated (see juliatuttle), 2014-08-25 17:59:46]

lgtm.

[gavinp, 2014-08-25 18:06:50]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-08-25 18:08:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/478573003/160001

[commit-bot: I haz the power, 2014-08-25 19:03:23]

Committed patchset #9 (160001) as 7bf7f493fdaf8c784266543344d75ffd7ac98138

[commit-bot: I haz the power, 2014-09-10 02:36:23]

Patchset 9 (id:??) landed as
https://crrev.com/b02ee6e3d42dc4181251bbf4c4df0e3cc91994f5
Cr-Commit-Position: refs/heads/master@{#291728}