[tracing] Sanitize process memory dumps for background mode

base/trace_event/process_memory_dump.cc

Index: base/trace_event/process_memory_dump.cc
diff --git a/base/trace_event/process_memory_dump.cc b/base/trace_event/process_memory_dump.cc
index ada6aa449e210b58158c6e02223086751b90de4b..82698662fa66af316c38dcb938ca7f0d50bbba82 100644
--- a/base/trace_event/process_memory_dump.cc
+++ b/base/trace_event/process_memory_dump.cc
@@ -12,6 +12,8 @@
 #include "base/process/process_metrics.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/heap_profiler_heap_dump_writer.h"
+#include "base/trace_event/memory_dump_request_args.h"
+#include "base/trace_event/memory_infra_background_whitelist.h"
 #include "base/trace_event/process_memory_totals.h"
 #include "base/trace_event/trace_event_argument.h"
 #include "build/build_config.h"
@@ -148,28 +150,44 @@ size_t ProcessMemoryDump::CountResidentBytes(void* start_address,
 #endif  // defined(COUNT_RESIDENT_BYTES_SUPPORTED)
 
 ProcessMemoryDump::ProcessMemoryDump(
-    scoped_refptr<MemoryDumpSessionState> session_state)
+    scoped_refptr<MemoryDumpSessionState> session_state,
+    const MemoryDumpArgs& dump_args)
     : has_process_totals_(false),
       has_process_mmaps_(false),
-      session_state_(std::move(session_state)) {}
+      session_state_(std::move(session_state)),
+      dump_args_(dump_args) {
+  if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND) {
+    black_hole_mad_.reset(new MemoryAllocatorDump("dummy", this));

[Primiano Tucci (use gerrit), 2016/06/02 20:24:04]

I'd create this only if we need it. If we do everything right there will be no
need of the black hole, so you can avoid this malloc.
Maybe create an helper GetBlackHoleMad(). Also if you have that you can remove
all these DCHECK_NE and add a NOTREACHED in that.
Also I'd s/"dummy"/"discarded"/ just to give more hints about what this is
doing.

[ssid, 2016/06/03 01:59:46]

Added a get method. dcheck_ne was added because of tests, explained below.
Removed anyways.

+  }
+}
 
 ProcessMemoryDump::~ProcessMemoryDump() {}
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name) {
-  return AddAllocatorDumpInternal(
+  auto mad = AddAllocatorDumpInternal(
       WrapUnique(new MemoryAllocatorDump(absolute_name, this)));
+  DCHECK_NE(black_hole_mad_.get(), mad);
+  return mad;

[Primiano Tucci (use gerrit), 2016/06/02 20:24:04]

you can keep this as it was if you use the helper.

[ssid, 2016/06/03 01:59:46]

This is what I tried to explain in my previous comment.
I have added this dcheck here only because it makes it possible to add unittest
for the AddInternal method.
Now that there's bool i can remove this.

 }
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateAllocatorDump(
     const std::string& absolute_name,
     const MemoryAllocatorDumpGuid& guid) {
-  return AddAllocatorDumpInternal(
+  auto mad = AddAllocatorDumpInternal(
       WrapUnique(new MemoryAllocatorDump(absolute_name, this, guid)));
+  DCHECK_NE(black_hole_mad_.get(), mad);
+  return mad;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::AddAllocatorDumpInternal(
     std::unique_ptr<MemoryAllocatorDump> mad) {
+  // In background mode return dummy dump if invalid dump name is given.
+  if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND &&
+      !IsMemoryAllocatorDumpNameWhitelisted(mad->absolute_name())) {
+    return black_hole_mad_.get();
+  }
+
   auto insertion_result = allocator_dumps_.insert(
       std::make_pair(mad->absolute_name(), std::move(mad)));
   MemoryAllocatorDump* inserted_mad = insertion_result.first->second.get();
@@ -181,7 +199,11 @@ MemoryAllocatorDump* ProcessMemoryDump::AddAllocatorDumpInternal(
 MemoryAllocatorDump* ProcessMemoryDump::GetAllocatorDump(
     const std::string& absolute_name) const {
   auto it = allocator_dumps_.find(absolute_name);
-  return it == allocator_dumps_.end() ? nullptr : it->second.get();
+  if (it != allocator_dumps_.end())
+    return it->second.get();
+  return dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND

[Primiano Tucci (use gerrit), 2016/06/02 20:24:04]

nit: At this point stay consistent and keep doing:
if (... == background)
  return blackhole;
return nullptr.

[ssid, 2016/06/03 01:59:46]

Done.

+             ? black_hole_mad_.get()
+             : nullptr;
 }
 
 MemoryAllocatorDump* ProcessMemoryDump::GetOrCreateAllocatorDump(
@@ -192,6 +214,10 @@ MemoryAllocatorDump* ProcessMemoryDump::GetOrCreateAllocatorDump(
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
+  // Global dumps are disabled in background mode.
+  if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
+    return black_hole_mad_.get();
+
   // A shared allocator dump can be shared within a process and the guid could
   // have been created already.
   MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
@@ -206,6 +232,10 @@ MemoryAllocatorDump* ProcessMemoryDump::CreateSharedGlobalAllocatorDump(
 
 MemoryAllocatorDump* ProcessMemoryDump::CreateWeakSharedGlobalAllocatorDump(
     const MemoryAllocatorDumpGuid& guid) {
+  // Global dumps are disabled in background mode.
+  if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
+    return black_hole_mad_.get();
+
   MemoryAllocatorDump* mad = GetSharedGlobalAllocatorDump(guid);
   if (mad)
     return mad;
@@ -328,6 +358,10 @@ void ProcessMemoryDump::AddOwnershipEdge(
 
 void ProcessMemoryDump::AddSuballocation(const MemoryAllocatorDumpGuid& source,
                                          const std::string& target_node_name) {
+  // Do not create new dumps for suballocations in background mode.
+  if (dump_args_.level_of_detail == MemoryDumpLevelOfDetail::BACKGROUND)
+    return;

[Primiano Tucci (use gerrit), 2016/06/02 20:24:04]

+ NOTREACHED

[ssid, 2016/06/03 01:59:46]

Hmm, that needs an ugly fix of checking background mode before creating
suballocation in many dump providers. If you do not like silently ignoring this
part, I will just remove this change and create sub allocations.

[Primiano Tucci (use gerrit), 2016/06/03 16:24:36]

Ahh I thought you were not expecting suballocation to be created.
It's fine leave it as it is. If you expect some dumpers to still invoke
AddSuballocation, as long as they are not too many (so we don't waste cpu time)
it's ok to keep this silent.

+
   std::string child_mad_name = target_node_name + "/__" + source.ToString();
   MemoryAllocatorDump* target_child_mad = CreateAllocatorDump(child_mad_name);
   AddOwnershipEdge(source, target_child_mad->guid());