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
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 again
|
+ 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
Scott Hess - ex-Googler
2016/04/15 00:38:14
OK. Leaving the last line, because IMHO it's pert
|
+ 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 disp
Scott Hess - ex-Googler
2016/04/15 00:38:14
I'll have to think about this. Corruption general
|
+ 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 p
Scott Hess - ex-Googler
2016/04/15 00:38:14
OK. Adding http://crbug.com/603304 about this. I
|
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; |
} |
} |