[sql] Database recovery system for Shortcuts.

components/omnibox/browser/shortcuts_database.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 "components/omnibox/browser/shortcuts_database.h"
 
 #include <string>
 
+#include "base/bind.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "components/omnibox/browser/autocomplete_match_type.h"
 #include "sql/meta_table.h"
+#include "sql/recovery.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 #include "ui/base/page_transition_types.h"
 
 
 // Helpers --------------------------------------------------------------------
 
 namespace {
 
 // Current version number. We write databases at the "current" version number,
@@ skipping 23 matching lines @@
 bool DeleteShortcut(const char* field_name,
                     const std::string& id,
                     sql::Connection& db) {
   sql::Statement s(db.GetUniqueStatement(
       base::StringPrintf("DELETE FROM omni_box_shortcuts WHERE %s = ?",
                          field_name).c_str()));
   s.BindString(0, id);
   return s.Run();
 }
 
+void DatabaseErrorCallback(sql::Connection* db,
+                           const base::FilePath& db_path,
+                           int extended_error,
+                           sql::Statement* stmt) {
+  if (sql::Recovery::ShouldRecoverOrRaze(extended_error)) {
+    // Prevent reentrant calls.
+    db->reset_error_callback();
+
+    // After this call, the |db| handle is poisoned so that future calls will
+    // return errors until the handle is re-opened.
+    sql::Recovery::RecoverDatabaseOrRaze(db, db_path);
+  }
+
+  // The default handling is to assert on debug and to ignore on release.
+  if (!sql::Connection::ShouldIgnoreSqliteError(extended_error))

[Mark P, 2016/04/19 23:34:27]

You misunderstood my comment.

I meant in the current revision of the code, you call try to open the database. 
If it fails, you then call RecoverDatabase(), which may display an error.  Then
at the end, you display the original error with opening the database.  This last
error I'd think can be easily interpreted as the final error, i.e., the error
with recovering the database (if any); if recovery succeeded, display this error
here may make it harder to recognize that fact.

I'd think you should display the original error, then attempt recovery and
display the result of the recovery attempt next, i.e., display errors in the
order they are encountered.

[Scott Hess - ex-Googler, 2016/05/13 21:24:35]

I'll just have the recovery path return.  Connection::OnSqliteError() already
logs at ERROR level, if anyone is paying attention.  I think the errors which
lead to recovery attempts should be distinct from the kinds of development
errors the DLOG(FATAL) is intended to expose.  To the extent that someone
manages to write code that causes corruption, crashing here wouldn't be helpful
(as compared to a statement error, where the crash happens compiling the
statement).

+    DLOG(FATAL) << db->GetErrorMessage();
+}
+
 }  // namespace
 
 // ShortcutsDatabase::Shortcut::MatchCore -------------------------------------
 
 ShortcutsDatabase::Shortcut::MatchCore::MatchCore(
     const base::string16& fill_into_edit,
     const GURL& destination_url,
     const base::string16& contents,
     const std::string& contents_class,
     const base::string16& description,
@@ skipping 48 matching lines @@
 
 // ShortcutsDatabase ----------------------------------------------------------
 
 ShortcutsDatabase::ShortcutsDatabase(const base::FilePath& database_path)
     : database_path_(database_path) {
 }
 
 bool ShortcutsDatabase::Init() {
   db_.set_histogram_tag("Shortcuts");
 
+  // To recover from corruption.
+  db_.set_error_callback(
+      base::Bind(&DatabaseErrorCallback, &db_, database_path_));
+
   // Set the database page size to something a little larger to give us
   // better performance (we're typically seek rather than bandwidth limited).
   // This only has an effect before any tables have been created, otherwise
   // this is a NOP. Must be a power of 2 and a max of 8192.
   db_.set_page_size(4096);
 
   // Run the database in exclusive mode. Nobody else should be accessing the
   // database while we're running, and this will give somewhat improved perf.
   db_.set_exclusive_locking();
 
@@ skipping 114 matching lines @@
             "UPDATE omni_box_shortcuts SET type = %d",
             static_cast<int>(AutocompleteMatchType::HISTORY_TITLE)).c_str()) &&
         db_.Execute("ALTER TABLE omni_box_shortcuts "
             "ADD COLUMN keyword VARCHAR") &&
         transaction.Commit())) {
       return false;
     }
   }
 
   if (!sql::MetaTable::DoesTableExist(&db_)) {
-    meta_table_.Init(&db_, kCurrentVersionNumber, kCompatibleVersionNumber);
     sql::Transaction transaction(&db_);
     if (!(transaction.Begin() &&
-      // Migrate old SEARCH_OTHER_ENGINE values to the new type value.
-      db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
-          "SET type = 13 WHERE type = 9").c_str()) &&
-      // Migrate old EXTENSION_APP values to the new type value.
-      db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
-           "SET type = 14 WHERE type = 10").c_str()) &&
-      // Migrate old CONTACT values to the new type value.
-      db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
-           "SET type = 15 WHERE type = 11").c_str()) &&
-      // Migrate old BOOKMARK_TITLE values to the new type value.
-      db_.Execute(base::StringPrintf("UPDATE omni_box_shortcuts "
-           "SET type = 16 WHERE type = 12").c_str()) &&
-      transaction.Commit())) {
+          meta_table_.Init(
+              &db_, kCurrentVersionNumber, kCompatibleVersionNumber) &&
+          // Migrate old SEARCH_OTHER_ENGINE values to the new type value.
+          db_.Execute(
+              "UPDATE omni_box_shortcuts SET type = 13 WHERE type = 9") &&
+          // Migrate old EXTENSION_APP values to the new type value.
+          db_.Execute(
+              "UPDATE omni_box_shortcuts SET type = 14 WHERE type = 10") &&
+          // Migrate old CONTACT values to the new type value.
+          db_.Execute(
+              "UPDATE omni_box_shortcuts SET type = 15 WHERE type = 11") &&
+          // Migrate old BOOKMARK_TITLE values to the new type value.
+          db_.Execute(
+              "UPDATE omni_box_shortcuts SET type = 16 WHERE type = 12") &&
+          transaction.Commit())) {
       return false;
     }
   }
   return true;
 }