Minor fixes to Mojo MemoryDumpProvider.

mojo/edk/system/core.cc

 // Copyright 2013 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 "mojo/edk/system/core.h"
 
 #include <string.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/containers/stack_container.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "mojo/edk/system/channel.h"
 #include "mojo/edk/system/configuration.h"
 #include "mojo/edk/system/data_pipe_consumer_dispatcher.h"
 #include "mojo/edk/system/data_pipe_producer_dispatcher.h"
 #include "mojo/edk/system/handle_signals_state.h"
 #include "mojo/edk/system/message_pipe_dispatcher.h"
 #include "mojo/edk/system/platform_handle_dispatcher.h"
@@ skipping 88 matching lines @@
 #elif defined(OS_WIN)
   platform_handle->type = MOJO_PLATFORM_HANDLE_TYPE_WINDOWS_HANDLE;
   platform_handle->value = reinterpret_cast<uint64_t>(handle.release().handle);
 #endif  // defined(OS_WIN)
 
   return MOJO_RESULT_OK;
 }
 
 }  // 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()) {
     // If this races with IO thread shutdown the callback will be dropped and
     // the NodeController will be shutdown on this thread anyway, which is also
     // just fine.
     scoped_refptr<base::TaskRunner> io_task_runner =
         node_controller_->io_task_runner();
     io_task_runner->PostTask(FROM_HERE,
                              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) {
   GetNodeController()->SetIOTaskRunner(io_task_runner);
 }
 
 NodeController* Core::GetNodeController() {
   base::AutoLock lock(node_controller_lock_);
   if (!node_controller_)
     node_controller_.reset(new NodeController(this));
   return node_controller_.get();
 }
 
 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(
     const ProcessErrorCallback& callback) {
   default_process_error_callback_ = callback;
 }
 
 ScopedMessagePipeHandle Core::CreatePartialMessagePipe(ports::PortRef* peer) {
   RequestContext request_context;
   ports::PortRef local_port;
@@ skipping 39 matching lines @@
   GetNodeController()->ClosePeerConnection(peer_connection_id);
 }
 
 void Core::SetMachPortProvider(base::PortProvider* port_provider) {
 #if defined(OS_MACOSX) && !defined(OS_IOS)
   GetNodeController()->CreateMachPortRelay(port_provider);
 #endif
 }
 
 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(
     const std::vector<Dispatcher::DispatcherInTransit>& dispatchers,
     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) {
     for (auto d : dispatchers)
       d.dispatcher->Close();
     return false;
   }
   return true;
 }
 
 MojoResult Core::CreatePlatformHandleWrapper(
     ScopedPlatformHandle platform_handle,
     MojoHandle* wrapper_handle) {
   MojoHandle h = AddDispatcher(
       PlatformHandleDispatcher::Create(std::move(platform_handle)));
   if (h == MOJO_HANDLE_INVALID)
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
   *wrapper_handle = h;
   return MOJO_RESULT_OK;
 }
 
 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) {
     PlatformHandleDispatcher* phd =
         static_cast<PlatformHandleDispatcher*>(d.get());
     *platform_handle = phd->PassPlatformHandle();
   } else {
     result = MOJO_RESULT_INVALID_ARGUMENT;
   }
   d->Close();
@@ skipping 28 matching lines @@
     MojoHandle mojo_handle,
     base::SharedMemoryHandle* shared_memory_handle,
     size_t* num_bytes,
     bool* read_only) {
   if (!shared_memory_handle)
     return MOJO_RESULT_INVALID_ARGUMENT;
 
   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;
   }
 
   SharedBufferDispatcher* shm_dispatcher =
       static_cast<SharedBufferDispatcher*>(dispatcher.get());
   scoped_refptr<PlatformSharedBuffer> platform_shared_buffer =
       shm_dispatcher->PassPlatformSharedBuffer();
 
   if (!platform_shared_buffer)
