[memory-infra] Remove MemoryDumpManagerDelegate

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

 // Copyright 2017 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 "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"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "base/trace_event/memory_dump_request_args.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "services/resource_coordinator/public/cpp/memory/coordinator.h"
 #include "services/resource_coordinator/public/interfaces/memory/memory_instrumentation.mojom.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 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),
       pending_memory_dump_guid_(0) {
   if (config.connector() != nullptr) {
     config.connector()->BindInterface(config.service_name(),
                                       mojo::MakeRequest(&coordinator_));
   } else {
     task_runner_ = base::ThreadTaskRunnerHandle::Get();
     config.coordinator()->BindCoordinatorRequest(
         mojo::MakeRequest(&coordinator_));
   }
   coordinator_->RegisterProcessLocalDumpManager(
       binding_.CreateInterfacePtrAndBind());
+
+  // 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);
 }
 
-MemoryDumpManagerDelegateImpl::~MemoryDumpManagerDelegateImpl() {}
+ProcessLocalDumpManagerImpl::~ProcessLocalDumpManagerImpl() {}
 
-bool MemoryDumpManagerDelegateImpl::IsCoordinator() const {
-  return task_runner_ != nullptr;
+void ProcessLocalDumpManagerImpl::RequestProcessMemoryDump(
+    const base::trace_event::MemoryDumpRequestArgs& args,
+    const RequestProcessMemoryDumpCallback& callback) {
+  base::trace_event::MemoryDumpManager::GetInstance()->CreateProcessDump(
+      args, callback);
 }
 
-void MemoryDumpManagerDelegateImpl::RequestProcessMemoryDump(
-    const base::trace_event::MemoryDumpRequestArgs& args,
-    const RequestProcessMemoryDumpCallback& callback) {
-  MemoryDumpManagerDelegate::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
   // in the browser process, we do not drop parallel requests in the delegate
   // and so they will be queued by the Coordinator service (see
   // CoordinatorImpl::RequestGlobalMemoryDump). If the delegate is in a child
   // 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,
                    base::Unretained(coordinator_.get()), args, callback));
     return;
   }
 
   {
     base::AutoLock lock(pending_memory_dump_guid_lock_);
     if (pending_memory_dump_guid_) {
       callback.Run(args.dump_guid, false);
       return;
     }
     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;
 }
 
 }  // namespace memory_instrumentation