Reinitialize the cookie database if the meta table gets corrupted.

Reinitialize the cookie database if the meta table gets corrupted.

This has been shown to occur in the wild. At the moment users in this situation will simply get a failure to load or persist cookies (though cookies will work within the current session). After this change, users will see their Cookie database wiped out and reinitialized, but loading and persisting should work going forward.

A histogram tracks how often this scenario is detected - it should probably spike as this is rolled out to new channels and then taper off (hopefully approaching 0).

BUG=111376
TEST=unit_tests --gtest_filter=SQLitePersistentCookieStoreTest.TestInvalidMetaTableRecovery

[erikwright (departed), 2012-03-01 21:06:46]

PTAL. Depends on http://codereview.chromium.org/9569028/

[Scott Hess - ex-Googler, 2012-03-01 22:15:15]

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:774: if
(!file_util::Delete(path_, false) ||
I am a small bit concerned about whether we'll see cases, especially on Windows,
where this fails.  It mostly shouldn't, but over time I've come to distrust
Delete() on Windows (because I have code which deletes a file and later the file
is still there).  I don't think you should attempt to resolve this here, because
it may be correlated with other oddness.  But I do think you should throw a
histogram in the overall version-check if() to track that we're making progress.

[I've considered adding a SQLite API to zero out an open database.  That would
address the problem of deleting with an open file handle (which I suspect is a
cause in many cases, think virus checkers or something), while also being better
than using DROP TABLE type statements to accomplish it.  But I haven't written
it, yet.]

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:777: db_.get(),
kCurrentVersionNumber, kCompatibleVersionNumber)) {
I wish this wasn't replicating other code.  It also looks like you might need to
call InitTable() somewhere after meta_table_.Init().  The caller will call it in
case of FAILURE of this function.

Put another way, I hates it.  The EnsureDatabaseVersion() and InitTable() code
is confusing, at best.  Maybe the correct sequence should be:

If there is no meta table, assume a fresh db and create the meta table and the
cookie tables.

If there is a meta table, and there is no version info, nuke the database and go
to the above case.  Exactly once.

If there is a meta table, and there is version info, upgrade as necessary.

[erikwright (departed), 2012-03-02 01:04:27]

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:774: if
(!file_util::Delete(path_, false) ||
On 2012/03/01 22:15:15, shess wrote:
> I am a small bit concerned about whether we'll see cases, especially on
Windows,
> where this fails.  It mostly shouldn't, but over time I've come to distrust
> Delete() on Windows (because I have code which deletes a file and later the
file
> is still there).  I don't think you should attempt to resolve this here,
because
> it may be correlated with other oddness.  But I do think you should throw a
> histogram in the overall version-check if() to track that we're making
progress.
> 
> [I've considered adding a SQLite API to zero out an open database.  That would
> address the problem of deleting with an open file handle (which I suspect is a
> cause in many cases, think virus checkers or something), while also being
better
> than using DROP TABLE type statements to accomplish it.  But I haven't written
> it, yet.]

Done.

http://codereview.chromium.org/9567022/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:777: db_.get(),
kCurrentVersionNumber, kCompatibleVersionNumber)) {
On 2012/03/01 22:15:15, shess wrote:
> I wish this wasn't replicating other code.  It also looks like you might need
to
> call InitTable() somewhere after meta_table_.Init().  The caller will call it
in
> case of FAILURE of this function.

The caller actually calls InitTable only if this function succeeds. It's pretty
bad, and confusing, I agree, though the current form works (as demonstrated by
the unit test).

> 
> Put another way, I hates it.  The EnsureDatabaseVersion() and InitTable() code
> is confusing, at best.  Maybe the correct sequence should be:
> 
> If there is no meta table, assume a fresh db and create the meta table and the
> cookie tables.
> 
> If there is a meta table, and there is no version info, nuke the database and
go
> to the above case.  Exactly once.
> 
> If there is a meta table, and there is version info, upgrade as necessary.

I would be happy to refactor this (actually, the entire cookie subsystem) into
something more sensible. I suppose, however, that if there is any possibility of
merging to 17 or 18 I should start with something that is minimal and correct.

I will send a status report to the relevant release managers and get their
feedback.

[erikwright (departed), 2012-03-03 04:29:16]

Hi Scott,

The latest patch is a version that is moderately refactored and more clearly
delineates the steps in loading/checking/initializing/recovering/migrating the
database schema.

It was not possible to do any kind of refactoring of this sort without slight
changes to the actual logic, as the behaviour was defined by its odd shape, if
that makes any sense.

I would be uncomfortable committing this to the M18 branch, so my suggestion is
to commit the original minimal patch, get some canaries/dev out of it, merge to
M18, and then check in the better version to trunk.

If I don't get feedback from you before Sunday evening, I will check in the
minor patch with TBR in order to be sure to get a Canary on Monday.

Thanks,

Erik

[Scott Hess - ex-Googler, 2012-03-04 21:33:14]

I'll try to review on Monday, but I'm fine with going TBR to get some testing
in.  I've had too many interrupts to give it the consideration it needs :-).

One thing Anthony asked about was how bad it would be to assume the best? 
Broadly put, we know all the tables and indices which could ever have been
created, so we could potentially do something like deleting all the indices and
the meta table, move the existing table aside, create everything, then populate
the created table from the saved table, then delete the saved table.  I pointed
out that we don't believe that many people are impacted, so nuking might be
safer, but I figured I'd throw it over the fence, just in case.

[Scott Hess - ex-Googler, 2012-03-04 21:35:14]

BTW, on Friday I wrote a skeleton of sqlite3_nuke(), which would effectively
zero out the sqlite_master and truncate the database, similar to the effect
you'd get doing VACUUM on an empty database.  I need to research a little to
make sure it's doing the right stuff, but the nice thing about this is that
there's little chance of a mismatch between SQLite-level operations and
filesystem operations - it would be like any other transaction.

[Scott Hess - ex-Googler, 2012-03-08 00:48:44]

Sorry if this review is out-dated - I kept thinking "Wait, isn't this already
in?", but then thinking "No, I don't think all of it is."

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (left):

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:775: 
If the code gets to this point without cur_version getting to
kCurrentVersionNumber, that's a problem.  Just continuing anyway was clearly
wrong, unless statements are written in a way which degrades gracefully, rather
than trying to reference columns or tables which don't exist.  Can you think of
any reason not to CHECK() that the version equals the expected version?  I was
going to suggest histogramming it, but if a histogram is reported to the server
and nobody looks at it, does it exist?

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:573: // we know something
is wrong.
I don't like making these assumptions on how MetaTable works.  I'm not going to
argue for whether the existing API is the right API, but abusing it won't make
things better.  You should be calling Init() with the current version numbers,
and relying on it not populating the table if it already exists.

Since we do know that we have created databases which do contain invalidness, I
would support adding a static function to test for that case.  Something like:
  static bool MetaTable::IsTableInvalid(*db);
which returns true if the table exists but doesn't contain both versions. 
Obviously it should be documented with a short description of the problem and a
suggestion not to ever call it.  Then you can just call that before you even get
started on setting up the schema.

Another possible solution would be to add something like:
  static bool MetaTable::ReInit(*db, int* v, int* c);
It would return false if the table didn't exist or if it didn't contain both
versions.  Then you could do something like:

if (MetaTable::ReInit(db, &v, &c)) {
  return EnsureDatabaseVersion(v, c);
} else {
  if (MetaTable::DoesTableExist(db)) {
    // Nuke here.
  }
  return InitializeSchema();
}

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:619: transaction.Commit();
I don't really care for this style, versus a bunch of if() statements, since it
only allows a narrow class of changes to be applied easily.  But I will defer to
your ownership of this code.

http://codereview.chromium.org/9567022/diff/7002/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:792: " (creation_utc)") ||
If/when you get the conflict with my change to remove this index, make sure you
remove both cases.