memory-infra: Enable LevelDB global dumps in background mode
The existing LocalStorageContextMojo creates a leveldb dump that
does not have size. The size is added by LevelDBDatabaseImpl on the
service side using a global dump. Since we need this size to be
attributed to local storage, the background mode should support
global dumps edges.
This CL enable support for global dumps in background mode by removing
if checks and updating the whitelist to explicitly allow global dump
keys.
Bug: 762639
Change-Id: I37ee050636529988acf56b220b3a892dbe45630b
Reviewed-on: https://chromium-review.googlesource.com/657383
Commit-Queue: Lalit Maganti <lalitm@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503422}diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 86874a5..6d4925b9 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -63,6 +63,7 @@
kWhitelistedMDPName, kBackgroundButNotSummaryWhitelistedMDPName, nullptr};
const char* const kTestMDPWhitelistForSummary[] = {kWhitelistedMDPName,
nullptr};
+
void RegisterDumpProvider(
MemoryDumpProvider* mdp,
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
diff --git a/base/trace_event/memory_infra_background_whitelist.cc b/base/trace_event/memory_infra_background_whitelist.cc
index 38101c3..9bc4447 100644
--- a/base/trace_event/memory_infra_background_whitelist.cc
+++ b/base/trace_event/memory_infra_background_whitelist.cc
@@ -9,6 +9,8 @@
#include <string>
+#include "base/strings/string_util.h"
+
namespace base {
namespace trace_event {
namespace {
@@ -258,6 +260,14 @@
}
bool IsMemoryAllocatorDumpNameWhitelisted(const std::string& name) {
+ // Global dumps are explicitly whitelisted for background use.
+ if (base::StartsWith(name, "global/", CompareCase::SENSITIVE)) {
+ for (size_t i = sizeof("global/"); i < name.size(); i++)
+ if (!base::IsHexDigit(name[i]))
+ return false;
+ return true;
+ }
+
// Remove special characters, numbers (including hexadecimal which are marked
// by '0x') from the given string.
const size_t length = name.size();
diff --git a/base/trace_event/process_memory_dump.cc b/base/trace_event/process_memory_dump.cc
index 82e55ac..17b7656f 100644
--- a/base/trace_event/process_memory_dump.cc
+++ b/base/trace_event/process_memory_dump.cc
@@ -244,14 +244,10 @@
MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
const MemoryAllocatorDumpGuid& guid) {
- // Global dumps are disabled in background mode.
- if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
- return GetBlackHoleMad();
-
// A shared allocator dump can be shared within a process and the guid could
// have been created already.
MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
- if (mad) {
+ if (mad && mad != black_hole_mad_.get()) {
// The weak flag is cleared because this method should create a non-weak
// dump.
mad->clear_flags(MemoryAllocatorDump::Flags::WEAK);
@@ -262,12 +258,8 @@
MemoryAllocatorDump* ProcessMemoryDump::CreateWeakSharedGlobalAllocatorDump(
const MemoryAllocatorDumpGuid& guid) {
- // Global dumps are disabled in background mode.
- if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
- return GetBlackHoleMad();
-
MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
- if (mad)
+ if (mad && mad != black_hole_mad_.get())
return mad;
mad = CreateAllocatorDump(GetSharedGlobalAllocatorDumpName(guid), guid);
mad->set_flags(MemoryAllocatorDump::Flags::WEAK);
diff --git a/base/trace_event/process_memory_dump_unittest.cc b/base/trace_event/process_memory_dump_unittest.cc
index 4bc16fb..af73720 100644
--- a/base/trace_event/process_memory_dump_unittest.cc
+++ b/base/trace_event/process_memory_dump_unittest.cc
@@ -35,7 +35,7 @@
const MemoryDumpArgs kDetailedDumpArgs = {MemoryDumpLevelOfDetail::DETAILED};
const char* const kTestDumpNameWhitelist[] = {
"Whitelisted/TestName", "Whitelisted/TestName_0x?",
- "Whitelisted/0x?/TestName", nullptr};
+ "Whitelisted/0x?/TestName", "Whitelisted/0x?", nullptr};
TracedValue* GetHeapDump(const ProcessMemoryDump& pmd, const char* name) {
auto it = pmd.heap_dumps().find(name);
@@ -429,17 +429,17 @@
EXPECT_EQ(black_hole_mad,
pmd->CreateAllocatorDump("Whitelisted/TestName/__12/Google"));
- // Global dumps.
- MemoryAllocatorDumpGuid guid(1);
- EXPECT_EQ(black_hole_mad, pmd->CreateSharedGlobalAllocatorDump(guid));
- EXPECT_EQ(black_hole_mad, pmd->CreateWeakSharedGlobalAllocatorDump(guid));
- EXPECT_EQ(black_hole_mad, pmd->GetSharedGlobalAllocatorDump(guid));
-
// Suballocations.
+ MemoryAllocatorDumpGuid guid(1);
pmd->AddSuballocation(guid, "malloc/allocated_objects");
EXPECT_EQ(0u, pmd->allocator_dumps_edges_.size());
EXPECT_EQ(0u, pmd->allocator_dumps_.size());
+ // Global dumps.
+ EXPECT_NE(black_hole_mad, pmd->CreateSharedGlobalAllocatorDump(guid));
+ EXPECT_NE(black_hole_mad, pmd->CreateWeakSharedGlobalAllocatorDump(guid));
+ EXPECT_NE(black_hole_mad, pmd->GetSharedGlobalAllocatorDump(guid));
+
// Valid dump names.
EXPECT_NE(black_hole_mad, pmd->CreateAllocatorDump("Whitelisted/TestName"));
EXPECT_NE(black_hole_mad,
@@ -450,6 +450,21 @@
// GetAllocatorDump is consistent.
EXPECT_EQ(black_hole_mad, pmd->GetAllocatorDump("NotWhitelisted/TestName"));
EXPECT_NE(black_hole_mad, pmd->GetAllocatorDump("Whitelisted/TestName"));
+
+ // Test whitelisted entries.
+ ASSERT_TRUE(IsMemoryAllocatorDumpNameWhitelisted("Whitelisted/TestName"));
+
+ // Global dumps should be whitelisted.
+ ASSERT_TRUE(IsMemoryAllocatorDumpNameWhitelisted("global/13456"));
+
+ // Global dumps with non-guids should not be.
+ ASSERT_FALSE(IsMemoryAllocatorDumpNameWhitelisted("global/random"));
+
+ // Random names should not.
+ ASSERT_FALSE(IsMemoryAllocatorDumpNameWhitelisted("NotWhitelisted/TestName"));
+
+ // Check hex processing.
+ ASSERT_TRUE(IsMemoryAllocatorDumpNameWhitelisted("Whitelisted/0xA1b2"));
}
#if defined(COUNT_RESIDENT_BYTES_SUPPORTED)
diff --git a/third_party/leveldatabase/env_chromium.cc b/third_party/leveldatabase/env_chromium.cc
index e2123e3..d6831ab7 100644
--- a/third_party/leveldatabase/env_chromium.cc
+++ b/third_party/leveldatabase/env_chromium.cc
@@ -1309,18 +1309,14 @@
public:
bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
base::trace_event::ProcessMemoryDump* pmd) override {
- // Don't dump in background mode ("from the field") until whitelisted.
- if (args.level_of_detail ==
- base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND) {
- return true;
- }
-
auto db_visitor = [](const base::trace_event::MemoryDumpArgs& args,
base::trace_event::ProcessMemoryDump* pmd,
TrackedDB* db) {
auto* dump = DBTracker::GetOrCreateAllocatorDump(pmd, db);
- // TODO(ssid): Do not add string attribute in background mode.
- dump->AddString("name", "", db->name());
+ if (args.level_of_detail !=
+ base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND) {
+ dump->AddString("name", "", db->name());
+ }
};
DBTracker::GetInstance()->VisitDatabases(
@@ -1357,10 +1353,6 @@
base::trace_event::MemoryAllocatorDump* DBTracker::GetOrCreateAllocatorDump(
base::trace_event::ProcessMemoryDump* pmd,
TrackedDB* db) {
- if (pmd->dump_args().level_of_detail ==
- base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND) {
- return nullptr;
- }
std::string dump_name = base::StringPrintf("leveldatabase/0x%" PRIXPTR,
reinterpret_cast<uintptr_t>(db));
auto* dump = pmd->GetAllocatorDump(dump_name);
diff --git a/third_party/leveldatabase/env_chromium.h b/third_party/leveldatabase/env_chromium.h
index f3d3e34..fb283d0 100644
--- a/third_party/leveldatabase/env_chromium.h
+++ b/third_party/leveldatabase/env_chromium.h
@@ -289,6 +289,7 @@
friend class ChromiumEnvDBTrackerTest;
FRIEND_TEST_ALL_PREFIXES(ChromiumEnvDBTrackerTest, IsTrackedDB);
+ FRIEND_TEST_ALL_PREFIXES(ChromiumEnvDBTrackerTest, GetOrCreateAllocatorDump);
DBTracker();
~DBTracker();
diff --git a/third_party/leveldatabase/env_chromium_unittest.cc b/third_party/leveldatabase/env_chromium_unittest.cc
index 6bb850b..5225087 100644
--- a/third_party/leveldatabase/env_chromium_unittest.cc
+++ b/third_party/leveldatabase/env_chromium_unittest.cc
@@ -15,12 +15,16 @@
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/test/test_suite.h"
+#include "base/trace_event/process_memory_dump.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/leveldatabase/env_chromium.h"
#include "third_party/leveldatabase/src/include/leveldb/db.h"
#define FPL FILE_PATH_LITERAL
+using base::trace_event::MemoryDumpArgs;
+using base::trace_event::MemoryDumpLevelOfDetail;
+using base::trace_event::ProcessMemoryDump;
using leveldb::DB;
using leveldb::Env;
using leveldb::ReadOptions;
@@ -291,6 +295,30 @@
base::ScopedTempDir scoped_temp_dir_;
};
+TEST_F(ChromiumEnvDBTrackerTest, GetOrCreateAllocatorDump) {
+ Options options;
+ options.create_if_missing = true;
+ std::string name = temp_path().AsUTF8Unsafe();
+ DBTracker::TrackedDB* tracked_db;
+ Status s = DBTracker::GetInstance()->OpenDatabase(options, name, &tracked_db);
+ ASSERT_TRUE(s.ok()) << s.ToString();
+
+ const MemoryDumpArgs detailed_args = {MemoryDumpLevelOfDetail::DETAILED};
+ ProcessMemoryDump pmd(nullptr, detailed_args);
+ auto* mad =
+ DBTracker::GetInstance()->GetOrCreateAllocatorDump(&pmd, tracked_db);
+ delete tracked_db;
+ ASSERT_TRUE(mad != nullptr);
+
+ // Check that the size was added.
+ auto& entries = mad->entries();
+ ASSERT_EQ(1ul, entries.size());
+ EXPECT_EQ(base::trace_event::MemoryAllocatorDump::kNameSize, entries[0].name);
+ EXPECT_EQ(base::trace_event::MemoryAllocatorDump::kUnitsBytes,
+ entries[0].units);
+ EXPECT_GE(entries[0].value_uint64, 0ul);
+}
+
TEST_F(ChromiumEnvDBTrackerTest, OpenDatabase) {
struct KeyValue {
const char* key;
