[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..8772ab90ca79630c0772c3eb3440b838b0a1d865 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,37 @@ 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) {
+  // 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 -------------------------------------
@@ -123,6 +156,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
@@ -134,6 +171,8 @@ bool ShortcutsDatabase::Init() {
   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();
 }
 
@@ -257,22 +296,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;
     }
   }