Fix downloads db state=3 corruption using version=23

Fix downloads db state=3 corruption using version=23

dcommitted because the bots don't like binary files.

Add a layer of abstraction between the |state| db field and DownloadItem::DownloadState. This will actually prevent the same problem from happening again when I reland http://codereview.chromium.org/10704026/

BUG=140687
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150542

[benjhayden, 2012-08-07 15:53:06]

PTAL

[Randy Smith (Not in Mondays), 2012-08-07 16:22:55]

LGTM.

[commit-bot: I haz the power, 2012-08-07 16:23:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10823203/3

[commit-bot: I haz the power, 2012-08-07 16:23:51]

Presubmit check for 10823203-3 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/history

[benjhayden, 2012-08-07 17:41:24]

Asanka, PTAL at PS3. Chris's suggestion to take the code for a spin revealed a
secondary crash: something like "could not find resource 0".

[asanka, 2012-08-07 17:57:54]

On 2012/08/07 17:41:24, benjhayden_chromium wrote:
> Asanka, PTAL at PS3. Chris's suggestion to take the code for a spin revealed a
> secondary crash: something like "could not find resource 0".

If the state is the only corruption, how is it that there's no interrupt reason
for the download item?

[benjhayden, 2012-08-07 18:14:30]

On 2012/08/07 17:57:54, asanka wrote:
> On 2012/08/07 17:41:24, benjhayden_chromium wrote:
> > Asanka, PTAL at PS3. Chris's suggestion to take the code for a spin revealed
a
> > secondary crash: something like "could not find resource 0".
> 
> If the state is the only corruption, how is it that there's no interrupt
reason
> for the download item?

I used sqlite3 to just change the state field on a successful completed record
from 1 to 3, then restarted chrome. I didn't see an error reason field in the
db. When items are loaded from history, last_reason_ defaults to
DOWNLOAD_INTERRUPT_REASON_NONE.
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/download/d...

[asanka, 2012-08-07 18:46:00]

LGTM

[benjhayden, 2012-08-07 20:15:18]

PTAL
Tested manually:
from a fresh build from tot, download a file to completion
close chrome
sqlite3 profile/Default/History
update downloads set state=3 where id=1;
apply PS4, build and run chrome
opening chrome://downloads should display your download in red with 'Download
Error', not crash

[benjhayden, 2012-08-07 20:16:21]

Scott, would you mind looking over my migration code?

[Randy Smith (Not in Mondays), 2012-08-07 20:36:16]

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
UMA?  (You're welcome to say not in this CL, but I do want it, because I don't
want us to just ignore problems like the one that just happened.)

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:200: return false;
DCHECK?  If we get an invalid state here, it means something in the rest of the
program has done something wrong.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.h:76: bool MigrateDownloadsState();
I'd be inclined to suggest this take an argument which is the DB version number
we're coming from, and you have conditionals inside of it to see if you need to
do the migration.

[sky, 2012-08-07 20:54:17]

In general we don't have this level of defensive code in Chrome.

Also, you should add a test to verify migration.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 20:36:16, rdsmith wrote:
> UMA?  (You're welcome to say not in this CL, but I do want it, because I don't
> want us to just ignore problems like the one that just happened.)

Why add this? At best we do a DCHECK for this sort of thing.

[asanka, 2012-08-07 20:56:14]

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
If you skip this early, wouldn't you risk not updating next_db_handle_ ? Or is
that okay since this entry is considered invalid?

[Randy Smith (Not in Mondays), 2012-08-07 21:01:35]

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 20:54:17, sky wrote:
> On 2012/08/07 20:36:16, rdsmith wrote:
> > UMA?  (You're welcome to say not in this CL, but I do want it, because I
don't
> > want us to just ignore problems like the one that just happened.)
> 
> Why add this? At best we do a DCHECK for this sort of thing.

The basic problem is that almost any sanitization would have meant we didn't
notice the bug that started all this, which showed up as a crash on canary.  So
I want something that has some chance of letting us know if there's a lot of DB
corruption out in the wild, so that we can respond to it and make sure it's not
the result of a bug in the code.  Is there some better pattern?

[sky, 2012-08-07 22:36:35]

On Tue, Aug 7, 2012 at 2:01 PM,  <rdsmith@chromium.org> wrote:
>
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> File chrome/browser/history/download_database.cc (right):
>
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> chrome/browser/history/download_database.cc:182: continue;
> On 2012/08/07 20:54:17, sky wrote:
>>
>> On 2012/08/07 20:36:16, rdsmith wrote:
>> > UMA?  (You're welcome to say not in this CL, but I do want it,
>
> because I don't
>>
>> > want us to just ignore problems like the one that just happened.)
>
>
>> Why add this? At best we do a DCHECK for this sort of thing.
>
>
> The basic problem is that almost any sanitization would have meant we
> didn't notice the bug that started all this, which showed up as a crash
> on canary.  So I want something that has some chance of letting us know
> if there's a lot of DB corruption out in the wild, so that we can
> respond to it and make sure it's not the result of a bug in the code.
> Is there some better pattern?

Code like this wouldn't have helped identify the problem. Well, I
guess it wouldn't crash but the database would have been in a bad
state. The problem with this is where do you draw the line? What about
verifying the time is valid? What about ensuring total_byes is >= 0?

Add a test for migration and I'll approve this.

  -Scott

[Randy Smith (Not in Mondays), 2012-08-08 00:01:27]

On 2012/08/07 22:36:35, sky wrote:
> On Tue, Aug 7, 2012 at 2:01 PM,  <mailto:rdsmith@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> > File chrome/browser/history/download_database.cc (right):
> >
> >
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> > chrome/browser/history/download_database.cc:182: continue;
> > On 2012/08/07 20:54:17, sky wrote:
> >>
> >> On 2012/08/07 20:36:16, rdsmith wrote:
> >> > UMA?  (You're welcome to say not in this CL, but I do want it,
> >
> > because I don't
> >>
> >> > want us to just ignore problems like the one that just happened.)
> >
> >
> >> Why add this? At best we do a DCHECK for this sort of thing.
> >
> >
> > The basic problem is that almost any sanitization would have meant we
> > didn't notice the bug that started all this, which showed up as a crash
> > on canary.  So I want something that has some chance of letting us know
> > if there's a lot of DB corruption out in the wild, so that we can
> > respond to it and make sure it's not the result of a bug in the code.
> > Is there some better pattern?
> 
> Code like this wouldn't have helped identify the problem. Well, I
> guess it wouldn't crash but the database would have been in a bad
> state. The problem with this is where do you draw the line? What about
> verifying the time is valid? What about ensuring total_byes is >= 0?

Fair enough.  Possibly the right thing to do is to target this CL at purely the
introduced corruption, and leave any database sanitization due to random cosmic
ray errors for another CL (including maybe making it more thorough, and maybe
also somewhere along the line having automated tests to try and detect DB
changes that weren't intended).  Now that I think about it, I think that's what
I'd prefer, but I certainly wouldn't block code like this to do a little bit of
sanitization.  

But if it does go in, I do want *something* in place that might give us a hint
that we've messed up if we do so in the future.  Maybe UMA is just a gesture in
that direction, but it's something.

Ben, up to you which way you go; getting something in quickly to fix the
crashers is the  highest priority.  
> 
> Add a test for migration and I'll approve this.
> 
>   -Scott

[Randy Smith (Not in Mondays), 2012-08-08 00:01:59]

LGTM conditional on either not doing general from-DB sanitization or putting UMA
in so we can track if we have to do it.

[benjhayden, 2012-08-08 01:11:06]

On 2012/08/07 22:36:35, sky wrote:
> On Tue, Aug 7, 2012 at 2:01 PM,  <mailto:rdsmith@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> > File chrome/browser/history/download_database.cc (right):
> >
> >
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
> > chrome/browser/history/download_database.cc:182: continue;
> > On 2012/08/07 20:54:17, sky wrote:
> >>
> >> On 2012/08/07 20:36:16, rdsmith wrote:
> >> > UMA?  (You're welcome to say not in this CL, but I do want it,
> >
> > because I don't
> >>
> >> > want us to just ignore problems like the one that just happened.)
> >
> >
> >> Why add this? At best we do a DCHECK for this sort of thing.
> >
> >
> > The basic problem is that almost any sanitization would have meant we
> > didn't notice the bug that started all this, which showed up as a crash
> > on canary.  So I want something that has some chance of letting us know
> > if there's a lot of DB corruption out in the wild, so that we can
> > respond to it and make sure it's not the result of a bug in the code.
> > Is there some better pattern?
> 
> Code like this wouldn't have helped identify the problem. Well, I
> guess it wouldn't crash but the database would have been in a bad
> state. The problem with this is where do you draw the line? What about
> verifying the time is valid? What about ensuring total_byes is >= 0?

This isn't just sanitization. This is a layer of abstraction that will actually
prevent the same problem from happening when i reland the change that caused
this mess.

> 
> Add a test for migration and I'll approve this.

Adding a test now. I'll say PTAL when done if you're online, otherwise i'll tbr.

> 
>   -Scott

[benjhayden, 2012-08-08 01:11:52]

working on a test for the migration.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 20:54:17, sky wrote:
> On 2012/08/07 20:36:16, rdsmith wrote:
> > UMA?  (You're welcome to say not in this CL, but I do want it, because I
don't
> > want us to just ignore problems like the one that just happened.)
> 
> Why add this? At best we do a DCHECK for this sort of thing.

This layer of abstraction separates the compiler-generated enum values for
DownloadState from the db schema values for |state| in order to protect against
the kind of change to DownloadState that caused this mess.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 20:36:16, rdsmith wrote:
> UMA?  (You're welcome to say not in this CL, but I do want it, because I don't
> want us to just ignore problems like the one that just happened.)

Done.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 20:56:14, asanka wrote:
> If you skip this early, wouldn't you risk not updating next_db_handle_ ? Or is
> that okay since this entry is considered invalid?

Done.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/07 21:01:35, rdsmith wrote:
> On 2012/08/07 20:54:17, sky wrote:
> > On 2012/08/07 20:36:16, rdsmith wrote:
> > > UMA?  (You're welcome to say not in this CL, but I do want it, because I
> don't
> > > want us to just ignore problems like the one that just happened.)
> > 
> > Why add this? At best we do a DCHECK for this sort of thing.
> 
> The basic problem is that almost any sanitization would have meant we didn't
> notice the bug that started all this, which showed up as a crash on canary. 
So
> I want something that has some chance of letting us know if there's a lot of
DB
> corruption out in the wild, so that we can respond to it and make sure it's
not
> the result of a bug in the code.  Is there some better pattern?

This isn't just sanitization. It's a mapping, a layer of abstraction to protect
the schema from changes to DownloadState.
I also added the UMA.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:200: return false;
On 2012/08/07 20:36:16, rdsmith wrote:
> DCHECK?  If we get an invalid state here, it means something in the rest of
the
> program has done something wrong.

Is it possible for UpdateDownload to see a download in the REMOVING state?
How sure are you?
I'll put in a todo.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.h:76: bool MigrateDownloadsState();
On 2012/08/07 20:36:16, rdsmith wrote:
> I'd be inclined to suggest this take an argument which is the DB version
number
> we're coming from, and you have conditionals inside of it to see if you need
to
> do the migration.

That's not the pattern in history_database.cc

[Randy Smith (Not in Mondays), 2012-08-08 01:52:25]

Still LGTM.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:182: continue;
On 2012/08/08 01:11:52, benjhayden_chromium wrote:
> On 2012/08/07 21:01:35, rdsmith wrote:
> > On 2012/08/07 20:54:17, sky wrote:
> > > On 2012/08/07 20:36:16, rdsmith wrote:
> > > > UMA?  (You're welcome to say not in this CL, but I do want it, because I
> > don't
> > > > want us to just ignore problems like the one that just happened.)
> > > 
> > > Why add this? At best we do a DCHECK for this sort of thing.
> > 
> > The basic problem is that almost any sanitization would have meant we didn't
> > notice the bug that started all this, which showed up as a crash on canary. 
> So
> > I want something that has some chance of letting us know if there's a lot of
> DB
> > corruption out in the wild, so that we can respond to it and make sure it's
> not
> > the result of a bug in the code.  Is there some better pattern?
> 
> This isn't just sanitization. It's a mapping, a layer of abstraction to
protect
> the schema from changes to DownloadState.
> I also added the UMA.

Good point.  Ok.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.cc:200: return false;
On 2012/08/08 01:11:52, benjhayden_chromium wrote:
> On 2012/08/07 20:36:16, rdsmith wrote:
> > DCHECK?  If we get an invalid state here, it means something in the rest of
> the
> > program has done something wrong.
> 
> Is it possible for UpdateDownload to see a download in the REMOVING state?
> How sure are you?

Not sure enough for this CL :-} :-|.

> I'll put in a todo.

Sounds good.

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
chrome/browser/history/download_database.h:76: bool MigrateDownloadsState();
On 2012/08/08 01:11:52, benjhayden_chromium wrote:
> On 2012/08/07 20:36:16, rdsmith wrote:
> > I'd be inclined to suggest this take an argument which is the DB version
> number
> > we're coming from, and you have conditionals inside of it to see if you need
> to
> > do the migration.
> 
> That's not the pattern in history_database.cc

Ok.

[commit-bot: I haz the power, 2012-08-08 03:19:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10823203/3017

[commit-bot: I haz the power, 2012-08-08 03:19:21]

Presubmit check for 10823203-3017 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/history

Presubmit checks took 2.6s to calculate.

[commit-bot: I haz the power, 2012-08-08 03:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10823203/3017

[benjhayden, 2012-08-08 03:33:42]

I fixed the version in the typical_history sqlite3 file, but it isn't showing up
on the bot because it's a binary file. Looks like i'll need to dcommit this.
I need sleep. I literally grow horns without it.
I'll dcommit this as soon as i wake up, so it should make it through the
waterfall before mtv has even had their coffee.

[sky, 2012-08-08 04:14:34]

LGTM

[sky, 2012-08-08 04:55:59]

On Tue, Aug 7, 2012 at 6:11 PM,  <benjhayden@chromium.org> wrote:
> On 2012/08/07 22:36:35, sky wrote:
>>
>> On Tue, Aug 7, 2012 at 2:01 PM,  <mailto:rdsmith@chromium.org> wrote:
>> >
>> >
>
>
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
>>
>> > File chrome/browser/history/download_database.cc (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/10823203/diff/9004/chrome/browser/history/down...
>>
>> > chrome/browser/history/download_database.cc:182: continue;
>> > On 2012/08/07 20:54:17, sky wrote:
>> >>
>> >> On 2012/08/07 20:36:16, rdsmith wrote:
>> >> > UMA?  (You're welcome to say not in this CL, but I do want it,
>> >
>> > because I don't
>> >>
>> >> > want us to just ignore problems like the one that just happened.)
>> >
>> >
>> >> Why add this? At best we do a DCHECK for this sort of thing.
>> >
>> >
>> > The basic problem is that almost any sanitization would have meant we
>> > didn't notice the bug that started all this, which showed up as a crash
>> > on canary.  So I want something that has some chance of letting us know
>> > if there's a lot of DB corruption out in the wild, so that we can
>> > respond to it and make sure it's not the result of a bug in the code.
>> > Is there some better pattern?
>
>
>> Code like this wouldn't have helped identify the problem. Well, I
>> guess it wouldn't crash but the database would have been in a bad
>> state. The problem with this is where do you draw the line? What about
>> verifying the time is valid? What about ensuring total_byes is >= 0?
>
>
> This isn't just sanitization. This is a layer of abstraction that will
> actually
> prevent the same problem from happening when i reland the change that caused
> this mess.

I was referring to the DCHECKs. I agree having a database specific
mapping is a good idea.

  -Scott

>
>
>
>> Add a test for migration and I'll approve this.
>
>
> Adding a test now. I'll say PTAL when done if you're online, otherwise i'll
> tbr.
>
>
>>    -Scott
>
>
>
>
> http://codereview.chromium.org/10823203/