[sql] Database recovery system for Shortcuts.

components/omnibox/browser/shortcuts_database.cc

Index: components/omnibox/browser/shortcuts_database.cc
diff --git a/components/omnibox/browser/shortcuts_database.cc b/components/omnibox/browser/shortcuts_database.cc
index 7ba8ea800dda54bcc5ca0ada4b976ca1333fb75f..44baa6084746b61bda432ff86e814ec58cd2245d 100644
--- a/components/omnibox/browser/shortcuts_database.cc
+++ b/components/omnibox/browser/shortcuts_database.cc
@@ -6,12 +6,14 @@
 
 #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"
@@ -55,6 +57,24 @@ bool DeleteShortcut(const char* field_name,
   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 -------------------------------------
@@ -123,6 +143,10 @@ ShortcutsDatabase::ShortcutsDatabase(const base::FilePath& 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
@@ -257,22 +281,23 @@ bool ShortcutsDatabase::EnsureTable() {
   }
 
   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;
     }
   }