[sql] Framework for allowing tests to handle errors.

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index 09d8195df502d440430f8124538733d8a4c776e9..6985e7dac6dac373248c6ac5049530dd993da697 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -64,6 +64,38 @@ class ScopedWritableSchema {
   sqlite3* db_;
 };
 
+// Helper to wrap the sqlite3_backup_*() step of Raze().  Return
+// SQLite error code from running the backup step.
+int BackupDatabase(sqlite3* src, sqlite3* dst, const char* db_name) {
+  sqlite3_backup* backup = sqlite3_backup_init(dst, db_name, src, db_name);
+  if (!backup) {
+    // Since this call only sets things up, this indicates a gross
+    // error in SQLite.
+    DLOG(FATAL) << "Unable to start sqlite3_backup().";
+    return SQLITE_ABORT;
+  }
+
+  // -1 backs up the entire database.
+  int rc = sqlite3_backup_step(backup, -1);
+  int pages = sqlite3_backup_pagecount(backup);
+  sqlite3_backup_finish(backup);
+
+  // Exactly one page should have been backed up.  If this breaks,
+  // check this function to make sure assumptions aren't being broken.
+  if (rc == SQLITE_DONE)
+    DCHECK_EQ(pages, 1);
+
+  return rc;
+}
+
+// Hooks for ScopedErrorIgnorer.
+sql::ScopedErrorIgnorer* g_current_ignorer = NULL;
+bool ShouldIgnoreError(int error) {
+  if (g_current_ignorer)
+    return g_current_ignorer->ShouldIgnoreError(error);
+  return false;
+}
+
 }  // namespace
 
 namespace sql {
@@ -295,33 +327,51 @@ bool Connection::Raze() {
   // page_size" can be used to query such a database.
   ScopedWritableSchema writable_schema(db_);
 
-  sqlite3_backup* backup = sqlite3_backup_init(db_, "main",
-                                               null_db.db_, "main");
-  if (!backup) {
-    DLOG(FATAL) << "Unable to start sqlite3_backup().";
-    return false;
-  }
-
-  // -1 backs up the entire database.
-  int rc = sqlite3_backup_step(backup, -1);
-  int pages = sqlite3_backup_pagecount(backup);
-  sqlite3_backup_finish(backup);
+  const char* kMain = "main";
+  int rc = BackupDatabase(null_db.db_, db_, kMain);
 
   // The destination database was locked.
   if (rc == SQLITE_BUSY) {
     return false;
   }
 
+  // SQLITE_NOTADB can happen if page 1 exists but is not formatted

[erikwright (departed), 2013/06/11 18:54:19]

Can/should this block be a separate CL?

[Scott Hess - ex-Googler, 2013/06/12 22:42:46]

It's in here so that the example tests can pass.  But I can put together a more
targeted test and get this out of here.

+  // correctly.  SQLITE_IOERR_SHORT_READ can happen if the database
+  // isn't even big enough for one page.  Either way, reach in and
+  // truncate it before trying again.
+  // TODO(shess): Maybe it would be worthwhile to just truncate from
+  // the get-go?
+  if (rc == SQLITE_NOTADB || rc == SQLITE_IOERR_SHORT_READ) {
+    sqlite3_file* file = NULL;
+    rc = sqlite3_file_control(db_, "main", SQLITE_FCNTL_FILE_POINTER, &file);
+    if (rc != SQLITE_OK) {
+      DLOG(FATAL) << "Failure getting file handle.";
+      return false;
+    } else if (!file) {
+      DLOG(FATAL) << "File handle is empty.";
+      return false;
+    }
+
+    rc = file->pMethods->xTruncate(file, 0);
+    if (rc != SQLITE_OK) {
+      DLOG(FATAL) << "Failed to truncate file.";
+      return false;
+    }
+
+    rc = BackupDatabase(null_db.db_, db_, kMain);
+
+    if (rc != SQLITE_DONE) {
+      DLOG(FATAL) << "Failed retrying Raze().";
+    }
+  }
+
   // The entire database should have been backed up.
   if (rc != SQLITE_DONE) {
+    // TODO(shess): Figure out which other cases can happen.
     DLOG(FATAL) << "Unable to copy entire null database.";
     return false;
   }
 
-  // Exactly one page should have been backed up.  If this breaks,
-  // check this function to make sure assumptions aren't being broken.
-  DCHECK_EQ(pages, 1);
-
   return true;
 }
 
@@ -439,7 +489,8 @@ bool Connection::Execute(const char* sql) {
 
   // This needs to be a FATAL log because the error case of arriving here is
   // that there's a malformed SQL statement. This can arise in development if
-  // a change alters the schema but not all queries adjust.
+  // a change alters the schema but not all queries adjust.  This can happen
+  // in production if the schema is corrupted.
   if (error == SQLITE_ERROR)
     DLOG(FATAL) << "SQL Error in " << sql << ", " << GetErrorMessage();
   return error == SQLITE_OK;
@@ -657,11 +708,10 @@ bool Connection::OpenInternal(const std::string& file_name) {
   // assumptions about who might change things in the database.
   // http://crbug.com/56559
   if (exclusive_locking_) {
-    // TODO(shess): This should probably be a full CHECK().  Code
-    // which requests exclusive locking but doesn't get it is almost
-    // certain to be ill-tested.
-    if (!Execute("PRAGMA locking_mode=EXCLUSIVE"))
-      DLOG(FATAL) << "Could not set locking mode: " << GetErrorMessage();
+    // TODO(shess): This should probably be a failure.  Code which
+    // requests exclusive locking but doesn't get it is almost certain
+    // to be ill-tested.
+    ignore_result(Execute("PRAGMA locking_mode=EXCLUSIVE"));
   }
 
   // http://www.sqlite.org/pragma.html#pragma_journal_mode
@@ -686,19 +736,16 @@ bool Connection::OpenInternal(const std::string& file_name) {
     DCHECK_LE(page_size_, kSqliteMaxPageSize);
     const std::string sql =
         base::StringPrintf("PRAGMA page_size=%d", page_size_);
-    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      DLOG(FATAL) << "Could not set page size: " << GetErrorMessage();
+    ignore_result(ExecuteWithTimeout(sql.c_str(), kBusyTimeout));
   }
 
   if (cache_size_ != 0) {
     const std::string sql =
         base::StringPrintf("PRAGMA cache_size=%d", cache_size_);
-    if (!ExecuteWithTimeout(sql.c_str(), kBusyTimeout))
-      DLOG(FATAL) << "Could not set cache size: " << GetErrorMessage();
+    ignore_result(ExecuteWithTimeout(sql.c_str(), kBusyTimeout));
   }
 
   if (!ExecuteWithTimeout("PRAGMA secure_delete=ON", kBusyTimeout)) {
-    DLOG(FATAL) << "Could not enable secure_delete: " << GetErrorMessage();
     Close();
     return false;
   }
@@ -763,7 +810,8 @@ int Connection::OnSqliteError(int err, sql::Statement *stmt) {
     return error_delegate_->OnError(err, this, stmt);
 
   // The default handling is to assert on debug and to ignore on release.
-  DLOG(FATAL) << GetErrorMessage();
+  if (!ShouldIgnoreError(err))
+    DLOG(FATAL) << GetErrorMessage();
   return err;
 }
 
@@ -785,4 +833,53 @@ bool Connection::IntegrityCheck(std::vector<std::string>* messages) {
   return stmt.Succeeded();
 }
 
+ScopedErrorIgnorer::ScopedErrorIgnorer()
+    : checked_(false),
+      next_(g_current_ignorer) {
+  g_current_ignorer = this;
+}
+
+ScopedErrorIgnorer::~ScopedErrorIgnorer() {
+  CHECK(checked_);

[erikwright (departed), 2013/06/11 18:54:19]

This should probably be ADD_FAILURE instead of CHECK.

As it stands now, it will cause a crash, obscuring actual test failures. If you
use ADD_FAILURE it will just add an additional failure that can be ignored.

That also seems to imply that ScopedErrorIgnorer should probably be only defined
in test code, and you should provide some way (maybe a pure-virtual or
something) to be able to access its state from production code but carry on
without needing its implementation if there doesn't happen to be an active
instance of it.

[Scott Hess - ex-Googler, 2013/06/12 22:42:46]

Basically I hate all this :-).

OK, so would it make sense to have a class like sql::Connection::ErrorIgnorer
which is pure virtual with something like ShouldIgnoreError().  Then
ScopedErrorIgnorer is in a separate file, and links into the sql::Connection
global via a private setter and friend access.  To limit it to tests,
ScopedErrorIgnorer can be in a test_support_sql target for purposes of exposing
itself to tests which want to use it (like base.gyp:test_support_base).

Does that make sense?

+  DCHECK_EQ(g_current_ignorer, this);
+  g_current_ignorer = next_;
+}
+
+void ScopedErrorIgnorer::IgnoreError(int err) {
+  DCHECK_EQ(0u, ignore_errors_.count(err));
+  ignore_errors_.insert(err);
+}
+
+bool ScopedErrorIgnorer::CheckIgnoredErrors() {
+  checked_ = true;
+  return errors_ignored_ == ignore_errors_;
+}
+
+// static
+bool ScopedErrorIgnorer::ShouldIgnoreError(int err) {
+  // Do not ignore if no scoped ignorer.
+  if (!g_current_ignorer)
+    return false;
+
+  // Check extended code.
+  if (g_current_ignorer->ignore_errors_.count(err) > 0) {
+    // Record that the error was seen and ignore it.
+    g_current_ignorer->errors_ignored_.insert(err);
+    return true;
+  }
+
+  // Trim extended codes.
+  err &= 0xff;
+
+  // Check extended code.

[erikwright (departed), 2013/06/11 18:54:19]

It seems like either line 874 or 864 is wrong?

[Scott Hess - ex-Googler, 2013/06/12 22:42:46]

Doh, this one is checking the non-extended code.

+  if (g_current_ignorer->ignore_errors_.count(err) > 0) {
+    // Record that the error was seen and ignore it.
+    g_current_ignorer->errors_ignored_.insert(err);
+    return true;
+  }
+
+  // Unexpected error, do not ignore.
+  return false;
+}
+
 }  // namespace sql