Skip to content

Commit

Permalink
Revert 235492 "[sql] Recover Favicons v5 databases, with more re..."
Browse files Browse the repository at this point in the history 
Speculative revert to find the cause for Mac size regression
(will revert this revert later)

> [sql] Recover Favicons v5 databases, with more recovery automation.
> 
> An entirely automated recovery system runs afoul of questions about
> whether the corrupt database's schema can be trusted.
> sql::Recovery::AutoRecoverTable() uses a schema created by the caller
> to construct the recovery virtual table and then copies the data over.
> 
> sql::Recovery::SetupMeta() and GetMetaVersionNumber() simplify
> accessing meta-table info in the corrupt database.
> 
> sql::test::IntegrityCheck() and CorruptSizeInHeader() helpers to
> simplify common testing operations.
> 
> Rewrite ThumbnailDatabase v6 and v7 recovery code and tests using
> these changes, and add a v5 recovery path.  Additionally handle
> deprecated versions.
> 
> BUG=240396,109482
> 
> Review URL: https://codereview.chromium.org/50493012

TBR=shess@chromium.org

Review URL: https://codereview.chromium.org/74933002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235595 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
kinuko@chromium.org committed Nov 18, 2013
1 parent e650b46 commit 5f65959
Show file tree
Hide file tree
Showing 9 changed files with 247 additions and 910 deletions.
354 changes: 176 additions & 178 deletions chrome/browser/history/thumbnail_database.cc
View file

Large diffs are not rendered by default.

111 changes: 50 additions & 61 deletions chrome/browser/history/thumbnail_database_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,12 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
{
sql::Statement statement(
raw_db.GetUniqueStatement("PRAGMA integrity_check"));
EXPECT_TRUE(statement.Step());
ASSERT_EQ("ok", statement.ColumnString(0));
}

const char kIndexName[] = "icon_mapping_page_url_idx";
const int idx_root_page = GetRootPage(&raw_db, kIndexName);
Expand All @@ -813,7 +818,10 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
ASSERT_NE("ok", sql::test::IntegrityCheck(&raw_db));
sql::Statement statement(
raw_db.GetUniqueStatement("PRAGMA integrity_check"));
EXPECT_TRUE(statement.Step());
ASSERT_NE("ok", statement.ColumnString(0));
}

// Open the database and access the corrupt index.
Expand All @@ -836,7 +844,10 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
sql::Statement statement(
raw_db.GetUniqueStatement("PRAGMA integrity_check"));
EXPECT_TRUE(statement.Step());
EXPECT_EQ("ok", statement.ColumnString(0));

// Check that the expected tables exist.
VerifyTablesAndColumns(&raw_db);
Expand All @@ -856,8 +867,21 @@ TEST_F(ThumbnailDatabaseTest, Recovery) {
kIconUrl1, kLargeSize, sizeof(kBlob1), kBlob1));
}

// Corrupt the database again by adjusting the header.
EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_));
// Corrupt the database again by making the actual file shorter than
// the header expects.
{
int64 db_size = 0;
EXPECT_TRUE(file_util::GetFileSize(file_name_, &db_size));
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
EXPECT_TRUE(raw_db.Execute("CREATE TABLE t(x)"));
}
file_util::ScopedFILE file(file_util::OpenFile(file_name_, "rb+"));
ASSERT_TRUE(file.get() != NULL);
EXPECT_EQ(0, fseek(file.get(), static_cast<long>(db_size), SEEK_SET));
EXPECT_TRUE(file_util::TruncateFile(file.get()));
}

// Database is unusable at the SQLite level.
{
Expand Down Expand Up @@ -894,10 +918,23 @@ TEST_F(ThumbnailDatabaseTest, Recovery6) {
// (which would upgrade it).
EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v6.sql"));

// Corrupt the database again by adjusting the header. This form of
// corruption will cause immediate failures during Open(), before
// the migration code runs, so the version-6 recovery will occur.
EXPECT_TRUE(sql::test::CorruptSizeInHeader(file_name_));
// Corrupt the database by making the actual file shorter than the
// SQLite header expects. This form of corruption will cause
// immediate failures during Open(), before the migration code runs,
// so the version-6 recovery will occur.
{
int64 db_size = 0;
EXPECT_TRUE(file_util::GetFileSize(file_name_, &db_size));
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
EXPECT_TRUE(raw_db.Execute("CREATE TABLE t(x)"));
}
file_util::ScopedFILE file(file_util::OpenFile(file_name_, "rb+"));
ASSERT_TRUE(file.get() != NULL);
EXPECT_EQ(0, fseek(file.get(), static_cast<long>(db_size), SEEK_SET));
EXPECT_TRUE(file_util::TruncateFile(file.get()));
}

// Database is unusable at the SQLite level.
{
Expand Down Expand Up @@ -933,58 +970,10 @@ TEST_F(ThumbnailDatabaseTest, Recovery6) {
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));

// Check that the expected tables exist.
VerifyTablesAndColumns(&raw_db);
}
}

