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

chrome/browser/history/thumbnail_database.cc

Index: chrome/browser/history/thumbnail_database.cc
diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc
index fcce91f348f4bc0c42a814d7f83b477497c819cf..9a5a4f440bff8229ea55e38a5968f613d18d9996 100644
--- a/chrome/browser/history/thumbnail_database.cc
+++ b/chrome/browser/history/thumbnail_database.cc
@@ -275,6 +275,60 @@ void GenerateDiagnostics(sql::Connection* db,
   }
 }
 
+// Create v5 schema for recovery code.
+bool InitSchemaV5(sql::Connection* db) {
+  // This schema was derived from the strings used when v5 was in
+  // force.  The [favicons] index and the [icon_mapping] items were
+  // copied from the current strings, after verifying that the
+  // resulting schema exactly matches the schema created by the
+  // original versions of those strings.  This allows the linker to
+  // share the strings if they match, while preferring correctness of
+  // the current versions change.
+
+  const char kFaviconsV5[] =
+      "CREATE TABLE favicons("

[pkotwicz, 2013/11/04 02:38:31]

Nit: It would be nice if we were consistent in using either "CREATE TABLE" or
"CREATE TABLE IF NOT EXISTS"

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

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.

+      "id INTEGER PRIMARY KEY,"
+      "url LONGVARCHAR NOT NULL,"
+      "last_updated INTEGER DEFAULT 0,"
+      "image_data BLOB,"
+      "icon_type INTEGER DEFAULT 1,"
+      "sizes LONGVARCHAR"
+      ")";
+  const char kFaviconsIndexV5[] =
+      "CREATE INDEX IF NOT EXISTS favicons_url ON favicons(url)";
+  if (!db->Execute(kFaviconsV5) || !db->Execute(kFaviconsIndexV5))
+    return false;
+
+  const char kIconMappingV5[] =
+      "CREATE TABLE IF NOT EXISTS icon_mapping"
+      "("
+      "id INTEGER PRIMARY KEY,"
+      "page_url LONGVARCHAR NOT NULL,"
+      "icon_id INTEGER"
+      ")";
+  const char kIconMappingUrlIndexV5[] =
+      "CREATE INDEX IF NOT EXISTS icon_mapping_page_url_idx"
+      " ON icon_mapping(page_url)";
+  const char kIconMappingIdIndexV5[] =
+      "CREATE INDEX IF NOT EXISTS icon_mapping_icon_id_idx"
+      " ON icon_mapping(icon_id)";
+  if (!db->Execute(kIconMappingV5) ||
+      !db->Execute(kIconMappingUrlIndexV5) ||
+      !db->Execute(kIconMappingIdIndexV5)) {
+    return false;
+  }
+
+  return true;
+}
+
+// TODO(shess): Consider InitSchemaV7().  InitSchemaV5() is worthwhile
+// because there appear to be 10s of thousands of marooned v5
+// databases in the wild.  Once recovery reaches stable, the number of
+// corrupt-but-recoverable databases should drop, possibly to the
+// point where it is not worthwhile to maintain previous-version
+// recovery code.
+// TODO(shess): Alternately, think on a way to more cleanly represent
+// versioned schema going forward.
 bool InitTables(sql::Connection* db) {
   const char kIconMappingSql[] =
       "CREATE TABLE IF NOT EXISTS icon_mapping"
@@ -342,22 +396,31 @@ bool InitIndices(sql::Connection* db) {
 enum RecoveryEventType {
   RECOVERY_EVENT_RECOVERED = 0,
   RECOVERY_EVENT_FAILED_SCOPER,
-  RECOVERY_EVENT_FAILED_META_VERSION_ERROR,
-  RECOVERY_EVENT_FAILED_META_VERSION_NONE,
+  RECOVERY_EVENT_FAILED_META_VERSION_ERROR,  // obsolete
+  RECOVERY_EVENT_FAILED_META_VERSION_NONE,  // obsolete
   RECOVERY_EVENT_FAILED_META_WRONG_VERSION6,  // obsolete
-  RECOVERY_EVENT_FAILED_META_WRONG_VERSION5,
+  RECOVERY_EVENT_FAILED_META_WRONG_VERSION5,  // obsolete
   RECOVERY_EVENT_FAILED_META_WRONG_VERSION,
-  RECOVERY_EVENT_FAILED_RECOVER_META,
+  RECOVERY_EVENT_FAILED_RECOVER_META,  // obsolete
   RECOVERY_EVENT_FAILED_META_INSERT,  // obsolete
   RECOVERY_EVENT_FAILED_INIT,
-  RECOVERY_EVENT_FAILED_RECOVER_FAVICONS,
-  RECOVERY_EVENT_FAILED_FAVICONS_INSERT,
-  RECOVERY_EVENT_FAILED_RECOVER_FAVICON_BITMAPS,
-  RECOVERY_EVENT_FAILED_FAVICON_BITMAPS_INSERT,
-  RECOVERY_EVENT_FAILED_RECOVER_ICON_MAPPING,
-  RECOVERY_EVENT_FAILED_ICON_MAPPING_INSERT,
+  RECOVERY_EVENT_FAILED_RECOVER_FAVICONS,  // obsolete
+  RECOVERY_EVENT_FAILED_FAVICONS_INSERT,  // obsolete
+  RECOVERY_EVENT_FAILED_RECOVER_FAVICON_BITMAPS,  // obsolete
+  RECOVERY_EVENT_FAILED_FAVICON_BITMAPS_INSERT,  // obsolete
+  RECOVERY_EVENT_FAILED_RECOVER_ICON_MAPPING,  // obsolete
+  RECOVERY_EVENT_FAILED_ICON_MAPPING_INSERT,  // obsolete
   RECOVERY_EVENT_RECOVERED_VERSION6,
   RECOVERY_EVENT_FAILED_META_INIT,
+  RECOVERY_EVENT_FAILED_META_VERSION,
+  RECOVERY_EVENT_DEPRECATED,
+  RECOVERY_EVENT_FAILED_V5_INITSCHEMA,
+  RECOVERY_EVENT_FAILED_V5_AUTORECOVER_FAVICONS,
+  RECOVERY_EVENT_FAILED_V5_AUTORECOVER_ICON_MAPPING,
+  RECOVERY_EVENT_RECOVERED_VERSION5,
+  RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICONS,
+  RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICON_BITMAPS,
+  RECOVERY_EVENT_FAILED_AUTORECOVER_ICON_MAPPING,
 
   // Always keep this at the end.
   RECOVERY_EVENT_MAX,
@@ -398,75 +461,78 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
     return;
   }
 
-  // Setup the meta recovery table, and check that the version number
-  // is covered by the recovery code.
-  // TODO(shess): sql::Recovery should provide a helper to handle meta.
-  int version = 0;  // For reporting which version was recovered.
-  {
-    const char kRecoverySql[] =
-        "CREATE VIRTUAL TABLE temp.recover_meta USING recover"
-        "("
-        "corrupt.meta,"
-        "key TEXT NOT NULL,"
-        "value TEXT"  // Really?  Never int?
-        ")";
-    if (!recovery->db()->Execute(kRecoverySql)) {
-      // TODO(shess): Failure to create the recover_meta table could
-      // mean that the main database is too corrupt to access, or that
-      // the meta table doesn't exist.
+  // Setup the meta recovery table and fetch the version number from
+  // the corrupt database.
+  int version = 0;
+  if (!recovery->SetupMeta() || !recovery->GetMetaVersionNumber(&version)) {
+    // TODO(shess): Prior histograms indicate all failures are in
+    // creating the recover virtual table for corrupt.meta.  The table
+    // may not exist, or the database may be too far gone.  Either
+    // way, unclear how to resolve.
+    sql::Recovery::Rollback(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_VERSION);
+    return;
+  }
+
+  // Recover v5 database to v5 schema.  Next pass through Init() will
+  // migrate to v7.
+  if (version == 5) {
+    sql::MetaTable recover_meta_table;
+    if (!recover_meta_table.Init(recovery->db(), version, version)) {
       sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_RECOVER_META);
+      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_INIT);
       return;
     }
 
-    {
-      const char kRecoveryVersionSql[] =
-          "SELECT value FROM recover_meta WHERE key = 'version'";
-      sql::Statement recovery_version(
-          recovery->db()->GetUniqueStatement(kRecoveryVersionSql));
-      if (!recovery_version.Step()) {
-        if (!recovery_version.Succeeded()) {
-          RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_VERSION_ERROR);
-          // TODO(shess): An error while processing the statement is
-          // probably not recoverable.
-        } else {
-          RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_VERSION_NONE);
-          // TODO(shess): If a positive version lock cannot be achieved,
-          // the database could still be recovered by optimistically
-          // attempting to copy things.  In the limit, the schema found
-          // could be inspected.  Less clear is whether optimistic
-          // recovery really makes sense.
-        }
-        recovery_version.Clear();
-        sql::Recovery::Rollback(recovery.Pass());
-        return;
-      }
-      version = recovery_version.ColumnInt(0);
-
-      // Recovery code is generally schema-dependent.  Version 7 and
-      // version 6 are very similar, so can be handled together.
-      // Track version 5, to see whether it's worth writing recovery
-      // code for.
-      if (version != 7 && version != 6) {
-        if (version == 5) {
-          RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION5);
-        } else {
-          RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
-        }
-        recovery_version.Clear();
-        sql::Recovery::Rollback(recovery.Pass());
-        return;
-      }
+    // TODO(shess): These tests are separate for histogram purposes,
+    // but once things look stable it can be tightened up.
+    if (!InitSchemaV5(recovery->db())) {
+      sql::Recovery::Rollback(recovery.Pass());
+      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_V5_INITSCHEMA);
+      return;
     }
 