@@ skipping 37 matching lines @@
 }
 
 MojoTimeTicks Core::GetTimeTicksNow() {
   return base::TimeTicks::Now().ToInternalValue();
 }
 
 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;
   }
   dispatcher->Close();
   return MOJO_RESULT_OK;
 }
 
 MojoResult Core::QueryHandleSignalsState(
     MojoHandle handle,
     MojoHandleSignalsState* signals_state) {
@@ skipping 75 matching lines @@
 
   if (num_handles > kMaxHandlesPerMessage)
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
 
   // If serialization fails below this point, one or more handles may be closed.
   // This requires an active RequestContext.
   RequestContext request_context;
 
   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;
     }
   }
   DCHECK_EQ(num_handles, dispatchers.size());
 
   std::unique_ptr<ports::UserMessageEvent> message;
   MojoResult rv = UserMessageImpl::CreateEventForNewSerializedMessage(
       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);
     }
   }
 
   return rv;
 }
 
 MojoResult Core::FreeMessage(MojoMessageHandle message_handle) {
   if (!message_handle)
     return MOJO_RESULT_INVALID_ARGUMENT;
 
@@ skipping 44 matching lines @@
       new MessagePipeDispatcher(GetNodeController(), port0, pipe_id, 0));
   if (*message_pipe_handle0 == MOJO_HANDLE_INVALID)
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
 
   *message_pipe_handle1 = AddDispatcher(
       new MessagePipeDispatcher(GetNodeController(), port1, pipe_id, 1));
   if (*message_pipe_handle1 == MOJO_HANDLE_INVALID) {
     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;
   }
 
   return MOJO_RESULT_OK;
 }
 
 MojoResult Core::WriteMessage(MojoHandle message_pipe_handle,
                               const void* bytes,
                               uint32_t num_bytes,
                               const MojoHandle* handles,
@@ skipping 88 matching lines @@
   return MOJO_RESULT_OK;
 }
 
 MojoResult Core::FuseMessagePipes(MojoHandle handle0, MojoHandle handle1) {
   RequestContext request_context;
   scoped_refptr<Dispatcher> dispatcher0;
   scoped_refptr<Dispatcher> dispatcher1;
 
   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)
       valid_handles = false;
   }
 
   if (!valid_handles) {
     if (dispatcher0)
       dispatcher0->Close();
     if (dispatcher1)
@@ skipping 74 matching lines @@
     producer->Close();
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
   }
 
   *data_pipe_producer_handle = AddDispatcher(producer);
   *data_pipe_consumer_handle = AddDispatcher(consumer);
   if (*data_pipe_producer_handle == MOJO_HANDLE_INVALID ||
       *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();
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
   }
 
   return MOJO_RESULT_OK;
 }
 
 MojoResult Core::WriteData(MojoHandle data_pipe_producer_handle,
@@ skipping 217 matching lines @@
 }
 
 MojoResult Core::UnwrapPlatformSharedBufferHandle(
     MojoHandle mojo_handle,
     MojoPlatformHandle* platform_handle,
     size_t* size,
     MojoPlatformSharedBufferHandleFlags* flags) {
   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;
   }
 
   if (dispatcher->GetType() != Dispatcher::Type::SHARED_BUFFER) {
     dispatcher->Close();
     return MOJO_RESULT_INVALID_ARGUMENT;
   }
 
   SharedBufferDispatcher* shm_dispatcher =
       static_cast<SharedBufferDispatcher*>(dispatcher.get());
   scoped_refptr<PlatformSharedBuffer> platform_shared_buffer =
       shm_dispatcher->PassPlatformSharedBuffer();
   DCHECK(platform_shared_buffer);
 
   DCHECK(size);
   *size = platform_shared_buffer->GetNumBytes();
 
   DCHECK(flags);
   *flags = MOJO_PLATFORM_SHARED_BUFFER_HANDLE_FLAG_NONE;
   if (platform_shared_buffer->IsReadOnly())
     *flags |= MOJO_PLATFORM_SHARED_BUFFER_HANDLE_FLAG_READ_ONLY;
 
   ScopedPlatformHandle handle = platform_shared_buffer->PassPlatformHandle();
   return ScopedPlatformHandleToMojoPlatformHandle(std::move(handle),
                                                   platform_handle);
 }
 
 void Core::GetActiveHandlesForTest(std::vector<MojoHandle>* handles) {
-  base::AutoLock lock(handles_.GetLock());
-  handles_.GetActiveHandlesForTest(handles);
+  base::AutoLock lock(handles_->GetLock());
+  handles_->GetActiveHandlesForTest(handles);
 }
 
 // static
 void Core::PassNodeControllerToIOThread(
     std::unique_ptr<NodeController> node_controller) {
   // It's OK to leak this reference. At this point we know the IO loop is still
   // running, and we know the NodeController will observe its eventual
   // destruction. This tells the NodeController to delete itself when that
   // happens.
   node_controller.release()->DestroyOnIOThreadShutdown();
 }
 
 }  // namespace edk
 }  // namespace mojo