Minor fixes to Mojo MemoryDumpProvider.

mojo/edk/system/core.cc

Index: mojo/edk/system/core.cc
diff --git a/mojo/edk/system/core.cc b/mojo/edk/system/core.cc
index b8c66b574c19a396ade03d7bf9e7357b6d03d7ff..0513c42aa375854f6002eb652364c955aa2e46e2 100644
--- a/mojo/edk/system/core.cc
+++ b/mojo/edk/system/core.cc
@@ -18,6 +18,7 @@
 #include "base/rand_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "base/trace_event/memory_dump_manager.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/embedder_internal.h"
 #include "mojo/edk/embedder/platform_shared_buffer.h"
@@ -126,7 +127,11 @@ MojoResult ScopedPlatformHandleToMojoPlatformHandle(
 
 }  // namespace
 
-Core::Core() {}
+Core::Core() {
+  handles_.reset(new HandleTable);
+  base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
+      handles_.get(), "MojoHandleTable", nullptr);
+}
 
 Core::~Core() {
   if (node_controller_ && node_controller_->io_task_runner()) {
@@ -139,6 +144,8 @@ Core::~Core() {
                              base::Bind(&Core::PassNodeControllerToIOThread,
                                         base::Passed(&node_controller_)));
   }
+  base::trace_event::MemoryDumpManager::GetInstance()
+      ->UnregisterAndDeleteDumpProviderSoon(std::move(handles_));

[ssid, 2017/06/08 18:12:36]

Can we clear the memory used by |handles_| befoe passing to MDM?

{
  base::AutoLock lock(handles_->GetLock());
  handles_->handles_.clear();
}

Just to make sure that we don't keep around extra memory just because
memory-infra is hung / taking time.
  

[erikchen, 2017/06/09 00:18:12]

Sorry, I feel strongly about *not* doing what you suggested. 

Either this is a serious issue that should be indicated in the comments and
uniformly followed by all MDPs that invoke this method, presumably with
investigation to figure out why it's hung/taking time to fix it, or else we
shouldn't worry about this and assume that the MemoryDumpManager is functioning
appropriately.

"defensive programming" against potential issues clutters code without
[frequently] addressing the root cause of the potential issues.

[ssid, 2017/06/09 00:38:28]

The comment on the api says "The |mdp| will be deleted at some point in the near
future." which means it could take a long time too.
It need not be just the case where memory-infra hangs. It is a possible scenario
where the main threads are busy and the background thread of memory-infra never
got to execute tasks for a long time. This would still mean we keep around extra
memory for that time, especially when the table can be big.

I do not think this is defensive programming, i think it is more efficient
programming.

[erikchen, 2017/06/09 02:06:25]

I read "near future" as not taking a long time.
I guess we disagree about this point. I'm happy to do whatever rockot@ prefers.

[Primiano Tucci (use gerrit), 2017/06/09 11:29:58]

Hmm I am no OWNERz of this file but I lean towards Erik's way of thinking here.
That UnregisterAndDeleteDumpProviderSoon at most should defer the deletion to
the time it takes to complete the currently outstanding dump (if any).
Essentially is there to prevent that we delete the MDP while we iterate on it,
and guarantees linearization of the dtor.
So realistically I expect that object to be freed in <1s or so.
If we have issues of the form "the object is not really deleted soon" that is a
bug and we should fix the bug, not work around that to minimize the impact.
If there are cases where the dump can get stuck (probably there are) we should
introduce a timeout mechanism to recover and blacklist (I think we should do
that regardless, today we rely too much on MDP behaving sanely)

 }
 
 void Core::SetIOTaskRunner(scoped_refptr<base::TaskRunner> io_task_runner) {
@@ -153,8 +160,8 @@ NodeController* Core::GetNodeController() {
 }
 
 scoped_refptr<Dispatcher> Core::GetDispatcher(MojoHandle handle) {
-  base::AutoLock lock(handles_.GetLock());
-  return handles_.GetDispatcher(handle);
+  base::AutoLock lock(handles_->GetLock());
+  return handles_->GetDispatcher(handle);
 }
 
 void Core::SetDefaultProcessErrorCallback(
@@ -214,8 +221,8 @@ void Core::SetMachPortProvider(base::PortProvider* port_provider) {
 }
 
 MojoHandle Core::AddDispatcher(scoped_refptr<Dispatcher> dispatcher) {
-  base::AutoLock lock(handles_.GetLock());
-  return handles_.AddDispatcher(dispatcher);
+  base::AutoLock lock(handles_->GetLock());
+  return handles_->AddDispatcher(dispatcher);
 }
 
 bool Core::AddDispatchersFromTransit(
@@ -223,8 +230,8 @@ bool Core::AddDispatchersFromTransit(
     MojoHandle* handles) {
   bool failed = false;
   {
-    base::AutoLock lock(handles_.GetLock());
-    if (!handles_.AddDispatchersFromTransit(dispatchers, handles))
+    base::AutoLock lock(handles_->GetLock());
+    if (!handles_->AddDispatchersFromTransit(dispatchers, handles))
       failed = true;
   }
   if (failed) {
@@ -249,9 +256,9 @@ MojoResult Core::CreatePlatformHandleWrapper(
 MojoResult Core::PassWrappedPlatformHandle(
     MojoHandle wrapper_handle,
     ScopedPlatformHandle* platform_handle) {
-  base::AutoLock lock(handles_.GetLock());
+  base::AutoLock lock(handles_->GetLock());
   scoped_refptr<Dispatcher> d;
-  MojoResult result = handles_.GetAndRemoveDispatcher(wrapper_handle, &d);
+  MojoResult result = handles_->GetAndRemoveDispatcher(wrapper_handle, &d);
   if (result != MOJO_RESULT_OK)
     return result;
   if (d->GetType() == Dispatcher::Type::PLATFORM_HANDLE) {
@@ -300,15 +307,15 @@ MojoResult Core::PassSharedMemoryHandle(
   scoped_refptr<Dispatcher> dispatcher;
   MojoResult result = MOJO_RESULT_OK;
   {
-    base::AutoLock lock(handles_.GetLock());
+    base::AutoLock lock(handles_->GetLock());
     // Get the dispatcher and check it before removing it from the handle table
     // to ensure that the dispatcher is of the correct type. This ensures we
     // don't close and remove the wrong type of dispatcher.
-    dispatcher = handles_.GetDispatcher(mojo_handle);
+    dispatcher = handles_->GetDispatcher(mojo_handle);
     if (!dispatcher || dispatcher->GetType() != Dispatcher::Type::SHARED_BUFFER)
       return MOJO_RESULT_INVALID_ARGUMENT;
 
-    result = handles_.GetAndRemoveDispatcher(mojo_handle, &dispatcher);
+    result = handles_->GetAndRemoveDispatcher(mojo_handle, &dispatcher);
     if (result != MOJO_RESULT_OK)
       return result;
   }
@@ -366,8 +373,8 @@ MojoResult Core::Close(MojoHandle handle) {
   RequestContext request_context;
   scoped_refptr<Dispatcher> dispatcher;
   {
-    base::AutoLock lock(handles_.GetLock());
-    MojoResult rv = handles_.GetAndRemoveDispatcher(handle, &dispatcher);
+    base::AutoLock lock(handles_->GetLock());
+    MojoResult rv = handles_->GetAndRemoveDispatcher(handle, &dispatcher);
     if (rv != MOJO_RESULT_OK)
       return rv;
   }
@@ -463,10 +470,10 @@ MojoResult Core::AllocMessage(uint32_t num_bytes,
 
   std::vector<Dispatcher::DispatcherInTransit> dispatchers;
   {
-    base::AutoLock lock(handles_.GetLock());
-    MojoResult rv = handles_.BeginTransit(handles, num_handles, &dispatchers);
+    base::AutoLock lock(handles_->GetLock());
+    MojoResult rv = handles_->BeginTransit(handles, num_handles, &dispatchers);
     if (rv != MOJO_RESULT_OK) {
-      handles_.CancelTransit(dispatchers);
+      handles_->CancelTransit(dispatchers);
       return rv;
     }
   }
@@ -477,12 +484,12 @@ MojoResult Core::AllocMessage(uint32_t num_bytes,
       num_bytes, dispatchers.data(), num_handles, &message);
 
   {
-    base::AutoLock lock(handles_.GetLock());
+    base::AutoLock lock(handles_->GetLock());
     if (rv == MOJO_RESULT_OK) {
-      handles_.CompleteTransitAndClose(dispatchers);
+      handles_->CompleteTransitAndClose(dispatchers);
       *message_handle = reinterpret_cast<MojoMessageHandle>(message.release());
     } else {
-      handles_.CancelTransit(dispatchers);
+      handles_->CancelTransit(dispatchers);
     }
   }
 
@@ -547,8 +554,8 @@ MojoResult Core::CreateMessagePipe(
     scoped_refptr<Dispatcher> unused;
     unused->Close();
 
-    base::AutoLock lock(handles_.GetLock());
-    handles_.GetAndRemoveDispatcher(*message_pipe_handle0, &unused);
+    base::AutoLock lock(handles_->GetLock());
+    handles_->GetAndRemoveDispatcher(*message_pipe_handle0, &unused);
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
   }
 
@@ -657,9 +664,11 @@ MojoResult Core::FuseMessagePipes(MojoHandle handle0, MojoHandle handle1) {
 
   bool valid_handles = true;
   {
-    base::AutoLock lock(handles_.GetLock());
-    MojoResult result0 = handles_.GetAndRemoveDispatcher(handle0, &dispatcher0);
-    MojoResult result1 = handles_.GetAndRemoveDispatcher(handle1, &dispatcher1);
+    base::AutoLock lock(handles_->GetLock());
+    MojoResult result0 =
+        handles_->GetAndRemoveDispatcher(handle0, &dispatcher0);
+    MojoResult result1 =
+        handles_->GetAndRemoveDispatcher(handle1, &dispatcher1);
     if (result0 != MOJO_RESULT_OK || result1 != MOJO_RESULT_OK ||
         dispatcher0->GetType() != Dispatcher::Type::MESSAGE_PIPE ||
         dispatcher1->GetType() != Dispatcher::Type::MESSAGE_PIPE)
@@ -754,8 +763,8 @@ MojoResult Core::CreateDataPipe(
       *data_pipe_consumer_handle == MOJO_HANDLE_INVALID) {
     if (*data_pipe_producer_handle != MOJO_HANDLE_INVALID) {
       scoped_refptr<Dispatcher> unused;
-      base::AutoLock lock(handles_.GetLock());
-      handles_.GetAndRemoveDispatcher(*data_pipe_producer_handle, &unused);
+      base::AutoLock lock(handles_->GetLock());
+      handles_->GetAndRemoveDispatcher(*data_pipe_producer_handle, &unused);
     }
     producer->Close();
     consumer->Close();
@@ -993,8 +1002,8 @@ MojoResult Core::UnwrapPlatformSharedBufferHandle(
   scoped_refptr<Dispatcher> dispatcher;
   MojoResult result = MOJO_RESULT_OK;
   {
-    base::AutoLock lock(handles_.GetLock());
-    result = handles_.GetAndRemoveDispatcher(mojo_handle, &dispatcher);
+    base::AutoLock lock(handles_->GetLock());
+    result = handles_->GetAndRemoveDispatcher(mojo_handle, &dispatcher);
     if (result != MOJO_RESULT_OK)
       return result;
   }
@@ -1024,8 +1033,8 @@ MojoResult Core::UnwrapPlatformSharedBufferHandle(
 }
 
 void Core::GetActiveHandlesForTest(std::vector<MojoHandle>* handles) {
-  base::AutoLock lock(handles_.GetLock());
-  handles_.GetActiveHandlesForTest(handles);
+  base::AutoLock lock(handles_->GetLock());
+  handles_->GetActiveHandlesForTest(handles);
 }
 
 // static