[tracing] Add separate dump provider for sql connection

The sql connection memory dump is not thread safe since the connections
can get deleted while a dump is happening. To make this thread safe,
this CL introduces a dump provider class owned by the connection. This
class holds a lock when dumping and deleting the database. Also, to
workaround thread safe dump provider registration, it uses the
UnregisterAndDeleteDumpProviderAsync api added in crrev.com/1430073002.

BUG=466141

Review URL: https://codereview.chromium.org/1434993002

Cr-Commit-Position: refs/heads/master@{#369161}

sql/BUILD.gn

@@ -8,6 +8,8 @@ component("sql") {
   sources = [
     "connection.cc",
     "connection.h",
+    "connection_memory_dump_provider.cc",
+    "connection_memory_dump_provider.h",
     "error_delegate_util.cc",
     "error_delegate_util.h",
     "init_status.h",

sql/connection.cc

@@ -27,7 +27,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_manager.h"
-#include "base/trace_event/process_memory_dump.h"
+#include "sql/connection_memory_dump_provider.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
 #include "third_party/sqlite/sqlite3.h"
@@ -254,44 +254,6 @@ bool Connection::ShouldIgnoreSqliteCompileError(int error) {
       basic_error == SQLITE_CORRUPT;
 }
 
-bool Connection::OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
-                              base::trace_event::ProcessMemoryDump* pmd) {
-  if (args.level_of_detail ==
-          base::trace_event::MemoryDumpLevelOfDetail::LIGHT ||
-      !db_) {
-    return true;
-  }
-
-  // The high water mark is not tracked for the following usages.
-  int cache_size, dummy_int;
-  sqlite3_db_status(db_, SQLITE_DBSTATUS_CACHE_USED, &cache_size, &dummy_int,
-                    0 /* resetFlag */);
-  int schema_size;
-  sqlite3_db_status(db_, SQLITE_DBSTATUS_SCHEMA_USED, &schema_size, &dummy_int,
-                    0 /* resetFlag */);
-  int statement_size;
-  sqlite3_db_status(db_, SQLITE_DBSTATUS_STMT_USED, &statement_size, &dummy_int,
-                    0 /* resetFlag */);
-
-  std::string name = base::StringPrintf(
-      "sqlite/%s_connection/%p",
-      histogram_tag_.empty() ? "Unknown" : histogram_tag_.c_str(), this);
-  base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(name);
-  dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
-                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                  cache_size + schema_size + statement_size);
-  dump->AddScalar("cache_size",
-                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                  cache_size);
-  dump->AddScalar("schema_size",
-                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                  schema_size);
-  dump->AddScalar("statement_size",
-                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
-                  statement_size);
-  return true;
-}
-
 void Connection::ReportDiagnosticInfo(int extended_error, Statement* stmt) {
   AssertIOAllowed();
 
@@ -386,13 +348,9 @@ Connection::Connection()
       update_time_histogram_(NULL),
       query_time_histogram_(NULL),
       clock_(new TimeSource()) {
-  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
-      this, "sql::Connection", nullptr);
 }
 
 Connection::~Connection() {
-  base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
-      this);
   Close();
 }
 
@@ -511,6 +469,17 @@ void Connection::CloseInternal(bool forced) {
     // of the function. http://crbug.com/136655.
     AssertIOAllowed();
 
+    // Reseting acquires a lock to ensure no dump is happening on the database
+    // at the same time. Unregister takes ownership of provider and it is safe
+    // since the db is reset. memory_dump_provider_ could be null if db_ was
+    // poisoned.
+    if (memory_dump_provider_) {
+      memory_dump_provider_->ResetDatabase();
+      base::trace_event::MemoryDumpManager::GetInstance()
+          ->UnregisterAndDeleteDumpProviderSoon(
+              std::move(memory_dump_provider_));
+    }
+
     int rc = sqlite3_close(db_);
     if (rc != SQLITE_OK) {
       UMA_HISTOGRAM_SPARSE_SLOWLY("Sqlite.CloseFailure", rc);
@@ -1848,6 +1817,12 @@ bool Connection::OpenInternal(const std::string& file_name,
       mmap_enabled_ = true;
   }
 
+  DCHECK(!memory_dump_provider_);
+  memory_dump_provider_.reset(
+      new ConnectionMemoryDumpProvider(db_, histogram_tag_));
+  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
+      memory_dump_provider_.get(), "sql::Connection", nullptr);
+
   return true;
 }
 

