[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}
diff --git a/sql/BUILD.gn b/sql/BUILD.gn
index 58097b7..b79eca87 100644
--- a/sql/BUILD.gn
+++ b/sql/BUILD.gn
@@ -8,6 +8,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",
diff --git a/sql/connection.cc b/sql/connection.cc
index 9daa7d1..003005a 100644
--- a/sql/connection.cc
+++ b/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 @@
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 @@
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 @@
// 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 @@
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;
}
diff --git a/sql/connection.h b/sql/connection.h
index e121b9fe..b35e2fa 100644
--- a/sql/connection.h
+++ b/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 @@
namespace sql {
+class ConnectionMemoryDumpProvider;
class Recovery;
class Statement;
@@ -105,7 +105,7 @@
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 @@
// 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 @@
// 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 @@
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 @@
// 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);
};
diff --git a/sql/connection_memory_dump_provider.cc b/sql/connection_memory_dump_provider.cc
new file mode 100644
index 0000000..0284bd0
--- /dev/null
+++ b/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
diff --git a/sql/connection_memory_dump_provider.h b/sql/connection_memory_dump_provider.h
new file mode 100644
index 0000000..bcad0f8
--- /dev/null
+++ b/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
diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc
index d25f7c91..c6f80d84 100644
--- a/sql/connection_unittest.cc
+++ b/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 @@
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);
}
diff --git a/sql/sql.gyp b/sql/sql.gyp
index 362597e..2e02427d 100644
--- a/sql/sql.gyp
+++ b/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',
diff --git a/sql/sql_memory_dump_provider.h b/sql/sql_memory_dump_provider.h
index 01d7f04d..3d894e2 100644
--- a/sql/sql_memory_dump_provider.h
+++ b/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
-#define SQL_PROCESS_MEMORY_DUMP_PROVIDER_H
+#ifndef SQL_SQL_MEMORY_DUMP_PROVIDER_H
+#define SQL_SQL_MEMORY_DUMP_PROVIDER_H
#include "base/macros.h"
#include "base/memory/singleton.h"
@@ -34,4 +34,4 @@
} // namespace sql
-#endif // SQL_PROCESS_MEMORY_DUMP_PROVIDER_H
+#endif // SQL_SQL_MEMORY_DUMP_PROVIDER_H
