[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) {
+  // Attempt to fix corrupt databases.  SQLITE_CANTOPEN and SQLITE_NOTADB
+  // generally mean that the file doesn't even look like a SQLite database, for
+  // instance if the header is broken, but there are a few cases that can be
+  // recovered.  SQLITE_CORRUPT generally means that data within the database
+  // contradicts itself, but SQLite is able to read it.

[Mark P, 2016/04/07 23:09:43]

Do you want this explanation here rather than than by the ShouldRecoverOrRaze()
function?  Having it in two places seems redundant and likely to get out of
sync.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Acknowledged.

+  // TODO(shess): At least some of the SQLITE_IOERR cases might be cleared by
+  // this, too.  Probable loss of data, but the disk block is unlikely to fix
+  // itself.

[Mark P, 2016/04/07 23:09:43]

ditto about this TODO

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

Leftover from when the code checked directly against SQLITE_CORRUPT and other
errors.  That required adding a dep on third_party/sqlite which seemed
inappropriate.

+  if (sql::Recovery::ShouldRecoverOrRaze(extended_error)) {
+    // Prevent reentrant calls.
+    db->reset_error_callback();
+
+    // Attempt to recover the database by creating a new database with the
+    // schema from the current database, and copying over as much of the data as
+    // possible.  Then restore the new database over the original.  If the
+    // database is unreadable, the new database will be empty.
+    //
+    // After this call, the |db| handle is poisoned so that future calls will
+    // return errors until the handle is re-opened.

[Mark P, 2016/04/07 23:09:43]

This comment duplicates much of the comment by the function definition.  Is any
of it necessary?

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

OK.  Leaving the last line, because IMHO it's pertinent enough to the local
stack and likely unexpected.

+    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/07 23:09:43]

Do you still want to run this code / possibly display the message if the above
code decides to recover the database?

If yes, consider whether it would be better for developers to display this
message first or the (possibly failed) recovery message first.  (I'd think the
original error first would be better.)

If no, then you can struct this function differently.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

I'll have to think about this.  Corruption generally stems from things like
unexpected power failures, so a developer is unlikely to see that.  This DLOG is
expected to fire for cases like SQLITE_ERROR, which would indicate that a
statement had a syntax error, or an inappropriate schema change happened,
something like that.  So is there an interesting case at the intersection where
a developer caused a corruption?  Hard to say.

If a developer was unlucky enough to have a legitimate corruption, I would
expect this to FATAL once, then work going forward.  Not a great outcome, but
most developers would just ignore it or log a bug against me.  That's find, then
I can discuss it with them and determine if there's a real problem or a red
herring.  Usually it's been a red herring, but it has caught real bugs.

+    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();
 
   // Attach the database to our index file.
+  // TODO(shess): The return value is never checked, so failures will never be
+  // addressed.  Someone needs to handle invalid schema.

[Mark P, 2016/04/07 23:09:43]

This shouldn't be a comment.  File a bug against pkasting@ to either do
something with the return value (and comment in the .h file the meaning of the
return value) or change the return value to void.

[Scott Hess - ex-Googler, 2016/04/15 00:38:14]

OK.  Adding http://crbug.com/603304 about this.  I'm fine with adding some
instrumentation and working to resolve this, as I'm also looking for ways to
generalize the diagnostics for other databases.

   return db_.Open(database_path_) && EnsureTable();
 }
 
 bool ShortcutsDatabase::AddShortcut(const Shortcut& shortcut) {
   sql::Statement s(db_.GetCachedStatement(
       SQL_FROM_HERE,
       "INSERT INTO omni_box_shortcuts (id, text, fill_into_edit, url, "
           "contents, contents_class, description, description_class, "
           "transition, type, keyword, last_access_time, number_of_hits) "
           "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)"));
@@ skipping 103 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;
 }