Implement sql::Connection::RazeAndClose().

Raze() clears out the database, but cannot be called within a
transaction.  Close() can only be called once all statements have
cleared.  Error callbacks happen while executing statements (thus
often in a transaction, and at least one statement is outstanding).

RazeAndClose() forcibly breaks outstanding transactions, calls Raze()
to clear the database, then calls Close() to close the handle.  All
future operations against the database should fail safely (without
affecting storage and without crashing).

BUG=166419, 136846


Review URL: https://chromiumcodereview.appspot.com/12096073

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@181152 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/activity_database.cc

@@ -114,9 +114,7 @@ void ActivityDatabase::Close() {
 }
 
 void ActivityDatabase::KillDatabase() {
-  db_.RollbackTransaction();
-  db_.Raze();
-  db_.Close();
+  db_.RazeAndClose();
 }
 
 }  // namespace extensions

chrome/browser/extensions/activity_database.h

@@ -47,6 +47,9 @@ class ActivityDatabase : public base::RefCountedThreadSafe<ActivityDatabase> {
   // Record an Action in the database.
   void RecordAction(scoped_refptr<Action> action);
 
+  // Break any outstanding transactions, raze the database, and close
+  // it.  Future calls on the current database handle will fail, when
+  // next opened the database will be empty.
   void KillDatabase();
 
   bool initialized() const { return initialized_; }

chrome/browser/history/thumbnail_database.cc

@@ -209,8 +209,7 @@ sql::InitStatus ThumbnailDatabase::Init(
                << "structure.";
     UMA_HISTOGRAM_BOOLEAN("History.InvalidFaviconsDBStructure", true);
 
-    db_.Raze();
-    db_.Close();
+    db_.RazeAndClose();
     return sql::INIT_FAILURE;
   }
 

chrome/browser/net/sqlite_persistent_cookie_store.cc

@@ -1044,9 +1044,8 @@ void SQLitePersistentCookieStore::Backend::KillDatabase() {
   if (db_.get()) {
     // This Backend will now be in-memory only. In a future run we will recreate
     // the database. Hopefully things go better then!
-    bool success = db_->Raze();
+    bool success = db_->RazeAndClose();
     UMA_HISTOGRAM_BOOLEAN("Cookie.KillDatabaseResult", success);
-    db_->Close();
     meta_table_.Reset();
     db_.reset();
   }

sql/connection.cc

@@ -74,30 +74,23 @@ bool StatementID::operator<(const StatementID& other) const {
 ErrorDelegate::~ErrorDelegate() {
 }
 
-Connection::StatementRef::StatementRef()
-    : connection_(NULL),
-      stmt_(NULL) {
-}
-
-Connection::StatementRef::StatementRef(sqlite3_stmt* stmt)
-    : connection_(NULL),
-      stmt_(stmt) {
-}
-
 Connection::StatementRef::StatementRef(Connection* connection,
-                                       sqlite3_stmt* stmt)
+                                       sqlite3_stmt* stmt,
+                                       bool was_valid)
     : connection_(connection),
-      stmt_(stmt) {
-  connection_->StatementRefCreated(this);
+      stmt_(stmt),
+      was_valid_(was_valid) {
+  if (connection)
+    connection_->StatementRefCreated(this);
 }
 
 Connection::StatementRef::~StatementRef() {
   if (connection_)
     connection_->StatementRefDeleted(this);
-  Close();
+  Close(false);
 }
 
-void Connection::StatementRef::Close() {
+void Connection::StatementRef::Close(bool forced) {
   if (stmt_) {
     // Call to AssertIOAllowed() cannot go at the beginning of the function
     // because Close() is called unconditionally from destructor to clean
@@ -111,6 +104,11 @@ void Connection::StatementRef::Close() {
     stmt_ = NULL;
   }
   connection_ = NULL;  // The connection may be getting deleted.
+
+  // Forced close is expected to happen from a statement error
+  // handler.  In that case maintain the sense of |was_valid_| which
+  // previously held for this ref.
+  was_valid_ = was_valid_ && forced;
 }
 
 Connection::Connection()
@@ -121,6 +119,7 @@ Connection::Connection()
       transaction_nesting_(0),
       needs_rollback_(false),
       in_memory_(false),
+      poisoned_(false),
       error_delegate_(NULL) {
 }
 
@@ -141,22 +140,28 @@ bool Connection::OpenInMemory() {
   return OpenInternal(":memory:");
 }
 
-void Connection::Close() {
+void Connection::CloseInternal(bool forced) {
   // TODO(shess): Calling "PRAGMA journal_mode = DELETE" at this point
   // will delete the -journal file.  For ChromiumOS or other more
   // embedded systems, this is probably not appropriate, whereas on
   // desktop it might make some sense.
 
   // sqlite3_close() needs all prepared statements to be finalized.
-  // Release all cached statements, then assert that the client has
-  // released all statements.
+
+  // Release cached statements.
   statement_cache_.clear();
-  DCHECK(open_statements_.empty());
 
-  // Additionally clear the prepared statements, because they contain
-  // weak references to this connection.  This case has come up when
-  // error-handling code is hit in production.
-  ClearCache();
+  // With cached statements released, in-use statements will remain.
+  // Closing the database while statements are in use is an API
+  // violation, except for forced close (which happens from within a
+  // statement's error handler).
+  DCHECK(forced || open_statements_.empty());
+
+  // Deactivate any outstanding statements so sqlite3_close() works.
+  for (StatementRefSet::iterator i = open_statements_.begin();
+       i != open_statements_.end(); ++i)
+    (*i)->Close(forced);
+  open_statements_.clear();
 
   if (db_) {
     // Call to AssertIOAllowed() cannot go at the beginning of the function
@@ -172,11 +177,23 @@ void Connection::Close() {
   }
 }
 
+void Connection::Close() {
+  // If the database was already closed by RazeAndClose(), then no
+  // need to close again.  Clear the |poisoned_| bit so that incorrect
+  // API calls are caught.
+  if (poisoned_) {
+    poisoned_ = false;
+    return;
+  }
+
+  CloseInternal(false);
+}
+
 void Connection::Preload() {
   AssertIOAllowed();
 
   if (!db_) {
-    DLOG(FATAL) << "Cannot preload null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot preload null db";
     return;
   }
 
@@ -202,7 +219,7 @@ bool Connection::Raze() {
   AssertIOAllowed();
 
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
@@ -292,7 +309,7 @@ bool Connection::Raze() {
 
 bool Connection::RazeWithTimout(base::TimeDelta timeout) {
   if (!db_) {
-    DLOG(FATAL) << "Cannot raze null db";
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
     return false;
   }
 
@@ -301,6 +318,29 @@ bool Connection::RazeWithTimout(base::TimeDelta timeout) {
   return Raze();
 }
 
+bool Connection::RazeAndClose() {
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Cannot raze null db";
+    return false;
+  }
+
+  // Raze() cannot run in a transaction.
+  while (transaction_nesting_) {
+    RollbackTransaction();
+  }
+
+  bool result = Raze();
+
+  CloseInternal(true);
+
+  // Mark the database so that future API calls fail appropriately,
+  // but don't DCHECK (because after calling this function they are
+  // expected to fail).
+  poisoned_ = true;
+
+  return result;
+}
+
 bool Connection::BeginTransaction() {
   if (needs_rollback_) {
     DCHECK_GT(transaction_nesting_, 0);
@@ -324,7 +364,7 @@ bool Connection::BeginTransaction() {
 
 void Connection::RollbackTransaction() {
   if (!transaction_nesting_) {
-    DLOG(FATAL) << "Rolling back a nonexistent transaction";
+    DLOG_IF(FATAL, !poisoned_) << "Rolling back a nonexistent transaction";
     return;
   }
 
@@ -341,7 +381,7 @@ void Connection::RollbackTransaction() {
 
 bool Connection::CommitTransaction() {
   if (!transaction_nesting_) {
-    DLOG(FATAL) << "Rolling back a nonexistent transaction";
+    DLOG_IF(FATAL, !poisoned_) << "Rolling back a nonexistent transaction";
     return false;
   }
   transaction_nesting_--;
@@ -362,12 +402,19 @@ bool Connection::CommitTransaction() {
 
 int Connection::ExecuteAndReturnErrorCode(const char* sql) {
   AssertIOAllowed();
-  if (!db_)
-    return false;
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
+    return SQLITE_ERROR;
+  }
   return sqlite3_exec(db_, sql, NULL, NULL, NULL);
 }
 
 bool Connection::Execute(const char* sql) {
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
+    return false;
+  }
+
   int error = ExecuteAndReturnErrorCode(sql);
   if (error != SQLITE_OK)
     error = OnSqliteError(error, NULL);
@@ -381,8 +428,10 @@ bool Connection::Execute(const char* sql) {
 }
 
 bool Connection::ExecuteWithTimeout(const char* sql, base::TimeDelta timeout) {
-  if (!db_)
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
     return false;
+  }
 
   ScopedBusyTimeout busy_timeout(db_);
   busy_timeout.SetTimeout(timeout);
@@ -417,8 +466,9 @@ scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
     const char* sql) {
   AssertIOAllowed();
 
+  // Return inactive statement.
   if (!db_)
-    return new StatementRef();  // Return inactive statement.
+    return new StatementRef(NULL, NULL, poisoned_);
 
   sqlite3_stmt* stmt = NULL;
   int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
@@ -428,28 +478,34 @@ scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
 
     // It could also be database corruption.
     OnSqliteError(rc, NULL);
-    return new StatementRef();
+    return new StatementRef(NULL, NULL, false);
   }
-  return new StatementRef(this, stmt);
+  return new StatementRef(this, stmt, true);
 }
 
 scoped_refptr<Connection::StatementRef> Connection::GetUntrackedStatement(
     const char* sql) const {
+  // Return inactive statement.
   if (!db_)
-    return new StatementRef();  // Return inactive statement.
+    return new StatementRef(NULL, NULL, poisoned_);
 
   sqlite3_stmt* stmt = NULL;
   int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
   if (rc != SQLITE_OK) {
     // This is evidence of a syntax error in the incoming SQL.
     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
-    return new StatementRef();
+    return new StatementRef(NULL, NULL, false);
   }
-  return new StatementRef(stmt);
+  return new StatementRef(NULL, stmt, true);
 }
 
 bool Connection::IsSQLValid(const char* sql) {
   AssertIOAllowed();
+  if (!db_) {
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
+    return false;
+  }
+
   sqlite3_stmt* stmt = NULL;
   if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK)
     return false;
@@ -492,15 +548,15 @@ bool Connection::DoesColumnExist(const char* table_name,
 
 int64 Connection::GetLastInsertRowId() const {
   if (!db_) {
-    DLOG(FATAL) << "Illegal use of connection without a db";
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_last_insert_rowid(db_);
 }
 
 int Connection::GetLastChangeCount() const {
   if (!db_) {
-    DLOG(FATAL) << "Illegal use of connection without a db";
+    DLOG_IF(FATAL, !poisoned_) << "Illegal use of connection without a db";
     return 0;
   }
   return sqlite3_changes(db_);
@@ -537,6 +593,14 @@ bool Connection::OpenInternal(const std::string& file_name) {
     return false;
   }
 
+  // If |poisoned_| is set, it means an error handler called
+  // RazeAndClose().  Until regular Close() is called, the caller
+  // should be treating the database as open, but is_open() currently
+  // only considers the sqlite3 handle's state.
+  // TODO(shess): Revise is_open() to consider poisoned_, and review
+  // to see if any non-testing code even depends on it.
+  DLOG_IF(FATAL, poisoned_) << "sql::Connection is already open.";
+
   int err = sqlite3_open(file_name.c_str(), &db_);
   if (err != SQLITE_OK) {
     // Histogram failures specific to initial open for debugging
@@ -641,17 +705,6 @@ void Connection::StatementRefDeleted(StatementRef* ref) {
     open_statements_.erase(i);
 }
 
-void Connection::ClearCache() {
-  statement_cache_.clear();
-
-  // The cache clear will get most statements. There may be still be references
-  // to some statements that are held by others (including one-shot statements).
-  // This will deactivate them so they can't be used again.
-  for (StatementRefSet::iterator i = open_statements_.begin();
-       i != open_statements_.end(); ++i)
-    (*i)->Close();
-}
-
 int Connection::OnSqliteError(int err, sql::Statement *stmt) {
   // Strip extended error codes.
   int base_err = err&0xff;