discardable_memory: Using mojo IPC to replace Chrome IPC

components/discardable_memory/client/client_discardable_shared_memory_manager.cc

Index: components/discardable_memory/client/client_discardable_shared_memory_manager.cc
diff --git a/components/discardable_memory/client/client_discardable_shared_memory_manager.cc b/components/discardable_memory/client/client_discardable_shared_memory_manager.cc
index 22239e13869644b0243684a01e7a3c60eea03de1..df3e7ebf94292917b7515facaa52768f07cae324 100644
--- a/components/discardable_memory/client/client_discardable_shared_memory_manager.cc
+++ b/components/discardable_memory/client/client_discardable_shared_memory_manager.cc
@@ -21,9 +21,11 @@
 #include "base/process/process_metrics.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/trace_event.h"
+#include "mojo/public/cpp/system/platform_handle.h"
 
 namespace discardable_memory {
 namespace {
@@ -82,29 +84,56 @@ class DiscardableMemoryImpl : public base::DiscardableMemory {
   DISALLOW_COPY_AND_ASSIGN(DiscardableMemoryImpl);
 };
 
-void SendDeletedDiscardableSharedMemoryMessage(
-    ClientDiscardableSharedMemoryManager::Delegate* delegate,
-    DiscardableSharedMemoryId id) {
-  delegate->DeletedDiscardableSharedMemory(id);
+void OnManagerConnectionError(
+    mojom::DiscardableSharedMemoryManagerPtr* manager) {
+  manager->reset();
+}
+
+void InitManagerOnIOThread(mojom::DiscardableSharedMemoryManagerPtr* manager,

[reveman, 2016/11/18 05:26:50]

s/IOThread/IO/ and align this function name with the choice of member variable
name.

[Peng, 2016/11/18 16:28:27]

Done.

[Peng, 2016/11/18 16:28:27]

Done.

+                           mojom::DiscardableSharedMemoryManagerPtrInfo info) {
+  manager->Bind(std::move(info));
+  manager->set_connection_error_handler(
+      base::Bind(&OnManagerConnectionError, manager));
+}
+
+void DeletedDiscardableSharedMemoryOnIOThread(

[reveman, 2016/11/18 05:26:50]

nit: s/OnIOThread/OnIO/

[Peng, 2016/11/18 16:28:27]

Done.

[Peng, 2016/11/18 16:28:27]

Done.

+    mojom::DiscardableSharedMemoryManagerPtr* manager,
+    int32_t id) {
+  if (!manager->is_bound())

[reveman, 2016/11/18 05:26:50]

when will this happen? can we DCHECK instead?

[Peng, 2016/11/18 16:28:27]

Done.

[Peng, 2016/11/18 16:28:27]

Probably when the browser is being terminated. The mojo connection is broken,
the OnManagerConnectionError is called and the manager will be unbound. In that
case, I think we just want to return here instead of crash.

[reveman, 2016/11/18 21:55:25]

Makes sense. Thanks for explaining.

+    return;
+  (*manager)->DeletedDiscardableSharedMemory(id);
 }
 
 }  // namespace
 
 ClientDiscardableSharedMemoryManager::ClientDiscardableSharedMemoryManager(
-    Delegate* delegate)
-    : heap_(base::GetPageSize()), delegate_(delegate) {
+    mojom::DiscardableSharedMemoryManagerPtrInfo info,
+    base::SingleThreadTaskRunner* io_task_runner)
+    : io_task_runner_(io_task_runner),
+      manager_(new mojom::DiscardableSharedMemoryManagerPtr),
+      heap_(new DiscardableSharedMemoryHeap(base::GetPageSize())) {
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
       this, "ClientDiscardableSharedMemoryManager",
       base::ThreadTaskRunnerHandle::Get());
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&InitManagerOnIOThread, manager_.get(), base::Passed(&info)));
 }
 
 ClientDiscardableSharedMemoryManager::~ClientDiscardableSharedMemoryManager() {
   base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider(
       this);
   // TODO(reveman): Determine if this DCHECK can be enabled. crbug.com/430533
-  // DCHECK_EQ(heap_.GetSize(), heap_.GetSizeOfFreeLists());
-  if (heap_.GetSize())
+  // DCHECK_EQ(heap_->GetSize(), heap_->GetSizeOfFreeLists());
+  if (heap_->GetSize())
     MemoryUsageChanged(0, 0);
+  heap_.reset();
+
+  // Delete the manager on IO thread, so any pinding tasks on IO thread will be
+  // executed before the manager is deleted.
+  auto* manager = manager_.release();
+  if (!io_task_runner_->DeleteSoon(FROM_HERE, manager))

[reveman, 2016/11/18 05:26:50]

Can this fail in practice? In that case a wrapper function like
https://cs.chromium.org/chromium/src/storage/browser/fileapi/sandbox_file_sys...
would make it a bit cleaner. Otherwise,

bool posted = io_task_runner_->DeleteSoon(FROM_HERE, manager_.release());
DCHECK(posted);

would be preferred.

[Peng, 2016/11/18 16:28:27]

Done.

+    delete manager;
 }
 
 std::unique_ptr<base::DiscardableMemory>
