diff options
author | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-07-17 04:45:13 +0000 |
---|---|---|
committer | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-07-17 04:45:13 +0000 |
commit | fed734a145c6defbf1063b3b9a497aee88db720d (patch) | |
tree | 3f8fe3a0e49f57a42c1bb514e4f7c145c6c5195c /sql | |
parent | 5b7355bcdf73ef995001d0cf8fbf2385012a7a52 (diff) | |
download | chromium_src-fed734a145c6defbf1063b3b9a497aee88db720d.zip chromium_src-fed734a145c6defbf1063b3b9a497aee88db720d.tar.gz chromium_src-fed734a145c6defbf1063b3b9a497aee88db720d.tar.bz2 |
[sql] Retry Open() if error handler fixed things.
Since the caller cannot have existing database-derived state before
the database is opened, Open() can be safely retried after the error
handler has razed the database.
BUG=109482
Review URL: https://chromiumcodereview.appspot.com/18641004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@211936 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sql')
-rw-r--r-- | sql/connection.cc | 19 | ||||
-rw-r--r-- | sql/connection.h | 9 | ||||
-rw-r--r-- | sql/connection_unittest.cc | 31 |
3 files changed, 41 insertions, 18 deletions
diff --git a/sql/connection.cc b/sql/connection.cc index 5f76a7f..95d09c2 100644 --- a/sql/connection.cc +++ b/sql/connection.cc @@ -194,15 +194,15 @@ bool Connection::Open(const base::FilePath& path) { } #if defined(OS_WIN) - return OpenInternal(WideToUTF8(path.value())); + return OpenInternal(WideToUTF8(path.value()), RETRY_ON_POISON); #elif defined(OS_POSIX) - return OpenInternal(path.value()); + return OpenInternal(path.value(), RETRY_ON_POISON); #endif } bool Connection::OpenInMemory() { in_memory_ = true; - return OpenInternal(":memory:"); + return OpenInternal(":memory:", NO_RETRY); } void Connection::CloseInternal(bool forced) { @@ -238,8 +238,8 @@ void Connection::CloseInternal(bool forced) { AssertIOAllowed(); // TODO(shess): Histogram for failure. sqlite3_close(db_); - db_ = NULL; } + db_ = NULL; } void Connection::Close() { @@ -699,7 +699,8 @@ const char* Connection::GetErrorMessage() const { return sqlite3_errmsg(db_); } -bool Connection::OpenInternal(const std::string& file_name) { +bool Connection::OpenInternal(const std::string& file_name, + Connection::Retry retry_flag) { AssertIOAllowed(); if (db_) { @@ -723,8 +724,11 @@ bool Connection::OpenInternal(const std::string& file_name) { UMA_HISTOGRAM_ENUMERATION("Sqlite.OpenFailure", err & 0xff, 50); OnSqliteError(err, NULL); + bool was_poisoned = poisoned_; Close(); - db_ = NULL; + + if (was_poisoned && retry_flag == RETRY_ON_POISON) + return OpenInternal(file_name, NO_RETRY); return false; } @@ -806,7 +810,10 @@ bool Connection::OpenInternal(const std::string& file_name) { } if (!ExecuteWithTimeout("PRAGMA secure_delete=ON", kBusyTimeout)) { + bool was_poisoned = poisoned_; Close(); + if (was_poisoned && retry_flag == RETRY_ON_POISON) + return OpenInternal(file_name, NO_RETRY); return false; } diff --git a/sql/connection.h b/sql/connection.h index 8155a8f..1c4d72c 100644 --- a/sql/connection.h +++ b/sql/connection.h @@ -360,7 +360,14 @@ class SQL_EXPORT Connection { // Internal initialize function used by both Init and InitInMemory. The file // name is always 8 bits since we want to use the 8-bit version of // sqlite3_open. The string can also be sqlite's special ":memory:" string. - bool OpenInternal(const std::string& file_name); + // + // |retry_flag| controls retrying the open if the error callback + // addressed errors using RazeAndClose(). + enum Retry { + NO_RETRY = 0, + RETRY_ON_POISON + }; + bool OpenInternal(const std::string& file_name, Retry retry_flag); // Internal close function used by Close() and RazeAndClose(). // |forced| indicates that orderly-shutdown checks should not apply. diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc index fc430ac..8cf32fb 100644 --- a/sql/connection_unittest.cc +++ b/sql/connection_unittest.cc @@ -491,7 +491,8 @@ TEST_F(SQLConnectionTest, RazeNOTADB2) { // Test that a callback from Open() can raze the database. This is // essential for cases where the Open() can fail entirely, so the -// Raze() cannot happen later. +// 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 @@ -499,7 +500,7 @@ TEST_F(SQLConnectionTest, RazeNOTADB2) { // 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, RazeOpenCallback) { +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())); @@ -519,19 +520,27 @@ TEST_F(SQLConnectionTest, RazeOpenCallback) { 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. + { + sql::ScopedErrorIgnorer ignore_errors; + ignore_errors.IgnoreError(SQLITE_CORRUPT); + ASSERT_TRUE(db().Open(db_path())); + ASSERT_FALSE(db().Execute("PRAGMA auto_vacuum")); + db().Close(); + ASSERT_TRUE(ignore_errors.CheckIgnoredErrors()); + } + db().set_error_callback(base::Bind(&SQLConnectionTest::RazeErrorCallback, base::Unretained(this), SQLITE_CORRUPT)); - // Open() will see SQLITE_CORRUPT due to size mismatch when - // attempting to run a pragma, and the callback will RazeAndClose(). - // Later statements will fail, including the final secure_delete, - // which will fail the Open() itself. - ASSERT_FALSE(db().Open(db_path())); - db().Close(); - - // Now empty, the open should succeed with an empty database. - EXPECT_TRUE(db().Open(db_path())); + // When the PRAGMA calls in Open() raise SQLITE_CORRUPT, the error + // callback will call RazeAndClose(). Open() will then fail and be + // retried. The second Open() on the empty database will succeed + // cleanly. + ASSERT_TRUE(db().Open(db_path())); + ASSERT_TRUE(db().Execute("PRAGMA auto_vacuum")); EXPECT_EQ(0, SqliteMasterCount(&db())); } |