Disk cache: Prevent obscure file corruption and deal...

Disk cache: Prevent obscure file corruption and deal
with the result of that corruption.

1. Now we mark any open entry as dirty, even if we are
supposed to delete the entry right away because if we
crash before that, we may end up clearing the dirty
flag of a dirty entry.

2. When we look for a parent of a given entry we now
double check that the entry is the one that we want (and
not just another entry with the same key).

3. If we have a loop on the hash collision list (result of
failing to do 1 and 2 above), we figure that out.

4. Now every time we open an entry from an LRU list we end
up using the same code path (with the proper handling of
dirty entries).

BUG=69135
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72563

[gavinp, 2011-01-25 20:23:51]

I think we might want to think about splitting up MatchEntry.  Otherwise,
awesome fix!

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1572: bool* match_error) {
I'm concerned this function is doing too much.  The call sites really show it:
most of them set find_parent to false, and cons up an Addr() and a bool just for
show.  I don't like seeing that pattern too much, since it pollutes the call
sites.

My hope is that we could write a new method MatchEntryParent in terms of
MatchEntry or a MatchEntryPrivate?  Could we perhaps write MatchEntryPrivate
that always finds both a parent and an element, and the two callers return a
different one?  That'd simplify the loop (no calls to if (find_parent)
everywhere, we just always grab it), and I think the call sites would look
better too?

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/eviction.cc
File net/disk_cache/eviction.cc (left):

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/eviction.cc#oldc...
net/disk_cache/eviction.cc:481: // entries that live in this list).
Awesome!  I'm always happy to see TODOs go.

[rvargas (doing something else), 2011-01-25 20:47:53]

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6292011/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1572: bool* match_error) {
Yeah, I thought about that too. In fact, I initially wrote the change thinking
that we could get rid of find_parent and just use the address as an indication
that there is a child already (so we need the parent), but there is a caller
that is not happy with that pattern.

The other thing that I don't particularly like is that when we create a new
entry we basically go through the loop twice, once looking for the entry and
once looking for the parent (if needed). In other words, changing the way this
method works sounds like a good thing... and it looks like it won't be so much a
splitting of the function but rather making this function do a little more, but
adding small wrappers.

Do you mind if we leave that change for another CL?


On 2011/01/25 20:23:51, gavinp wrote:
> I'm concerned this function is doing too much.  The call sites really show it:
> most of them set find_parent to false, and cons up an Addr() and a bool just
for
> show.  I don't like seeing that pattern too much, since it pollutes the call
> sites.
> 
> My hope is that we could write a new method MatchEntryParent in terms of
> MatchEntry or a MatchEntryPrivate?  Could we perhaps write MatchEntryPrivate
> that always finds both a parent and an element, and the two callers return a
> different one?  That'd simplify the loop (no calls to if (find_parent)
> everywhere, we just always grab it), and I think the call sites would look
> better too?

[gavinp, 2011-01-25 20:54:01]

Ricardo,

Go ahead an leave that change for another issue, we shouldn't delay this bug fix
going in.

This is LGTM provided you create that issue and assign it to yourself.  Thanks!