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

4 files changed, 255 insertions, 69 deletions

diff --git a/sql/connection.cc b/sql/connection.cc
index 5c0c15d..c19f6f2 100644
--- a/sql/connection.cc
+++ b/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,7 +548,7 @@ 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_);
@@ -500,7 +556,7 @@ int64 Connection::GetLastInsertRowId() const {
 
 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;
diff --git a/sql/connection.h b/sql/connection.h
index fa7c99d..cebc774 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -164,7 +164,7 @@ class SQL_EXPORT Connection {
   // empty. You can call this or Open.
   bool OpenInMemory() WARN_UNUSED_RESULT;
 
-  // Returns trie if the database has been successfully opened.
+  // Returns true if the database has been successfully opened.
   bool is_open() const { return !!db_; }
 
   // Closes the database. This is automatically performed on destruction for
@@ -221,6 +221,16 @@ class SQL_EXPORT Connection {
   bool Raze();
   bool RazeWithTimout(base::TimeDelta timeout);
 
+  // Breaks all outstanding transactions (as initiated by
+  // BeginTransaction()), calls Raze() to destroy the database, then
+  // closes the database.  After this is called, any operations
+  // against the connections (or statements prepared by the
+  // connection) should fail safely.
+  //
+  // The value from Raze() is returned, with Close() called in all
+  // cases.
+  bool RazeAndClose();
+
   // Transactions --------------------------------------------------------------
 
   // Transaction management. We maintain a virtual transaction stack to emulate
@@ -341,6 +351,10 @@ class SQL_EXPORT Connection {
   // sqlite3_open. The string can also be sqlite's special ":memory:" string.
   bool OpenInternal(const std::string& file_name);
 
+  // Internal close function used by Close() and RazeAndClose().
+  // |forced| indicates that orderly-shutdown checks should not apply.
+  void CloseInternal(bool forced);
+
   // Check whether the current thread is allowed to make IO calls, but only
   // if database wasn't open in memory. Function is inlined to be a no-op in
   // official build.
@@ -365,14 +379,24 @@ class SQL_EXPORT Connection {
   // should always check validity before using.
   class SQL_EXPORT StatementRef : public base::RefCounted<StatementRef> {
    public:
-    // Default constructor initializes to an invalid statement.
-    StatementRef();
-    explicit StatementRef(sqlite3_stmt* stmt);
-    StatementRef(Connection* connection, sqlite3_stmt* stmt);
+    // |connection| is the sql::Connection instance associated with
+    // the statement, and is used for tracking outstanding statements
+    // and for error handling.  Set to NULL for invalid or untracked
+    // refs.  |stmt| is the actual statement, and should only be NULL
+    // to create an invalid ref.  |was_valid| indicates whether the
+    // statement should be considered valid for diagnistic purposes.
+    // |was_valid| can be true for NULL |stmt| if the connection has
+    // been forcibly closed by an error handler.
+    StatementRef(Connection* connection, sqlite3_stmt* stmt, bool was_valid);
 
     // When true, the statement can be used.
     bool is_valid() const { return !!stmt_; }
 
+    // When true, the statement is either currently valid, or was
+    // previously valid but the connection was forcibly closed.  Used
+    // for diagnostic checks.
+    bool was_valid() const { return was_valid_; }
+
     // If we've not been linked to a connection, this will be NULL.
     // TODO(shess): connection_ can be NULL in case of GetUntrackedStatement(),
     // which prevents Statement::OnError() from forwarding errors.
@@ -383,8 +407,9 @@ class SQL_EXPORT Connection {
     sqlite3_stmt* stmt() const { return stmt_; }
 
     // Destroys the compiled statement and marks it NULL. The statement will
-    // no longer be active.
-    void Close();
+    // no longer be active.  |forced| is used to indicate if orderly-shutdown
+    // checks should apply (see Connection::RazeAndClose()).
+    void Close(bool forced);
 
     // Check whether the current thread is allowed to make IO calls, but only
     // if database wasn't open in memory.
@@ -397,6 +422,7 @@ class SQL_EXPORT Connection {
 
     Connection* connection_;
     sqlite3_stmt* stmt_;
+    bool was_valid_;
 
     DISALLOW_COPY_AND_ASSIGN(StatementRef);
   };
@@ -411,9 +437,6 @@ class SQL_EXPORT Connection {
   void StatementRefCreated(StatementRef* ref);
   void StatementRefDeleted(StatementRef* ref);
 
-  // Frees all cached statements from statement_cache_.
-  void ClearCache();
-
   // Called by Statement objects when an sqlite function returns an error.
   // The return value is the error code reflected back to client code.
   int OnSqliteError(int err, Statement* stmt);
@@ -464,6 +487,12 @@ class SQL_EXPORT Connection {
   // with Open().
   bool in_memory_;
 
+  // |true| if the connection was closed using RazeAndClose().  Used
+  // to enable diagnostics to distinguish calls to never-opened
+  // databases (incorrect use of the API) from calls to once-valid
+  // databases.
+  bool poisoned_;
+
   // This object handles errors resulting from all forms of executing sqlite
   // commands or statements. It can be null which means default handling.
   scoped_ptr<ErrorDelegate> error_delegate_;
diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc
index 983a689..2aaeb27 100644
--- a/sql/connection_unittest.cc
+++ b/sql/connection_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
 #include "sql/connection.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
@@ -279,6 +280,111 @@ TEST_F(SQLConnectionTest, RazeLocked) {
   ASSERT_TRUE(db().Raze());
 }
 
+// Basic test of RazeAndClose() operation.
+TEST_F(SQLConnectionTest, RazeAndClose) {
+  const char* kCreateSql = "CREATE TABLE foo (id INTEGER PRIMARY KEY, value)";
+  const char* kPopulateSql = "INSERT INTO foo (value) VALUES (12)";
+
+  // Test that RazeAndClose() closes the database, and that the
+  // database is empty when re-opened.
+  ASSERT_TRUE(db().Execute(kCreateSql));
+  ASSERT_TRUE(db().Execute(kPopulateSql));
+  ASSERT_TRUE(db().RazeAndClose());
+  ASSERT_FALSE(db().is_open());
+  db().Close();
+  ASSERT_TRUE(db().Open(db_path()));
+  {
+    sql::Statement s(db().GetUniqueStatement("SELECT * FROM sqlite_master"));
+    ASSERT_FALSE(s.Step());
+  }
+
+  // Test that RazeAndClose() can break transactions.
+  ASSERT_TRUE(db().Execute(kCreateSql));
+  ASSERT_TRUE(db().Execute(kPopulateSql));
+  ASSERT_TRUE(db().BeginTransaction());
+  ASSERT_TRUE(db().RazeAndClose());
+  ASSERT_FALSE(db().is_open());
+  ASSERT_FALSE(db().CommitTransaction());
+  db().Close();
+  ASSERT_TRUE(db().Open(db_path()));
+  {
+    sql::Statement s(db().GetUniqueStatement("SELECT * FROM sqlite_master"));
+    ASSERT_FALSE(s.Step());
+  }
+}
+
+// Test that various operations fail without crashing after
+// RazeAndClose().
+TEST_F(SQLConnectionTest, RazeAndCloseDiagnostics) {
+  const char* kCreateSql = "CREATE TABLE foo (id INTEGER PRIMARY KEY, value)";
+  const char* kPopulateSql = "INSERT INTO foo (value) VALUES (12)";
+  const char* kSimpleSql = "SELECT 1";
+
+  ASSERT_TRUE(db().Execute(kCreateSql));
+  ASSERT_TRUE(db().Execute(kPopulateSql));
+
+  // Test baseline expectations.
+  db().Preload();
+  ASSERT_TRUE(db().DoesTableExist("foo"));
+  ASSERT_TRUE(db().IsSQLValid(kSimpleSql));
+  ASSERT_EQ(SQLITE_OK, db().ExecuteAndReturnErrorCode(kSimpleSql));
+  ASSERT_TRUE(db().Execute(kSimpleSql));
+  ASSERT_TRUE(db().is_open());
+  {
+    sql::Statement s(db().GetUniqueStatement(kSimpleSql));
+    ASSERT_TRUE(s.Step());
+  }
+  {
+    sql::Statement s(db().GetCachedStatement(SQL_FROM_HERE, kSimpleSql));
+    ASSERT_TRUE(s.Step());
+  }
+  ASSERT_TRUE(db().BeginTransaction());
+  ASSERT_TRUE(db().CommitTransaction());
+  ASSERT_TRUE(db().BeginTransaction());
+  db().RollbackTransaction();
+
+  ASSERT_TRUE(db().RazeAndClose());
+
+  // At this point, they should all fail, but not crash.
+  db().Preload();
+  ASSERT_FALSE(db().DoesTableExist("foo"));
+  ASSERT_FALSE(db().IsSQLValid(kSimpleSql));
+  ASSERT_EQ(SQLITE_ERROR, db().ExecuteAndReturnErrorCode(kSimpleSql));
+  ASSERT_FALSE(db().Execute(kSimpleSql));
+  ASSERT_FALSE(db().is_open());
+  {
+    sql::Statement s(db().GetUniqueStatement(kSimpleSql));
+    ASSERT_FALSE(s.Step());
+  }
+  {
+    sql::Statement s(db().GetCachedStatement(SQL_FROM_HERE, kSimpleSql));
+    ASSERT_FALSE(s.Step());
+  }
+  ASSERT_FALSE(db().BeginTransaction());
+  ASSERT_FALSE(db().CommitTransaction());
+  ASSERT_FALSE(db().BeginTransaction());
+  db().RollbackTransaction();
+
+  // Close normally to reset the poisoned flag.
+  db().Close();
+
+  // DEATH tests not supported on Android or iOS.
+#if !defined(OS_ANDROID) && !defined(OS_IOS)
+  // Once the real Close() has been called, various calls enforce API
+  // usage by becoming fatal in debug mode.  Since DEATH tests are
+  // expensive, just test one of them.
+  if (DLOG_IS_ON(FATAL)) {
+    ASSERT_DEATH({
+        db().IsSQLValid(kSimpleSql);
+      }, "Illegal use of connection without a db");
+  }
+#endif
+}
+
+// TODO(shess): Spin up a background thread to hold other_db, to more
+// closely match real life.  That would also allow testing
+// RazeWithTimeout().
+
 #if defined(OS_ANDROID)
 TEST_F(SQLConnectionTest, SetTempDirForSQL) {
 
@@ -291,7 +397,3 @@ TEST_F(SQLConnectionTest, SetTempDirForSQL) {
   ASSERT_TRUE(meta_table.Init(&db(), 4, 4));
 }
 #endif
-
-// TODO(shess): Spin up a background thread to hold other_db, to more
-// closely match real life.  That would also allow testing
-// RazeWithTimeout().
diff --git a/sql/statement.cc b/sql/statement.cc
index cd55bf7..d92be01 100644
--- a/sql/statement.cc
+++ b/sql/statement.cc
@@ -15,7 +15,7 @@ namespace sql {
 // we don't have to NULL-check the ref_ to see if the statement is valid: we
 // only have to check the ref's validity bit.
 Statement::Statement()
-    : ref_(new Connection::StatementRef),
+    : ref_(new Connection::StatementRef(NULL, NULL, false)),
       succeeded_(false) {
 }
 
@@ -37,13 +37,15 @@ void Statement::Assign(scoped_refptr<Connection::StatementRef> ref) {
 }
 
 void Statement::Clear() {
-  Assign(new Connection::StatementRef);
+  Assign(new Connection::StatementRef(NULL, NULL, false));
   succeeded_ = false;
 }
 
 bool Statement::CheckValid() const {
-  if (!is_valid())
-    DLOG(FATAL) << "Cannot call mutating statements on an invalid statement.";
+  // Allow operations to fail silently if a statement was invalidated
+  // because the database was closed by an error handler.
+  DLOG_IF(FATAL, !ref_->was_valid())
+      << "Cannot call mutating statements on an invalid statement.";
   return is_valid();
 }
 
@@ -306,7 +308,7 @@ bool Statement::CheckOk(int err) const {
 int Statement::CheckError(int err) {
   // Please don't add DCHECKs here, OnSqliteError() already has them.
   succeeded_ = (err == SQLITE_OK || err == SQLITE_ROW || err == SQLITE_DONE);
-  if (!succeeded_ && is_valid() && ref_->connection())
+  if (!succeeded_ && ref_ && ref_->connection())
     return ref_->connection()->OnSqliteError(err, this);
   return err;
 }