[sql] Recover Favicons v5 databases, with more recovery automation.

[sql] Recover Favicons v5 databases, with more recovery automation.

An entirely automated recovery system runs afoul of questions about
whether the corrupt database's schema can be trusted.
sql::Recovery::AutoRecoverTable() uses a schema created by the caller
to construct the recovery virtual table and then copies the data over.

sql::Recovery::SetupMeta() and GetMetaVersionNumber() simplify
accessing meta-table info in the corrupt database.

sql::test::IntegrityCheck() and CorruptSizeInHeader() helpers to
simplify common testing operations.

Rewrite ThumbnailDatabase v6 and v7 recovery code and tests using
these changes, and add a v5 recovery path.  Additionally handle
deprecated versions.

BUG=240396, 109482
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235492

[Scott Hess - ex-Googler, 2013-10-31 22:15:58]

Getting to the big time!

So far, things have been going well.  The histogrammed failure cases mostly show
that failures happen when first touching something - for instance, first attempt
to write to the database being recovered, or first attempt to read from the
corrupt database.  This is almost sane.  IMHO this CL is starting down the track
of codifying things in preparation for extending recovery support to other
databases, which should be much cleaner than this debugging-in-the-wild stuff. 
So hopefully soonish I can stop bothering you and start bothering other people
for reviews ...

Thanks for following along!

[pkotwicz, 2013-11-01 15:05:51]

I will take a look at the CL on Saturday

[Scott Hess - ex-Googler, 2013-11-01 15:10:13]

On 2013/11/01 15:05:51, pkotwicz wrote:
> I will take a look at the CL on Saturday

No rush.  I'm excited, but also would like to not look at it again for a few
days :-).

[pkotwicz, 2013-11-04 02:38:30]

LGTM with a few comments

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
File chrome/browser/history/thumbnail_database.cc (right):

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
chrome/browser/history/thumbnail_database.cc:289: "CREATE TABLE favicons("
Nit: It would be nice if we were consistent in using either "CREATE TABLE" or
"CREATE TABLE IF NOT EXISTS"

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
chrome/browser/history/thumbnail_database.cc:1257: // Version check. We should
not encounter a database too old for us to handle
Unrelated: Shouldn't this comment be "too new for us to handle".

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc#newcode349
sql/recovery.cc:349: base::StringPrintf(", ignore%" PRIuS " ANY", i));
Is the comma at the beginning of the statement desired?

[Scott Hess - ex-Googler, 2013-11-04 21:41:39]

Thanks for the expedient review!

I'm going to hold off check-in until after the branch has happened.

Also, I checked in a change last week to histogram GetLastChangeCount() for
these tables.  So there will be a minor change along those lines to make it
merge right.

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
File chrome/browser/history/thumbnail_database.cc (right):

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
chrome/browser/history/thumbnail_database.cc:289: "CREATE TABLE favicons("
On 2013/11/04 02:38:31, pkotwicz wrote:
> Nit: It would be nice if we were consistent in using either "CREATE TABLE" or
> "CREATE TABLE IF NOT EXISTS"

As a general rule, I would prefer if we never ever did "CREATE TABLE IF NOT
EXISTS", because it implies that we don't know the current state of the
database, and thus cannot reason about the final state, either.  If the table
already exists, that's a fatal error, we should blow everything away! 
Unfortunately, given the way our code has evolved over time, I'm not totally
clear where that change could be made safely.  For this database, probably could
happen now, though it would require tweaks to the setup ordering (either upgrade
or create, never create-then-upgrade).

In any case, the comment above is why this function is inconsistent.  I'll
change this one to use IF NOT EXISTS to be consistent.

https://codereview.chromium.org/50493012/diff/70001/chrome/browser/history/th...
chrome/browser/history/thumbnail_database.cc:1257: // Version check. We should
not encounter a database too old for us to handle
On 2013/11/04 02:38:31, pkotwicz wrote:
> Unrelated: Shouldn't this comment be "too new for us to handle".

I think this means that the too-new case fails (with sql::INIT_TOO_NEW), while
the not-too-new case continues, which is safe because we should never see a
database too old to handle.

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc#newcode349
sql/recovery.cc:349: base::StringPrintf(", ignore%" PRIuS " ANY", i));
On 2013/11/04 02:38:31, pkotwicz wrote:
> Is the comma at the beginning of the statement desired?

Hmm, leftover from a previous version which appended everything rather than
building a vector.  Surprised the unit test didn't fubar... well, it accepts the
empty segment, must be a bug in the virtual table implementation.  Thanks for
catching this!

[Scott Hess - ex-Googler, 2013-11-12 20:25:24]

[Sorry for the long delay, I was ill last week.]

sky@ OWNERS for history/
jar@ OWNERS for histograms.xml

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc
File sql/recovery.cc (right):

https://codereview.chromium.org/50493012/diff/70001/sql/recovery.cc#newcode349
sql/recovery.cc:349: base::StringPrintf(", ignore%" PRIuS " ANY", i));
On 2013/11/04 21:41:44, shess wrote:
> On 2013/11/04 02:38:31, pkotwicz wrote:
> > Is the comma at the beginning of the statement desired?
> 
> Hmm, leftover from a previous version which appended everything rather than
> building a vector.  Surprised the unit test didn't fubar... well, it accepts
the
> empty segment, must be a bug in the virtual table implementation.  Thanks for
> catching this!

Weird - virtual table doesn't even see the empty column decl.  I guess maybe
it's a sqlite bug.

[jar (doing other things), 2013-11-12 23:32:28]

histograms.xml LGTM

[Scott Hess - ex-Googler, 2013-11-14 00:49:05]

On 2013/11/12 20:25:24, shess wrote:
> sky@ OWNERS for history/

Ping for sky@.  Thanks.

[sky, 2013-11-14 22:47:00]

LGTM

[commit-bot: I haz the power, 2013-11-14 23:39:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-15 01:16:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-15 03:48:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-15 07:14:10]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 07:25:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-15 11:47:29]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 18:22:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-15 21:50:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-16 00:17:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/50493012/220001

[commit-bot: I haz the power, 2013-11-16 01:52:07]

Change committed as 235492