Properly handle the Module SBDatabase when it's corrupted

Properly handle the Module SBDatabase when it's corrupted

BUG=595444
Committed: https://crrev.com/78dad59bea7ba92dfa77efd3d79fed9d6baad85b
Cr-Commit-Position: refs/heads/master@{#381809}

[proberge, 2016-03-17 15:01:13]

proberge@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-03-17 16:46:39]

lgtm

after one fix.   Also, please test by creating a corrupt file and watching it
delete/replace it.

Thanks.

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:1735:
module_whitelist_store_.get() ? module_whitelist_store_->Delete() : true;
This seems better than the above methods, overall, but is there a chance that
module_whitelist_store_ would be NULL _and_ the file exists? I think the answer
is no, but just wanted to confirm.

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.h:330:
FAILURE_MODULE_WHITELIST_DELETE = 38,
Add this to histograms.xml

[proberge, 2016-03-17 18:10:44]

On 2016/03/17 16:46:39, Nathan Parker wrote:
> lgtm
> 
> after one fix.   Also, please test by creating a corrupt file and watching it
> delete/replace it.
> 
> Thanks.
> 
>
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> 
>
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/safe_browsing_database.cc:1735:
> module_whitelist_store_.get() ? module_whitelist_store_->Delete() : true;
> This seems better than the above methods, overall, but is there a chance that
> module_whitelist_store_ would be NULL _and_ the file exists? I think the
answer
> is no, but just wanted to confirm.
> 
>
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/safe_browsing_database.h (right):
> 
>
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/safe_browsing_database.h:330:
> FAILURE_MODULE_WHITELIST_DELETE = 38,
> Add this to histograms.xml

Done. Here's what I observed by corrupting the file while Chromium was closed.
LMK if it's not the expected behavior:
* When starting Chromium, ALL SafeBrowsing DBs are deleted and this DCHECK here
is hit. Chromium immediately shuts down.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...
* Starting Chromium again works. After a few minutes, all databases are
downloaded.

[proberge, 2016-03-17 18:11:00]

proberge@chromium.org changed reviewers:
+ rkaplow@chromium.org

[proberge, 2016-03-17 18:11:00]

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:1735:
module_whitelist_store_.get() ? module_whitelist_store_->Delete() : true;
On 2016/03/17 16:46:39, Nathan Parker wrote:
> This seems better than the above methods, overall, but is there a chance that
> module_whitelist_store_ would be NULL _and_ the file exists? I think the
answer
> is no, but just wanted to confirm.

It's the same as the other whitelist delete logic (see lines 1682 to 1697). I
know very little about this code, but I think that if module_whitelist_store_ is
NULL then it can't be the corruption cause and will get deleted the next time it
is.

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.h (right):

https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.h:330:
FAILURE_MODULE_WHITELIST_DELETE = 38,
On 2016/03/17 16:46:39, Nathan Parker wrote:
> Add this to histograms.xml

Done.

[Nathan Parker, 2016-03-17 18:23:30]

On 2016/03/17 at 18:10:44, proberge wrote:
> On 2016/03/17 16:46:39, Nathan Parker wrote:
> > lgtm
> > 
> > after one fix.   Also, please test by creating a corrupt file and watching
it
> > delete/replace it.
> > 
> > Thanks.
> > 
> >
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> > File chrome/browser/safe_browsing/safe_browsing_database.cc (right):
> > 
> >
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> > chrome/browser/safe_browsing/safe_browsing_database.cc:1735:
> > module_whitelist_store_.get() ? module_whitelist_store_->Delete() : true;
> > This seems better than the above methods, overall, but is there a chance
that
> > module_whitelist_store_ would be NULL _and_ the file exists? I think the
answer
> > is no, but just wanted to confirm.
> > 
> >
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> > File chrome/browser/safe_browsing/safe_browsing_database.h (right):
> > 
> >
https://codereview.chromium.org/1807253002/diff/1/chrome/browser/safe_browsin...
> > chrome/browser/safe_browsing/safe_browsing_database.h:330:
> > FAILURE_MODULE_WHITELIST_DELETE = 38,
> > Add this to histograms.xml
> 
> Done. Here's what I observed by corrupting the file while Chromium was closed.
LMK if it's not the expected behavior:
> * When starting Chromium, ALL SafeBrowsing DBs are deleted and this DCHECK
here is hit. Chromium immediately shuts down.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...
> * Starting Chromium again works. After a few minutes, all databases are
downloaded.

Yup, that's what I'd expect.  Thanks.

[rkaplow, 2016-03-17 18:28:11]

lgtm

[proberge, 2016-03-17 18:28:30]

The CQ bit was checked by proberge@chromium.org

[proberge, 2016-03-17 18:28:31]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/1807253002/#ps20001
(title: "Add to histograms.xml")

[commit-bot: I haz the power, 2016-03-17 18:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1807253002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1807253002/20001

[commit-bot: I haz the power, 2016-03-17 20:16:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-17 20:16:46]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[proberge, 2016-03-17 20:19:05]

The CQ bit was checked by proberge@chromium.org

[commit-bot: I haz the power, 2016-03-17 20:19:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1807253002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1807253002/20001

[commit-bot: I haz the power, 2016-03-17 22:34:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-17 22:35:49]

Description was changed from

==========
Properly handle the Module SBDatabase when it's corrupted

BUG=595444
==========

to

==========
Properly handle the Module SBDatabase when it's corrupted

BUG=595444

Committed: https://crrev.com/78dad59bea7ba92dfa77efd3d79fed9d6baad85b
Cr-Commit-Position: refs/heads/master@{#381809}
==========

[commit-bot: I haz the power, 2016-03-17 22:35:50]

Patchset 2 (id:??) landed as
https://crrev.com/78dad59bea7ba92dfa77efd3d79fed9d6baad85b
Cr-Commit-Position: refs/heads/master@{#381809}