From 5207e145f18d167484e8941c7fa4f6b38224bc76 Mon Sep 17 00:00:00 2001 From: shess Date: Wed, 12 Apr 2017 16:55:33 -0700 Subject: [PATCH] [sql] Convert thumbnails and top-sites databases to auto-recovery. sql::Recovery::RecoverDatabase() seems to work pretty well for Shortcuts, so backfill to use it for these two databases. Histograms indicate that there's nothing really special about these databases which demands special treatment. BUG=240396,597785 Review-Url: https://codereview.chromium.org/2727553006 Cr-Commit-Position: refs/heads/master@{#464213} --- .../core/browser/thumbnail_database.cc | 240 +++--------------- .../browser/thumbnail_database_unittest.cc | 30 +-- .../core/browser/top_sites_database.cc | 137 ++++------ .../browser/top_sites_database_unittest.cc | 40 +-- sql/recovery.cc | 55 +++- sql/recovery.h | 26 +- sql/recovery_unittest.cc | 48 ++++ tools/metrics/histograms/histograms.xml | 31 ++- 8 files changed, 222 insertions(+), 385 deletions(-) diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc index f476c6e0db1ab3..d9bc22defb1f9e 100644 --- a/components/history/core/browser/thumbnail_database.cc +++ b/components/history/core/browser/thumbnail_database.cc @@ -82,9 +82,9 @@ namespace { // fatal (in fact, very old data may be expired immediately at startup // anyhow). -// Version 8: ???????? by rogerm@chromium.org on 2015-??-?? +// Version 8: 982ef2c1/r323176 by rogerm@chromium.org on 2015-03-31 // Version 7: 911a634d/r209424 by qsr@chromium.org on 2013-07-01 -// Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20 +// Version 6: 610f923b/r152367 by pkotwicz@chromium.org on 2012-08-20 (depr.) // Version 5: e2ee8ae9/r105004 by groby@chromium.org on 2011-10-12 (deprecated) // Version 4: 5f104d76/r77288 by sky@chromium.org on 2011-03-08 (deprecated) // Version 3: 09911bf3/r15 by initial.commit on 2008-07-26 (deprecated) @@ -94,7 +94,7 @@ namespace { // the new version and a test to verify that Init() works with it. const int kCurrentVersionNumber = 8; const int kCompatibleVersionNumber = 8; -const int kDeprecatedVersionNumber = 5; // and earlier. +const int kDeprecatedVersionNumber = 6; // and earlier. void FillIconMapping(const sql::Statement& statement, const GURL& page_url, @@ -146,8 +146,7 @@ void GenerateDiagnostics(sql::Connection* db, } // NOTE(shess): Schema modifications must consider initial creation in -// |InitImpl()|, recovery in |RecoverDatabaseOrRaze()|, and history pruning in -// |RetainDataForPageUrls()|. +// |InitImpl()| and history pruning in |RetainDataForPageUrls()|. bool InitTables(sql::Connection* db) { const char kIconMappingSql[] = "CREATE TABLE IF NOT EXISTS icon_mapping" @@ -190,8 +189,7 @@ bool InitTables(sql::Connection* db) { } // NOTE(shess): Schema modifications must consider initial creation in -// |InitImpl()|, recovery in |RecoverDatabaseOrRaze()|, and history pruning in -// |RetainDataForPageUrls()|. +// |InitImpl()| and history pruning in |RetainDataForPageUrls()|. bool InitIndices(sql::Connection* db) { const char kIconMappingUrlIndexSql[] = "CREATE INDEX IF NOT EXISTS icon_mapping_page_url_idx" @@ -218,199 +216,6 @@ bool InitIndices(sql::Connection* db) { return true; } -enum RecoveryEventType { - RECOVERY_EVENT_RECOVERED = 0, - RECOVERY_EVENT_FAILED_SCOPER, - RECOVERY_EVENT_FAILED_META_VERSION_ERROR, // obsolete - RECOVERY_EVENT_FAILED_META_VERSION_NONE, // obsolete - RECOVERY_EVENT_FAILED_META_WRONG_VERSION6, // obsolete - RECOVERY_EVENT_FAILED_META_WRONG_VERSION5, // obsolete - RECOVERY_EVENT_FAILED_META_WRONG_VERSION, - RECOVERY_EVENT_FAILED_RECOVER_META, // obsolete - RECOVERY_EVENT_FAILED_META_INSERT, // obsolete - RECOVERY_EVENT_FAILED_INIT, - RECOVERY_EVENT_FAILED_RECOVER_FAVICONS, // obsolete - RECOVERY_EVENT_FAILED_FAVICONS_INSERT, // obsolete - RECOVERY_EVENT_FAILED_RECOVER_FAVICON_BITMAPS, // obsolete - RECOVERY_EVENT_FAILED_FAVICON_BITMAPS_INSERT, // obsolete - RECOVERY_EVENT_FAILED_RECOVER_ICON_MAPPING, // obsolete - RECOVERY_EVENT_FAILED_ICON_MAPPING_INSERT, // obsolete - RECOVERY_EVENT_RECOVERED_VERSION6, // obsolete - RECOVERY_EVENT_FAILED_META_INIT, - RECOVERY_EVENT_FAILED_META_VERSION, - RECOVERY_EVENT_DEPRECATED, - RECOVERY_EVENT_FAILED_V5_INITSCHEMA, // obsolete - RECOVERY_EVENT_FAILED_V5_AUTORECOVER_FAVICONS, // obsolete - RECOVERY_EVENT_FAILED_V5_AUTORECOVER_ICON_MAPPING, // obsolete - RECOVERY_EVENT_RECOVERED_VERSION5, // obsolete - RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICONS, - RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICON_BITMAPS, - RECOVERY_EVENT_FAILED_AUTORECOVER_ICON_MAPPING, - RECOVERY_EVENT_FAILED_COMMIT, - - // Always keep this at the end. - RECOVERY_EVENT_MAX, -}; - -void RecordRecoveryEvent(RecoveryEventType recovery_event) { - UMA_HISTOGRAM_ENUMERATION("History.FaviconsRecovery", - recovery_event, RECOVERY_EVENT_MAX); -} - -// Recover the database to the extent possible, razing it if recovery -// is not possible. -// TODO(shess): This is mostly just a safe proof of concept. In the -// real world, this database is probably not worthwhile recovering, as -// opposed to just razing it and starting over whenever corruption is -// detected. So this database is a good test subject. -void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) { - // NOTE(shess): This code is currently specific to the version - // number. I am working on simplifying things to loosen the - // dependency, meanwhile contact me if you need to bump the version. - DCHECK_EQ(8, kCurrentVersionNumber); - - // TODO(shess): Reset back after? - db->reset_error_callback(); - - // For histogram purposes. - size_t favicons_rows_recovered = 0; - size_t favicon_bitmaps_rows_recovered = 0; - size_t icon_mapping_rows_recovered = 0; - int64_t original_size = 0; - base::GetFileSize(db_path, &original_size); - - std::unique_ptr recovery = sql::Recovery::Begin(db, db_path); - if (!recovery) { - // TODO(shess): Unable to create recovery connection. This - // implies something substantial is wrong. At this point |db| has - // been poisoned so there is nothing really to do. - // - // Possible responses are unclear. If the failure relates to a - // problem somehow specific to the temporary file used to back the - // database, then an in-memory database could possibly be used. - // This could potentially allow recovering the main database, and - // might be simple to implement w/in Begin(). - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_SCOPER); - return; - } - - // Setup the meta recovery table and fetch the version number from - // the corrupt database. - int version = 0; - if (!recovery->SetupMeta() || !recovery->GetMetaVersionNumber(&version)) { - // TODO(shess): Prior histograms indicate all failures are in - // creating the recover virtual table for corrupt.meta. The table - // may not exist, or the database may be too far gone. Either - // way, unclear how to resolve. - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_VERSION); - return; - } - - // This code may be able to fetch version information that the regular - // deprecation path cannot. - // NOTE(shess,rogerm): v6 is not currently deprecated in the normal Init() - // path, but is deprecated in the recovery path in the interest of keeping - // the code simple. http://crbug.com/327485 for numbers. - DCHECK_LE(kDeprecatedVersionNumber, 6); - if (version <= 6) { - sql::Recovery::Unrecoverable(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_DEPRECATED); - return; - } - - // Earlier versions have been handled or deprecated. - if (version < 7) { - sql::Recovery::Unrecoverable(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION); - return; - } - - // Recover to current schema version. - sql::MetaTable recover_meta_table; - if (!recover_meta_table.Init(recovery->db(), kCurrentVersionNumber, - kCompatibleVersionNumber)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_INIT); - return; - } - - // Create a fresh version of the database. The recovery code uses - // conflict-resolution to handle duplicates, so the indices are - // necessary. - if (!InitTables(recovery->db()) || !InitIndices(recovery->db())) { - // TODO(shess): Unable to create the new schema in the new - // database. The new database should be a temporary file, so - // being unable to work with it is pretty unclear. - // - // What are the potential responses, even? The recovery database - // could be opened as in-memory. If the temp database had a - // filesystem problem and the temp filesystem differs from the - // main database, then that could fix it. - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_INIT); - return; - } - - if (!recovery->AutoRecoverTable("favicons", &favicons_rows_recovered)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICONS); - return; - } - if (!recovery->AutoRecoverTable("favicon_bitmaps", - &favicon_bitmaps_rows_recovered)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_FAVICON_BITMAPS); - return; - } - if (!recovery->AutoRecoverTable("icon_mapping", - &icon_mapping_rows_recovered)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_ICON_MAPPING); - return; - } - - // TODO(shess): Is it possible/likely to have broken foreign-key - // issues with the tables? - // - icon_mapping.icon_id maps to no favicons.id - // - favicon_bitmaps.icon_id maps to no favicons.id - // - favicons.id is referenced by no icon_mapping.icon_id - // - favicons.id is referenced by no favicon_bitmaps.icon_id - // This step is possibly not worth the effort necessary to develop - // and sequence the statements, as it is basically a form of garbage - // collection. - - if (!sql::Recovery::Recovered(std::move(recovery))) { - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_COMMIT); - return; - } - - // Track the size of the recovered database relative to the size of - // the input database. The size should almost always be smaller, - // unless the input database was empty to start with. If the - // percentage results are very low, something is awry. - int64_t final_size = 0; - if (original_size > 0 && - base::GetFileSize(db_path, &final_size) && - final_size > 0) { - int percentage = static_cast(original_size * 100 / final_size); - UMA_HISTOGRAM_PERCENTAGE("History.FaviconsRecoveredPercentage", - std::max(100, percentage)); - } - - // Using 10,000 because these cases mostly care about "none - // recovered" and "lots recovered". More than 10,000 rows recovered - // probably means there's something wrong with the profile. - UMA_HISTOGRAM_COUNTS_10000("History.FaviconsRecoveredRowsFavicons", - static_cast(favicons_rows_recovered)); - UMA_HISTOGRAM_COUNTS_10000("History.FaviconsRecoveredRowsFaviconBitmaps", - static_cast(favicon_bitmaps_rows_recovered)); - UMA_HISTOGRAM_COUNTS_10000("History.FaviconsRecoveredRowsIconMapping", - static_cast(icon_mapping_rows_recovered)); - - RecordRecoveryEvent(RECOVERY_EVENT_RECOVERED); -} - void DatabaseErrorCallback(sql::Connection* db, const base::FilePath& db_path, HistoryBackendClient* backend_client, @@ -425,11 +230,36 @@ void DatabaseErrorCallback(sql::Connection* db, } // Attempt to recover corrupt databases. - int error = (extended_error & 0xFF); - if (error == SQLITE_CORRUPT || - error == SQLITE_CANTOPEN || - error == SQLITE_NOTADB) { - RecoverDatabaseOrRaze(db, db_path); + if (sql::Recovery::ShouldRecover(extended_error)) { + // NOTE(shess): This approach is valid as of version 8. When bumping the + // version, it will PROBABLY remain valid, but consider whether any schema + // changes might break automated recovery. + DCHECK_EQ(8, kCurrentVersionNumber); + + // Prevent reentrant calls. + db->reset_error_callback(); + + // TODO(shess): Is it possible/likely to have broken foreign-key + // issues with the tables? + // - icon_mapping.icon_id maps to no favicons.id + // - favicon_bitmaps.icon_id maps to no favicons.id + // - favicons.id is referenced by no icon_mapping.icon_id + // - favicons.id is referenced by no favicon_bitmaps.icon_id + // This step is possibly not worth the effort necessary to develop + // and sequence the statements, as it is basically a form of garbage + // collection. + + // After this call, the |db| handle is poisoned so that future calls will + // return errors until the handle is re-opened. + sql::Recovery::RecoverDatabaseWithMetaVersion(db, db_path); + + // The DLOG(FATAL) below is intended to draw immediate attention to errors + // in newly-written code. Database corruption is generally a result of OS + // or hardware issues, not coding errors at the client level, so displaying + // the error would probably lead to confusion. 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. diff --git a/components/history/core/browser/thumbnail_database_unittest.cc b/components/history/core/browser/thumbnail_database_unittest.cc index 61873b37978cd5..b20bbaf3f78998 100644 --- a/components/history/core/browser/thumbnail_database_unittest.cc +++ b/components/history/core/browser/thumbnail_database_unittest.cc @@ -636,34 +636,8 @@ TEST_F(ThumbnailDatabaseTest, Version6) { ASSERT_TRUE(db.get() != NULL); VerifyTablesAndColumns(&db->db_); - EXPECT_TRUE(CheckPageHasIcon(db.get(), - kPageUrl1, - favicon_base::FAVICON, - kIconUrl1, - kLargeSize, - sizeof(kBlob1), - kBlob1)); - EXPECT_TRUE(CheckPageHasIcon(db.get(), - kPageUrl2, - favicon_base::FAVICON, - kIconUrl2, - kLargeSize, - sizeof(kBlob2), - kBlob2)); - EXPECT_TRUE(CheckPageHasIcon(db.get(), - kPageUrl3, - favicon_base::FAVICON, - kIconUrl1, - kLargeSize, - sizeof(kBlob1), - kBlob1)); - EXPECT_TRUE(CheckPageHasIcon(db.get(), - kPageUrl3, - favicon_base::TOUCH_ICON, - kIconUrl3, - kLargeSize, - sizeof(kBlob2), - kBlob2)); + // Version 6 is deprecated, the data should all be gone. + VerifyDatabaseEmpty(&db->db_); } // Test loading version 7 database. diff --git a/components/history/core/browser/top_sites_database.cc b/components/history/core/browser/top_sites_database.cc index 3ae0a1088dc4b7..435930053a2a3d 100644 --- a/components/history/core/browser/top_sites_database.cc +++ b/components/history/core/browser/top_sites_database.cc @@ -58,16 +58,13 @@ namespace { // anyhow). // Version 3: b6d6a783/r231648 by beaudoin@chromium.org on 2013-10-29 -// Version 2: eb0b24e6/r87284 by satorux@chromium.org on 2011-05-31 +// Version 2: eb0b24e6/r87284 by satorux@chromium.org on 2011-05-31 (deprecated) // Version 1: 809cc4d8/r64072 by sky@chromium.org on 2010-10-27 (deprecated) // NOTE(shess): When changing the version, add a new golden file for // the new version and a test to verify that Init() works with it. -// NOTE(shess): RecoverDatabaseOrRaze() depends on the specific -// version number. The code is subtle and in development, contact me -// if the necessary changes are not obvious. static const int kVersionNumber = 3; -static const int kDeprecatedVersionNumber = 1; // and earlier. +static const int kDeprecatedVersionNumber = 2; // and earlier. bool InitTables(sql::Connection* db) { const char kThumbnailsSql[] = @@ -108,9 +105,8 @@ void SetRedirects(const std::string& redirects, MostVisitedURL* url) { // Track various failure (and success) cases in recovery code. // // TODO(shess): The recovery code is complete, but by nature runs in challenging -// circumstances, so initially the default error response is to leave the -// existing database in place. This histogram is intended to expose the -// failures seen in the fleet. Frequent failure cases can be explored more +// circumstances, so errors will happen. This histogram is intended to expose +// the failures seen in the fleet. Frequent failure cases can be explored more // deeply to see if the complexity to fix them is warranted. Infrequent failure // cases can be resolved by marking the database unrecoverable (which will // delete the data). @@ -126,12 +122,12 @@ enum RecoveryEventType { // Sqlite.RecoveryEvent can usually be used to get more detail about the // specific failure (see sql/recovery.cc). - RECOVERY_EVENT_FAILED_SCOPER, + OBSOLETE_RECOVERY_EVENT_FAILED_SCOPER, RECOVERY_EVENT_FAILED_META_VERSION, RECOVERY_EVENT_FAILED_META_WRONG_VERSION, - RECOVERY_EVENT_FAILED_META_INIT, - RECOVERY_EVENT_FAILED_SCHEMA_INIT, - RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS, + OBSOLETE_RECOVERY_EVENT_FAILED_META_INIT, + OBSOLETE_RECOVERY_EVENT_FAILED_SCHEMA_INIT, + OBSOLETE_RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS, RECOVERY_EVENT_FAILED_COMMIT, // Track invariants resolved by FixThumbnailsTable(). @@ -139,6 +135,9 @@ enum RecoveryEventType { RECOVERY_EVENT_INVARIANT_REDIRECT, RECOVERY_EVENT_INVARIANT_CONTIGUOUS, + // Track automated full-database recovery. + RECOVERY_EVENT_FAILED_AUTORECOVER, + // Always keep this at the end. RECOVERY_EVENT_MAX, }; @@ -203,89 +202,48 @@ void FixThumbnailsTable(sql::Connection* db) { RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_CONTIGUOUS); } -// Recover the database to the extent possible, razing it if recovery is not -// possible. -void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) { +// Recover the database to the extent possible, then fixup any broken +// constraints. +void RecoverAndFixup(sql::Connection* db, const base::FilePath& db_path) { // NOTE(shess): If the version changes, review this code. DCHECK_EQ(3, kVersionNumber); - // It is almost certain that some operation against |db| will fail, prevent - // reentry. - db->reset_error_callback(); - - // For generating histogram stats. - size_t thumbnails_recovered = 0; - int64_t original_size = 0; - base::GetFileSize(db_path, &original_size); - - std::unique_ptr recovery = sql::Recovery::Begin(db, db_path); + std::unique_ptr recovery = + sql::Recovery::BeginRecoverDatabase(db, db_path); if (!recovery) { - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_SCOPER); + RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER); return; } - // Setup the meta recovery table and fetch the version number from the corrupt - // database. + // If the [meta] table does not exist, or the [version] key cannot be found, + // then the schema is indeterminate. The only plausible approach would be to + // validate that the schema contains all of the tables and indices and columns + // expected, but that complexity may not be warranted, this case has only been + // seen for a few thousand database files. int version = 0; if (!recovery->SetupMeta() || !recovery->GetMetaVersionNumber(&version)) { - // TODO(shess): Prior histograms indicate all failures are in creating the - // recover virtual table for corrupt.meta. The table may not exist, or the - // database may be too far gone. Either way, unclear how to resolve. - sql::Recovery::Rollback(std::move(recovery)); + sql::Recovery::Unrecoverable(std::move(recovery)); RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_VERSION); return; } - // This code runs in a context which may be able to read version information - // that the regular deprecation path cannot. The effect of this code will be - // to raze the database. + // In this case the next open will clear the database anyhow. if (version <= kDeprecatedVersionNumber) { sql::Recovery::Unrecoverable(std::move(recovery)); RecordRecoveryEvent(RECOVERY_EVENT_DEPRECATED); return; } - // TODO(shess): Earlier versions have been deprecated, later versions should - // be impossible. Unrecoverable() seems like a feasible response if this is - // infrequent enough. - if (version != 2 && version != 3) { + // TODO(shess): Consider marking corrupt databases from the future + // Unrecoverable(), since this histogram value has never been seen. OTOH, + // this may be too risky, because if future code was correlated with + // corruption then rollback would be a sensible response. + if (version > kVersionNumber) { RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION); sql::Recovery::Rollback(std::move(recovery)); return; } - // Both v2 and v3 recover to current schema version. - sql::MetaTable recover_meta_table; - if (!recover_meta_table.Init(recovery->db(), kVersionNumber, - kVersionNumber)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_INIT); - return; - } - - // Create a fresh version of the schema. The recovery code uses - // conflict-resolution to handle duplicates, so any indices are necessary. - if (!InitTables(recovery->db())) { - // TODO(shess): Unable to create the new schema in the new database. The - // new database should be a temporary file, so being unable to work with it - // is pretty unclear. - // - // What are the potential responses, even? The recovery database could be - // opened as in-memory. If the temp database had a filesystem problem and - // the temp filesystem differs from the main database, then that could fix - // it. - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_SCHEMA_INIT); - return; - } - - // In the v2 case the missing column will get default values. - if (!recovery->AutoRecoverTable("thumbnails", &thumbnails_recovered)) { - sql::Recovery::Rollback(std::move(recovery)); - RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS); - return; - } - // TODO(shess): Inline this? FixThumbnailsTable(recovery->db()); @@ -297,23 +255,6 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) { return; } - // Track the size of the recovered database relative to the size of the input - // database. The size should almost always be smaller, unless the input - // database was empty to start with. If the percentage results are very low, - // something is awry. - int64_t final_size = 0; - if (original_size > 0 && base::GetFileSize(db_path, &final_size) && - final_size > 0) { - UMA_HISTOGRAM_PERCENTAGE("History.TopSitesRecoveredPercentage", - final_size * 100 / original_size); - } - - // Using 10,000 because these cases mostly care about "none recovered" and - // "lots recovered". More than 10,000 rows recovered probably means there's - // something wrong with the profile. - UMA_HISTOGRAM_COUNTS_10000("History.TopSitesRecoveredRowsThumbnails", - static_cast(thumbnails_recovered)); - RecordRecoveryEvent(RECOVERY_EVENT_RECOVERED); } @@ -325,11 +266,21 @@ void DatabaseErrorCallback(sql::Connection* db, // be the history thread, but at this level I can't see how to reach that. // Attempt to recover corrupt databases. - int error = (extended_error & 0xFF); - if (error == SQLITE_CORRUPT || - error == SQLITE_CANTOPEN || - error == SQLITE_NOTADB) { - RecoverDatabaseOrRaze(db, db_path); + 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. + RecoverAndFixup(db, db_path); + + // The DLOG(FATAL) below is intended to draw immediate attention to errors + // in newly-written code. Database corruption is generally a result of OS + // or hardware issues, not coding errors at the client level, so displaying + // the error would probably lead to confusion. The ignored call signals the + // test-expectation framework that the error was handled. + ignore_result(sql::Connection::IsExpectedSqliteError(extended_error)); + return; } // TODO(shess): This database's error histograms look like: diff --git a/components/history/core/browser/top_sites_database_unittest.cc b/components/history/core/browser/top_sites_database_unittest.cc index 289b983040405c..29683551742caf 100644 --- a/components/history/core/browser/top_sites_database_unittest.cc +++ b/components/history/core/browser/top_sites_database_unittest.cc @@ -86,31 +86,15 @@ TEST_F(TopSitesDatabaseTest, Version1) { VerifyDatabaseEmpty(db.db_.get()); } +// Version 2 is deprecated, the resulting schema should be current, +// with no data. TEST_F(TopSitesDatabaseTest, Version2) { ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v2.sql")); TopSitesDatabase db; ASSERT_TRUE(db.Init(file_name_)); - VerifyTablesAndColumns(db.db_.get()); - - // Basic operational check. - MostVisitedURLList urls; - std::map thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(3u, urls.size()); - ASSERT_EQ(3u, thumbnails.size()); - EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. - // kGoogleThumbnail includes nul terminator. - ASSERT_EQ(sizeof(kGoogleThumbnail) - 1, - thumbnails[urls[0].url].thumbnail->size()); - EXPECT_TRUE(!memcmp(thumbnails[urls[0].url].thumbnail->front(), - kGoogleThumbnail, sizeof(kGoogleThumbnail) - 1)); - - ASSERT_TRUE(db.RemoveURL(urls[1])); - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(2u, urls.size()); - ASSERT_EQ(2u, thumbnails.size()); + VerifyDatabaseEmpty(db.db_.get()); } TEST_F(TopSitesDatabaseTest, Version3) { @@ -198,29 +182,15 @@ TEST_F(TopSitesDatabaseTest, Recovery2) { ASSERT_TRUE(expecter.SawExpectedErrors()); } - // Corruption should be detected and recovered during Init(). After recovery, - // the Version2 checks should work. + // Corruption should be detected and recovered during Init(). { sql::test::ScopedErrorExpecter expecter; expecter.ExpectError(SQLITE_CORRUPT); TopSitesDatabase db; ASSERT_TRUE(db.Init(file_name_)); - VerifyTablesAndColumns(db.db_.get()); - - // Basic operational check. - MostVisitedURLList urls; - std::map thumbnails; - db.GetPageThumbnails(&urls, &thumbnails); - ASSERT_EQ(3u, urls.size()); - ASSERT_EQ(3u, thumbnails.size()); - EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. - // kGoogleThumbnail includes nul terminator. - ASSERT_EQ(sizeof(kGoogleThumbnail) - 1, - thumbnails[urls[0].url].thumbnail->size()); - EXPECT_TRUE(!memcmp(thumbnails[urls[0].url].thumbnail->front(), - kGoogleThumbnail, sizeof(kGoogleThumbnail) - 1)); + VerifyDatabaseEmpty(db.db_.get()); ASSERT_TRUE(expecter.SawExpectedErrors()); } diff --git a/sql/recovery.cc b/sql/recovery.cc index a805c520a88ba1..64eefa51104efc 100644 --- a/sql/recovery.cc +++ b/sql/recovery.cc @@ -126,6 +126,9 @@ enum RecoveryEventType { // successfully deleted. RECOVERY_SUCCESS_AUTORECOVERDB_NOTADB_DELETE, + // Failed to find required [meta.version] information. + RECOVERY_FAILED_AUTORECOVERDB_META_VERSION, + // Add new items before this one, always keep this one at the end. RECOVERY_EVENT_MAX, }; @@ -602,8 +605,9 @@ bool SchemaCopyHelper(Connection* db, const char* prefix) { // results indicate that everything is working reasonably. // // static -void Recovery::RecoverDatabase(Connection* db, - const base::FilePath& db_path) { +std::unique_ptr Recovery::BeginRecoverDatabase( + Connection* db, + const base::FilePath& db_path) { std::unique_ptr recovery = sql::Recovery::Begin(db, db_path); if (!recovery) { // Close the underlying sqlite* handle. Windows does not allow deleting @@ -620,7 +624,7 @@ void Recovery::RecoverDatabase(Connection* db, probe_db.AttachDatabase(db_path, "corrupt") || probe_db.GetErrorCode() != SQLITE_NOTADB) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_BEGIN); - return; + return nullptr; } } @@ -629,7 +633,7 @@ void Recovery::RecoverDatabase(Connection* db, // recoverable _with_ manual intervention). Clear away the broken database. if (!sql::Connection::Delete(db_path)) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_DELETE); - return; + return nullptr; } // Windows deletion is complicated by file scanners and malware - sometimes @@ -639,18 +643,18 @@ void Recovery::RecoverDatabase(Connection* db, Connection probe_db; if (!probe_db.Open(db_path)) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_REOPEN); - return; + return nullptr; } if (!probe_db.Execute("PRAGMA auto_vacuum")) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NOTADB_QUERY); - return; + return nullptr; } } // The rest of the recovery code could be run on the re-opened database, but // the database is empty, so there would be no point. RecordRecoveryEvent(RECOVERY_SUCCESS_AUTORECOVERDB_NOTADB_DELETE); - return; + return nullptr; } #if DCHECK_IS_ON() @@ -682,7 +686,7 @@ void Recovery::RecoverDatabase(Connection* db, !SchemaCopyHelper(recovery->db(), "CREATE UNIQUE INDEX ")) { // No RecordRecoveryEvent() here because SchemaCopyHelper() already did. Recovery::Rollback(std::move(recovery)); - return; + return nullptr; } // Run auto-recover against each table, skipping the sequence table. This is @@ -698,13 +702,13 @@ void Recovery::RecoverDatabase(Connection* db, if (!recovery->AutoRecoverTable(name.c_str(), &rows_recovered)) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_TABLE); Recovery::Rollback(std::move(recovery)); - return; + return nullptr; } } if (!s.Succeeded()) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_NAMESELECT); Recovery::Rollback(std::move(recovery)); - return; + return nullptr; } } @@ -715,7 +719,7 @@ void Recovery::RecoverDatabase(Connection* db, if (!recovery->AutoRecoverTable("sqlite_sequence", &rows_recovered)) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_SEQUENCE); Recovery::Rollback(std::move(recovery)); - return; + return nullptr; } } @@ -728,10 +732,37 @@ void Recovery::RecoverDatabase(Connection* db, if (!recovery->db()->Execute(kCreateMetaItems)) { RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_AUX); Recovery::Rollback(std::move(recovery)); - return; + return nullptr; } RecordRecoveryEvent(RECOVERY_SUCCESS_AUTORECOVERDB); + return recovery; +} + +void Recovery::RecoverDatabase(Connection* db, const base::FilePath& db_path) { + std::unique_ptr recovery = BeginRecoverDatabase(db, db_path); + + // ignore_result() because BeginRecoverDatabase() and Recovered() already + // provide suitable histogram coverage. + if (recovery) + ignore_result(Recovery::Recovered(std::move(recovery))); +} + +void Recovery::RecoverDatabaseWithMetaVersion(Connection* db, + const base::FilePath& db_path) { + std::unique_ptr recovery = BeginRecoverDatabase(db, db_path); + if (!recovery) + return; + + int version = 0; + if (!recovery->SetupMeta() || !recovery->GetMetaVersionNumber(&version)) { + sql::Recovery::Unrecoverable(std::move(recovery)); + RecordRecoveryEvent(RECOVERY_FAILED_AUTORECOVERDB_META_VERSION); + return; + } + + // ignore_result() because BeginRecoverDatabase() and Recovered() already + // provide suitable histogram coverage. ignore_result(Recovery::Recovered(std::move(recovery))); } diff --git a/sql/recovery.h b/sql/recovery.h index efc69cbc837ff3..e5e4abf8608f89 100644 --- a/sql/recovery.h +++ b/sql/recovery.h @@ -156,18 +156,28 @@ class SQL_EXPORT Recovery { bool GetMetaVersionNumber(int* version_number); // Attempt to recover the database by creating a new database with schema from - // |db|, then copying over as much data as possible. After this call, the - // |db| handle will be poisoned (though technically remaining open) so that - // future calls will return errors until the handle is re-opened. - // - // If a corrupt database contains tables without unique indices, the resulting - // table may contain duplication. If this is not acceptable, the client - // should use the manual process as described in the example at the top of the - // file, cleaning up data at the appropriate points. + // |db|, then copying over as much data as possible. If successful, the + // recovery handle is returned to allow the caller to make additional changes, + // such as validating constraints not expressed in the schema. // // In case of SQLITE_NOTADB, the database is deemed unrecoverable and deleted. + static std::unique_ptr BeginRecoverDatabase( + Connection* db, + const base::FilePath& db_path) WARN_UNUSED_RESULT; + + // Call BeginRecoverDatabase() to recover the database, then commit the + // changes using Recovered(). After this call, the |db| handle will be + // poisoned (though technically remaining open) so that future calls will + // return errors until the handle is re-opened. static void RecoverDatabase(Connection* db, const base::FilePath& db_path); + // Variant on RecoverDatabase() which requires that the database have a valid + // meta table with a version value. The meta version value is used by some + // clients to make assertions about the database schema. If this information + // cannot be determined, the database is considered unrecoverable. + static void RecoverDatabaseWithMetaVersion(Connection* db, + const base::FilePath& db_path); + // Returns true for SQLite errors which RecoverDatabase() can plausibly fix. // This does not guarantee that RecoverDatabase() will successfully recover // the database. diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc index 111a610450a169..752688225d9729 100644 --- a/sql/recovery_unittest.cc +++ b/sql/recovery_unittest.cc @@ -834,6 +834,54 @@ TEST_F(SQLRecoveryTest, RecoverDatabaseDelete) { EXPECT_EQ("", GetSchema(&db())); } +// Allow callers to validate the database between recovery and commit. +TEST_F(SQLRecoveryTest, BeginRecoverDatabase) { + // Create a table with a broken index. + ASSERT_TRUE(db().Execute("CREATE TABLE t (id INTEGER PRIMARY KEY, c TEXT)")); + ASSERT_TRUE(db().Execute("CREATE UNIQUE INDEX t_id ON t (id)")); + ASSERT_TRUE(db().Execute("INSERT INTO t VALUES (1, 'hello world')")); + ASSERT_TRUE(db().Execute("INSERT INTO t VALUES (2, 'testing')")); + ASSERT_TRUE(db().Execute("INSERT INTO t VALUES (3, 'nope')")); + + // Inject corruption into the index. + db().Close(); + const char kDeleteSql[] = "DELETE FROM t WHERE id = 3"; + ASSERT_TRUE(sql::test::CorruptTableOrIndex(db_path(), "t_id", kDeleteSql)); + ASSERT_TRUE(Reopen()); + + // id as read from index. + const char kSelectIndexIdSql[] = "SELECT id FROM t INDEXED BY t_id"; + EXPECT_EQ("1,2,3", ExecuteWithResults(&db(), kSelectIndexIdSql, "|", ",")); + + // id as read from table. + const char kSelectTableIdSql[] = "SELECT id FROM t NOT INDEXED"; + EXPECT_EQ("1,2", ExecuteWithResults(&db(), kSelectTableIdSql, "|", ",")); + + // Run recovery code, then rollback. Database remains the same. + { + std::unique_ptr recovery = + sql::Recovery::BeginRecoverDatabase(&db(), db_path()); + ASSERT_TRUE(recovery); + sql::Recovery::Rollback(std::move(recovery)); + } + db().Close(); + ASSERT_TRUE(Reopen()); + EXPECT_EQ("1,2,3", ExecuteWithResults(&db(), kSelectIndexIdSql, "|", ",")); + EXPECT_EQ("1,2", ExecuteWithResults(&db(), kSelectTableIdSql, "|", ",")); + + // Run recovery code, then commit. The failing row is dropped. + { + std::unique_ptr recovery = + sql::Recovery::BeginRecoverDatabase(&db(), db_path()); + ASSERT_TRUE(recovery); + ASSERT_TRUE(sql::Recovery::Recovered(std::move(recovery))); + } + db().Close(); + ASSERT_TRUE(Reopen()); + EXPECT_EQ("1,2", ExecuteWithResults(&db(), kSelectIndexIdSql, "|", ",")); + EXPECT_EQ("1,2", ExecuteWithResults(&db(), kSelectTableIdSql, "|", ",")); +} + // Test histograms recorded when the invalid database cannot be attached. TEST_F(SQLRecoveryTest, AttachFailure) { // Create a valid database, then write junk over the header. This should lead diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 22bbbcdf0ed7d3..18ad474ad9b674 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -22498,6 +22498,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + No longer tracked as of March 2017. + rpop@google.com Size of the recovered Favicons database relative to the original corrupt @@ -22509,6 +22512,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + No longer tracked as of March 2017. + rpop@google.com Rows recovered from [favicon_bitmaps] table in Favicons recovery. @@ -22516,11 +22522,17 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + No longer tracked as of March 2017. + rpop@google.com Rows recovered from [favicons] table in Favicons recovery. + + No longer tracked as of March 2017. + rpop@google.com Rows recovered from [icon_mapping] table in Favicons recovery. @@ -22528,6 +22540,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + No longer tracked as of March 2017. + rpop@google.com Track results of SQLite database recovery code in thumbnail_database.cc. @@ -97867,6 +97882,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. Error states noted in thumbnail_database.cc recovery code. + + History.FaviconsRecovery no longer tracked as of March 2017. + Successful recovery. sql::Recovery failed init. @@ -97965,7 +97983,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. Recovery found deprecated version and razed. - + sql::Recovery failed init. @@ -97974,13 +97992,14 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. Recovery meta table has an unexpected version. - + Failed sql::MetaTable::Init(). - + Failed to init target schema. - + Failed recovery on thumbnails table. @@ -112692,6 +112711,10 @@ from previous Chrome versions. Autorecover failed setup with NOTADB, then successfully deleted the unrecoverable db and verified that it now works. + + Autorecover failed because required version information was missing from the + [meta] table. +