Remove ref counting on sql::ErrorDelegate

BUG=151841
Test=None

R=shess
TBR=jamesr,erikwright

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

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

chrome/browser/net/sqlite_persistent_cookie_store.cc

@@ -75,8 +75,6 @@ class SQLitePersistentCookieStore::Backend
         num_cookies_read_(0),
         num_priority_waiting_(0),
         total_priority_requests_(0) {
-    error_delegate_ =
-      new KillDatabaseErrorDelegate(this, GetErrorHandlerForCookieDb());
   }
 
   // Creates or loads the SQLite database.
@@ -111,35 +109,26 @@ class SQLitePersistentCookieStore::Backend
    public:
     KillDatabaseErrorDelegate(Backend* backend,
                               sql::ErrorDelegate* wrapped_delegate);
+
+    virtual ~KillDatabaseErrorDelegate() {}
+
     // ErrorDelegate implementation.
     virtual int OnError(int error,
                         sql::Connection* connection,
                         sql::Statement* stmt) OVERRIDE;
 
-    void reset_backend() {
-      backend_ = NULL;
-    }
-
-   protected:
-    virtual ~KillDatabaseErrorDelegate() {}
-
    private:
 
     // Do not increment the count on Backend, as that would create a circular
-    // reference (Backend -> Connection -> ErrorDelegate -> Backend). Instead,
-    // Backend will call reset_backend() when it is going away.
+    // reference (Backend -> Connection -> ErrorDelegate -> Backend).
     Backend* backend_;
-    scoped_refptr<sql::ErrorDelegate> wrapped_delegate_;
+    scoped_ptr<sql::ErrorDelegate> wrapped_delegate_;
 
     DISALLOW_COPY_AND_ASSIGN(KillDatabaseErrorDelegate);
   };
 
   // You should call Close() before destructing this object.
   ~Backend() {
-    if (error_delegate_.get()) {
-      error_delegate_->reset_backend();
-      error_delegate_ = NULL;
-    }
     DCHECK(!db_.get()) << "Close should have already been called.";
     DCHECK(num_pending_ == 0 && pending_.empty());
   }
@@ -227,7 +216,6 @@ class SQLitePersistentCookieStore::Backend
 
   FilePath path_;
   scoped_ptr<sql::Connection> db_;
-  scoped_refptr<KillDatabaseErrorDelegate> error_delegate_;
   sql::MetaTable meta_table_;
 
   typedef std::list<PendingOperation*> PendingOperationsList;
@@ -371,10 +359,9 @@ int SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::OnError(
     MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(&Backend::KillDatabase, backend_));
 
-    // Avoid being called more than once. There should still be a reference to
-    // this ErrorDelegate in the backend, but just in case don't refer to any
-    // members from here forward.
-    connection->set_error_delegate(wrapped_delegate_.get());
+    // Avoid being called more than once. This will destroy the
+    // KillDatabaseErrorDelegate. Do not refer to any members from here forward.
+    connection->set_error_delegate(wrapped_delegate_.release());
   }
 
   return error;
@@ -627,7 +614,8 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
   }
 
   db_.reset(new sql::Connection);
