Move ErrorDelegate to its own file and add static utility functions to ErrorDelegate

BUG=151841
Test=None
R=shess, erikwright
TBR=cpu

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

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

8 files changed, 159 insertions, 123 deletions

diff --git a/chrome/browser/diagnostics/sqlite_diagnostics.cc b/chrome/browser/diagnostics/sqlite_diagnostics.cc
index 3fba29e..6d51e7e 100644
--- a/chrome/browser/diagnostics/sqlite_diagnostics.cc
+++ b/chrome/browser/diagnostics/sqlite_diagnostics.cc
@@ -95,7 +95,6 @@ class HistogramUniquifier {
  public:
   static const char* name() {
     const char* kHistogramNames[] = {
-      "Sqlite.Cookie.Error",
       "Sqlite.History.Error",
       "Sqlite.Thumbnail.Error",
       "Sqlite.Text.Error",
@@ -107,24 +106,20 @@ class HistogramUniquifier {
 
 }  // namespace
 
-sql::ErrorDelegate* GetErrorHandlerForCookieDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<0> >();
-}
-
 sql::ErrorDelegate* GetErrorHandlerForHistoryDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<1> >();
+  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<0> >();
 }
 
 sql::ErrorDelegate* GetErrorHandlerForThumbnailDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<2> >();
+  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<1> >();
 }
 
 sql::ErrorDelegate* GetErrorHandlerForTextDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<3> >();
+  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<2> >();
 }
 
 sql::ErrorDelegate* GetErrorHandlerForWebDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<4> >();
+  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<3> >();
 }
 
 DiagnosticTest* MakeSqliteWebDbTest() {
diff --git a/chrome/browser/diagnostics/sqlite_diagnostics.h b/chrome/browser/diagnostics/sqlite_diagnostics.h
index c416e88..2d03458 100644
--- a/chrome/browser/diagnostics/sqlite_diagnostics.h
+++ b/chrome/browser/diagnostics/sqlite_diagnostics.h
@@ -11,9 +11,8 @@ namespace sql {
   class ErrorDelegate;
 }
 
-// The following five factories create the error handlers that we use when
+// The following four factories create the error handlers that we use when
 // issuing sqlite commands during normal browser operation.
-sql::ErrorDelegate* GetErrorHandlerForCookieDb();
 sql::ErrorDelegate* GetErrorHandlerForHistoryDb();
 sql::ErrorDelegate* GetErrorHandlerForThumbnailDb();
 sql::ErrorDelegate* GetErrorHandlerForTextDb();
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index f979574..e970eb1 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -29,6 +29,7 @@
 #include "googleurl/src/gurl.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
+#include "sql/error_delegate_util.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -107,8 +108,7 @@ class SQLitePersistentCookieStore::Backend
 
   class KillDatabaseErrorDelegate : public sql::ErrorDelegate {
    public:
-    KillDatabaseErrorDelegate(Backend* backend,
-                              sql::ErrorDelegate* wrapped_delegate);
+    KillDatabaseErrorDelegate(Backend* backend);
 
     virtual ~KillDatabaseErrorDelegate() {}
 
@@ -119,10 +119,17 @@ class SQLitePersistentCookieStore::Backend
 
    private:
 
+    class HistogramUniquifier {
+     public:
+      static const char* name() { return "Sqlite.Cookie.Error"; }
+    };
+
     // Do not increment the count on Backend, as that would create a circular
     // reference (Backend -> Connection -> ErrorDelegate -> Backend).
     Backend* backend_;
-    scoped_ptr<sql::ErrorDelegate> wrapped_delegate_;
+
+    // True if the delegate has previously attempted to kill the database.
+    bool attempted_to_kill_database_;
 
     DISALLOW_COPY_AND_ASSIGN(KillDatabaseErrorDelegate);
   };
@@ -273,95 +280,24 @@ class SQLitePersistentCookieStore::Backend
 };
 
 SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::