@@ -138,11 +167,11 @@ ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(
   if (pages < allocation_pages)
     slack = allocation_pages - pages;
 
-  size_t heap_size_prior_to_releasing_purged_memory = heap_.GetSize();
+  size_t heap_size_prior_to_releasing_purged_memory = heap_->GetSize();
   for (;;) {
     // Search free lists for suitable span.
     std::unique_ptr<DiscardableSharedMemoryHeap::Span> free_span =
-        heap_.SearchFreeLists(pages, slack);
+        heap_->SearchFreeLists(pages, slack);
     if (!free_span.get())
       break;
 
@@ -155,7 +184,7 @@ ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(
         base::DiscardableSharedMemory::FAILED) {
       DCHECK(!free_span->shared_memory()->IsMemoryResident());
       // We have to release purged memory before |free_span| can be destroyed.
-      heap_.ReleasePurgedMemory();
+      heap_->ReleasePurgedMemory();
       DCHECK(!free_span->shared_memory());
       continue;
     }
@@ -164,50 +193,50 @@ ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(
 
     // Memory usage is guaranteed to have changed after having removed
     // at least one span from the free lists.
-    MemoryUsageChanged(heap_.GetSize(), heap_.GetSizeOfFreeLists());
+    MemoryUsageChanged(heap_->GetSize(), heap_->GetSizeOfFreeLists());
 
     return base::MakeUnique<DiscardableMemoryImpl>(this, std::move(free_span));
   }
 
   // Release purged memory to free up the address space before we attempt to
   // allocate more memory.
-  heap_.ReleasePurgedMemory();
+  heap_->ReleasePurgedMemory();
 
   // Make sure crash keys are up to date in case allocation fails.
-  if (heap_.GetSize() != heap_size_prior_to_releasing_purged_memory)
-    MemoryUsageChanged(heap_.GetSize(), heap_.GetSizeOfFreeLists());
+  if (heap_->GetSize() != heap_size_prior_to_releasing_purged_memory)
+    MemoryUsageChanged(heap_->GetSize(), heap_->GetSizeOfFreeLists());
 
   size_t pages_to_allocate =
       std::max(kAllocationSize / base::GetPageSize(), pages);
   size_t allocation_size_in_bytes = pages_to_allocate * base::GetPageSize();
 
-  DiscardableSharedMemoryId new_id =
-      g_next_discardable_shared_memory_id.GetNext();
+  int32_t new_id = g_next_discardable_shared_memory_id.GetNext();
 
   // Ask parent process to allocate a new discardable shared memory segment.
-  std::unique_ptr<base::DiscardableSharedMemory> shared_memory(
-      AllocateLockedDiscardableSharedMemory(allocation_size_in_bytes, new_id));
+  auto shared_memory =

[reveman, 2016/11/18 05:26:50]

nit: please keep the explicit type instead of using auto. It makes it more clear
what type of object is returned and as a result makes the code easier to
understand imo. 

[Peng, 2016/11/18 16:28:27]

Done.

+      AllocateLockedDiscardableSharedMemory(allocation_size_in_bytes, new_id);
 
   // Create span for allocated memory.
-  std::unique_ptr<DiscardableSharedMemoryHeap::Span> new_span(
-      heap_.Grow(std::move(shared_memory), allocation_size_in_bytes, new_id,
-                 base::Bind(&SendDeletedDiscardableSharedMemoryMessage,
-                            delegate_, new_id)));
+  std::unique_ptr<DiscardableSharedMemoryHeap::Span> new_span(heap_->Grow(
+      std::move(shared_memory), allocation_size_in_bytes, new_id,
+      base::Bind(
+          &ClientDiscardableSharedMemoryManager::DeletedDiscardableSharedMemory,
+          base::Unretained(this), new_id)));
   new_span->set_is_locked(true);
 
   // Unlock and insert any left over memory into free lists.
   if (pages < pages_to_allocate) {
     std::unique_ptr<DiscardableSharedMemoryHeap::Span> leftover =
-        heap_.Split(new_span.get(), pages);
+        heap_->Split(new_span.get(), pages);
     leftover->shared_memory()->Unlock(
         leftover->start() * base::GetPageSize() -
             reinterpret_cast<size_t>(leftover->shared_memory()->memory()),
         leftover->length() * base::GetPageSize());
     leftover->set_is_locked(false);
-    heap_.MergeIntoFreeLists(std::move(leftover));
+    heap_->MergeIntoFreeLists(std::move(leftover));
   }
 
-  MemoryUsageChanged(heap_.GetSize(), heap_.GetSizeOfFreeLists());
+  MemoryUsageChanged(heap_->GetSize(), heap_->GetSizeOfFreeLists());
 
   return base::MakeUnique<DiscardableMemoryImpl>(this, std::move(new_span));
 }
@@ -222,8 +251,8 @@ bool ClientDiscardableSharedMemoryManager::OnMemoryDump(
         pmd->CreateAllocatorDump(
             base::StringPrintf("discardable/child_0x%" PRIXPTR,
                                reinterpret_cast<uintptr_t>(this)));
-    const size_t total_size = heap_.GetSize();
-    const size_t freelist_size = heap_.GetSizeOfFreeLists();
+    const size_t total_size = heap_->GetSize();
+    const size_t freelist_size = heap_->GetSizeOfFreeLists();
     total_dump->AddScalar(base::trace_event::MemoryAllocatorDump::kNameSize,
                           base::trace_event::MemoryAllocatorDump::kUnitsBytes,
                           total_size - freelist_size);
@@ -233,29 +262,29 @@ bool ClientDiscardableSharedMemoryManager::OnMemoryDump(
     return true;
   }
 
-  return heap_.OnMemoryDump(pmd);
+  return heap_->OnMemoryDump(pmd);
 }
 
 ClientDiscardableSharedMemoryManager::Statistics
 ClientDiscardableSharedMemoryManager::GetStatistics() const {
   base::AutoLock lock(lock_);
   Statistics stats;
-  stats.total_size = heap_.GetSize();
-  stats.freelist_size = heap_.GetSizeOfFreeLists();
+  stats.total_size = heap_->GetSize();
+  stats.freelist_size = heap_->GetSizeOfFreeLists();
   return stats;
 }
 
 void ClientDiscardableSharedMemoryManager::ReleaseFreeMemory() {
   base::AutoLock lock(lock_);
 
-  size_t heap_size_prior_to_releasing_memory = heap_.GetSize();
+  size_t heap_size_prior_to_releasing_memory = heap_->GetSize();
 
   // Release both purged and free memory.
-  heap_.ReleasePurgedMemory();
-  heap_.ReleaseFreeMemory();
+  heap_->ReleasePurgedMemory();
+  heap_->ReleaseFreeMemory();
 
-  if (heap_.GetSize() != heap_size_prior_to_releasing_memory)
-    MemoryUsageChanged(heap_.GetSize(), heap_.GetSizeOfFreeLists());
+  if (heap_->GetSize() != heap_size_prior_to_releasing_memory)
+    MemoryUsageChanged(heap_->GetSize(), heap_->GetSizeOfFreeLists());
 }
 
 bool ClientDiscardableSharedMemoryManager::LockSpan(
@@ -306,10 +335,10 @@ void ClientDiscardableSharedMemoryManager::ReleaseSpan(
   if (!span->shared_memory())
     return;
 
-  heap_.MergeIntoFreeLists(std::move(span));
+  heap_->MergeIntoFreeLists(std::move(span));
 
   // Bytes of free memory changed.
-  MemoryUsageChanged(heap_.GetSize(), heap_.GetSizeOfFreeLists());
+  MemoryUsageChanged(heap_->GetSize(), heap_->GetSizeOfFreeLists());
 }
 
 base::trace_event::MemoryAllocatorDump*
@@ -318,27 +347,75 @@ ClientDiscardableSharedMemoryManager::CreateMemoryAllocatorDump(
     const char* name,
     base::trace_event::ProcessMemoryDump* pmd) const {
   base::AutoLock lock(lock_);
-  return heap_.CreateMemoryAllocatorDump(span, name, pmd);
+  return heap_->CreateMemoryAllocatorDump(span, name, pmd);
 }
 
 std::unique_ptr<base::DiscardableSharedMemory>
 ClientDiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
     size_t size,
-    DiscardableSharedMemoryId id) {
+    int32_t id) {
   TRACE_EVENT2("renderer",
                "ClientDiscardableSharedMemoryManager::"
                "AllocateLockedDiscardableSharedMemory",
                "size", size, "id", id);
-
-  base::SharedMemoryHandle handle = base::SharedMemory::NULLHandle();
-  delegate_->AllocateLockedDiscardableSharedMemory(size, id, &handle);
-  std::unique_ptr<base::DiscardableSharedMemory> memory(
-      new base::DiscardableSharedMemory(handle));
-  if (!memory->Map(size))
-    base::TerminateBecauseOutOfMemory(size);
+  std::unique_ptr<base::DiscardableSharedMemory> memory;
+  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
+                            base::WaitableEvent::InitialState::NOT_SIGNALED);
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&ClientDiscardableSharedMemoryManager::AllocateOnIOThread,
+                 base::Unretained(this), size, id, &memory, &event));
+  // Waiting until IPC is finished in the IO thread.

