Bake targeted error histogram support directly into sql::Connection.

Previously there was a convoluted template system to unique histogram names for purposes of tracking database errors separately.  This was needed due to the static cache used by the histogram macros.  Instead, just log the histogram manually without using a static cache.

Additionally, pull the histogram functionality right up into the connection, rather than requiring formalized delegation to get it.

BUG=none

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

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

15 files changed, 48 insertions, 109 deletions

diff --git a/chrome/browser/diagnostics/sqlite_diagnostics.cc b/chrome/browser/diagnostics/sqlite_diagnostics.cc
index feb67d3..dc0d07f 100644
--- a/chrome/browser/diagnostics/sqlite_diagnostics.cc
+++ b/chrome/browser/diagnostics/sqlite_diagnostics.cc
@@ -15,7 +15,6 @@
 #include "chrome/common/chrome_paths.h"
 #include "content/public/common/content_constants.h"
 #include "sql/connection.h"
-#include "sql/diagnostic_error_delegate.h"
 #include "sql/statement.h"
 #include "third_party/sqlite/sqlite3.h"
 #include "webkit/appcache/appcache_interfaces.h"
@@ -88,35 +87,8 @@ class SqliteIntegrityTest : public DiagnosticTest {
   DISALLOW_COPY_AND_ASSIGN(SqliteIntegrityTest);
 };
 
-// Uniquifier to use the sql::DiagnosticErrorDelegate template which
-// requires a static name() method.
-template <size_t unique>
-class HistogramUniquifier {
- public:
-  static const char* name() {
-    const char* kHistogramNames[] = {
-      "Sqlite.Thumbnail.Error",
-      "Sqlite.Text.Error",
-      "Sqlite.Web.Error"
-    };
-    return kHistogramNames[unique];
-  }
-};
-
 }  // namespace
 
