diff options
| -rw-r--r-- | chrome/browser/history/thumbnail_database_unittest.cc | 71 | ||||
| -rw-r--r-- | chrome/browser/history/top_sites_database.cc | 278 | ||||
| -rw-r--r-- | chrome/browser/history/top_sites_database.h | 8 | ||||
| -rw-r--r-- | chrome/browser/history/top_sites_database_unittest.cc | 216 | ||||
| -rw-r--r-- | sql/recovery.cc | 13 | ||||
| -rw-r--r-- | sql/recovery.h | 24 | ||||
| -rw-r--r-- | sql/recovery_unittest.cc | 91 | ||||
| -rw-r--r-- | sql/test/test_helpers.cc | 84 | ||||
| -rw-r--r-- | sql/test/test_helpers.h | 21 | ||||
| -rw-r--r-- | tools/metrics/histograms/histograms.xml | 65 |
10 files changed, 715 insertions, 156 deletions
diff --git a/chrome/browser/history/thumbnail_database_unittest.cc b/chrome/browser/history/thumbnail_database_unittest.cc index 6e00e62..9aa5c75 100644 --- a/chrome/browser/history/thumbnail_database_unittest.cc +++ b/chrome/browser/history/thumbnail_database_unittest.cc @@ -6,20 +6,15 @@ #include <vector> #include "base/basictypes.h" -#include "base/file_util.h" #include "base/files/file_enumerator.h" #include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" #include "base/memory/ref_counted_memory.h" #include "base/path_service.h" -#include "chrome/browser/history/history_database.h" #include "chrome/browser/history/thumbnail_database.h" -#include "chrome/common/chrome_constants.h" #include "chrome/common/chrome_paths.h" -#include "chrome/test/base/testing_profile.h" #include "sql/connection.h" -#include "sql/recovery.h" // For FullRecoverySupported(). -#include "sql/statement.h" +#include "sql/recovery.h" #include "sql/test/scoped_error_ignorer.h" #include "sql/test/test_helpers.h" #include "testing/gtest/include/gtest/gtest.h" @@ -64,49 +59,6 @@ WARN_UNUSED_RESULT bool CreateDatabaseFromSQL(const base::FilePath &db_path, return sql::test::CreateDatabaseFromSQL(db_path, sql_path); } -int GetPageSize(sql::Connection* db) { - sql::Statement s(db->GetUniqueStatement("PRAGMA page_size")); - EXPECT_TRUE(s.Step()); - return s.ColumnInt(0); -} - -// Get |name|'s root page number in the database. -int GetRootPage(sql::Connection* db, const char* name) { - const char kPageSql[] = "SELECT rootpage FROM sqlite_master WHERE name = ?"; - sql::Statement s(db->GetUniqueStatement(kPageSql)); - s.BindString(0, name); - EXPECT_TRUE(s.Step()); - return s.ColumnInt(0); -} - -// Helper to read a SQLite page into a buffer. |page_no| is 1-based -// per SQLite usage. -bool ReadPage(const base::FilePath& path, size_t page_no, - char* buf, size_t page_size) { - file_util::ScopedFILE file(base::OpenFile(path, "rb")); - if (!file.get()) - return false; - if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET)) - return false; - if (1u != fread(buf, page_size, 1, file.get())) - return false; - return true; -} - -// Helper to write a SQLite page into a buffer. |page_no| is 1-based -// per SQLite usage. -bool WritePage(const base::FilePath& path, size_t page_no, - const char* buf, size_t page_size) { - file_util::ScopedFILE file(base::OpenFile(path, "rb+")); - if (!file.get()) - return false; - if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET)) - return false; - if (1u != fwrite(buf, page_size, 1, file.get())) - return false; - return true; -} - // Verify that the up-to-date database has the expected tables and // columns. Functional tests only check whether the things which // should be there are, but do not check if extraneous items are @@ -791,23 +743,12 @@ TEST_F(ThumbnailDatabaseTest, Recovery) { sql::Connection raw_db; EXPECT_TRUE(raw_db.Open(file_name_)); ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db)); - - const char kIndexName[] = "icon_mapping_page_url_idx"; - const int idx_root_page = GetRootPage(&raw_db, kIndexName); - const int page_size = GetPageSize(&raw_db); - scoped_ptr<char[]> buf(new char[page_size]); - EXPECT_TRUE(ReadPage(file_name_, idx_root_page, buf.get(), page_size)); - - { - const char kDeleteSql[] = "DELETE FROM icon_mapping WHERE page_url = ?"; - sql::Statement statement(raw_db.GetUniqueStatement(kDeleteSql)); - statement.BindString(0, URLDatabase::GURLToDatabaseURL(kPageUrl2)); - EXPECT_TRUE(statement.Run()); - } - raw_db.Close(); - - EXPECT_TRUE(WritePage(file_name_, idx_root_page, buf.get(), page_size)); } + const char kIndexName[] = "icon_mapping_page_url_idx"; + const char kDeleteSql[] = + "DELETE FROM icon_mapping WHERE page_url = 'http://yahoo.com/'"; + EXPECT_TRUE( + sql::test::CorruptTableOrIndex(file_name_, kIndexName, kDeleteSql)); // Database should be corrupt at the SQLite level. { diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc index 6972486..e820a1c 100644 --- a/chrome/browser/history/top_sites_database.cc +++ b/chrome/browser/history/top_sites_database.cc @@ -4,6 +4,7 @@ #include "base/file_util.h" #include "base/memory/ref_counted.h" +#include "base/metrics/histogram.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h" #include "chrome/browser/history/history_types.h" @@ -11,7 +12,10 @@ #include "chrome/browser/history/top_sites_database.h" #include "chrome/common/thumbnail_score.h" #include "sql/connection.h" +#include "sql/recovery.h" +#include "sql/statement.h" #include "sql/transaction.h" +#include "third_party/sqlite/sqlite3.h" // Description of database table: // @@ -48,11 +52,11 @@ namespace { // Version 2: eb0b24e6/r87284 by satorux@chromium.org on 2011-05-31 // Version 1: 809cc4d8/r64072 by sky@chromium.org on 2010-10-27 (deprecated) -// From the version 1 to 2, one column was added. Old versions of Chrome -// should be able to read version 2 files just fine. Same thing for version 2 -// to 3. // 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. @@ -89,6 +93,254 @@ void SetRedirects(const std::string& redirects, history::MostVisitedURL* url) { url->redirects.push_back(GURL(redirects_vector[i])); } +// 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 +// 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). +// +// Based on the thumbnail_database.cc recovery code, FAILED_SCOPER should +// dominate, followed distantly by FAILED_META, with few or no other failures. +enum RecoveryEventType { + // Database successfully recovered. + RECOVERY_EVENT_RECOVERED = 0, + + // Database successfully deprecated. + RECOVERY_EVENT_DEPRECATED, + + // Sqlite.RecoveryEvent can usually be used to get more detail about the + // specific failure (see sql/recovery.cc). + 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, + RECOVERY_EVENT_FAILED_COMMIT, + + // Track invariants resolved by FixThumbnailsTable(). + RECOVERY_EVENT_INVARIANT_RANK, + RECOVERY_EVENT_INVARIANT_REDIRECT, + RECOVERY_EVENT_INVARIANT_CONTIGUOUS, + + // Always keep this at the end. + RECOVERY_EVENT_MAX, +}; + +void RecordRecoveryEvent(RecoveryEventType recovery_event) { + UMA_HISTOGRAM_ENUMERATION("History.TopSitesRecovery", + recovery_event, RECOVERY_EVENT_MAX); +} + +// Most corruption comes down to atomic updates between pages being broken +// somehow. This can result in either missing data, or overlapping data, +// depending on the operation broken. This table has large rows, which will use +// overflow pages, so it is possible (though unlikely) that a chain could fit +// together and yield a row with errors. +void FixThumbnailsTable(sql::Connection* db) { + // Enforce invariant separating forced and non-forced thumbnails. + const char kFixRankSql[] = + "DELETE FROM thumbnails " + "WHERE (url_rank = -1 AND last_forced = 0) " + "OR (url_rank <> -1 AND last_forced <> 0)"; + ignore_result(db->Execute(kFixRankSql)); + if (db->GetLastChangeCount() > 0) + RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_RANK); + + // Enforce invariant that url is in its own redirects. + const char kFixRedirectsSql[] = + "DELETE FROM thumbnails " + "WHERE url <> substr(redirects, -length(url), length(url))"; + ignore_result(db->Execute(kFixRedirectsSql)); + if (db->GetLastChangeCount() > 0) + RecordRecoveryEvent(RECOVERY_EVENT_INVARIANT_REDIRECT); + + // Enforce invariant that url_rank>=0 forms a contiguous series. + // TODO(shess): I have not found an UPDATE+SUBSELECT method of managing this. + // It can be done with a temporary table and a subselect, but doing it + // manually is easier to follow. Another option would be to somehow integrate + // the renumbering into the table recovery code. + const char kByRankSql[] = + "SELECT url_rank, rowid FROM thumbnails WHERE url_rank <> -1 " + "ORDER BY url_rank"; + sql::Statement select_statement(db->GetUniqueStatement(kByRankSql)); + + const char kAdjustRankSql[] = + "UPDATE thumbnails SET url_rank = ? WHERE rowid = ?"; + sql::Statement update_statement(db->GetUniqueStatement(kAdjustRankSql)); + + // Update any rows where |next_rank| doesn't match |url_rank|. + int next_rank = 0; + bool adjusted = false; + while (select_statement.Step()) { + const int url_rank = select_statement.ColumnInt(0); + if (url_rank != next_rank) { + adjusted = true; + update_statement.Reset(true); + update_statement.BindInt(0, next_rank); + update_statement.BindInt64(1, select_statement.ColumnInt64(1)); + update_statement.Run(); + } + ++next_rank; + } + if (adjusted) + 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) { + // 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 original_size = 0; + base::GetFileSize(db_path, &original_size); + + scoped_ptr<sql::Recovery> recovery = sql::Recovery::Begin(db, db_path); + if (!recovery) { + 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(recovery.Pass()); + 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. + if (version <= kDeprecatedVersionNumber) { + sql::Recovery::Unrecoverable(recovery.Pass()); + 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) { + RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION); + sql::Recovery::Rollback(recovery.Pass()); + 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(recovery.Pass()); + 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(recovery.Pass()); + RecordRecoveryEvent(RECOVERY_EVENT_FAILED_SCHEMA_INIT); + return; + } + + // The |1| is because v2 [thumbnails] has one less column than v3 did. In the + // v2 case the column will get default values. + if (!recovery->AutoRecoverTable("thumbnails", 1, &thumbnails_recovered)) { + sql::Recovery::Rollback(recovery.Pass()); + RecordRecoveryEvent(RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS); + return; + } + + // TODO(shess): Inline this? + FixThumbnailsTable(recovery->db()); + + if (!sql::Recovery::Recovered(recovery.Pass())) { + // TODO(shess): Very unclear what this failure would actually mean, and what + // should be done. Add histograms to Recovered() implementation to get some + // insight. + 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 final_size = 0; + if (original_size > 0 && + base::GetFileSize(db_path, &final_size) && + final_size > 0) { + UMA_HISTOGRAM_PERCENTAGE("History.TopSitesRecoveredPercentage", + original_size * 100 / final_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", + thumbnails_recovered); + + RecordRecoveryEvent(RECOVERY_EVENT_RECOVERED); +} + +void DatabaseErrorCallback(sql::Connection* db, + const base::FilePath& db_path, + int extended_error, + sql::Statement* stmt) { + // TODO(shess): Assert that this is running on a safe thread. AFAICT, should + // 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); + } + + // TODO(shess): This database's error histograms look like: + // 84% SQLITE_CORRUPT, SQLITE_CANTOPEN, SQLITE_NOTADB + // 7% SQLITE_ERROR + // 6% SQLITE_IOERR variants + // 2% SQLITE_READONLY + // .4% SQLITE_FULL + // nominal SQLITE_TOBIG, SQLITE_AUTH, and SQLITE_BUSY. In the case of + // thumbnail_database.cc, as soon as the recovery code landed, SQLITE_IOERR + // shot to leadership. If the I/O error is system-level, there is probably no + // hope, but if it is restricted to something about the database file, it is + // possible that the recovery code could be brought to bear. In fact, it is + // possible that running recovery would be a reasonable default when errors + // are seen. + + // The default handling is to assert on debug and to ignore on release. + if (!sql::Connection::ShouldIgnoreSqliteError(extended_error)) + DLOG(FATAL) << db->GetErrorMessage(); +} + } // namespace namespace history { @@ -96,7 +348,7 @@ namespace history { // static const int TopSitesDatabase::kRankOfForcedURL = -1; -//static +// static const int TopSitesDatabase::kRankOfNonExistingURL = -2; TopSitesDatabase::TopSitesDatabase() { @@ -106,6 +358,22 @@ TopSitesDatabase::~TopSitesDatabase() { } bool TopSitesDatabase::Init(const base::FilePath& db_name) { + // Retry failed InitImpl() in case the recovery system fixed things. + // TODO(shess): Instrument to figure out if there are any persistent failure + // cases which do not resolve themselves. + const size_t kAttempts = 2; + + for (size_t i = 0; i < kAttempts; ++i) { + if (InitImpl(db_name)) + return true; + + meta_table_.Reset(); + db_.reset(); + } + return false; +} + +bool TopSitesDatabase::InitImpl(const base::FilePath& db_name) { const bool file_existed = base::PathExists(db_name); db_.reset(CreateDB(db_name)); @@ -451,6 +719,8 @@ sql::Connection* TopSitesDatabase::CreateDB(const base::FilePath& db_name) { scoped_ptr<sql::Connection> db(new sql::Connection()); // Settings copied from ThumbnailDatabase. db->set_histogram_tag("TopSites"); + db->set_error_callback(base::Bind(&DatabaseErrorCallback, + db.get(), db_name)); db->set_page_size(4096); db->set_cache_size(32); diff --git a/chrome/browser/history/top_sites_database.h b/chrome/browser/history/top_sites_database.h index af45121..e21ccb0 100644 --- a/chrome/browser/history/top_sites_database.h +++ b/chrome/browser/history/top_sites_database.h @@ -61,6 +61,9 @@ class TopSitesDatabase { FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version1); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version2); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Version3); + FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery1); + FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery2); + FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, Recovery3); FRIEND_TEST_ALL_PREFIXES(TopSitesDatabaseTest, AddRemoveEditThumbnails); // Rank of all URLs that are forced and therefore cannot be automatically @@ -90,6 +93,11 @@ class TopSitesDatabase { // Returns |url|'s current rank or kRankOfNonExistingURL if not present. int GetURLRank(const MostVisitedURL& url); + // Helper function to implement internals of Init(). This allows + // Init() to retry in case of failure, since some failures will + // invoke recovery code. + bool InitImpl(const base::FilePath& db_name); + sql::Connection* CreateDB(const base::FilePath& db_name); scoped_ptr<sql::Connection> db_; diff --git a/chrome/browser/history/top_sites_database_unittest.cc b/chrome/browser/history/top_sites_database_unittest.cc index 2c88ebb..b8a700a 100644 --- a/chrome/browser/history/top_sites_database_unittest.cc +++ b/chrome/browser/history/top_sites_database_unittest.cc @@ -2,23 +2,35 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <map> + #include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" #include "base/path_service.h" #include "base/strings/utf_string_conversions.h" +#include "chrome/browser/history/history_types.h" #include "chrome/browser/history/top_sites_database.h" #include "chrome/common/chrome_paths.h" #include "chrome/tools/profiles/thumbnail-inl.h" #include "sql/connection.h" -#include "sql/statement.h" +#include "sql/recovery.h" +#include "sql/test/scoped_error_ignorer.h" #include "sql/test/test_helpers.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/sqlite/sqlite3.h" +#include "url/gurl.h" namespace { // URL with url_rank 0 in golden files. const GURL kUrl0 = GURL("http://www.google.com/"); +// URL with url_rank 1 in golden files. +const GURL kUrl1 = GURL("http://www.google.com/chrome/intl/en/welcome.html"); + +// URL with url_rank 2 in golden files. +const GURL kUrl2 = GURL("https://chrome.google.com/webstore?hl=en"); + // Create the test database at |db_path| from the golden file at // |ascii_path| in the "History/" subdir of the test data dir. WARN_UNUSED_RESULT bool CreateDatabaseFromSQL(const base::FilePath &db_path, @@ -138,6 +150,208 @@ TEST_F(TopSitesDatabaseTest, Version3) { ASSERT_EQ(2u, thumbnails.size()); } +// Version 1 is deprecated, the resulting schema should be current, +// with no data. +TEST_F(TopSitesDatabaseTest, Recovery1) { + // Recovery module only supports some platforms at this time. + if (!sql::Recovery::FullRecoverySupported()) + return; + + // Create an example database. + EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v1.sql")); + + // Corrupt the database by adjusting the header size. + EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_)); + + // Database is unusable at the SQLite level. + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check")); + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } + + // Corruption should be detected and recovered during Init(). + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + VerifyTablesAndColumns(db.db_.get()); + VerifyDatabaseEmpty(db.db_.get()); + + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } +} + +TEST_F(TopSitesDatabaseTest, Recovery2) { + // Recovery module only supports some platforms at this time. + if (!sql::Recovery::FullRecoverySupported()) + return; + + // Create an example database. + EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v2.sql")); + + // Corrupt the database by adjusting the header. + EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_)); + + // Database is unusable at the SQLite level. + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check")); + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } + + // Corruption should be detected and recovered during Init(). After recovery, + // the Version2 checks should work. + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + VerifyTablesAndColumns(db.db_.get()); + + // Basic operational check. + MostVisitedURLList urls; + std::map<GURL, Images> 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(ignore_errors.CheckIgnoredErrors()); + } +} + +TEST_F(TopSitesDatabaseTest, Recovery3) { + // Recovery module only supports some platforms at this time. + if (!sql::Recovery::FullRecoverySupported()) + return; + + // Create an example database. + EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v3.sql")); + + // Corrupt the database by adjusting the header. + EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_)); + + // Database is unusable at the SQLite level. + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + EXPECT_FALSE(raw_db.IsSQLValid("PRAGMA integrity_check")); + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } + + // Corruption should be detected and recovered during Init(). + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + MostVisitedURLList urls; + std::map<GURL, Images> 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(ignore_errors.CheckIgnoredErrors()); + } + + // Double-check database integrity. + { + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // Corrupt the thumnails.url auto-index by deleting an element from the table + // but leaving it in the index. + const char kIndexName[] = "sqlite_autoindex_thumbnails_1"; + // TODO(shess): Refactor CorruptTableOrIndex() to make parameterized + // statements easy. + const char kDeleteSql[] = + "DELETE FROM thumbnails WHERE url = " + "'http://www.google.com/chrome/intl/en/welcome.html'"; + EXPECT_TRUE( + sql::test::CorruptTableOrIndex(file_name_, kIndexName, kDeleteSql)); + + // SQLite can operate on the database, but notices the corruption in integrity + // check. + { + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_NE("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // Open the database and access the corrupt index. + { + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + + // Data for kUrl1 was deleted, but the index entry remains, this will + // throw SQLITE_CORRUPT. The corruption handler will recover the database + // and poison the handle, so the outer call fails. + EXPECT_EQ(TopSitesDatabase::kRankOfNonExistingURL, + db.GetURLRank(MostVisitedURL(kUrl1, string16()))); + + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } + } + + // Check that the database is recovered at the SQLite level. + { + sql::Connection raw_db; + EXPECT_TRUE(raw_db.Open(file_name_)); + ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db)); + } + + // After recovery, the database accesses won't throw errors. The top-ranked + // item is removed, but the ranking was revised in post-processing. + { + TopSitesDatabase db; + ASSERT_TRUE(db.Init(file_name_)); + VerifyTablesAndColumns(db.db_.get()); + + EXPECT_EQ(TopSitesDatabase::kRankOfNonExistingURL, + db.GetURLRank(MostVisitedURL(kUrl1, string16()))); + + MostVisitedURLList urls; + std::map<GURL, Images> thumbnails; + db.GetPageThumbnails(&urls, &thumbnails); + ASSERT_EQ(2u, urls.size()); + ASSERT_EQ(2u, thumbnails.size()); + EXPECT_EQ(kUrl0, urls[0].url); // [0] because of url_rank. + EXPECT_EQ(kUrl2, urls[1].url); // [1] because of url_rank. + } +} + TEST_F(TopSitesDatabaseTest, AddRemoveEditThumbnails) { ASSERT_TRUE(CreateDatabaseFromSQL(file_name_, "TopSites.v3.sql")); diff --git a/sql/recovery.cc b/sql/recovery.cc index 89abbc9..42f1a9a 100644 --- a/sql/recovery.cc +++ b/sql/recovery.cc @@ -382,6 +382,12 @@ bool Recovery::AutoRecoverTable(const char* table_name, // SQLite's affinity detection is documented at: // http://www.sqlite.org/datatype3.html#affname // The gist of it is that CHAR, TEXT, and INT use substring matches. + // TODO(shess): It would be nice to unit test the type handling, + // but it is not obvious to me how to write a test which would + // fail appropriately when something was broken. It would have to + // somehow use data which would allow detecting the various type + // coercions which happen. If STRICT could be enabled, type + // mismatches could be detected by which rows are filtered. if (column_type.find("INT") != std::string::npos) { if (pk_column == 1) { rowid_ofs = create_column_decls.size(); @@ -393,12 +399,13 @@ bool Recovery::AutoRecoverTable(const char* table_name, column_decl += " TEXT"; } else if (column_type == "BLOB") { column_decl += " BLOB"; + } else if (column_type.find("DOUB") != std::string::npos) { + column_decl += " FLOAT"; } else { // TODO(shess): AFAICT, there remain: // - contains("CLOB") -> TEXT - // - contains("REAL") -> REAL - // - contains("FLOA") -> REAL - // - contains("DOUB") -> REAL + // - contains("REAL") -> FLOAT + // - contains("FLOA") -> FLOAT // - other -> "NUMERIC" // Just code those in as they come up. NOTREACHED() << " Unsupported type " << column_type; diff --git a/sql/recovery.h b/sql/recovery.h index 2475b0f..60fb674 100644 --- a/sql/recovery.h +++ b/sql/recovery.h @@ -27,10 +27,28 @@ namespace sql { // scoped_ptr<sql::Recovery> r = // sql::Recovery::Begin(orig_db, orig_db_path); // if (r) { -// if (r.db()->Execute(kCreateSchemaSql) && -// r.db()->Execute(kCopyDataFromOrigSql)) { -// sql::Recovery::Recovered(r.Pass()); +// // Create the schema to recover to. On failure, clear the +// // database. +// if (!r.db()->Execute(kCreateSchemaSql)) { +// sql::Recovery::Unrecoverable(r.Pass()); +// return; // } +// +// // Recover data in "mytable". +// size_t rows_recovered = 0; +// if (!r.AutoRecoverTable("mytable", 0, &rows_recovered)) { +// sql::Recovery::Unrecoverable(r.Pass()); +// return; +// } +// +// // Manually cleanup additional constraints. +// if (!r.db()->Execute(kCleanupSql)) { +// sql::Recovery::Unrecoverable(r.Pass()); +// return; +// } +// +// // Commit the recovered data to the original database file. +// sql::Recovery::Recovered(r.Pass()); // } // } // diff --git a/sql/recovery_unittest.cc b/sql/recovery_unittest.cc index e3c9738..ce3884b 100644 --- a/sql/recovery_unittest.cc +++ b/sql/recovery_unittest.cc @@ -2,17 +2,18 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include <string> + #include "base/bind.h" -#include "base/file_util.h" +#include "base/files/file_path.h" #include "base/files/scoped_temp_dir.h" -#include "base/logging.h" #include "base/strings/string_number_conversions.h" -#include "base/strings/stringprintf.h" #include "sql/connection.h" #include "sql/meta_table.h" #include "sql/recovery.h" #include "sql/statement.h" #include "sql/test/scoped_error_ignorer.h" +#include "sql/test/test_helpers.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/sqlite/sqlite3.h" @@ -57,51 +58,6 @@ std::string GetSchema(sql::Connection* db) { return ExecuteWithResults(db, kSql, "|", "\n"); } -#if !defined(USE_SYSTEM_SQLITE) -int GetPageSize(sql::Connection* db) { - sql::Statement s(db->GetUniqueStatement("PRAGMA page_size")); - EXPECT_TRUE(s.Step()); - return s.ColumnInt(0); -} - -// Get |name|'s root page number in the database. -int GetRootPage(sql::Connection* db, const char* name) { - const char kPageSql[] = "SELECT rootpage FROM sqlite_master WHERE name = ?"; - sql::Statement s(db->GetUniqueStatement(kPageSql)); - s.BindString(0, name); - EXPECT_TRUE(s.Step()); - return s.ColumnInt(0); -} - -// Helper to read a SQLite page into a buffer. |page_no| is 1-based -// per SQLite usage. -bool ReadPage(const base::FilePath& path, size_t page_no, - char* buf, size_t page_size) { - file_util::ScopedFILE file(base::OpenFile(path, "rb")); - if (!file.get()) - return false; - if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET)) - return false; - if (1u != fread(buf, page_size, 1, file.get())) - return false; - return true; -} - -// Helper to write a SQLite page into a buffer. |page_no| is 1-based -// per SQLite usage. -bool WritePage(const base::FilePath& path, size_t page_no, - const char* buf, size_t page_size) { - file_util::ScopedFILE file(base::OpenFile(path, "rb+")); - if (!file.get()) - return false; - if (0 != fseek(file.get(), (page_no - 1) * page_size, SEEK_SET)) - return false; - if (1u != fwrite(buf, page_size, 1, file.get())) - return false; - return true; -} -#endif // !defined(USE_SYSTEM_SQLITE) - class SQLRecoveryTest : public testing::Test { public: SQLRecoveryTest() {} @@ -296,24 +252,12 @@ TEST_F(SQLRecoveryTest, RecoverCorruptIndex) { ASSERT_TRUE(db().CommitTransaction()); } - - - // Capture the index's root page into |buf|. - int index_page = GetRootPage(&db(), "x_id"); - int page_size = GetPageSize(&db()); - scoped_ptr<char[]> buf(new char[page_size]); - ASSERT_TRUE(ReadPage(db_path(), index_page, buf.get(), page_size)); - - // Delete the row from the table and index. - ASSERT_TRUE(db().Execute("DELETE FROM x WHERE id = 0")); - - // Close to clear any cached data. db().Close(); - // Put the stale index page back. - ASSERT_TRUE(WritePage(db_path(), index_page, buf.get(), page_size)); - - // At this point, the index references a value not in the table. + // Delete a row from the table, while leaving the index entry which + // references it. + const char kDeleteSql[] = "DELETE FROM x WHERE id = 0"; + ASSERT_TRUE(sql::test::CorruptTableOrIndex(db_path(), "x_id", kDeleteSql)); ASSERT_TRUE(Reopen()); @@ -368,25 +312,12 @@ TEST_F(SQLRecoveryTest, RecoverCorruptTable) { ASSERT_TRUE(db().CommitTransaction()); } - - // Capture the table's root page into |buf|. - // Find the page the table is stored on. - const int table_page = GetRootPage(&db(), "x"); - const int page_size = GetPageSize(&db()); - scoped_ptr<char[]> buf(new char[page_size]); - ASSERT_TRUE(ReadPage(db_path(), table_page, buf.get(), page_size)); - - // Delete the row from the table and index. - ASSERT_TRUE(db().Execute("DELETE FROM x WHERE id = 0")); - - // Close to clear any cached data. db().Close(); - // Put the stale table page back. - ASSERT_TRUE(WritePage(db_path(), table_page, buf.get(), page_size)); + // Delete a row from the index while leaving a table entry. + const char kDeleteSql[] = "DELETE FROM x WHERE id = 0"; + ASSERT_TRUE(sql::test::CorruptTableOrIndex(db_path(), "x", kDeleteSql)); - // At this point, the table contains a value not referenced by the - // index. // TODO(shess): Figure out a query which causes SQLite to notice // this organically. Meanwhile, just handle it manually. diff --git a/sql/test/test_helpers.cc b/sql/test/test_helpers.cc index 6e12b04..8d56140 100644 --- a/sql/test/test_helpers.cc +++ b/sql/test/test_helpers.cc @@ -21,6 +21,26 @@ size_t CountSQLItemsOfType(sql::Connection* db, const char* type) { return s.ColumnInt(0); } +// Get page size for the database. +bool GetPageSize(sql::Connection* db, int* page_size) { + sql::Statement s(db->GetUniqueStatement("PRAGMA page_size")); + if (!s.Step()) + return false; + *page_size = s.ColumnInt(0); + return true; +} + +// Get |name|'s root page number in the database. +bool GetRootPage(sql::Connection* db, const char* name, int* page_number) { + const char kPageSql[] = "SELECT rootpage FROM sqlite_master WHERE name = ?"; + sql::Statement s(db->GetUniqueStatement(kPageSql)); + s.BindString(0, name); + if (!s.Step()) + return false; + *page_number = s.ColumnInt(0); + return true; +} + // Helper for reading a number from the SQLite header. // See net/base/big_endian.h. unsigned ReadBigEndian(unsigned char* buf, size_t bytes) { @@ -88,6 +108,70 @@ bool CorruptSizeInHeader(const base::FilePath& db_path) { return true; } +bool CorruptTableOrIndex(const base::FilePath& db_path, + const char* tree_name, + const char* update_sql) { + sql::Connection db; + if (!db.Open(db_path)) + return false; + + int page_size = 0; + if (!GetPageSize(&db, &page_size)) + return false; + + int page_number = 0; + if (!GetRootPage(&db, tree_name, &page_number)) + return false; + + // SQLite uses 1-based page numbering. + const long int page_ofs = (page_number - 1) * page_size; + scoped_ptr<char[]> page_buf(new char[page_size]); + + // Get the page into page_buf. + file_util::ScopedFILE file(base::OpenFile(db_path, "rb+")); + if (!file.get()) + return false; + if (0 != fseek(file.get(), page_ofs, SEEK_SET)) + return false; + if (1u != fread(page_buf.get(), page_size, 1, file.get())) + return false; + + // Require the page to be a leaf node. A multilevel tree would be + // very hard to restore correctly. + if (page_buf[0] != 0xD && page_buf[0] != 0xA) + return false; + + // The update has to work, and make changes. + if (!db.Execute(update_sql)) + return false; + if (db.GetLastChangeCount() == 0) + return false; + + // Ensure that the database is fully flushed. + db.Close(); + + // Check that the stored page actually changed. This catches usage + // errors where |update_sql| is not related to |tree_name|. + scoped_ptr<char[]> check_page_buf(new char[page_size]); + // The on-disk data should have changed. + if (0 != fflush(file.get())) + return false; + if (0 != fseek(file.get(), page_ofs, SEEK_SET)) + return false; + if (1u != fread(check_page_buf.get(), page_size, 1, file.get())) + return false; + if (!memcmp(check_page_buf.get(), page_buf.get(), page_size)) + return false; + + // Put the original page back. + if (0 != fseek(file.get(), page_ofs, SEEK_SET)) + return false; + if (1u != fwrite(page_buf.get(), page_size, 1, file.get())) + return false; + + return true; +} + size_t CountSQLTables(sql::Connection* db) { return CountSQLItemsOfType(db, "table"); } diff --git a/sql/test/test_helpers.h b/sql/test/test_helpers.h index b9d5e9b..d9dda22 100644 --- a/sql/test/test_helpers.h +++ b/sql/test/test_helpers.h @@ -9,6 +9,7 @@ #include "base/basictypes.h" #include "base/compiler_specific.h" +#include "base/files/file_path.h" // Collection of test-only convenience functions. @@ -33,6 +34,26 @@ namespace test { // Returns false if any error occurs accessing the file. bool CorruptSizeInHeader(const base::FilePath& db_path) WARN_UNUSED_RESULT; +// Frequently corruption is a result of failure to atomically update +// pages in different structures. For instance, if an index update +// takes effect but the corresponding table update does not. This +// helper restores the prior version of a b-tree root after running an +// update which changed that b-tree. The named b-tree must exist and +// must be a leaf node (either index or table). Returns true if the +// on-disk file is successfully modified, and the restored page +// differs from the updated page. +// +// The resulting database should be possible to open, and many +// statements should work. SQLITE_CORRUPT will be thrown if a query +// through the index finds the row missing in the table. +// +// TODO(shess): It would be very helpful to allow a parameter to the +// sql statement. Perhaps a version with a string parameter would be +// sufficient, given affinity rules? +bool CorruptTableOrIndex(const base::FilePath& db_path, + const char* tree_name, + const char* update_sql) WARN_UNUSED_RESULT; + // Return the number of tables in sqlite_master. size_t CountSQLTables(sql::Connection* db) WARN_UNUSED_RESULT; diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 9b52f02..499a10e 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -5424,6 +5424,31 @@ other types of suffix sets. </summary> </histogram> +<histogram name="History.TopSitesRecoveredPercentage" units="%"> + <summary> + Size of the recovered TopSites database relative to the original corrupt + database. Recovery is VACUUM-like, so the resulting database should always + be smaller. Substantial 100% results would indicate empty databases being + recovered, substantial low% results would indicate very little data being + recovered. + </summary> +</histogram> + +<histogram name="History.TopSitesRecoveredRowsThumbnails"> + <summary> + Rows recovered from [thumbnails] table in TopSites recovery. + </summary> +</histogram> + +<histogram name="History.TopSitesRecovery" enum="HistoryTopSitesRecoveryEnum"> + <summary> + The TopSites recovery code is written conservatively, with successful + recovery committed and any failure leading to rollback. This tracks the + outcomes to determine which cases are high-frequency enough to warrant + adding additional code to handle them (versus simply deleting the data). + </summary> +</histogram> + <histogram name="History.TopSitesVisitsByRank" units="rank"> <summary> Page visits to each of a user's top 50 sites. Visits to all other sites go @@ -24821,6 +24846,46 @@ other types of suffix sets. </int> </enum> +<enum name="HistoryTopSitesRecoveryEnum" type="int"> + <summary>Error states noted in top_sites_database.cc recovery code.</summary> + <int value="0" label="RECOVERY_EVENT_RECOVERED">Successful recovery.</int> + <int value="1" label="RECOVERY_EVENT_DEPRECATED"> + Recovery found deprecated version and razed. + </int> + <int value="2" label="RECOVERY_EVENT_FAILED_SCOPER"> + sql::Recovery failed init. + </int> + <int value="3" label="RECOVERY_EVENT_FAILED_META_VERSION"> + Failed sql::Recovery::SetupMeta() or GetMetaVersionNumber(). + </int> + <int value="4" label="RECOVERY_EVENT_FAILED_META_WRONG_VERSION"> + Recovery meta table has an unexpected version. + </int> + <int value="5" label="RECOVERY_EVENT_FAILED_META_INIT"> + Failed sql::MetaTable::Init(). + </int> + <int value="6" label="RECOVERY_EVENT_FAILED_SCHEMA_INIT"> + Failed to init target schema. + </int> + <int value="7" label="RECOVERY_EVENT_FAILED_AUTORECOVER_THUMBNAILS"> + Failed recovery on thumbnails table. + </int> + <int value="8" label="RECOVERY_EVENT_FAILED_COMMIT"> + Failure from sql::Recovery::Recovered(). + </int> + <int value="9" label="RECOVERY_EVENT_INVARIANT_RANK"> + Rows were deleted because |url_rank| and |last_forced| didn't agree. Does + not prevent recovery. + </int> + <int value="10" label="RECOVERY_EVENT_INVARIANT_REDIRECT"> + Rows were deleted because |redirects| did not contain |url|. Does not + prevent recovery. + </int> + <int value="11" label="RECOVERY_EVENT_INVARIANT_CONTIGUOUS"> + |url_rank| was renumbered due to missing rows. Does not prevent recovery. + </int> +</enum> + <enum name="HttpAuthCount" type="int"> <int value="0" label="Basic Start"/> <int value="1" label="Basic Reject"/> |