-  db_->set_error_delegate(error_delegate_.get());
+  db_->set_error_delegate(
+      new KillDatabaseErrorDelegate(this, GetErrorHandlerForCookieDb()));
 
   if (!db_->Open(path_)) {
     NOTREACHED() << "Unable to open cookie DB.";

sql/connection.cc

@@ -50,9 +50,6 @@ bool StatementID::operator<(const StatementID& other) const {
   return strcmp(str_, other.str_) < 0;
 }
 
-ErrorDelegate::ErrorDelegate() {
-}
-
 ErrorDelegate::~ErrorDelegate() {
 }
 
@@ -102,7 +99,8 @@ Connection::Connection()
       exclusive_locking_(false),
       transaction_nesting_(0),
       needs_rollback_(false),
-      in_memory_(false) {
+      in_memory_(false),
+      error_delegate_(NULL) {
 }
 
 Connection::~Connection() {

sql/connection.h

@@ -12,6 +12,7 @@
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "sql/sql_export.h"
@@ -78,9 +79,9 @@ class Connection;
 // the OnError() callback.
 // The tipical usage is to centralize the code designed to handle database
 // corruption, low-level IO errors or locking violations.
-class SQL_EXPORT ErrorDelegate : public base::RefCounted<ErrorDelegate> {
+class SQL_EXPORT ErrorDelegate {
  public:
-  ErrorDelegate();
+  virtual ~ErrorDelegate();
 
   // |error| is an sqlite result code as seen in sqlite\preprocessed\sqlite3.h
   // |connection| is db connection where the error happened and |stmt| is
@@ -94,11 +95,6 @@ class SQL_EXPORT ErrorDelegate : public base::RefCounted<ErrorDelegate> {
   // re-tried then returning SQLITE_OK is appropiate; otherwise is recomended
   // that you return the original |error| or the appropiae error code.
   virtual int OnError(int error, Connection* connection, Statement* stmt) = 0;
-
- protected:
-  friend class base::RefCounted<ErrorDelegate>;
-
-  virtual ~ErrorDelegate();
 };
 
 class SQL_EXPORT Connection {
@@ -142,8 +138,9 @@ class SQL_EXPORT Connection {
   // Sets the object that will handle errors. Recomended that it should be set
   // before calling Open(). If not set, the default is to ignore errors on
   // release and assert on debug builds.
+  // Takes ownership of |delegate|.
   void set_error_delegate(ErrorDelegate* delegate) {
-    error_delegate_ = delegate;
+    error_delegate_.reset(delegate);
   }
 
   // Initialization ------------------------------------------------------------
@@ -445,7 +442,7 @@ class SQL_EXPORT Connection {
 
   // This object handles errors resulting from all forms of executing sqlite
   // commands or statements. It can be null which means default handling.
-  scoped_refptr<ErrorDelegate> error_delegate_;
+  scoped_ptr<ErrorDelegate> error_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };

sql/diagnostic_error_delegate.h

@@ -24,6 +24,8 @@ namespace sql {
 template <class UniqueT>
 class DiagnosticErrorDelegate : public ErrorDelegate {
  public:
+  DiagnosticErrorDelegate() {}
+  virtual ~DiagnosticErrorDelegate() {}
 
   virtual int OnError(int error, Connection* connection,
                       Statement* stmt) {
@@ -43,6 +45,8 @@ class DiagnosticErrorDelegate : public ErrorDelegate {
     // 26 currently but 50 gives them room to grow.
     UMA_HISTOGRAM_ENUMERATION(UniqueT::name(), error, 50);
   }
+
+ DISALLOW_COPY_AND_ASSIGN(DiagnosticErrorDelegate);
 };
 
 }  // namespace sql

sql/sqlite_features_unittest.cc

@@ -15,65 +15,63 @@
 
 namespace {
 
-
 class StatementErrorHandler : public sql::ErrorDelegate {
  public:
-  StatementErrorHandler() : error_(SQLITE_OK) {}
+  StatementErrorHandler(int* error, std::string* sql_text)
+    : error_(error),
+      sql_text_(sql_text) {}
+
+  virtual ~StatementErrorHandler() {}
 
   virtual int OnError(int error, sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
-    error_ = error;
+    *error_ = error;
     const char* sql_txt = stmt ? stmt->GetSQLStatement() : NULL;
-    sql_text_ = sql_txt ? sql_txt : "no statement available";
+    *sql_text_ = sql_txt ? sql_txt : "no statement available";
     return error;
   }
 
-  int error() const { return error_; }
-
-  void reset_error() {
-    sql_text_.clear();
-    error_ = SQLITE_OK;
-  }
-
-  const char* sql_statement() const { return sql_text_.c_str(); }
-
- protected:
-  virtual ~StatementErrorHandler() {}
-
  private:
-  int error_;
-  std::string sql_text_;
+  int* error_;
+  std::string* sql_text_;
+
+ DISALLOW_COPY_AND_ASSIGN(StatementErrorHandler);
 };
 
 class SQLiteFeaturesTest : public testing::Test {
  public:
-  SQLiteFeaturesTest() : error_handler_(new StatementErrorHandler) {}
+  SQLiteFeaturesTest() : error_(SQLITE_OK) {}
 
   void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(db_.Open(temp_dir_.path().AppendASCII("SQLStatementTest.db")));
 
-    // The |error_handler_| will be called if any sqlite statement operation
-    // returns an error code.
-    db_.set_error_delegate(error_handler_);
+    // The error delegate will set |error_| and |sql_text_| when any sqlite
+    // statement operation returns an error code.
+    db_.set_error_delegate(new StatementErrorHandler(&error_, &sql_text_));
   }
 
   void TearDown() {
     // If any error happened the original sql statement can be found in
-    // error_handler_->sql_statement().
-    EXPECT_EQ(SQLITE_OK, error_handler_->error());
+    // |sql_text_|.
+    EXPECT_EQ(SQLITE_OK, error_);
     db_.Close();
   }
 
   sql::Connection& db() { return db_; }
 
-  int sqlite_error() const { return error_handler_->error(); }
-  void reset_error() const { error_handler_->reset_error(); }
+  int sqlite_error() const {
+    return error_;
+  }
 
  private:
   ScopedTempDir temp_dir_;
   sql::Connection db_;
-  scoped_refptr<StatementErrorHandler> error_handler_;
+
+  // The error code of the most recent error.
+  int error_;
+  // Original statement which has caused the error.
+  std::string sql_text_;
 };
 
 // Do not include fts1 support, it is not useful, and nobody is

sql/statement_unittest.cc

@@ -11,66 +11,71 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/sqlite/sqlite3.h"
 
+namespace {
+
 class StatementErrorHandler : public sql::ErrorDelegate {
  public:
-  StatementErrorHandler() : error_(SQLITE_OK) {}
+  StatementErrorHandler(int* error, std::string* sql_text)
+    : error_(error),
+      sql_text_(sql_text) {}
+
+  virtual ~StatementErrorHandler() {}
 
   virtual int OnError(int error, sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
-    error_ = error;
+    *error_ = error;
     const char* sql_txt = stmt ? stmt->GetSQLStatement() : NULL;
-    sql_text_ = sql_txt ? sql_txt : "no statement available";
+    *sql_text_ = sql_txt ? sql_txt : "no statement available";
     return error;
   }
 
-  int error() const { return error_; }
-
-  void reset_error() {
-    sql_text_.clear();
-    error_ = SQLITE_OK;
-  }
-
-  const char* sql_statement() const { return sql_text_.c_str(); }
-
- protected:
-  virtual ~StatementErrorHandler() {}
-
  private:
-  int error_;
-  std::string sql_text_;
+  int* error_;
+  std::string* sql_text_;
+
+ DISALLOW_COPY_AND_ASSIGN(StatementErrorHandler);
 };
 
 class SQLStatementTest : public testing::Test {
  public:
-  SQLStatementTest() : error_handler_(new StatementErrorHandler) {}
+  SQLStatementTest() : error_(SQLITE_OK) {}
 
   void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(db_.Open(temp_dir_.path().AppendASCII("SQLStatementTest.db")));
-
-    // The |error_handler_| will be called if any sqlite statement operation
-    // returns an error code.
-    db_.set_error_delegate(error_handler_);
+    // The error delegate will set |error_| and |sql_text_| when any sqlite
+    // statement operation returns an error code.
+    db_.set_error_delegate(new StatementErrorHandler(&error_, &sql_text_));
   }
 
   void TearDown() {
     // If any error happened the original sql statement can be found in
-    // error_handler_->sql_statement().
-    EXPECT_EQ(SQLITE_OK, error_handler_->error());
+    // |sql_text_|.
+    EXPECT_EQ(SQLITE_OK, error_);
     db_.Close();
   }
 
   sql::Connection& db() { return db_; }
 
-  int sqlite_error() const { return error_handler_->error(); }
-  void reset_error() const { error_handler_->reset_error(); }
+  int sqlite_error() const { return error_; }
+
+  void ResetError() {
+    error_ = SQLITE_OK;
+    sql_text_.clear();
+  }
 
  private:
   ScopedTempDir temp_dir_;
   sql::Connection db_;
-  scoped_refptr<StatementErrorHandler> error_handler_;
+
+  // The error code of the most recent error.
+  int error_;
+  // Original statement which caused the error.
+  std::string sql_text_;
 };
 
+}  // namespace
+
 TEST_F(SQLStatementTest, Assign) {
   sql::Statement s;
   EXPECT_FALSE(s.is_valid());
@@ -121,7 +126,7 @@ TEST_F(SQLStatementTest, BasicErrorCallback) {
   s.BindCString(0, "bad bad");
   EXPECT_FALSE(s.Run());
   EXPECT_EQ(SQLITE_MISMATCH, sqlite_error());
-  reset_error();
+  ResetError();
 }
 
 TEST_F(SQLStatementTest, Reset) {

webkit/appcache/appcache_database_unittest.cc

@@ -20,6 +20,9 @@ const base::Time kZeroTime;
 
 class TestErrorDelegate : public sql::ErrorDelegate {
  public:
+  TestErrorDelegate() {}
+  virtual ~TestErrorDelegate() {}
+
   virtual int OnError(int error,
                       sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
@@ -27,7 +30,7 @@ class TestErrorDelegate : public sql::ErrorDelegate {
   }
 
  private:
-  virtual ~TestErrorDelegate() {}
+  DISALLOW_COPY_AND_ASSIGN(TestErrorDelegate);
 };
 
 }  // namespace
@@ -89,8 +92,7 @@ TEST(AppCacheDatabaseTest, EntryRecords) {
   EXPECT_TRUE(db.LazyOpen(true));
 
   // Set an error delegate that will make all operations return false on error.
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   AppCacheDatabase::EntryRecord entry;
 
@@ -163,8 +165,7 @@ TEST(AppCacheDatabaseTest, CacheRecords) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   const AppCacheDatabase::CacheRecord kZeroRecord;
   AppCacheDatabase::CacheRecord record;
@@ -206,8 +207,7 @@ TEST(AppCacheDatabaseTest, GroupRecords) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   const GURL kManifestUrl("http://blah/manifest");
   const GURL kOrigin(kManifestUrl.GetOrigin());
@@ -334,8 +334,7 @@ TEST(AppCacheDatabaseTest, NamespaceRecords) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   const GURL kFooNameSpace1("http://foo/namespace1");
   const GURL kFooNameSpace2("http://foo/namespace2");
@@ -435,8 +434,7 @@ TEST(AppCacheDatabaseTest, OnlineWhiteListRecords) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   const GURL kFooNameSpace1("http://foo/namespace1");
   const GURL kFooNameSpace2("http://foo/namespace2");
@@ -482,8 +480,7 @@ TEST(AppCacheDatabaseTest, DeletableResponseIds) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   std::vector<int64> ids;
 
@@ -559,8 +556,7 @@ TEST(AppCacheDatabaseTest, OriginUsage) {
   AppCacheDatabase db(kEmptyPath);
   EXPECT_TRUE(db.LazyOpen(true));
 
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate);
-  db.db_->set_error_delegate(error_delegate);
+  db.db_->set_error_delegate(new TestErrorDelegate());
 
   std::vector<AppCacheDatabase::CacheRecord> cache_records;
   EXPECT_EQ(0, db.GetOriginUsage(kOrigin));

webkit/database/databases_table_unittest.cc

@@ -13,14 +13,17 @@ namespace {
 
 class TestErrorDelegate : public sql::ErrorDelegate {
  public:
+  TestErrorDelegate() {}
+  virtual ~TestErrorDelegate() {}
+
   virtual int OnError(int error,
                       sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
     return error;
   }
 
- protected:
-  virtual ~TestErrorDelegate() {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestErrorDelegate);
 };
 
 }  // namespace
@@ -46,8 +49,7 @@ TEST(DatabasesTableTest, TestIt) {
   sql::Connection db;
 
   // Set an error delegate that will make all operations return false on error.
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate());
-  db.set_error_delegate(error_delegate);
+  db.set_error_delegate(new TestErrorDelegate());
 
   // Initialize the temp dir and the 'Databases' table.
   EXPECT_TRUE(db.OpenInMemory());

webkit/database/quota_table_unittest.cc

@@ -13,14 +13,17 @@ namespace {
 
 class TestErrorDelegate : public sql::ErrorDelegate {
  public:
+  TestErrorDelegate() {}
+  virtual ~TestErrorDelegate() {}
+
   virtual int OnError(int error,
                       sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
     return error;
   }
 
- protected:
-  virtual ~TestErrorDelegate() {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestErrorDelegate);
 };
 
 }  // namespace
@@ -38,8 +41,7 @@ TEST(QuotaTableTest, TestIt) {
   sql::Connection db;
 
   // Set an error delegate that will make all operations return false on error.
-  scoped_refptr<TestErrorDelegate> error_delegate(new TestErrorDelegate());
-  db.set_error_delegate(error_delegate);
+  db.set_error_delegate(new TestErrorDelegate());
 
   // Initialize the temp dir and the 'Databases' table.
   EXPECT_TRUE(db.OpenInMemory());