[memory-infra] Remove MemoryDumpManagerDelegate

services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc

Index: services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
diff --git a/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc b/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
similarity index 64%
rename from services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
rename to services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
index 1129bae1d1146bc5e475a59c2d7523acca891fe8..8287682178d120b9d1a9182d669747808e2594aa 100644
--- a/services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.cc
+++ b/services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "services/resource_coordinator/public/cpp/memory/memory_dump_manager_delegate_impl.h"
+#include "services/resource_coordinator/public/cpp/memory/process_local_dump_manager_impl.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -16,10 +16,19 @@
 
 namespace memory_instrumentation {
 
-MemoryDumpManagerDelegateImpl::Config::~Config() {}
+ProcessLocalDumpManagerImpl::Config::~Config() {}
 
-MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl(
-    const MemoryDumpManagerDelegateImpl::Config& config)
+// static
+void ProcessLocalDumpManagerImpl::InitializeInstance(const Config& config) {

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

I'd s/InitializeInstance/CreateInstance/

[ssid, 2017/04/20 01:18:21]

Done.

+  static ProcessLocalDumpManagerImpl* instance = nullptr;

[chiniforooshan, 2017/04/18 15:11:24]

Did you test this in a component build? This is part of the client library.
Unlike the service part, several components can (do) link to the client library.
I think if those components happen to be in the same process, e.g. tests running
in the single process mode, there will be multiple copies of this static
variable and you should get a DCHECK error that MDM is initialized more than
once.

That's why I did not use static variables in the client library in the original
CL.

[ssid, 2017/04/18 19:11:45]

I can't think of better way to manage the lifetime of this object than using a
singleton. I am not very familiar with this component build static variable
issue. Will this problem go away if I used base::Singleton? Since the static
variable will be present in base library rather than this library.
Also this function is not GetInstance() it is InitializeInstance() and this
should only be called once.
I can change to base::Singleton if you confirm that will fix the issue.

I guess the issue will not show up in component build because it is anyway
called only once.

[Primiano Tucci (use gerrit), 2017/04/19 15:44:34]

If we agree on this pattern, this seems fine to me.
The problem with component build is really ODR violation. As itis, this
translation unit is linked into several .so files, and each of them gets its own
instance of |instance| (wouldn't be any different with base::Singleton or
base::Leakyinstance or using a static g_instance in the anonymous namepace) 
see my comment on the build.gn file, the fix should be just source_set ->
component

[ssid, 2017/04/20 01:18:21]

Done. 

+  if (!instance) {
+    instance = new ProcessLocalDumpManagerImpl(config);
+  } else {
+    NOTREACHED();
+  }
+}
+
+ProcessLocalDumpManagerImpl::ProcessLocalDumpManagerImpl(const Config& config)
     : binding_(this),
       config_(config),
       task_runner_(nullptr),
@@ -34,21 +43,25 @@ MemoryDumpManagerDelegateImpl::MemoryDumpManagerDelegateImpl(
   }
   coordinator_->RegisterProcessLocalDumpManager(
       binding_.CreateInterfacePtrAndBind());
-}
 
-MemoryDumpManagerDelegateImpl::~MemoryDumpManagerDelegateImpl() {}
-
-bool MemoryDumpManagerDelegateImpl::IsCoordinator() const {
-  return task_runner_ != nullptr;
+  // Only one process should handle periodic dumping.
+  bool is_coordinator_process = !!config.coordinator();
+  base::trace_event::MemoryDumpManager::GetInstance()->Initialize(
+      base::BindRepeating(&ProcessLocalDumpManagerImpl::RequestGlobalMemoryDump,
+                          base::Unretained(this)),
+      is_coordinator_process);
 }
 
-void MemoryDumpManagerDelegateImpl::RequestProcessMemoryDump(
+ProcessLocalDumpManagerImpl::~ProcessLocalDumpManagerImpl() {}
+
+void ProcessLocalDumpManagerImpl::RequestProcessMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const RequestProcessMemoryDumpCallback& callback) {
-  MemoryDumpManagerDelegate::CreateProcessDump(args, callback);
+  base::trace_event::MemoryDumpManager::GetInstance()->CreateProcessDump(
+      args, callback);
 }
 
-void MemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump(
+void ProcessLocalDumpManagerImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const base::trace_event::GlobalMemoryDumpCallback& callback) {
   // Note: This condition is here to match the old behavior. If the delegate is
@@ -58,7 +71,7 @@ void MemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump(
   // process, parallel requests will be cancelled.
   //
   // TODO(chiniforooshan): Unify the child and browser behavior.
-  if (IsCoordinator()) {
+  if (task_runner_) {
     task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&mojom::Coordinator::RequestGlobalMemoryDump,
@@ -75,21 +88,24 @@ void MemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump(
     pending_memory_dump_guid_ = args.dump_guid;
   }
   auto callback_proxy =
-      base::Bind(&MemoryDumpManagerDelegateImpl::MemoryDumpCallbackProxy,
+      base::Bind(&ProcessLocalDumpManagerImpl::MemoryDumpCallbackProxy,
                  base::Unretained(this), callback);
   coordinator_->RequestGlobalMemoryDump(args, callback_proxy);
 }
 
-void MemoryDumpManagerDelegateImpl::MemoryDumpCallbackProxy(
+void ProcessLocalDumpManagerImpl::MemoryDumpCallbackProxy(
     const base::trace_event::GlobalMemoryDumpCallback& callback,
     uint64_t dump_guid,
     bool success) {
-  DCHECK_NE(0U, pending_memory_dump_guid_);
-  pending_memory_dump_guid_ = 0;
+  {
+    base::AutoLock lock(pending_memory_dump_guid_lock_);
+    DCHECK_NE(0U, pending_memory_dump_guid_);
+    pending_memory_dump_guid_ = 0;
+  }
   callback.Run(dump_guid, success);
 }
 
-void MemoryDumpManagerDelegateImpl::SetAsNonCoordinatorForTesting() {
+void ProcessLocalDumpManagerImpl::SetAsNonCoordinatorForTesting() {
   task_runner_ = nullptr;
 }