[reveman, 2016/11/18 05:26:50]

nit: "...IPC has finished on the IO thread."

[Peng, 2016/11/18 16:28:27]

Done.

+  event.Wait();
   return memory;
 }
 
+void ClientDiscardableSharedMemoryManager::AllocateOnIOThread(
+    size_t size,
+    int32_t id,
+    std::unique_ptr<base::DiscardableSharedMemory>* memory,
+    base::WaitableEvent* event) {
+  if (!manager_->is_bound())

[reveman, 2016/11/18 05:26:50]

can this happen? DCHECK instead?

[Peng, 2016/11/18 16:28:27]

When the mojo connection is broken, the |manager_| will be unbound by
OnManagerConnectionError() callback. In that case, we don't want to continue. so
we can return or CHECK(instead of DCHECK).

+    return;
+  (*manager_)->AllocateLockedDiscardableSharedMemory(
+      static_cast<uint32_t>(size), id,
+      base::Bind(
+          &ClientDiscardableSharedMemoryManager::OnAllocateCompletedOnIOThread,
+          base::Unretained(this), memory, event));
+}
+
+void ClientDiscardableSharedMemoryManager::OnAllocateCompletedOnIOThread(
+    std::unique_ptr<base::DiscardableSharedMemory>* memory,
+    base::WaitableEvent* event,
+    mojo::ScopedSharedBufferHandle mojo_handle) {
+  do {

[reveman, 2016/11/18 05:26:50]

Why this loop? I think the code would be easier to read if you just early out
with a return statement. Maybe use a ScopedClosure to ensure event->Signal() is
called.

[Peng, 2016/11/18 16:28:27]

Done.

+    if (!mojo_handle.is_valid())
+      break;
+    auto handle = base::SharedMemory::NULLHandle();

[reveman, 2016/11/18 05:26:50]

Can you avoid using auto here an below so it's more clear what the types we're
dealing with here are?

[Peng, 2016/11/18 16:28:27]

Done.

+    size_t memory_size = 0;
+    bool read_only = false;
+    auto result = mojo::UnwrapSharedMemoryHandle(
+        std::move(mojo_handle), &handle, &memory_size, &read_only);
+    if (result != MOJO_RESULT_OK)
+      break;
+    auto discardable_shared_memory =
+        base::MakeUnique<base::DiscardableSharedMemory>(handle);
+    if (!discardable_shared_memory->Map(memory_size)) {
+      base::TerminateBecauseOutOfMemory(memory_size);

[reveman, 2016/11/18 05:26:49]

nit: base::TerminateBecauseOutOfMemory never returns so the break below is not
needed

[Peng, 2016/11/18 16:28:27]

Done.

+      break;
+    }
+    *memory = std::move(discardable_shared_memory);
+  } while (false);
+  event->Signal();
+}
+
+void ClientDiscardableSharedMemoryManager::DeletedDiscardableSharedMemory(
+    int32_t id) {
+  io_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&DeletedDiscardableSharedMemoryOnIOThread,
+                            manager_.get(), id));
+}
+
 void ClientDiscardableSharedMemoryManager::MemoryUsageChanged(
     size_t new_bytes_total,
     size_t new_bytes_free) const {