-    // Either version 6 or version 7 recovers to current.
-    sql::MetaTable recover_meta_table;
-    if (!recover_meta_table.Init(recovery->db(), kCurrentVersionNumber,
-                                 kCompatibleVersionNumber)) {
+    if (!recovery->AutoRecoverTable("favicons", 0)) {
       sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_INIT);
+      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_V5_AUTORECOVER_FAVICONS);
       return;
     }
+
+    if (!recovery->AutoRecoverTable("icon_mapping", 0)) {
+      sql::Recovery::Rollback(recovery.Pass());
+      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_V5_AUTORECOVER_ICON_MAPPING);
+      return;
+    }
+
+    ignore_result(sql::Recovery::Recovered(recovery.Pass()));
+    RecordRecoveryEvent(RECOVERY_EVENT_RECOVERED_VERSION5);
+    return;
+  }
+
+  // This code may be able to fetch versions that the regular
+  // deprecation path cannot.
+  if (version <= kDeprecatedVersionNumber) {
+    sql::Recovery::Unrecoverable(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_DEPRECATED);
+    return;
+  }
+
+  // TODO(shess): Earlier versions have been handled or deprecated,
+  // later versions should be impossible.  Unrecoverable() seems
+  // reasonable.
+  if (version != 6 && version != 7) {
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
+    sql::Recovery::Rollback(recovery.Pass());
+    return;
+  }
+
+  // Both v6 and v7 recover to current schema version.
+  sql::MetaTable recover_meta_table;
+  if (!recover_meta_table.Init(recovery->db(), kCurrentVersionNumber,
+                               kCompatibleVersionNumber)) {
+    sql::Recovery::Rollback(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_INIT);
+    return;
   }
 
   // Create a fresh version of the database.  The recovery code uses
