[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

SQLITE_NOTADB happens when the SQLite header data is broken.  That data
includes meta-information about the file's structure, so any corruption
in the header would require manual intervention to recover - assuming
that the entire file is not broken.  SQLITE_NOTADB files are not even
partially readable through SQLite, so deletion unsticks the file going
forward.

BUG=597785
Review-Url: https://codereview.chromium.org/2710823005
Cr-Commit-Position: refs/heads/master@{#454374}
Committed: https://chromium.googlesource.com/chromium/src/+/00d65d49ff8e35b2658d0798fac66b1da040836a

[Scott Hess - ex-Googler, 2017-02-22 22:31:45]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 22:33:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-02-22 22:39:58]

Histograms

[Scott Hess - ex-Googler, 2017-02-23 17:50:31]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 17:51:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 19:01:22]

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

[commit-bot: I haz the power, 2017-02-23 19:01:23]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-02-27 22:28:32]

pwnall@chromium.org changed reviewers:
+ pwnall@chromium.org

[pwnall, 2017-02-27 22:28:33]

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode119
sql/recovery.cc:119: // succeeded, then querying the database failed.
Based on some of the comments below, do you need a "Should be impossible."
comment here?

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode606
sql/recovery.cc:606: // |db| was poisoned by Begin().  Probe for SQLITE_NOTADB
when attaching to
Is it possible to DCHECK that db was poisoned?

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode626
sql/recovery.cc:626: // Probe to see if that worked.  These reports should not
be possible in a
"that worked" -> "the database got deleted"?