-sql::ErrorDelegate* GetErrorHandlerForThumbnailDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<0> >();
-}
-
-sql::ErrorDelegate* GetErrorHandlerForTextDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<1> >();
-}
-
-sql::ErrorDelegate* GetErrorHandlerForWebDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier<2> >();
-}
-
 DiagnosticTest* MakeSqliteWebDbTest() {
   return new SqliteIntegrityTest(true, ASCIIToUTF16("Web DB"),
                                  FilePath(chrome::kWebDataFilename));
diff --git a/chrome/browser/diagnostics/sqlite_diagnostics.h b/chrome/browser/diagnostics/sqlite_diagnostics.h
index 395f5ba..2017c4b 100644
--- a/chrome/browser/diagnostics/sqlite_diagnostics.h
+++ b/chrome/browser/diagnostics/sqlite_diagnostics.h
@@ -7,16 +7,6 @@
 
 #include "chrome/browser/diagnostics/diagnostics_test.h"
 
-namespace sql {
-  class ErrorDelegate;
-}
-
-// The following three factories create the error handlers that we use when
-// issuing sqlite commands during normal browser operation.
-sql::ErrorDelegate* GetErrorHandlerForThumbnailDb();
-sql::ErrorDelegate* GetErrorHandlerForTextDb();
-sql::ErrorDelegate* GetErrorHandlerForWebDb();
-
 // Factories for the db integrity tests we run in diagnostic mode.
 DiagnosticTest* MakeSqliteWebDbTest();
 DiagnosticTest* MakeSqliteCookiesDbTest();
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index 779e67e..d53f672 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -219,8 +219,6 @@ class KillHistoryDatabaseErrorDelegate : public sql::ErrorDelegate {
   virtual int OnError(int error,
                       sql::Connection* connection,
                       sql::Statement* stmt) OVERRIDE {
-    sql::LogAndRecordErrorInHistogram<HistogramUniquifier>(error, connection);
-
     // Do not schedule killing database more than once. If the first time
     // failed, it is unlikely that a second time will be successful.
     if (!scheduled_killing_database_ && sql::IsErrorCatastrophic(error)) {
@@ -242,11 +240,6 @@ class KillHistoryDatabaseErrorDelegate : public sql::ErrorDelegate {
   }
 
  private:
-  class HistogramUniquifier {
-   public:
-    static const char* name() { return "Sqlite.History.Error"; }
-  };
-
   // Do not increment the count on |HistoryBackend| as that would create a
   // circular reference (HistoryBackend -> HistoryDatabase -> Connection ->
   // ErrorDelegate -> HistoryBackend).
diff --git a/chrome/browser/history/history_database.cc b/chrome/browser/history/history_database.cc
index 348683a..e4a5920 100644
--- a/chrome/browser/history/history_database.cc
+++ b/chrome/browser/history/history_database.cc
@@ -68,6 +68,8 @@ HistoryDatabase::~HistoryDatabase() {
 
 sql::InitStatus HistoryDatabase::Init(const FilePath& history_name,
                                       sql::ErrorDelegate* error_delegate) {
+  db_.set_error_histogram_name("Sqlite.History.Error");
+
   // Set the exceptional sqlite error handler.
   db_.set_error_delegate(error_delegate);
 
diff --git a/chrome/browser/history/text_database.cc b/chrome/browser/history/text_database.cc
index e9bb3c7..9d179be 100644
--- a/chrome/browser/history/text_database.cc
+++ b/chrome/browser/history/text_database.cc
@@ -130,8 +130,7 @@ bool TextDatabase::Init() {
       return false;
   }
 
-  // Set the exceptional sqlite error handler.
-  db_.set_error_delegate(GetErrorHandlerForTextDb());
+  db_.set_error_histogram_name("Sqlite.Text.Error");
 
   // Set the database page size to something a little larger to give us
   // better performance (we're typically seek rather than bandwidth limited).
diff --git a/chrome/browser/history/thumbnail_database.cc b/chrome/browser/history/thumbnail_database.cc
index af07679..ae7c82e 100644
--- a/chrome/browser/history/thumbnail_database.cc
+++ b/chrome/browser/history/thumbnail_database.cc
@@ -219,8 +219,7 @@ sql::InitStatus ThumbnailDatabase::Init(
 
 sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db,
                                                 const FilePath& db_name) {
-  // Set the exceptional sqlite error handler.
-  db->set_error_delegate(GetErrorHandlerForThumbnailDb());
+  db->set_error_histogram_name("Sqlite.Thumbnail.Error");
 
   // Thumbnails db now only stores favicons, so we don't need that big a page
   // size or cache.
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index dce0ce4..17afb1e 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -374,7 +374,7 @@ bool TopSitesDatabase::RemoveURL(const MostVisitedURL& url) {
 sql::Connection* TopSitesDatabase::CreateDB(const FilePath& db_name) {
   scoped_ptr<sql::Connection> db(new sql::Connection());
   // Settings copied from ThumbnailDatabase.
-  db->set_error_delegate(GetErrorHandlerForThumbnailDb());
+  db->set_error_histogram_name("Sqlite.Thumbnail.Error");
   db->set_page_size(4096);
   db->set_cache_size(32);
 
diff --git a/chrome/browser/media_gallery/media_gallery_database.cc b/chrome/browser/media_gallery/media_gallery_database.cc
index 1fdaaa3..76089f1 100644
--- a/chrome/browser/media_gallery/media_gallery_database.cc
+++ b/chrome/browser/media_gallery/media_gallery_database.cc
@@ -9,7 +9,6 @@
 
 #include "base/file_path.h"
 #include "base/logging.h"
-#include "sql/diagnostic_error_delegate.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
 
@@ -31,13 +30,6 @@ const int kCompatibleVersionNumber = 1;
 const FilePath::CharType kMediaGalleryDatabaseName[] =
     FILE_PATH_LITERAL(".media_gallery.db");
 
-class HistogramName {
- public:
-  static const char* name() {
-    return "Sqlite.MediaGallery.Error";
-  }
-};
-
 }  // namespace
 
 MediaGalleryDatabase::MediaGalleryDatabase() { }
@@ -45,8 +37,7 @@ MediaGalleryDatabase::MediaGalleryDatabase() { }
 MediaGalleryDatabase::~MediaGalleryDatabase() { }
 
 sql::InitStatus MediaGalleryDatabase::Init(const FilePath& database_dir) {
-  // Set the exceptional sqlite error handler.
-  db_.set_error_delegate(new sql::DiagnosticErrorDelegate<HistogramName>());
+  db_.set_error_histogram_name("Sqlite.MediaGallery.Error");
 
   // Set the database page size to something a little larger to give us
   // better performance (we're typically seek rather than bandwidth limited).
diff --git a/chrome/browser/net/sqlite_persistent_cookie_store.cc b/chrome/browser/net/sqlite_persistent_cookie_store.cc
index 611b8e0..370a9ef 100644
--- a/chrome/browser/net/sqlite_persistent_cookie_store.cc
+++ b/chrome/browser/net/sqlite_persistent_cookie_store.cc
@@ -108,7 +108,7 @@ class SQLitePersistentCookieStore::Backend
 
   class KillDatabaseErrorDelegate : public sql::ErrorDelegate {
    public:
-    KillDatabaseErrorDelegate(Backend* backend);
+    explicit KillDatabaseErrorDelegate(Backend* backend);
 
     virtual ~KillDatabaseErrorDelegate() {}
 
@@ -118,12 +118,6 @@ class SQLitePersistentCookieStore::Backend
                         sql::Statement* stmt) OVERRIDE;
 
    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_;
@@ -291,8 +285,6 @@ KillDatabaseErrorDelegate(Backend* backend)
 
 int SQLitePersistentCookieStore::Backend::KillDatabaseErrorDelegate::OnError(
     int error, sql::Connection* connection, sql::Statement* stmt) {
-  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)) {
@@ -549,6 +541,7 @@ bool SQLitePersistentCookieStore::Backend::InitializeDatabase() {
     UMA_HISTOGRAM_COUNTS("Cookie.DBSizeInKB", db_size / 1024 );
 
   db_.reset(new sql::Connection);
+  db_->set_error_histogram_name("Sqlite.Cookie.Error");
   db_->set_error_delegate(new KillDatabaseErrorDelegate(this));
 
   if (!db_->Open(path_)) {
diff --git a/chrome/browser/webdata/web_database.cc b/chrome/browser/webdata/web_database.cc
index fac52f5..38f898d 100644
--- a/chrome/browser/webdata/web_database.cc
+++ b/chrome/browser/webdata/web_database.cc
@@ -6,7 +6,6 @@
 
 #include <algorithm>
 
-#include "chrome/browser/diagnostics/sqlite_diagnostics.h"
 #include "chrome/browser/webdata/autofill_table.h"
 #include "chrome/browser/webdata/keyword_table.h"
 #include "chrome/browser/webdata/logins_table.h"
@@ -96,8 +95,7 @@ sql::InitStatus WebDatabase::Init(const FilePath& db_name) {
   if (!content::NotificationService::current())
     notification_service_.reset(content::NotificationService::Create());
 
-  // Set the exceptional sqlite error handler.
-  db_.set_error_delegate(GetErrorHandlerForWebDb());
+  db_.set_error_histogram_name("Sqlite.Web.Error");
 
   // We don't store that much data in the tables so use a small page size.
   // This provides a large benefit for empty tables (which is very likely with
diff --git a/sql/connection.cc b/sql/connection.cc
index 99bc381..e7fa87c 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -653,8 +653,34 @@ void Connection::ClearCache() {
 }
 
 int Connection::OnSqliteError(int err, sql::Statement *stmt) {
+  // Strip extended error codes.
+  int base_err = err&0xff;
+
+  static size_t kSqliteErrorMax = 50;
+  UMA_HISTOGRAM_ENUMERATION("Sqlite.Error", base_err, kSqliteErrorMax);
+  if (!error_histogram_name_.empty()) {
+    // TODO(shess): The histogram macros create a bit of static
+    // storage for caching the histogram object.  Since SQLite is
+    // being used for I/O, generally without error, this code
+    // shouldn't execute often enough for such caching to be crucial.
+    // If it becomes an issue, the object could be cached alongside
+    // error_histogram_name_.
+    base::Histogram* histogram =
+        base::LinearHistogram::FactoryGet(
+            error_histogram_name_, 1, kSqliteErrorMax, kSqliteErrorMax + 1,
+            base::Histogram::kUmaTargetedHistogramFlag);
+    if (histogram)
+      histogram->Add(base_err);
+  }
+
+  // Always log the error.
+  LOG(ERROR) << "sqlite error " << err
+             << ", errno " << GetLastErrno()
+             << ": " << GetErrorMessage();
+
   if (error_delegate_.get())
     return error_delegate_->OnError(err, this, stmt);
+
   // The default handling is to assert on debug and to ignore on release.
   DLOG(FATAL) << GetErrorMessage();
   return err;
diff --git a/sql/connection.h b/sql/connection.h
index 8cf4d71..b9f45ec 100644
--- a/sql/connection.h
+++ b/sql/connection.h
@@ -143,6 +143,13 @@ class SQL_EXPORT Connection {
     error_delegate_.reset(delegate);
   }
 
+  // SQLite error codes for errors on all connections are logged to
+  // enum histogram "Sqlite.Error".  Setting this additionally logs
+  // errors to the histogram |name|.
+  void set_error_histogram_name(const std::string& name) {
+    error_histogram_name_ = name;
+  }
+
   // Initialization ------------------------------------------------------------
 
   // Initializes the SQL connection for the given file, returning true if the
@@ -457,6 +464,9 @@ class SQL_EXPORT Connection {
   // commands or statements. It can be null which means default handling.
   scoped_ptr<ErrorDelegate> error_delegate_;
 
+  // Auxiliary error-code histogram.
+  std::string error_histogram_name_;
+
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };
 
diff --git a/webkit/appcache/appcache_database.cc b/webkit/appcache/appcache_database.cc
index 77a8f15..e8f4b0b 100644
--- a/webkit/appcache/appcache_database.cc
+++ b/webkit/appcache/appcache_database.cc
@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "sql/connection.h"
-#include "sql/diagnostic_error_delegate.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -138,15 +137,6 @@ const IndexInfo kIndexes[] = {
 const int kTableCount = ARRAYSIZE_UNSAFE(kTables);
 const int kIndexCount = ARRAYSIZE_UNSAFE(kIndexes);
 
-class HistogramUniquifier {
- public:
-  static const char* name() { return "Sqlite.AppCache.Error"; }
-};
-
-sql::ErrorDelegate* GetErrorHandlerForAppCacheDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier>();
-}
-
 bool CreateTable(sql::Connection* db, const TableInfo& info) {
   std::string sql("CREATE TABLE ");
   sql += info.table_name;
@@ -972,7 +962,7 @@ bool AppCacheDatabase::LazyOpen(bool create_if_needed) {
   db_.reset(new sql::Connection);
   meta_table_.reset(new sql::MetaTable);
 
-  db_->set_error_delegate(GetErrorHandlerForAppCacheDb());
+  db_->set_error_histogram_name("Sqlite.AppCache.Error");
 
   bool opened = false;
   if (use_in_memory_db) {
diff --git a/webkit/database/database_tracker.cc b/webkit/database/database_tracker.cc
index 2daeef1..824c8a2 100644
--- a/webkit/database/database_tracker.cc
+++ b/webkit/database/database_tracker.cc
@@ -16,7 +16,6 @@
 #include "base/utf_string_conversions.h"
 #include "net/base/net_errors.h"
 #include "sql/connection.h"
-#include "sql/diagnostic_error_delegate.h"
 #include "sql/meta_table.h"
 #include "sql/transaction.h"
 #include "third_party/sqlite/sqlite3.h"
@@ -26,19 +25,6 @@
 #include "webkit/quota/quota_manager.h"
 #include "webkit/quota/special_storage_policy.h"
 
-namespace {
-
-class HistogramUniquifier {
- public:
-  static const char* name() { return "Sqlite.DatabaseTracker.Error"; }
-};
-
-sql::ErrorDelegate* GetErrorHandlerForTrackerDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier>();
-}
-
-}  // anon namespace
-
 namespace webkit_database {
 
 const FilePath::CharType kDatabaseDirectoryName[] =
@@ -491,7 +477,7 @@ bool DatabaseTracker::LazyInit() {
         return false;
     }
 
-    db_->set_error_delegate(GetErrorHandlerForTrackerDb());
+    db_->set_error_histogram_name("Sqlite.DatabaseTracker.Error");
 
     databases_table_.reset(new DatabasesTable(db_.get()));
     meta_table_.reset(new sql::MetaTable());
diff --git a/webkit/quota/quota_database.cc b/webkit/quota/quota_database.cc
index c604c56..aaf6245 100644
--- a/webkit/quota/quota_database.cc
+++ b/webkit/quota/quota_database.cc
@@ -12,7 +12,6 @@
 #include "base/time.h"
 #include "googleurl/src/gurl.h"
 #include "sql/connection.h"
-#include "sql/diagnostic_error_delegate.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "sql/transaction.h"
@@ -30,15 +29,6 @@ const char kHostQuotaTable[] = "HostQuotaTable";
 const char kOriginInfoTable[] = "OriginInfoTable";
 const char kIsOriginTableBootstrapped[] = "IsOriginTableBootstrapped";
 
-class HistogramUniquifier {
- public:
-  static const char* name() { return "Sqlite.Quota.Error"; }
-};
-
-sql::ErrorDelegate* GetErrorHandlerForQuotaDb() {
-  return new sql::DiagnosticErrorDelegate<HistogramUniquifier>();
-}
-
 bool VerifyValidQuotaConfig(const char* key) {
   return (key != NULL &&
           (!strcmp(key, QuotaDatabase::kDesiredAvailableSpaceKey) ||
@@ -457,7 +447,7 @@ bool QuotaDatabase::LazyOpen(bool create_if_needed) {
   db_.reset(new sql::Connection);
   meta_table_.reset(new sql::MetaTable);
 
-  db_->set_error_delegate(GetErrorHandlerForQuotaDb());
+  db_->set_error_histogram_name("Sqlite.Quota.Error");
 
   bool opened = false;
   if (in_memory_only) {