[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::ShouldRecover(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::RecoverDatabase(db, db_path);
+
+    // Recoverable cases should not be correlated with programmer error, so the

[Mark P, 2016/06/27 23:20:13]

I have trouble understanding this sentence.  I don't see why it shouldn't be
"correlated" in some sense, nor is it obvious why the DCHECK wouldn't be
helpful.  Perhaps consider replacing the sentence with:
The DLOG(FATAL) below should not normally be relevant to developers. (Developers
do not often test against corrupt-yet-recoverable databases.)  Displaying the
error would probably lead to more confusion than help.

[Scott Hess - ex-Googler, 2016/06/29 21:39:30]

Probably nerdview.  Made an attempt at revision.

[Mark P, 2016/07/01 22:40:55]

Your new comment is much better, thanks.

+    // DLOG(FATAL) below is not helpful.  The ignored call signals the
+    // test-expectation framework that the error was handled.
+    ignore_result(sql::Connection::IsExpectedSqliteError(extended_error));
+    return;
+  }
+
+  // The default handling is to assert on debug and to ignore on release.
+  if (!sql::Connection::IsExpectedSqliteError(extended_error))
+    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;
 }