TEST_F(ThumbnailDatabaseTest, Recovery5) {
// Create an example database without loading into ThumbnailDatabase
// (which would upgrade it).
EXPECT_TRUE(CreateDatabaseFromSQL(file_name_, "Favicons.v5.sql"));

// Corrupt the database again by adjusting the header. This form of
// corruption will cause immediate failures during Open(), before
// the migration code runs, so the version-5 recovery will occur.
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());
}

// Database should be recovered during open.
{
sql::ScopedErrorIgnorer ignore_errors;
ignore_errors.IgnoreError(SQLITE_CORRUPT);
ThumbnailDatabase db;
ASSERT_EQ(sql::INIT_OK, db.Init(file_name_));

// Test that some data is present, copied from
// ThumbnailDatabaseTest.Version5 .
EXPECT_TRUE(
CheckPageHasIcon(&db, kPageUrl3, chrome::FAVICON,
kIconUrl1, gfx::Size(), sizeof(kBlob1), kBlob1));
EXPECT_TRUE(
CheckPageHasIcon(&db, kPageUrl3, chrome::TOUCH_ICON,
kIconUrl3, gfx::Size(), sizeof(kBlob2), kBlob2));

ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
}

// Check that the database is recovered at a SQLite level, and that
// the current schema is in place.
{
sql::Connection raw_db;
EXPECT_TRUE(raw_db.Open(file_name_));
ASSERT_EQ("ok", sql::test::IntegrityCheck(&raw_db));
sql::Statement statement(
raw_db.GetUniqueStatement("PRAGMA integrity_check"));
EXPECT_TRUE(statement.Step());
EXPECT_EQ("ok", statement.ColumnString(0));

// Check that the expected tables exist.
VerifyTablesAndColumns(&raw_db);
Expand Down
23 changes: 20 additions & 3 deletions sql/connection_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include "sql/statement.h"
#include "sql/test/error_callback_support.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"

Expand Down Expand Up @@ -503,14 +502,32 @@ TEST_F(SQLConnectionTest, RazeNOTADB2) {
// essential for cases where the Open() can fail entirely, so the
// Raze() cannot happen later. Additionally test that when the
// callback does this during Open(), the open is retried and succeeds.
//
// Most corruptions seen in the wild seem to happen when two pages in
// the database were not written transactionally (the transaction
// changed both, but one wasn't successfully written for some reason).
// A special case of that is when the header indicates that the
// database contains more pages than are in the file. This breaks
// things at a very basic level, verify that Raze() can handle it.
TEST_F(SQLConnectionTest, RazeCallbackReopen) {
const char* kCreateSql = "CREATE TABLE foo (id INTEGER PRIMARY KEY, value)";
ASSERT_TRUE(db().Execute(kCreateSql));
ASSERT_EQ(1, SqliteMasterCount(&db()));
int page_size = 0;
{
sql::Statement s(db().GetUniqueStatement("PRAGMA page_size"));
ASSERT_TRUE(s.Step());
page_size = s.ColumnInt(0);
}
db().Close();

// Corrupt the database so that nothing works, including PRAGMAs.
ASSERT_TRUE(sql::test::CorruptSizeInHeader(db_path()));
// Trim a single page from the end of the file.
{
file_util::ScopedFILE file(file_util::OpenFile(db_path(), "rb+"));
ASSERT_TRUE(file.get() != NULL);
ASSERT_EQ(0, fseek(file.get(), -page_size, SEEK_END));
ASSERT_TRUE(file_util::TruncateFile(file.get()));
}

// Open() will succeed, even though the PRAGMA calls within will
// fail with SQLITE_CORRUPT, as will this PRAGMA.
Expand Down
174 changes: 0 additions & 174 deletions sql/recovery.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,9 @@
#include "sql/recovery.h"

#include "base/files/file_path.h"
#include "base/format_macros.h"
#include "base/logging.h"
#include "base/metrics/sparse_histogram.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "sql/connection.h"
#include "sql/statement.h"
#include "third_party/sqlite/sqlite3.h"

namespace sql {
Expand Down Expand Up @@ -237,174 +233,4 @@ void Recovery::Shutdown(Recovery::Disposition raze) {
db_ = NULL;
}

bool Recovery::AutoRecoverTable(const char* table_name,
size_t extend_columns,
size_t* rows_recovered) {
// Query the info for the recovered table in database [main].
std::string query(
base::StringPrintf("PRAGMA main.table_info(%s)", table_name));
Statement s(db()->GetUniqueStatement(query.c_str()));

// The columns of the recover virtual table.
std::vector<std::string> create_column_decls;

// The columns to select from the recover virtual table when copying
// to the recovered table.
std::vector<std::string> insert_columns;

// If PRIMARY KEY is a single INTEGER column, then it is an alias
// for ROWID. The primary key can be compound, so this can only be
// determined after processing all column data and tracking what is
// seen. |pk_column_count| counts the columns in the primary key.
// |rowid_decl| stores the ROWID version of the last INTEGER column
// seen, which is at |rowid_ofs| in |create_column_decls|.
size_t pk_column_count = 0;
size_t rowid_ofs; // Only valid if rowid_decl is set.
std::string rowid_decl; // ROWID version of column |rowid_ofs|.

while (s.Step()) {
const std::string column_name(s.ColumnString(1));
const std::string column_type(s.ColumnString(2));
const bool not_null = s.ColumnBool(3);
const int default_type = s.ColumnType(4);
const bool default_is_null = (default_type == COLUMN_TYPE_NULL);
const int pk_column = s.ColumnInt(5);

if (pk_column > 0) {
// TODO(shess): http://www.sqlite.org/pragma.html#pragma_table_info
// documents column 5 as the index of the column in the primary key
// (zero for not in primary key). I find that it is always 1 for
// columns in the primary key. Since this code is very dependent on
// that pragma, review if the implementation changes.
DCHECK_EQ(pk_column, 1);
++pk_column_count;
}

// Construct column declaration as "name type [optional constraint]".
std::string column_decl = column_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.
if (column_type.find("INT") != std::string::npos) {
if (pk_column == 1) {
rowid_ofs = create_column_decls.size();
rowid_decl = column_name + " ROWID";
}
column_decl += " INTEGER";
} else if (column_type.find("CHAR") != std::string::npos ||
column_type.find("TEXT") != std::string::npos) {
column_decl += " TEXT";
} else if (column_type == "BLOB") {
column_decl += " BLOB";
} else {
// TODO(shess): AFAICT, there remain:
// - contains("CLOB") -> TEXT
// - contains("REAL") -> REAL
// - contains("FLOA") -> REAL
// - contains("DOUB") -> REAL
// - other -> "NUMERIC"
// Just code those in as they come up.
NOTREACHED() << " Unsupported type " << column_type;
return false;
}

// If column has constraint "NOT NULL", then inserting NULL into
// that column will fail. If the column has a non-NULL DEFAULT
// specified, the INSERT will handle it (see below). If the
// DEFAULT is also NULL, the row must be filtered out.
// TODO(shess): The above scenario applies to INSERT OR REPLACE,
// whereas INSERT OR IGNORE drops such rows.
// http://www.sqlite.org/lang_conflict.html
if (not_null && default_is_null)
column_decl += " NOT NULL";

create_column_decls.push_back(column_decl);

// Per the NOTE in the header file, convert NULL values to the
// DEFAULT. All columns could be IFNULL(column_name,default), but
// the NULL case would require special handling either way.
if (default_is_null) {
insert_columns.push_back(column_name);
} else {
// The default value appears to be pre-quoted, as if it is
// literally from the sqlite_master CREATE statement.
std::string default_value = s.ColumnString(4);
insert_columns.push_back(base::StringPrintf(
"IFNULL(%s,%s)", column_name.c_str(), default_value.c_str()));
}
}

// Receiving no column information implies that the table doesn't exist.
if (create_column_decls.empty())
return false;

// If the PRIMARY KEY was a single INTEGER column, convert it to ROWID.
if (pk_column_count == 1 && !rowid_decl.empty())
create_column_decls[rowid_ofs] = rowid_decl;

// Additional columns accept anything.
// TODO(shess): ignoreN isn't well namespaced. But it will fail to
// execute in case of conflicts.
for (size_t i = 0; i < extend_columns; ++i) {
create_column_decls.push_back(
base::StringPrintf("ignore%" PRIuS " ANY", i));
}

std::string recover_create(base::StringPrintf(
"CREATE VIRTUAL TABLE temp.recover_%s USING recover(corrupt.%s, %s)",
table_name,
table_name,
JoinString(create_column_decls, ',').c_str()));

std::string recover_insert(base::StringPrintf(
"INSERT OR REPLACE INTO main.%s SELECT %s FROM temp.recover_%s",
table_name,
JoinString(insert_columns, ',').c_str(),
table_name));

std::string recover_drop(base::StringPrintf(
"DROP TABLE temp.recover_%s", table_name));

if (!db()->Execute(recover_create.c_str()))
return false;

if (!db()->Execute(recover_insert.c_str())) {
ignore_result(db()->Execute(recover_drop.c_str()));
return false;
}

*rows_recovered = db()->GetLastChangeCount();

// TODO(shess): Is leaving the recover table around a breaker?
return db()->Execute(recover_drop.c_str());
}

bool Recovery::SetupMeta() {
const char kCreateSql[] =
"CREATE VIRTUAL TABLE temp.recover_meta USING recover"
"("
"corrupt.meta,"
"key TEXT NOT NULL,"
"value ANY" // Whatever is stored.
")";
return db()->Execute(kCreateSql);
}

bool Recovery::GetMetaVersionNumber(int* version) {
DCHECK(version);
// TODO(shess): DCHECK(db()->DoesTableExist("temp.recover_meta"));
// Unfortunately, DoesTableExist() queries sqlite_master, not
// sqlite_temp_master.

const char kVersionSql[] =
"SELECT value FROM temp.recover_meta WHERE key = 'version'";
sql::Statement recovery_version(db()->GetUniqueStatement(kVersionSql));
if (!recovery_version.Step())
return false;

*version = recovery_version.ColumnInt(0);
return true;
}

} // namespace sql
Loading

0 comments on commit 5f65959

Please sign in to comment.