Fix crash in InvokeOnMemoryDump when tracing

components/tracing/common/process_metrics_memory_dump_provider.cc

Index: components/tracing/common/process_metrics_memory_dump_provider.cc
diff --git a/components/tracing/common/process_metrics_memory_dump_provider.cc b/components/tracing/common/process_metrics_memory_dump_provider.cc
index 147e8b33fde1d3f5eaaf2c0879f46cc48e1a9dee..165026a86e573fb98ad4887fcf41b6159b10e936 100644
--- a/components/tracing/common/process_metrics_memory_dump_provider.cc
+++ b/components/tracing/common/process_metrics_memory_dump_provider.cc
@@ -213,6 +213,11 @@ std::unique_ptr<base::ProcessMetrics> CreateProcessMetrics(
 // static
 uint64_t ProcessMetricsMemoryDumpProvider::rss_bytes_for_testing = 0;
 
+// static
+using FactoryFunction =
+    std::unique_ptr<ProcessMetricsMemoryDumpProvider> (*)(base::ProcessId);

[Primiano Tucci (use gerrit), 2017/03/15 12:44:40]

this (the using) seems redudant, as you have already in the header.

[hjd, 2017/03/15 14:23:12]

Doesn't seem visible here?

../../components/tracing/common/process_metrics_memory_dump_provider.cc:217:1:
error: unknown type name 'FactoryFunction'
FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr;
^

[Primiano Tucci (use gerrit), 2017/03/15 15:18:14]

well it's because, 
1: is private
2: you need the scope. So once you move it to the public section than you can
write something like:

ProcessMemoryMemoryDumpProvider::FactoryFunction
ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr;

(did I say ProcessMetricsMemoryDumpProvider already? :P)

+FactoryFunction ProcessMetricsMemoryDumpProvider::factory_for_testing = nullptr;

[Primiano Tucci (use gerrit), 2017/03/15 12:44:40]

this instead is necessary, yup

+
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 
 // static
@@ -540,25 +545,41 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
 #endif  // defined(OS_MACOSX)
 
 // static
-void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
+std::unique_ptr<ProcessMetricsMemoryDumpProvider>
+ProcessMetricsMemoryDumpProvider::CreateMetricsProvider(

[Primiano Tucci (use gerrit), 2017/03/15 12:44:40]

since this function is quite small and is used only once (I think) could you
just do this in the RegisterForProcess function and avoiding this extra helper?
Never been too much of a fun of splitting out functions that are used only once,
as requires the reader to jump across code. unless the original function grows
too big, but that doesn't seem to be the case here, RegisterForProcess is quite
small

[hjd, 2017/03/15 14:23:12]

Done.

     base::ProcessId process) {
+  if (factory_for_testing) {
+    std::unique_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider(

[Primiano Tucci (use gerrit), 2017/03/15 12:44:40]

just return factory_for_testing(process); should be enough, and more compact
right?
(If for any reason doesn't compile, just do return
std::move(factory_for_testing(process)))

[hjd, 2017/03/15 14:23:12]

thanks :)

+        factory_for_testing(process));
+    return metrics_provider;
+  }
   std::unique_ptr<ProcessMetricsMemoryDumpProvider> metrics_provider(
       new ProcessMetricsMemoryDumpProvider(process));
-  base::trace_event::MemoryDumpProvider::Options options;
-  options.target_pid = process;
-  options.is_fast_polling_supported = true;
-  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
-      metrics_provider.get(), "ProcessMemoryMetrics", nullptr, options);
+  return metrics_provider;

[Primiano Tucci (use gerrit), 2017/03/15 12:44:40]

here instead you can just do:
return MakeUnique<PMDP>(process)

[hjd, 2017/03/15 14:23:12]

Hmm, I think it doesn't work because the constructor is private? 

In file included from
../../components/tracing/common/process_metrics_memory_dump_provider.cc:17:
../../base/memory/ptr_util.h:56:33: error: calling a protected constructor of
class 'tracing::ProcessMetricsMemoryDumpProvider'
  return std::unique_ptr<T>(new T(std::forward<Args>(args)...));
                                ^
../../components/tracing/common/process_metrics_memory_dump_provider.cc:554:28:
note: in instantiation of function template specialization
'base::MakeUnique<tracing::ProcessMetricsMemoryDumpProvider, int &>' requested
here
    owned_provider =
base::MakeUnique<ProcessMetricsMemoryDumpProvider>(process);
                           ^
../../components/tracing/common/process_metrics_memory_dump_provider.h:41:3:
note: declared protected here
  ProcessMetricsMemoryDumpProvider(base::ProcessId process);
  ^
1 error generated.
[7/25] CXX obj/content/browser/browser/browser_main_loop.o

+}
+
+// static
+void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
+    base::ProcessId process) {
+  std::unique_ptr<ProcessMetricsMemoryDumpProvider> owned_provider =
+      ProcessMetricsMemoryDumpProvider::CreateMetricsProvider(process);
+  ProcessMetricsMemoryDumpProvider* provider = owned_provider.get();
   bool did_insert =
       g_dump_providers_map.Get()
-          .insert(std::make_pair(process, std::move(metrics_provider)))
+          .insert(std::make_pair(process, std::move(owned_provider)))
           .second;
   if (!did_insert) {
     DLOG(ERROR) << "ProcessMetricsMemoryDumpProvider already registered for "
                 << (process == base::kNullProcessId
                         ? "current process"
                         : "process id " + base::IntToString(process));
+    return;
   }
+  base::trace_event::MemoryDumpProvider::Options options;
+  options.target_pid = process;
+  options.is_fast_polling_supported = true;
+  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
+      provider, "ProcessMemoryMetrics", nullptr, options);
 }
 
 // static