Reinitialize the cookie database if the meta table gets corrupted.

chrome/browser/net/sqlite_persistent_cookie_store.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/sqlite_persistent_cookie_store.h"
 
 #include <list>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 90 matching lines @@
 
  private:
   friend class base::RefCountedThreadSafe<SQLitePersistentCookieStore::Backend>;
 
   // You should call Close() before destructing this object.
   ~Backend() {
     DCHECK(!db_.get()) << "Close should have already been called.";
     DCHECK(num_pending_ == 0 && pending_.empty());
   }
 
-  // Database upgrade statements.
-  bool EnsureDatabaseVersion();
-
   class PendingOperation {
    public:
     typedef enum {
       COOKIE_ADD,
       COOKIE_UPDATEACCESS,
       COOKIE_DELETE,
     } OperationType;
 
     PendingOperation(OperationType op,
                      const net::CookieMonster::CanonicalCookie& cc)
@@ skipping 34 matching lines @@
                                     bool load_success);
 
   // Sends all metrics, including posting a ReportMetricsOnDBThread task.
   // Called after all priority and regular loading is complete.
   void ReportMetrics();
 
   // Sends DB-thread owned metrics (i.e., the combined duration of all DB-thread
   // 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
   // domains are loaded).
   void ChainLoadCookies(const LoadedCallback& loaded_callback);
 
   // Load all cookies for a set of domains/hosts
   bool LoadCookiesForDomains(const std::set<std::string>& key);
 
   // 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();
 
   void DeleteSessionCookies();
 
   FilePath path_;
   scoped_ptr<sql::Connection> db_;
   sql::MetaTable meta_table_;
 
   typedef std::list<PendingOperation*> PendingOperationsList;
@@ skipping 77 matching lines @@
   }
 
  private:
   base::TimeDelta* delta_;
   base::TimeDelta original_value_;
   base::Time start_;
 
   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,
                                    sql::MetaTable* meta_table,
                                    const FilePath& path) {
   // Buffer for accumulating debugging info about the database.  Place
   // more relevant information earlier, in case a large datum
   // overflows the fixed-size buffer.
   std::string debug_info;
 
@@ skipping 69 matching lines @@
     const LoadedCallback& loaded_callback, const base::Time& posted_at) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   IncrementTimeDelta increment(&cookie_load_duration_);
 
   UMA_HISTOGRAM_CUSTOM_TIMES(
       "Cookie.TimeLoadDBQueueWait",
       base::Time::Now() - posted_at,
       base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
       50);
 
-  if (!InitializeDatabase()) {
+  if (!PrepareDatabaseForAccess()) {
     BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SQLitePersistentCookieStore::Backend::CompleteLoadOnIOThread,
                  this, loaded_callback, false));
   } else {
     ChainLoadCookies(loaded_callback);
   }
 }
 
 void SQLitePersistentCookieStore::Backend::LoadKeyAndNotifyOnDBThread(
     const std::string& key,
     const LoadedCallback& loaded_callback,
     const base::Time& posted_at) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
   IncrementTimeDelta increment(&cookie_load_duration_);
 
   UMA_HISTOGRAM_CUSTOM_TIMES(
       "Cookie.TimeKeyLoadDBQueueWait",
       base::Time::Now() - posted_at,
       base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
       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()) {
       success = LoadCookiesForDomains(it->second);
       keys_to_load_.erase(it);
     } else {
       success = true;
     }
   }
 
@@ skipping 65 matching lines @@
 
   std::vector<net::CookieMonster::CanonicalCookie*> cookies;
   {
     base::AutoLock locked(lock_);
     cookies.swap(cookies_);
   }
 
   loaded_callback.Run(cookies);
 }
 
-bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
+bool SQLitePersistentCookieStore::Backend::PrepareDatabaseForAccess() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB));
 
   if (initialized_) {
     // Return false if we were previously initialized but the DB has since been
     // closed.
     return db_.get() ? true : false;
   }
 
   base::Time start = base::Time::Now();
 
   const FilePath dir = path_.DirName();
   if (!file_util::PathExists(dir) && !file_util::CreateDirectory(dir)) {
     return false;
   }
 
   int64 db_size = 0;
   if (file_util::GetFileSize(path_, &db_size)) {
     base::ThreadRestrictions::ScopedAllowIO allow_io;
     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",
     base::Time::Now() - start,
     base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
     50);
 
   start = base::Time::Now();
 
   // Retrieve all the domains
@@ skipping 22 matching lines @@
   UMA_HISTOGRAM_CUSTOM_TIMES(
     "Cookie.TimeInitializeDomainMap",
     base::Time::Now() - start,
     base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromMinutes(1),
     50);
 
   initialized_ = true;
   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));
   IncrementTimeDelta increment(&cookie_load_duration_);
 
   bool load_success = true;
 
   if (!db_.get()) {
     // Close() has been called on this store.
     load_success = false;
@@ skipping 77 matching lines @@
     smt.Reset();
   }
   {
     base::AutoLock locked(lock_);
     cookies_.insert(cookies_.end(), cookies.begin(), cookies.end());
   }
   return true;
 }
 
 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;
   }
 
   int cur_version = meta_table_.GetVersionNumber();
   if (cur_version == 2) {
     sql::Transaction transaction(db_.get());
     if (!transaction.Begin())
       return false;
     if (!db_->Execute("ALTER TABLE cookies ADD COLUMN last_access_utc "
                      "INTEGER DEFAULT 0") ||
         !db_->Execute("UPDATE cookies SET last_access_utc = creation_utc")) {
       LOG(WARNING) << "Unable to update cookie database to version 3.";
       return false;
     }
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
     meta_table_.SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
-    transaction.Commit();
+    if (!transaction.Commit())
+      return false;
   }
 
   if (cur_version == 3) {
     // The time epoch changed for Mac & Linux in this version to match Windows.
     // This patch came after the main epoch change happened, so some
     // developers have "good" times for cookies added by the more recent
     // versions. So we have to be careful to only update times that are under
     // the old system (which will appear to be from before 1970 in the new
     // system). The magic number used below is 1970 in our time units.
     sql::Transaction transaction(db_.get());
@@ skipping 13 matching lines @@
           "expires_utc > 0 AND expires_utc < 11644473600000000)"));
     ignore_result(db_->Execute(
         "UPDATE cookies "
         "SET last_access_utc = last_access_utc + 11644473600000000 "
         "WHERE rowid IN "
         "(SELECT rowid FROM cookies WHERE "
           "last_access_utc > 0 AND last_access_utc < 11644473600000000)"));
 #endif
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
-    transaction.Commit();
+    if (!transaction.Commit())
+      return false;
   }
 
   if (cur_version == 4) {
     const base::TimeTicks start_time = base::TimeTicks::Now();
     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;
 }
 
 void SQLitePersistentCookieStore::Backend::AddCookie(
     const net::CookieMonster::CanonicalCookie& cc) {
   BatchOperation(PendingOperation::COOKIE_ADD, cc);
 }
 
 void SQLitePersistentCookieStore::Backend::UpdateCookieAccessTime(
     const net::CookieMonster::CanonicalCookie& cc) {
@@ skipping 220 matching lines @@
   if (backend_.get())
     backend_->SetClearLocalStateOnExit(clear_local_state);
 }
 
 void SQLitePersistentCookieStore::Flush(const base::Closure& callback) {
   if (backend_.get())
     backend_->Flush(callback);
   else if (!callback.is_null())
     MessageLoop::current()->PostTask(FROM_HERE, callback);
 }