(minor confusion regarding what's happening here)

[Scott Hess - ex-Googler, 2017-02-27 23:36:36]

wordsmithing

[Scott Hess - ex-Googler, 2017-02-27 23:39:39]

OK, tried to tweak the comments up a bit.  ptal.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode119
sql/recovery.cc:119: // succeeded, then querying the database failed.
On 2017/02/27 22:28:33, pwnall wrote:
> Based on some of the comments below, do you need a "Should be impossible."
> comment here?

I don't know.  Substantially all of the FAILED events in this list should be
impossible.  Almost all of them actually happen, because by the time this code
is executing, something is _definitely_ wrong.  One approach would be to spend
the time writing handling code for all possible failures, but that has potential
to add a lot of complexity.  Instead, as I push handling into new areas, I also
push in coverage histograms to get a sense of which errors actually happen in
the fleet.

So I'm not sure if "Should be impossible" wouldn't be akin to comments like
"Adds one to variable |i|", where the comment is true, but not adding anything.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode606
sql/recovery.cc:606: // |db| was poisoned by Begin().  Probe for SQLITE_NOTADB
when attaching to
On 2017/02/27 22:28:33, pwnall wrote:
> Is it possible to DCHECK that db was poisoned?

The comment was only there because it's a non-obvious part of the Begin() API
contract.  Since recovery can be destructive, the goal is to prevent
accidentally running queries against a database in an indeterminate state.

I think I'll just add an explicit call to Poison(), plus a comment about why.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode626
sql/recovery.cc:626: // Probe to see if that worked.  These reports should not
be possible in a
On 2017/02/27 22:28:33, pwnall wrote:
> "that worked" -> "the database got deleted"?
> 
> (minor confusion regarding what's happening here)

Are you familiar with the problem where deleting a file doesn't actually work,
even though it claims to work?  Here's a super-complicated CL:
   https://codereview.chromium.org/2545283002/
The gist of it is that on Windows, file deletion doesn't really work like
everyone believes it works.  I've actually had cases in the past where
base::DeleteFile() returned true (meaning "deleted") but the file actually was
still there.

I think maybe it's better to just adjust the comment to not imply that this is
some new level of impossible versus the rest of the code.  That's just me
pre-judging the results, the histograms will provide real data.

[pwnall, 2017-02-27 23:54:52]

LGTM w/ one nit.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode606
sql/recovery.cc:606: // |db| was poisoned by Begin().  Probe for SQLITE_NOTADB
when attaching to
On 2017/02/27 23:39:39, Scott Hess wrote:
> On 2017/02/27 22:28:33, pwnall wrote:
> > Is it possible to DCHECK that db was poisoned?
> 
> The comment was only there because it's a non-obvious part of the Begin() API
> contract.  Since recovery can be destructive, the goal is to prevent
> accidentally running queries against a database in an indeterminate state.
> 
> I think I'll just add an explicit call to Poison(), plus a comment about why.

I'd have been fine with a DCHECK... it's like a comment that makes sure it stays
valid :D Either way works for me, you don't need to make a change here.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode626
sql/recovery.cc:626: // Probe to see if that worked.  These reports should not
be possible in a
On 2017/02/27 23:39:39, Scott Hess wrote:
> On 2017/02/27 22:28:33, pwnall wrote:
> > "that worked" -> "the database got deleted"?
> > 
> > (minor confusion regarding what's happening here)
> 
> Are you familiar with the problem where deleting a file doesn't actually work,
> even though it claims to work?  Here's a super-complicated CL:
>    https://codereview.chromium.org/2545283002/
> The gist of it is that on Windows, file deletion doesn't really work like
> everyone believes it works.  I've actually had cases in the past where
> base::DeleteFile() returned true (meaning "deleted") but the file actually was
> still there.
> 
> I think maybe it's better to just adjust the comment to not imply that this is
> some new level of impossible versus the rest of the code.  That's just me
> pre-judging the results, the histograms will provide real data.

Sorry for being unclear! I understood the problem (and remember it... also,
AFAIK it bit the installer folks pretty hard in the past too). I didn't like the
vagueness of "that worked". I really just wanted more precise language, like
"Check to see if the file actually got deleted. " You could add something like
"On Windows, Delete() sometimes returns success, but the file stays behind."

https://codereview.chromium.org/2710823005/diff/40001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/40001/sql/recovery.cc#newcode611
sql/recovery.cc:611: // Histograms from Recovery::Begin() show all current
failures are in
This is a really helpful comment! Thank you!

[Scott Hess - ex-Googler, 2017-02-28 01:14:50]

shess@chromium.org changed reviewers:
+ ericwilligers@chromium.org

[Scott Hess - ex-Googler, 2017-02-28 01:14:51]

+ericwilligers@chromium.org for histograms.xml review.

Eric, my goal with these histogram items is to log the coverage for exit points
from this function.  I think the .*_AUTORECOVERDB_.* items should sum to 100%,
though those items do have overlap with other items in the overall histogram. 
Thanks.

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/20001/sql/recovery.cc#newcode626
sql/recovery.cc:626: // Probe to see if that worked.  These reports should not
be possible in a
On 2017/02/27 23:54:51, pwnall wrote:
> On 2017/02/27 23:39:39, Scott Hess wrote:
> > On 2017/02/27 22:28:33, pwnall wrote:
> > > "that worked" -> "the database got deleted"?
> > > 
> > > (minor confusion regarding what's happening here)
> > 
> > Are you familiar with the problem where deleting a file doesn't actually
work,
> > even though it claims to work?  Here's a super-complicated CL:
> >    https://codereview.chromium.org/2545283002/
> > The gist of it is that on Windows, file deletion doesn't really work like
> > everyone believes it works.  I've actually had cases in the past where
> > base::DeleteFile() returned true (meaning "deleted") but the file actually
was
> > still there.
> > 
> > I think maybe it's better to just adjust the comment to not imply that this
is
> > some new level of impossible versus the rest of the code.  That's just me
> > pre-judging the results, the histograms will provide real data.
> 
> Sorry for being unclear! I understood the problem (and remember it... also,
> AFAIK it bit the installer folks pretty hard in the past too). I didn't like
the
> vagueness of "that worked". I really just wanted more precise language, like
> "Check to see if the file actually got deleted. " You could add something like
> "On Windows, Delete() sometimes returns success, but the file stays behind." 

Done.

[Eric Willigers, 2017-02-28 01:43:18]

ericwilligers@chromium.org changed reviewers:
+ jwd@chromium.org

[Eric Willigers, 2017-02-28 01:43:18]

+jwd for histogram OWNERS.
(I only review UseCounter histogram changes.)

[Scott Hess - ex-Googler, 2017-02-28 01:55:43]

On 2017/02/28 01:43:18, Eric Willigers wrote:
> +jwd for histogram OWNERS.
> (I only review UseCounter histogram changes.)

In that case, did you know that the git-cl command suggests you as a reviewer
for histograms.xml?

-scott

[Scott Hess - ex-Googler, 2017-03-01 22:29:09]

ref Windows delete in probe-after-delete comment.

[Scott Hess - ex-Googler, 2017-03-01 22:30:24]

shess@chromium.org changed reviewers:
- ericwilligers@chromium.org

[Scott Hess - ex-Googler, 2017-03-01 22:30:25]

jwd, ping histograms.xml?

The change should be close to a rubber-stamp, it's just adding additional items
to an enum.

[jwd, 2017-03-02 18:45:29]

https://codereview.chromium.org/2710823005/diff/60001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/60001/sql/recovery.cc#newcode25
sql/recovery.cc:25: enum RecoveryEventType {
Can you add a comment saying this enum is used for recording histograms, and
should not be reordered.

[Scott Hess - ex-Googler, 2017-03-02 18:54:31]

Comment on enum required to match histogram enumeration.

[Scott Hess - ex-Googler, 2017-03-02 18:55:52]

jwd,

Done, PTAL?

https://codereview.chromium.org/2710823005/diff/60001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/2710823005/diff/60001/sql/recovery.cc#newcode25
sql/recovery.cc:25: enum RecoveryEventType {
On 2017/03/02 18:45:29, jwd wrote:
> Can you add a comment saying this enum is used for recording histograms, and
> should not be reordered.

Done.

[jwd, 2017-03-02 20:00:32]

Thanks!
LGTM

[Scott Hess - ex-Googler, 2017-03-02 20:09:28]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2017-03-02 20:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org
Link to the patchset: https://codereview.chromium.org/2710823005/#ps80001
(title: "Comment on enum required to match histogram enumeration.")

[commit-bot: I haz the power, 2017-03-02 20:10:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 21:12:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488485368171220,
"parent_rev": "21a19aa03091e1b0b0be9ab87e97d86d15527b94", "commit_rev":
"00d65d49ff8e35b2658d0798fac66b1da040836a"}

[commit-bot: I haz the power, 2017-03-02 21:13:03]

Description was changed from

==========
[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

SQLITE_NOTADB happens when the SQLite header data is broken.  That data
includes meta-information about the file's structure, so any corruption
in the header would require manual intervention to recover - assuming
that the entire file is not broken.  SQLITE_NOTADB files are not even
partially readable through SQLite, so deletion unsticks the file going
forward.

BUG=597785
==========

to

==========
[sql] RecoverDatabase() deletes SQLITE_NOTADB databases.

SQLITE_NOTADB happens when the SQLite header data is broken.  That data
includes meta-information about the file's structure, so any corruption
in the header would require manual intervention to recover - assuming
that the entire file is not broken.  SQLITE_NOTADB files are not even
partially readable through SQLite, so deletion unsticks the file going
forward.

BUG=597785

Review-Url: https://codereview.chromium.org/2710823005
Cr-Commit-Position: refs/heads/master@{#454374}
Committed:
https://chromium.googlesource.com/chromium/src/+/00d65d49ff8e35b2658d0798fac6...
==========

[commit-bot: I haz the power, 2017-03-02 21:13:04]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/00d65d49ff8e35b2658d0798fac6...