Don't doom the wrong simple cache entry.

Don't doom the wrong simple cache entry.

Previously, dooming a previously doomed simple cache entry could doom an
existing entry. Now fixed.

R=pliard,pasko
BUG=289542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223073

[gavinp, 2013-08-30 13:27:56]

pliard, pasko: PTAL.

Note this is downstream of https://codereview.chromium.org/23486006/ , which is
the CL for adding the pending doom map to the backend.

[pasko, 2013-08-30 13:37:09]

On 2013/08/30 13:27:56, gavinp wrote:
> pliard, pasko: PTAL.
> 
> Note this is downstream of https://codereview.chromium.org/23486006/ , which
is
> the CL for adding the pending doom map to the backend.

Cool!

Though I am a little confused in what order I am supposed to give L G T Ms with
inter-dependent changes that may result in non-trivial merges. Should this one
rather be proposed as a part of the upstream change? This one could be just
adding a DISABLED_ test.

[Philippe, 2013-08-30 13:38:54]

Nice, thanks Gavin! LGTM. This is the pre-existing bug that you mentioned in my
CL, right?

[gavinp, 2013-08-30 14:50:40]

On 2013/08/30 13:38:54, Philippe wrote:
> Nice, thanks Gavin! LGTM. This is the pre-existing bug that you mentioned in
my
> CL, right?

Yes. The immediately upstream patch which adds "doomed_" to the SimpleEntryImpl
makes it worse though, as double-dooms cause assertion failures in the backend.
So this should land very shortly after that CL if it isn't merged with it.

[gavinp, 2013-08-30 14:54:50]

On 2013/08/30 13:37:09, pasko wrote:
> On 2013/08/30 13:27:56, gavinp wrote:
> > pliard, pasko: PTAL.
> > 
> > Note this is downstream of https://codereview.chromium.org/23486006/ , which
> is
> > the CL for adding the pending doom map to the backend.
> 
> Cool!
> 
> Though I am a little confused in what order I am supposed to give L G T Ms
with
> inter-dependent changes that may result in non-trivial merges. Should this one
> rather be proposed as a part of the upstream change? This one could be just
> adding a DISABLED_ test.

pasko,

I can merge this with the other CL if you'd like; I mildly prefer keeping it
separate as it is a logically distinct issue.

If this CL is LGTMed before the (two!) upstream issues it depends on, I just
will sit on the LGTM and rebase this while I wait for upstream dependencies to
clear.

[pasko, 2013-08-30 15:39:51]

On 2013/08/30 14:54:50, gavinp wrote:
> On 2013/08/30 13:37:09, pasko wrote:
> > On 2013/08/30 13:27:56, gavinp wrote:
> > > pliard, pasko: PTAL.
> > > 
> > > Note this is downstream of https://codereview.chromium.org/23486006/ ,
which
> > is
> > > the CL for adding the pending doom map to the backend.
> > 
> > Cool!
> > 
> > Though I am a little confused in what order I am supposed to give L G T Ms
> with
> > inter-dependent changes that may result in non-trivial merges. Should this
one
> > rather be proposed as a part of the upstream change? This one could be just
> > adding a DISABLED_ test.
> 
> pasko,
> 
> I can merge this with the other CL if you'd like; I mildly prefer keeping it
> separate as it is a logically distinct issue.
> 
> If this CL is LGTMed before the (two!) upstream issues it depends on, I just
> will sit on the LGTM and rebase this while I wait for upstream dependencies to
> clear.

As a reviewer I would ideally prefer to have separate CLs announced in the air,
then getting the PING from the author when upstream CLs were committed, so I
could send out a final L G T M.

[gavinp, 2013-08-30 15:43:16]

On 2013/08/30 15:39:51, pasko wrote:
> On 2013/08/30 14:54:50, gavinp wrote:
> > On 2013/08/30 13:37:09, pasko wrote:
> > > On 2013/08/30 13:27:56, gavinp wrote:
> > > > pliard, pasko: PTAL.
> > > > 
> > > > Note this is downstream of https://codereview.chromium.org/23486006/ ,
> which
> > > is
> > > > the CL for adding the pending doom map to the backend.
> > > 
> > > Cool!
> > > 
> > > Though I am a little confused in what order I am supposed to give L G T Ms
> > with
> > > inter-dependent changes that may result in non-trivial merges. Should this
> one
> > > rather be proposed as a part of the upstream change? This one could be
just
> > > adding a DISABLED_ test.
> > 
> > pasko,
> > 
> > I can merge this with the other CL if you'd like; I mildly prefer keeping it
> > separate as it is a logically distinct issue.
> > 
> > If this CL is LGTMed before the (two!) upstream issues it depends on, I just
> > will sit on the LGTM and rebase this while I wait for upstream dependencies
to
> > clear.
> 
> As a reviewer I would ideally prefer to have separate CLs announced in the
air,
> then getting the PING from the author when upstream CLs were committed, so I
> could send out a final L G T M.

OK, I'll ping you once the upstream change is clear to land, and then this one
can be reviewed. No problem! In the meantime I'll keep this rebased regularly.

[gavinp, 2013-09-11 19:25:43]

pasko: The upstream change is now in CQ. PTAL now, I'd like to land this quickly
after its upstream.