@@ -486,111 +552,22 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
     return;
   }
 
-  // Setup favicons table.
-  {
-    // Version 6 had the |sizes| column, version 7 removed it.  The
-    // recover virtual table treats more columns than expected as an
-    // error, but if _fewer_ columns are present, they can be treated
-    // as NULL.  SQLite requires this because ALTER TABLE adds columns
-    // to the schema, but not to the actual table storage.
-    const char kRecoverySql[] =
-        "CREATE VIRTUAL TABLE temp.recover_favicons USING recover"
-        "("
-        "corrupt.favicons,"
-        "id ROWID,"
-        "url TEXT NOT NULL,"
-        "icon_type INTEGER,"
-        "sizes TEXT"
-        ")";
-    if (!recovery->db()->Execute(kRecoverySql)) {
-      // TODO(shess): Failure to create the recovery table probably
-      // means unrecoverable.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_RECOVER_FAVICONS);
-      return;
-    }
-
-    // TODO(shess): Check if the DEFAULT 1 will just cover the
-    // COALESCE().  Either way, the new code has a literal 1 rather
-    // than a NULL, right?
-    const char kCopySql[] =
-        "INSERT OR REPLACE INTO main.favicons "
-        "SELECT id, url, COALESCE(icon_type, 1) FROM recover_favicons";
-    if (!recovery->db()->Execute(kCopySql)) {
-      // TODO(shess): The recover_favicons table should mask problems
-      // with the source file, so this implies failure to write to the
-      // recovery database.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_FAVICONS_INSERT);
-      return;
-    }
+  // [favicons] differs because v6 had an unused [sizes] column which
+  // was removed in v7.
+  if (!recovery->AutoRecoverTable("favicons", 1)) {
+    sql::Recovery::Rollback(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICONS);
+    return;
   }
-
-  // Setup favicons_bitmaps table.
-  {
-    const char kRecoverySql[] =
-        "CREATE VIRTUAL TABLE temp.recover_favicons_bitmaps USING recover"
-        "("
-        "corrupt.favicon_bitmaps,"
-        "id ROWID,"
-        "icon_id INTEGER STRICT NOT NULL,"
-        "last_updated INTEGER,"
-        "image_data BLOB,"
-        "width INTEGER,"
-        "height INTEGER"
-        ")";
-    if (!recovery->db()->Execute(kRecoverySql)) {
-      // TODO(shess): Failure to create the recovery table probably
-      // means unrecoverable.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_RECOVER_FAVICON_BITMAPS);
-      return;
-    }
-
-    const char kCopySql[] =
-        "INSERT OR REPLACE INTO main.favicon_bitmaps "
-        "SELECT id, icon_id, COALESCE(last_updated, 0), image_data, "
-        " COALESCE(width, 0), COALESCE(height, 0) "
-        "FROM recover_favicons_bitmaps";
-    if (!recovery->db()->Execute(kCopySql)) {
-      // TODO(shess): The recover_faviconbitmaps table should mask
-      // problems with the source file, so this implies failure to
-      // write to the recovery database.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_FAVICON_BITMAPS_INSERT);
-      return;
-    }
+  if (!recovery->AutoRecoverTable("favicon_bitmaps", 0)) {
+    sql::Recovery::Rollback(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICON_BITMAPS);
+    return;
   }
-
-  // Setup icon_mapping table.
-  {
-    const char kRecoverySql[] =
-        "CREATE VIRTUAL TABLE temp.recover_icon_mapping USING recover"
-        "("
-        "corrupt.icon_mapping,"
-        "id ROWID,"
-        "page_url TEXT STRICT NOT NULL,"
-        "icon_id INTEGER STRICT"
-        ")";
-    if (!recovery->db()->Execute(kRecoverySql)) {
-      // TODO(shess): Failure to create the recovery table probably
-      // means unrecoverable.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_RECOVER_ICON_MAPPING);
-      return;
-    }
-
-    const char kCopySql[] =
-        "INSERT OR REPLACE INTO main.icon_mapping "
-        "SELECT id, page_url, icon_id FROM recover_icon_mapping";
-    if (!recovery->db()->Execute(kCopySql)) {
-      // TODO(shess): The recover_icon_mapping table should mask
-      // problems with the source file, so this implies failure to
-      // write to the recovery database.
-      sql::Recovery::Rollback(recovery.Pass());
-      RecordRecoveryEvent(RECOVERY_EVENT_FAILED_ICON_MAPPING_INSERT);
-      return;
-    }
+  if (!recovery->AutoRecoverTable("icon_mapping", 0)) {
+    sql::Recovery::Rollback(recovery.Pass());
+    RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_ICON_MAPPING);
+    return;
   }
 
   // TODO(shess): Is it possible/likely to have broken foreign-key