Reinitialize the cookie database if the meta table gets corrupted.

chrome/browser/net/sqlite_persistent_cookie_store.cc

Index: chrome/browser/net/sqlite_persistent_cookie_store.cc
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index e3007c6122b03ffeff17e67a729fe88620e2568e..f791b91f9dcea0804b28dbfc8c8e983ee4b2ddb0 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -108,9 +108,6 @@ class SQLitePersistentCookieStore::Backend
     DCHECK(num_pending_ == 0 && pending_.empty());
   }
 
-  // Database upgrade statements.
-  bool EnsureDatabaseVersion();
-
   class PendingOperation {
    public:
     typedef enum {
@@ -165,8 +162,18 @@ class SQLitePersistentCookieStore::Backend
   // tasks).
   void ReportMetricsOnDBThread();
 
-  // Initialize the data base.
-  bool InitializeDatabase();
+  // Open the database and read enough information to allow us to respond to
+  // clients.
+  bool PrepareDatabaseForAccess();
+
+  // Opens the database file and creates or migrates the schema if necessary.
+  bool OpenDatabase();
+
+  // Initializes the cookies table, returning true on success.
+  bool InitializeSchema();
+
+  // Check the database version and perform migration steps if necessary.
+  bool EnsureDatabaseVersion();
 
   // Loads cookies for the next domain key from the DB, then either reschedules
   // itself or schedules the provided callback to run on the IO thread (if all
@@ -179,8 +186,10 @@ class SQLitePersistentCookieStore::Backend
   // Batch a cookie operation (add or delete)
   void BatchOperation(PendingOperation::OperationType op,
                       const net::CookieMonster::CanonicalCookie& cc);
+
   // Commit our pending operations to the database.
   void Commit();
+
   // Close() executed on the background thread.
   void InternalBackgroundClose();
 
@@ -278,36 +287,6 @@ class IncrementTimeDelta {
   DISALLOW_COPY_AND_ASSIGN(IncrementTimeDelta);
 };
 
-// Initializes the cookies table, returning true on success.
-bool InitTable(sql::Connection* db) {
-  if (!db->DoesTableExist("cookies")) {
-    if (!db->Execute("CREATE TABLE cookies ("
-                     "creation_utc INTEGER NOT NULL UNIQUE PRIMARY KEY,"
-                     "host_key TEXT NOT NULL,"
-                     "name TEXT NOT NULL,"
-                     "value TEXT NOT NULL,"
-                     "path TEXT NOT NULL,"
-                     "expires_utc INTEGER NOT NULL,"
-                     "secure INTEGER NOT NULL,"
-                     "httponly INTEGER NOT NULL,"
-                     "last_access_utc INTEGER NOT NULL, "
-                     "has_expires INTEGER NOT NULL DEFAULT 1, "
-                     "persistent INTEGER NOT NULL DEFAULT 1)"))
-      return false;
-  }
-
-  // Try to create the index every time. Older versions did not have this index,
-  // so we want those people to get it.
-  if (!db->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
-                   " (creation_utc)"))
-    return false;
-
-  if (!db->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies(host_key)"))
-    return false;
-
-  return true;
-}
-
 // TODO(shess): Send up a crash report containing information to help
 // debug http://crbug.com/111376 .
 void DumpSchemaInfoWithoutCrashing(sql::Connection* db,
@@ -397,7 +376,7 @@ void SQLitePersistentCookieStore::Backend::LoadAndNotifyOnDBThread(
       base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
       50);
 
-  if (!InitializeDatabase()) {
+  if (!PrepareDatabaseForAccess()) {
     BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SQLitePersistentCookieStore::Backend::CompleteLoadOnIOThread,
@@ -421,7 +400,7 @@ void SQLitePersistentCookieStore::Backend::LoadKeyAndNotifyOnDBThread(
       50);
 
   bool success = false;
-  if (InitializeDatabase()) {
+  if (PrepareDatabaseForAccess()) {
     std::map<std::string, std::set<std::string> >::iterator
       it = keys_to_load_.find(key);
     if (it != keys_to_load_.end()) {
@@ -507,7 +486,7 @@ void SQLitePersistentCookieStore::Backend::Notify(
   loaded_callback.Run(cookies);
 }
 
-bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
+bool SQLitePersistentCookieStore::Backend::PrepareDatabaseForAccess() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
   if (initialized_) {
@@ -529,20 +508,8 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
     UMA_HISTOGRAM_COUNTS("Cookie.DBSizeInKB", db_size / 1024 );
   }
 
-  db_.reset(new sql::Connection);
-  if (!db_->Open(path_)) {
-    NOTREACHED() << "Unable to open cookie DB.";
-    db_.reset();
+  if (!OpenDatabase())
     return false;
-  }
-
-  db_->set_error_delegate(GetErrorHandlerForCookieDb());
-
-  if (!EnsureDatabaseVersion() || !InitTable(db_.get())) {
-    NOTREACHED() << "Unable to open cookie DB.";
-    db_.reset();
-    return false;
-  }
 
   UMA_HISTOGRAM_CUSTOM_TIMES(
     "Cookie.TimeInitializeDB",
@@ -585,6 +552,73 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
   return true;
 }
 
+bool SQLitePersistentCookieStore::Backend::OpenDatabase() {
+  DCHECK(!db_.get());
+
+  db_.reset(new sql::Connection);
+  if (!db_->Open(path_)) {
+    NOTREACHED() << "Unable to open cookie DB.";
+    db_.reset();
+    return false;
+  }
+
+  db_->set_error_delegate(GetErrorHandlerForCookieDb());
+
+  if (!sql::MetaTable::DoesTableExist(db_.get()))
+    return InitializeSchema();
+
+  // We don't pass the current version or current compatible version because
+  // we are expecting the schema to already be populated - hence these values
+  // will be ignored. If we subsequently query for the version and get back 0
+  // we know something is wrong.

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

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();
}

+  if (!meta_table_.Init(db_.get(), 0, 0) ||
+      meta_table_.GetVersionNumber() == 0 ||
+      meta_table_.GetCompatibleVersionNumber() == 0) {
+    UMA_HISTOGRAM_COUNTS_100("Cookie.CorruptMetaTable", 1);
+
+    meta_table_.Reset();
+    db_.reset(new sql::Connection);
+    if (!file_util::Delete(path_, false) || !db_->Open(path_)) {
+      UMA_HISTOGRAM_COUNTS_100("Cookie.CorruptMetaTableRecoveryFailed", 1);
+      NOTREACHED() << "Unable to reset the cookie DB.";
+      meta_table_.Reset();
+      db_.reset();
+      return false;
+    }
+    db_->set_error_delegate(GetErrorHandlerForCookieDb());
+
+    return InitializeSchema();
+  }
+
+  return EnsureDatabaseVersion();
+}
+
+bool SQLitePersistentCookieStore::Backend::InitializeSchema() {
+  DCHECK(!db_.get()->DoesTableExist("cookies"));
+  sql::Transaction transaction(db_.get());
+
+  return transaction.Begin() &&
+      meta_table_.Init(db_.get(),
+                       kCurrentVersionNumber,
+                       kCompatibleVersionNumber) &&
+      db_->Execute("CREATE TABLE cookies ("
+                   "creation_utc INTEGER NOT NULL UNIQUE PRIMARY KEY,"
+                   "host_key TEXT NOT NULL,"
+                   "name TEXT NOT NULL,"
+                   "value TEXT NOT NULL,"
+                   "path TEXT NOT NULL,"
+                   "expires_utc INTEGER NOT NULL,"
+                   "secure INTEGER NOT NULL,"
+                   "httponly INTEGER NOT NULL,"
+                   "last_access_utc INTEGER NOT NULL, "
+                   "has_expires INTEGER NOT NULL DEFAULT 1, "
+                   "persistent INTEGER NOT NULL DEFAULT 1)") &&
+      db_->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
+                   " (creation_utc)") &&
+      db_->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies(host_key)") &&
+      transaction.Commit();

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

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.

+}
+
 void SQLitePersistentCookieStore::Backend::ChainLoadCookies(
     const LoadedCallback& loaded_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
@@ -682,12 +716,6 @@ bool SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
 }
 
 bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
-  // Version check.
-  if (!meta_table_.Init(
-      db_.get(), kCurrentVersionNumber, kCompatibleVersionNumber)) {
-    return false;
-  }
-
   if (meta_table_.GetCompatibleVersionNumber() > kCurrentVersionNumber) {
     LOG(WARNING) << "Cookie database is too new.";
     return false;
@@ -708,7 +736,8 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
     meta_table_.SetVersionNumber(cur_version);
     meta_table_.SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
-    transaction.Commit();
+    if (!transaction.Commit())
+      return false;
   }
 
   if (cur_version == 3) {
@@ -742,7 +771,8 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
 #endif
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
-    transaction.Commit();
+    if (!transaction.Commit())
+      return false;
   }
 
   if (cur_version == 4) {
@@ -750,29 +780,34 @@ bool SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
     sql::Transaction transaction(db_.get());
     if (!transaction.Begin())
       return false;
+
+    // The IF NOT EXISTS are because some databases were created without
+    // these indexes. As of version 5 all tables are create with them, so let's
+    // make sure that those old DBs get them as they go through this migration.
     if (!db_->Execute("ALTER TABLE cookies "
                       "ADD COLUMN has_expires INTEGER DEFAULT 1") ||
         !db_->Execute("ALTER TABLE cookies "
-                      "ADD COLUMN persistent INTEGER DEFAULT 1")) {
+                      "ADD COLUMN persistent INTEGER DEFAULT 1") ||
+        !db_->Execute("CREATE INDEX IF NOT EXISTS cookie_times ON cookies"
+                      " (creation_utc)") ||

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

If/when you get the conflict with my change to remove this index, make sure you
remove both cases.

+        !db_->Execute("CREATE INDEX IF NOT EXISTS domain ON cookies"
+                      "(host_key)")) {
       LOG(WARNING) << "Unable to update cookie database to version 5.";
       return false;
     }
+
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
     meta_table_.SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
-    transaction.Commit();
+    if (!transaction.Commit())
+      return false;
     UMA_HISTOGRAM_TIMES("Cookie.TimeDatabaseMigrationToV5",
                         base::TimeTicks::Now() - start_time);
   }
 
   // Put future migration cases here.
 
-  // When the version is too old, we just try to continue anyway, there should
-  // not be a released product that makes a database too old for us to handle.
-  LOG_IF(WARNING, cur_version < kCurrentVersionNumber) <<
-      "Cookie database version " << cur_version << " is too old to handle.";
-

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

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?

   return true;
 }