[pasko, 2013-09-12 10:46:48]

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
File net/disk_cache/entry_unittest.cc (right):

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3056: // Create, Doom, Create, Doom (1st
entry), Open.
do we want more sequences like that in the following CLs to be even more sure?
what would they be?

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3066: cb.GetResult(cache_->CreateEntry(key,
&entry1, cb.callback())));
The common style in all disk_cache testing is to write:
ASSERT_EQ(net::OK, CreateEntry(key, &entry1));

please use this pattern also in the test above. We should use  cache_->Method()
only when we do not want to wait for result, like testing optimistic operations.

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3080:
static_cast<disk_cache::SimpleEntryImpl*>(entry1);
why casting? could you just call Doom() on the entry or DoomEntry() on the
backend?

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:256: if (doomed_)
hm, so if the MarkAsDoomed() on the previous entry was called while the backend
was gone, we would still now go and doom the wrong entry? Or we could not doom
the wrong one because the wrong one could not be created while the backend was
gone?

subtle, heh? So maybe we just need to mark doomed_ = true unconditionally in
MarkAsDoomed()?

[gavinp, 2013-09-12 15:39:48]

pasko: PTAL.

I wish I had a better answer about more patterns.

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
File net/disk_cache/entry_unittest.cc (right):

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3056: // Create, Doom, Create, Doom (1st
entry), Open.
On 2013/09/12 10:46:48, pasko wrote:
> do we want more sequences like that in the following CLs to be even more sure?
> what would they be?

I'm not sure.

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3066: cb.GetResult(cache_->CreateEntry(key,
&entry1, cb.callback())));
On 2013/09/12 10:46:48, pasko wrote:
> The common style in all disk_cache testing is to write:
> ASSERT_EQ(net::OK, CreateEntry(key, &entry1));
> 
> please use this pattern also in the test above. We should use 
cache_->Method()
> only when we do not want to wait for result, like testing optimistic
operations.

Done.

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3080:
static_cast<disk_cache::SimpleEntryImpl*>(entry1);
On 2013/09/12 10:46:48, pasko wrote:
> why casting? could you just call Doom() on the entry or DoomEntry() on the
> backend?

I want to wait for the callback, so I know this doom is complete before I
attempt to OpenEntry below; I'm trying to make sure entry2 isn't being doomed.

I added a comment.

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/23823002/diff/68001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:256: if (doomed_)
On 2013/09/12 10:46:48, pasko wrote:
> hm, so if the MarkAsDoomed() on the previous entry was called while the
backend
> was gone, we would still now go and doom the wrong entry? Or we could not doom
> the wrong one because the wrong one could not be created while the backend was
> gone?
> 
> subtle, heh? So maybe we just need to mark doomed_ = true unconditionally in
> MarkAsDoomed()?

Done.

[gavinp, 2013-09-12 15:40:09]

Nota bene: this is based on trunk now.

[pasko, 2013-09-13 12:00:01]

LGTM, thank you, Gavin!

The subtlety of having two entries and only one being active is an interesting
one, we can continue operating on files on a non-active entry, but cannot create
or delete them. I wish I could write a clear description of what we are doing in
the design document. The whole approach would cause interesting cases for
generational cache and optional stream 2. Can we create an optional stream 2
when an entry was doomed and another was created to replace it? Are we going to
just fail with stream 2 operations?

[commit-bot: I haz the power, 2013-09-13 13:12:05]

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

[Deprecated (see juliatuttle), 2013-09-13 15:35:51]

On 2013/09/13 12:00:01, pasko wrote:
> LGTM, thank you, Gavin!
> 
> The subtlety of having two entries and only one being active is an interesting
> one, we can continue operating on files on a non-active entry, but cannot
create
> or delete them. I wish I could write a clear description of what we are doing
in
> the design document. The whole approach would cause interesting cases for
> generational cache and optional stream 2. Can we create an optional stream 2
> when an entry was doomed and another was created to replace it? Are we going
to
> just fail with stream 2 operations?

Currently the optional-stream-2 patch fails (with ERR_CACHE_WRITE_FAILURE) a
write that would create a third-stream file in a doomed entry.

[pasko, 2013-09-13 17:04:30]

On 2013/09/13 15:35:51, ttuttle wrote:
> On 2013/09/13 12:00:01, pasko wrote:
> > LGTM, thank you, Gavin!
> > 
> > The subtlety of having two entries and only one being active is an
interesting
> > one, we can continue operating on files on a non-active entry, but cannot
> create
> > or delete them. I wish I could write a clear description of what we are
doing
> in
> > the design document. The whole approach would cause interesting cases for
> > generational cache and optional stream 2. Can we create an optional stream 2
> > when an entry was doomed and another was created to replace it? Are we going
> to
> > just fail with stream 2 operations?
> 
> Currently the optional-stream-2 patch fails (with ERR_CACHE_WRITE_FAILURE) a
> write that would create a third-stream file in a doomed entry.

yeah, if it becomes a problem we can always switch to creating a temporary file
and immediately doing unlink on it. Manipulating zombie matter in correct ways
is what engineers .. do.

[commit-bot: I haz the power, 2013-09-13 17:34:40]

Change committed as 223073