sql/connection.h

@@ -20,7 +20,6 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
-#include "base/trace_event/memory_dump_provider.h"
 #include "sql/sql_export.h"
 
 struct sqlite3;
@@ -33,6 +32,7 @@ class HistogramBase;
 
 namespace sql {
 
+class ConnectionMemoryDumpProvider;
 class Recovery;
 class Statement;
 
@@ -105,7 +105,7 @@ class SQL_EXPORT TimeSource {
   DISALLOW_COPY_AND_ASSIGN(TimeSource);
 };
 
-class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider {
+class SQL_EXPORT Connection {
  private:
   class StatementRef;  // Forward declaration, see real one below.
 
@@ -113,7 +113,7 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider {
   // The database is opened by calling Open[InMemory](). Any uncommitted
   // transactions will be rolled back when this object is deleted.
   Connection();
-  ~Connection() override;
+  ~Connection();
 
   // Pre-init configuration ----------------------------------------------------
 
@@ -484,11 +484,6 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider {
   // with the syntax of a SQL statement, or problems with the database schema.
   static bool ShouldIgnoreSqliteCompileError(int error);
 
-  // base::trace_event::MemoryDumpProvider implementation.
-  bool OnMemoryDump(
-      const base::trace_event::MemoryDumpArgs& args,
-      base::trace_event::ProcessMemoryDump* process_memory_dump) override;
-
   // Collect various diagnostic information and post a crash dump to aid
   // debugging.  Dump rate per database is limited to prevent overwhelming the
   // crash server.
@@ -511,6 +506,7 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider {
 
   FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, CollectDiagnosticInfo);
   FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, GetAppropriateMmapSize);
+  FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, OnMemoryDump);
   FRIEND_TEST_ALL_PREFIXES(SQLConnectionTest, RegisterIntentToUpload);
 
   // Internal initialize function used by both Init and InitInMemory. The file
@@ -795,6 +791,9 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider {
   // changes.
   scoped_ptr<TimeSource> clock_;
 
+  // Stores the dump provider object when db is open.
+  scoped_ptr<ConnectionMemoryDumpProvider> memory_dump_provider_;
+
   DISALLOW_COPY_AND_ASSIGN(Connection);
 };
 

sql/connection_memory_dump_provider.cc

@@ -0,0 +1,74 @@
+// Copyright 2015 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/connection_memory_dump_provider.h"
+
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/process_memory_dump.h"
+#include "third_party/sqlite/sqlite3.h"
+
+namespace sql {
+
+ConnectionMemoryDumpProvider::ConnectionMemoryDumpProvider(
+    sqlite3* db,
+    const std::string& name)
+    : db_(db), connection_name_(name) {}
+
+ConnectionMemoryDumpProvider::~ConnectionMemoryDumpProvider() {}
+
+void ConnectionMemoryDumpProvider::ResetDatabase() {
+  base::AutoLock lock(lock_);
+  db_ = nullptr;
+}
+
+bool ConnectionMemoryDumpProvider::OnMemoryDump(
+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  if (args.level_of_detail == base::trace_event::MemoryDumpLevelOfDetail::LIGHT)
+    return true;
+
+  int cache_size = 0;
+  int schema_size = 0;
+  int statement_size = 0;
+  {
+    // Lock is acquired here so that db_ is not reset in ResetDatabase when
+    // collecting stats.
+    base::AutoLock lock(lock_);
+    if (!db_) {
+      return false;
+    }
+
+    // The high water mark is not tracked for the following usages.
+    int dummy_int;
+    int status = sqlite3_db_status(db_, SQLITE_DBSTATUS_CACHE_USED, &cache_size,
+                                   &dummy_int, 0 /* resetFlag */);
+    DCHECK_EQ(SQLITE_OK, status);
+    status = sqlite3_db_status(db_, SQLITE_DBSTATUS_SCHEMA_USED, &schema_size,
+                               &dummy_int, 0 /* resetFlag */);
+    DCHECK_EQ(SQLITE_OK, status);
+    status = sqlite3_db_status(db_, SQLITE_DBSTATUS_STMT_USED, &statement_size,
+                               &dummy_int, 0 /* resetFlag */);
+    DCHECK_EQ(SQLITE_OK, status);
+  }
+
+  std::string name = base::StringPrintf(
+      "sqlite/%s_connection/%p",
+      connection_name_.empty() ? "Unknown" : connection_name_.c_str(), this);
+  base::trace_event::MemoryAllocatorDump* dump = pmd->CreateAllocatorDump(name);
+  dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  cache_size + schema_size + statement_size);
+  dump->AddScalar("cache_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  cache_size);
+  dump->AddScalar("schema_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  schema_size);
+  dump->AddScalar("statement_size",
+                  base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+                  statement_size);
+  return true;
+}
+
+}  // namespace sql

sql/connection_memory_dump_provider.h

@@ -0,0 +1,41 @@
+// Copyright 2015 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_CONNECTION_MEMORY_DUMP_PROVIDER_H
+#define SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H
+
+#include <string>
+
+#include "base/macros.h"
+#include "base/synchronization/lock.h"
+#include "base/trace_event/memory_dump_provider.h"
+
+struct sqlite3;
+
+namespace sql {
+
+class ConnectionMemoryDumpProvider
+    : public base::trace_event::MemoryDumpProvider {
+ public:
+  ConnectionMemoryDumpProvider(sqlite3* db, const std::string& name);
+  ~ConnectionMemoryDumpProvider() override;
+
+  void ResetDatabase();
+
+  // base::trace_event::MemoryDumpProvider implementation.
+  bool OnMemoryDump(
+      const base::trace_event::MemoryDumpArgs& args,
+      base::trace_event::ProcessMemoryDump* process_memory_dump) override;
+
+ private:
+  sqlite3* db_;  // not owned.
+  base::Lock lock_;
+  std::string connection_name_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConnectionMemoryDumpProvider);
+};
+
+}  // namespace sql
+
+#endif  // SQL_CONNECTION_MEMORY_DUMP_PROVIDER_H

sql/connection_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/test/histogram_tester.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "sql/connection.h"
+#include "sql/connection_memory_dump_provider.h"
 #include "sql/correct_sql_test_base.h"
 #include "sql/meta_table.h"
 #include "sql/statement.h"
@@ -1308,7 +1309,7 @@ TEST_F(SQLConnectionTest, OnMemoryDump) {
   base::trace_event::ProcessMemoryDump pmd(nullptr);
   base::trace_event::MemoryDumpArgs args = {
       base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
-  ASSERT_TRUE(db().OnMemoryDump(args, &pmd));
+  ASSERT_TRUE(db().memory_dump_provider_->OnMemoryDump(args, &pmd));
   EXPECT_GE(pmd.allocator_dumps().size(), 1u);
 }
 

sql/sql.gyp

@@ -22,6 +22,8 @@
       'sources': [
         'connection.cc',
         'connection.h',
+        'connection_memory_dump_provider.cc',
+        'connection_memory_dump_provider.h',
         'error_delegate_util.cc',
         'error_delegate_util.h',
         'init_status.h',

sql/sql_memory_dump_provider.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SQL_PROCESS_MEMORY_DUMP_PROVIDER_H
+#ifndef SQL_SQL_MEMORY_DUMP_PROVIDER_H
-#define SQL_PROCESS_MEMORY_DUMP_PROVIDER_H
+#define SQL_SQL_MEMORY_DUMP_PROVIDER_H
 
 #include "base/macros.h"
 #include "base/memory/singleton.h"
@@ -34,4 +34,4 @@ class SQL_EXPORT SqlMemoryDumpProvider
 
 }  // namespace sql
 
-#endif  // SQL_PROCESS_MEMORY_DUMP_PROVIDER_H
+#endif  // SQL_SQL_MEMORY_DUMP_PROVIDER_H