discardable_memory: Using mojo IPC to replace Chrome IPC

components/discardable_memory/service/discardable_shared_memory_manager.cc

Index: components/discardable_memory/service/discardable_shared_memory_manager.cc
diff --git a/components/discardable_memory/service/discardable_shared_memory_manager.cc b/components/discardable_memory/service/discardable_shared_memory_manager.cc
index 1036dc84d697ed5324d2f181a15b7b618cdddf46..42fd43fbc82c8f3f505c0e728c26336acb29b87c 100644
--- a/components/discardable_memory/service/discardable_shared_memory_manager.cc
+++ b/components/discardable_memory/service/discardable_shared_memory_manager.cc
@@ -29,6 +29,8 @@
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "components/discardable_memory/common/discardable_shared_memory_heap.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/system/platform_handle.h"
 
 #if defined(OS_LINUX)
 #include "base/files/file_path.h"
@@ -57,6 +59,45 @@ uint64_t ClientProcessUniqueIdToTracingProcessId(int client_id) {
          1;
 }
 
+class DiscardableSharedMemoryManagerProxy

[reveman, 2016/11/23 17:07:44]

Please add a comment above this class describing what it's used for? The "Proxy"
suffix has confused me a couple of times when reviewing this code. It's
typically used on the client side while here it's used on the service side
instead. What do you think about calling this DiscardableSharedMemoryManagerHost
or HostDiscardableSharedMemoryManager instead?

[Peng, 2016/11/24 15:04:37]

How about MojoDiscardableSharedMemoryManagerImpl?

Done

