Fix crash in InvokeOnMemoryDump when tracing

components/tracing/common/process_metrics_memory_dump_provider.cc

 // 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 "components/tracing/common/process_metrics_memory_dump_provider.h"
 
 #include <fcntl.h>
 #include <stdint.h>
 
 #include <map>
@@ skipping 195 matching lines @@
   NOTREACHED();
   return std::unique_ptr<base::ProcessMetrics>();
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 }
 
 }  // namespace
 
 // 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
 FILE* ProcessMetricsMemoryDumpProvider::proc_smaps_for_testing = nullptr;
 
 // static
 int ProcessMetricsMemoryDumpProvider::fast_polling_statm_fd_for_testing = -1;
 
 bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
     const base::trace_event::MemoryDumpArgs& args,
@@ skipping 307 matching lines @@
   for (VMRegion& region : dyld_regions) {
     pmd->process_mmaps()->AddVMRegion(region);
   }
 
   pmd->set_has_process_mmaps();
   return true;
 }
 #endif  // defined(OS_MACOSX)
 
 // static
+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));
+  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> 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);
+  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
 void ProcessMetricsMemoryDumpProvider::UnregisterForProcess(
     base::ProcessId process) {
   auto iter = g_dump_providers_map.Get().find(process);
   if (iter == g_dump_providers_map.Get().end())
     return;
   base::trace_event::MemoryDumpManager::GetInstance()
       ->UnregisterAndDeleteDumpProviderSoon(std::move(iter->second));
@@ skipping 120 matching lines @@
 #endif
 }
 
 void ProcessMetricsMemoryDumpProvider::SuspendFastMemoryPolling() {
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   fast_polling_statm_fd_.reset();
 #endif
 }
 
 }  // namespace tracing