Unlink corrupt SimpleCache index files immediately after load.

Unlink corrupt SimpleCache index files immediately after load.

The crash on startup in the referenced bug is very bad in part because
the corrupt index which caused it is left on disk, so all startups
until user data is cleared will crash. For maximum safety, we should
unlink the index file ASAP after finding it is corrupt.

R=rdsmith@chromium.org
BUG=251465
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207813

[gavinp, 2013-06-19 17:17:31]

rdsmith, pasko: PTAL.

clamy: FYI.

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:91:
SimpleIndexFile::IndexCompletionCallback GetCallback() {
rdsmith: This fixes the funny scoping you complained about in an earlier review.

[Randy Smith (Not in Mondays), 2013-06-19 21:06:00]

LGTM.  Thanks for doing this--I kept meaning to go back and expand the unit
tests for the SimpleIndexFile, and never did :-J.

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:54: class
TestSimpleIndexFile : public SimpleIndexFile {
The pattern I've seen more often is friending the test class (in this case
SimpleIndexFileTest) directly, and including delegates in it for the private
member functions on the original that you want to use.  I don't object to this,
but I found it a bit weird, and it took me a little time to figure out why you
were doing it.  If you keep this version, put in a comment explaining what's
going on.

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:91:
SimpleIndexFile::IndexCompletionCallback GetCallback() {
On 2013/06/19 17:17:31, gavinp wrote:
> rdsmith: This fixes the funny scoping you complained about in an earlier
review.

Amusing.  Now that I understand what's going on, I'd also be good with the
original with a comment.  But this is fine too.

[gavinp, 2013-06-19 23:21:25]

Thanks Randy!

Pasko: PTAL.

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:54: class
TestSimpleIndexFile : public SimpleIndexFile {
On 2013/06/19 21:06:00, rdsmith wrote:
> The pattern I've seen more often is friending the test class (in this case
> SimpleIndexFileTest) directly, and including delegates in it for the private
> member functions on the original that you want to use.  I don't object to
this,
> but I found it a bit weird, and it took me a little time to figure out why you
> were doing it.  If you keep this version, put in a comment explaining what's
> going on.

Done.

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:91:
SimpleIndexFile::IndexCompletionCallback GetCallback() {
On 2013/06/19 21:06:00, rdsmith wrote:
> On 2013/06/19 17:17:31, gavinp wrote:
> > rdsmith: This fixes the funny scoping you complained about in an earlier
> review.
> 
> Amusing.  Now that I understand what's going on, I'd also be good with the
> original with a comment.  But this is fine too.

I prefer this; it makes the tests more succinct but leaves readability intact in
my opinion. I'll leave it as is, with no comment. Fair?

[Randy Smith (Not in Mondays), 2013-06-20 12:44:30]

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/14001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:91:
SimpleIndexFile::IndexCompletionCallback GetCallback() {
On 2013/06/19 23:21:25, gavinp wrote:
> On 2013/06/19 21:06:00, rdsmith wrote:
> > On 2013/06/19 17:17:31, gavinp wrote:
> > > rdsmith: This fixes the funny scoping you complained about in an earlier
> > review.
> > 
> > Amusing.  Now that I understand what's going on, I'd also be good with the
> > original with a comment.  But this is fine too.
> 
> I prefer this; it makes the tests more succinct but leaves readability intact
in
> my opinion. I'll leave it as is, with no comment. Fair?

SG; no worries.

[pasko, 2013-06-20 13:52:21]

I think the bugfix should go in a separate commit. It would reduce the chances
of hitting conflicts in tests if we get to cherry-picking. Would also make the
review faster.

https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file.cc:194: scoped_index_deleter.Release();
I don't like using automatic closure runner in this simple function. It
complicates the matter because of wtf-always-deleting-the-index until the very
end of the function where there is a Release(). Also it runs base::Bind and
other memory allocation heavy lifting only to throw everything away in .9999% of
cases.

I know, lisp-ish code looks beautiful, but could you just call
file_util::Delete(index_filename, false) on the two return paths? It will be
less lines of code and less metaprogramming.

[gavinp, 2013-06-20 17:25:46]

On 2013/06/20 13:52:21, pasko wrote:
> I think the bugfix should go in a separate commit. It would reduce the chances
> of hitting conflicts in tests if we get to cherry-picking. Would also make the
> review faster.

Will do, I'll pull out the style changes into a separate CL.

> 
>
https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
> File net/disk_cache/simple/simple_index_file.cc (right):
> 
>
https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
> net/disk_cache/simple/simple_index_file.cc:194:
scoped_index_deleter.Release();
> I don't like using automatic closure runner in this simple function. It
> complicates the matter because of wtf-always-deleting-the-index until the very
> end of the function where there is a Release(). Also it runs base::Bind and
> other memory allocation heavy lifting only to throw everything away in .9999%
of
> cases.
> 
> I know, lisp-ish code looks beautiful, but could you just call
> file_util::Delete(index_filename, false) on the two return paths? It will be
> less lines of code and less metaprogramming.

[gavinp, 2013-06-20 20:18:18]

Remediated to Pasko's review; good point on making this narrow.

pasko, rdsmith: PTAL.

rdsmith: I spoke offline to pasko, and because he's travelling tomorrow, he said
he'll take my word for it that I remediated to his comments, and I can land
based on your LGTM. I still want another pass, just because of how significant
this change is.

https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://chromiumcodereview.appspot.com/17265007/diff/25001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file.cc:194: scoped_index_deleter.Release();
On 2013/06/20 13:52:22, pasko wrote:
> I don't like using automatic closure runner in this simple function. It
> complicates the matter because of wtf-always-deleting-the-index until the very
> end of the function where there is a Release(). Also it runs base::Bind and
> other memory allocation heavy lifting only to throw everything away in .9999%
of
> cases.
> 
> I know, lisp-ish code looks beautiful, but could you just call
> file_util::Delete(index_filename, false) on the two return paths? It will be
> less lines of code and less metaprogramming.

Done. I was only trying to show off that I knew about base::IgnoreResult anyway,
and now that's done!

[Randy Smith (Not in Mondays), 2013-06-21 01:29:37]

LGTM.

[commit-bot: I haz the power, 2013-06-21 01:47:37]

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

[gavinp, 2013-06-21 14:40:44]

Committed patchset #7 manually as r207813 (presubmit successful).