+    : public mojom::DiscardableSharedMemoryManager {
+ public:
+  DiscardableSharedMemoryManagerProxy(
+      int32_t client_id,
+      ::discardable_memory::DiscardableSharedMemoryManager* manager)
+      : client_id_(client_id), manager_(manager) {}
+
+  ~DiscardableSharedMemoryManagerProxy() override {
+    manager_->ClientRemoved(client_id_);
+  }
+
+  // mojom::DiscardableSharedMemoryManager overrides:
+  void AllocateLockedDiscardableSharedMemory(
+      uint32_t size,
+      int32_t id,
+      const AllocateLockedDiscardableSharedMemoryCallback& callback) override {
+    base::SharedMemoryHandle handle;
+    manager_->AllocateLockedDiscardableSharedMemoryForClient(client_id_, size,
+                                                             id, &handle);
+    mojo::ScopedSharedBufferHandle memory;
+    if (handle != base::SharedMemory::NULLHandle()) {

[dcheng, 2016/11/25 00:07:08]

Is it necessary to check this condition? I would expect WrapSharedMemoryHandle()
to Just Work.

[Peng, 2016/11/25 16:41:53]

Done.

+      memory =
+          mojo::WrapSharedMemoryHandle(handle, size, false /* ready_only */);
+    }
+    return callback.Run(std::move(memory));
+  }
+
+  void DeletedDiscardableSharedMemory(int32_t id) override {
+    manager_->ClientDeletedDiscardableSharedMemory(id, client_id_);
+  }
+
+ private:
+  const int32_t client_id_;
+  ::discardable_memory::DiscardableSharedMemoryManager* const manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(DiscardableSharedMemoryManagerProxy);
+};
+
 class DiscardableMemoryImpl : public base::DiscardableMemory {
  public:
   DiscardableMemoryImpl(
@@ -180,7 +221,8 @@ DiscardableSharedMemoryManager::MemorySegment::MemorySegment(
 DiscardableSharedMemoryManager::MemorySegment::~MemorySegment() {}
 
 DiscardableSharedMemoryManager::DiscardableSharedMemoryManager()
-    : default_memory_limit_(GetDefaultMemoryLimit()),
+    : last_client_id_(0),
+      default_memory_limit_(GetDefaultMemoryLimit()),
       memory_limit_(default_memory_limit_),
       bytes_allocated_(0),
       memory_pressure_listener_(new base::MemoryPressureListener(
@@ -205,23 +247,39 @@ DiscardableSharedMemoryManager::~DiscardableSharedMemoryManager() {
       this);
 }
 
+// static
+DiscardableSharedMemoryManager*
+DiscardableSharedMemoryManager::CreateInstance() {
+  DCHECK(g_discardable_shared_memory_manager == nullptr);

[reveman, 2016/11/23 17:07:44]

nit: DCHECK(!g_discardable_shared_memory_manager)

[Peng, 2016/11/24 15:04:37]

It doesn't work. Because base::LazyInstance only overrides operator==.

+  return g_discardable_shared_memory_manager.Pointer();
+}
+
+// static
 DiscardableSharedMemoryManager* DiscardableSharedMemoryManager::current() {
+  DCHECK(!(g_discardable_shared_memory_manager == nullptr));

[reveman, 2016/11/23 17:07:44]

nit: DCHECK(g_discardable_shared_memory_manager)

[Peng, 2016/11/24 15:04:37]

It doesn't work. Because base::LazyInstance only overrides operator==.

   return g_discardable_shared_memory_manager.Pointer();
 }
 
+// static
+void DiscardableSharedMemoryManager::Bind(
+    mojom::DiscardableSharedMemoryManagerRequest request) {
+  auto* manager = current();
+  mojo::MakeStrongBinding(base::MakeUnique<DiscardableSharedMemoryManagerProxy>(
+                              ++manager->last_client_id_, manager),
+                          std::move(request));
+}
+
 std::unique_ptr<base::DiscardableMemory>
 DiscardableSharedMemoryManager::AllocateLockedDiscardableMemory(size_t size) {
   DCHECK_NE(size, 0u);
 
-  DiscardableSharedMemoryId new_id =
-      g_next_discardable_shared_memory_id.GetNext();
-  base::ProcessHandle current_process_handle = base::GetCurrentProcessHandle();
+  int32_t new_id = g_next_discardable_shared_memory_id.GetNext();
 
   // Note: Use DiscardableSharedMemoryHeap for in-process allocation
   // of discardable memory if the cost of each allocation is too high.
   base::SharedMemoryHandle handle;
-  AllocateLockedDiscardableSharedMemory(
-      current_process_handle, kInvalidUniqueClientID, size, new_id, &handle);
+  AllocateLockedDiscardableSharedMemory(kInvalidUniqueClientID, size, new_id,
+                                        &handle);
   std::unique_ptr<base::DiscardableSharedMemory> memory(
       new base::DiscardableSharedMemory(handle));
   if (!memory->Map(size))
@@ -308,17 +366,16 @@ bool DiscardableSharedMemoryManager::OnMemoryDump(
 
 void DiscardableSharedMemoryManager::
     AllocateLockedDiscardableSharedMemoryForClient(
-        base::ProcessHandle process_handle,
         int client_id,
         size_t size,
-        DiscardableSharedMemoryId id,
+        int32_t id,
         base::SharedMemoryHandle* shared_memory_handle) {
-  AllocateLockedDiscardableSharedMemory(process_handle, client_id, size, id,
+  AllocateLockedDiscardableSharedMemory(client_id, size, id,
                                         shared_memory_handle);
 }
 
 void DiscardableSharedMemoryManager::ClientDeletedDiscardableSharedMemory(
-    DiscardableSharedMemoryId id,
+    int32_t id,
     int client_id) {
   DeletedDiscardableSharedMemory(id, client_id);
 }
@@ -380,10 +437,9 @@ void DiscardableSharedMemoryManager::OnMemoryStateChange(
 }
 
 void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
-    base::ProcessHandle process_handle,
     int client_id,
     size_t size,
-    DiscardableSharedMemoryId id,
+    int32_t id,
     base::SharedMemoryHandle* shared_memory_handle) {
   base::AutoLock lock(lock_);
 
@@ -416,11 +472,7 @@ void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
     return;
   }
 
-  if (!memory->ShareToProcess(process_handle, shared_memory_handle)) {
-    LOG(ERROR) << "Cannot share discardable memory segment";
-    *shared_memory_handle = base::SharedMemory::NULLHandle();
-    return;
-  }
+  *shared_memory_handle = base::SharedMemory::DuplicateHandle(memory->handle());

[dcheng, 2016/11/25 00:07:08]

Nit: I would just DuplicateHandle up in the Proxy (then we don't need to be very
careful about our handle management here)

[Peng, 2016/11/25 16:41:53]

Because the handle inside of memory will be closed (line478) before returning
from this function. So we have to dup it here.

[dcheng, 2016/11/28 18:26:54]

Hmm... I was hoping we could wrap this in sort of scoper to be a bit safer about
closing the handle when it's needed...

Can we just move the DuplicateHandle() to line 483 so we don't need the extra
CloseHandle() call at least?

[Peng, 2016/11/29 15:20:13]

Done.

 
   // Close file descriptor to avoid running out.
   memory->Close();
@@ -428,6 +480,7 @@ void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
   base::CheckedNumeric<size_t> checked_bytes_allocated = bytes_allocated_;
   checked_bytes_allocated += memory->mapped_size();
   if (!checked_bytes_allocated.IsValid()) {
+    base::SharedMemory::CloseHandle(*shared_memory_handle);
     *shared_memory_handle = base::SharedMemory::NULLHandle();
     return;
   }
@@ -445,7 +498,7 @@ void DiscardableSharedMemoryManager::AllocateLockedDiscardableSharedMemory(
 }
 
 void DiscardableSharedMemoryManager::DeletedDiscardableSharedMemory(
-    DiscardableSharedMemoryId id,
+    int32_t id,
     int client_id) {
   base::AutoLock lock(lock_);