-KillDatabaseErrorDelegate(Backend* backend,
-                          sql::ErrorDelegate* wrapped_delegate)
+KillDatabaseErrorDelegate(Backend* backend)
     : backend_(backend),
-      wrapped_delegate_(wrapped_delegate) {
+      attempted_to_kill_database_(false) {
 }
 
 int SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::OnError(
     int error, sql::Connection* connection, sql::Statement* stmt) {
-  if (wrapped_delegate_.get())
-    error = wrapped_delegate_->OnError(error, connection, stmt);
-
-  bool delete_db = false;
-
-  switch (error) {
-    case SQLITE_DONE:
-    case SQLITE_OK:
-      // Theoretically, the wrapped delegate might have resolved the error, and
-      // we would end up here.
-      break;
-
-    case SQLITE_CORRUPT:
-    case SQLITE_NOTADB:
-      // Highly unlikely we would ever recover from these.
-      delete_db = true;
-      break;
-
-    case SQLITE_CANTOPEN:
-      // TODO(erikwright): Figure out what this means.
-      break;
-
-    case SQLITE_IOERR:
-      // This could be broken blocks, in which case deleting the DB would be a
-      // good idea. But it might also be transient.
-      // TODO(erikwright): Figure out if we can distinguish between the two,
-      // or determine through metrics analysis to what extent these failures are
-      // transient.
-      break;
-
-    case SQLITE_BUSY:
-      // Presumably transient.
-      break;
-
-    case SQLITE_TOOBIG:
-    case SQLITE_FULL:
-    case SQLITE_NOMEM:
-      // Not a problem with the database.
-      break;
-
-    case SQLITE_READONLY:
-      // Presumably either transient or we don't have the privileges to
-      // move/delete the file anyway.
-      break;
-
-    case SQLITE_CONSTRAINT:
-    case SQLITE_ERROR:
-      // These probably indicate a programming error or a migration failure that
-      // we prefer not to mask.
-      break;
-
-    case SQLITE_LOCKED:
-    case SQLITE_INTERNAL:
-    case SQLITE_PERM:
-    case SQLITE_ABORT:
-    case SQLITE_INTERRUPT:
-    case SQLITE_NOTFOUND:
-    case SQLITE_PROTOCOL:
-    case SQLITE_EMPTY:
-    case SQLITE_SCHEMA:
-    case SQLITE_MISMATCH:
-    case SQLITE_MISUSE:
-    case SQLITE_NOLFS:
-    case SQLITE_AUTH:
-    case SQLITE_FORMAT:
-    case SQLITE_RANGE:
-    case SQLITE_ROW:
-      // None of these appear in error reports, so for now let's not try to
-      // guess at how to handle them.
-      break;
-  }
+  sql::LogAndRecordErrorInHistogram<HistogramUniquifier>(error, connection);
+
+  // Do not attempt to kill database more than once. If the first time failed,
+  // it is unlikely that a second time will be successful.
+  if (!attempted_to_kill_database_ && sql::IsErrorCatastrophic(error)) {
+    attempted_to_kill_database_ = true;
 
-  if (delete_db && backend_) {
     // Don't just do the close/delete here, as we are being called by |db| and
     // that seems dangerous.
     MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(&Backend::KillDatabase, backend_));
-
-    // 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;
@@ -614,8 +550,7 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
   }
 
   db_.reset(new sql::Connection);
-  db_->set_error_delegate(
-      new KillDatabaseErrorDelegate(this, GetErrorHandlerForCookieDb()));
+  db_->set_error_delegate(new KillDatabaseErrorDelegate(this));
 
   if (!db_->Open(path_)) {
     NOTREACHED() << "Unable to open cookie DB.";
diff --git a/sql/connection.h b/sql/connection.h
index 1aef064..319517e 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -83,17 +83,17 @@ class SQL_EXPORT ErrorDelegate {
  public:
   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
-  // our best guess at the statement that triggered the error.  Do not store
-  // these pointers.
+  // |error| is an sqlite result code as seen in sqlite3.h. |connection| is the
+  // db connection where the error happened and |stmt| is our best guess at the
+  // statement that triggered the error. Do not store these pointers.
   //
   // |stmt| MAY BE NULL if there is no statement causing the problem (i.e. on
   // initialization).
   //
-  // If the error condition has been fixed an the original statement succesfuly
-  // re-tried then returning SQLITE_OK is appropiate; otherwise is recomended
-  // that you return the original |error| or the appropiae error code.
+  // If the error condition has been fixed and the original statement succesfuly
+  // re-tried then returning SQLITE_OK is appropriate; otherwise it is
+  // recommended that you return the original |error| or the appropriate error
+  // code.
   virtual int OnError(int error, Connection* connection, Statement* stmt) = 0;
 };
 
diff --git a/sql/diagnostic_error_delegate.h b/sql/diagnostic_error_delegate.h
index 4b8ce32..78b3d9d 100644
--- a/sql/diagnostic_error_delegate.h
+++ b/sql/diagnostic_error_delegate.h
@@ -6,8 +6,8 @@
 #define SQL_DIAGNOSTIC_ERROR_DELEGATE_H_
 
 #include "base/logging.h"
-#include "base/metrics/histogram.h"
 #include "sql/connection.h"
+#include "sql/error_delegate_util.h"
 #include "sql/sql_export.h"
 
 namespace sql {
@@ -15,12 +15,8 @@ namespace sql {
 // This class handles the exceptional sqlite errors that we might encounter
 // if for example the db is corrupted. Right now we just generate a UMA
 // histogram for release and an assert for debug builds.
-//
-// Why is it a template you ask? well, that is a funny story. The histograms
-// need to be singletons that is why they are always static at the function
-// scope, but we cannot use the Singleton class because they are not default
-// constructible. The template parameter makes the compiler to create unique
-// classes that don't share the same static variable.
+// See error_delegate_util.h for an explanation as to why this class is a
+// template.
 template <class UniqueT>
 class DiagnosticErrorDelegate : public ErrorDelegate {
  public:
@@ -29,24 +25,12 @@ class DiagnosticErrorDelegate : public ErrorDelegate {
 
   virtual int OnError(int error, Connection* connection,
                       Statement* stmt) {
-    LOG(ERROR) << "sqlite error " << error
-               << ", errno " << connection->GetLastErrno()
-               << ": " << connection->GetErrorMessage();
-    RecordErrorInHistogram(error);
+    LogAndRecordErrorInHistogram<UniqueT>(error, connection);
     return error;
   }
 
  private:
-  static void RecordErrorInHistogram(int error) {
-    // Trim off the extended error codes.
-    error &= 0xff;
-
-    // The histogram values from sqlite result codes go currently from 1 to
-    // 26 currently but 50 gives them room to grow.
-    UMA_HISTOGRAM_ENUMERATION(UniqueT::name(), error, 50);
-  }
-
- DISALLOW_COPY_AND_ASSIGN(DiagnosticErrorDelegate);
+  DISALLOW_COPY_AND_ASSIGN(DiagnosticErrorDelegate);
 };
 
 }  // namespace sql
diff --git a/sql/error_delegate_util.cc b/sql/error_delegate_util.cc
new file mode 100644
index 0000000..37fe006
--- /dev/null
+++ b/sql/error_delegate_util.cc
@@ -0,0 +1,80 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sql/error_delegate_util.h"
+
+#include "third_party/sqlite/sqlite3.h"
+
+namespace sql {
+
+bool IsErrorCatastrophic(int error) {
+  switch (error) {
+    case SQLITE_DONE:
+    case SQLITE_OK:
+      // Theoretically, the wrapped delegate might have resolved the error, and
+      // we would end up here.
+      return false;
+
+    case SQLITE_CORRUPT:
+    case SQLITE_NOTADB:
+      // Highly unlikely we would ever recover from these.
+      return true;
+
+    case SQLITE_CANTOPEN:
+      // TODO(erikwright): Figure out what this means.
+      return false;
+
+    case SQLITE_IOERR:
+      // This could be broken blocks, in which case deleting the DB would be a
+      // good idea. But it might also be transient.
+      // TODO(erikwright): Figure out if we can distinguish between the two,
+      // or determine through metrics analysis to what extent these failures are
+      // transient.
+      return false;
+
+    case SQLITE_BUSY:
+      // Presumably transient.
+      return false;
+
+    case SQLITE_TOOBIG:
+    case SQLITE_FULL:
+    case SQLITE_NOMEM:
+      // Not a problem with the database.
+      return false;
+
+    case SQLITE_READONLY:
+      // Presumably either transient or we don't have the privileges to
+      // move/delete the file anyway.
+      return false;
+
+    case SQLITE_CONSTRAINT:
+    case SQLITE_ERROR:
+      // These probgably indicate a programming error or a migration failure
+      // that we prefer not to mask.
+      return false;
+
+    case SQLITE_LOCKED:
+    case SQLITE_INTERNAL:
+    case SQLITE_PERM:
+    case SQLITE_ABORT:
+    case SQLITE_INTERRUPT:
+    case SQLITE_NOTFOUND:
+    case SQLITE_PROTOCOL:
+    case SQLITE_EMPTY:
+    case SQLITE_SCHEMA:
+    case SQLITE_MISMATCH:
+    case SQLITE_MISUSE:
+    case SQLITE_NOLFS:
+    case SQLITE_AUTH:
+    case SQLITE_FORMAT:
+    case SQLITE_RANGE:
+    case SQLITE_ROW:
+      // None of these appear in error reports, so for now let's not try to
+      // guess at how to handle them.
+      return false;
+  }
+  return false;
+}
+
+}  // namespace sql
diff --git a/sql/error_delegate_util.h b/sql/error_delegate_util.h
new file mode 100644
index 0000000..6b90ccf
--- /dev/null
+++ b/sql/error_delegate_util.h
@@ -0,0 +1,41 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SQL_ERROR_DELEGATE_UTIL_H_
+#define SQL_ERROR_DELEGATE_UTIL_H_
+
+#include "base/metrics/histogram.h"
+#include "sql/connection.h"
+#include "sql/sql_export.h"
+
+namespace sql {
+
+// Returns true if it is highly unlikely that the database can recover from
+// |error|.
+SQL_EXPORT bool IsErrorCatastrophic(int error);
+
+// Log error in console in debug mode and generate a UMA histogram in release
+// mode for |error| for |UniqueT::name()|.
+// This function is templated because histograms need to be singletons. That is
+// why they are always static at the function scope. The template parameter
+// makes the compiler create unique functions that don't share the same static
+// variable.
+template <class UniqueT>
+void LogAndRecordErrorInHistogram(int error,
+                                  sql::Connection* connection) {
+  LOG(ERROR) << "sqlite error " << error
+             << ", errno " << connection->GetLastErrno()
+             << ": " << connection->GetErrorMessage();
+
+  // Trim off the extended error codes.
+  error &= 0xff;
+
+  // The histogram values from sqlite result codes currently go from 1 to 26
+  // but 50 gives them room to grow.
+  UMA_HISTOGRAM_ENUMERATION(UniqueT::name(), error, 50);
+}
+
+}  // namespace sql
+
+#endif  // SQL_ERROR_DELEGATE_UTIL_H_
diff --git a/sql/sql.gyp b/sql/sql.gyp
index 72d361c..86962b3 100644
--- a/sql/sql.gyp
+++ b/sql/sql.gyp
@@ -19,6 +19,8 @@
         'connection.cc',
         'connection.h',
         'diagnostic_error_delegate.h',
+        'error_delegate_util.cc',
+        'error_delegate_util.h',
         'init_status.h',
         'meta_table.cc',
         'meta_table.h',