Provide base::ScopedMxHandle helper for use in Fuchsia implementations.

base::ScopedMxHandle uses ScopedGeneric to manage an mx_handle_t. ScopedGeneric is updated to provide a receive() API, allowing its use as a direct out-parameter, since most Magenta APIs return handles that way. Bug: 740791, 706592 Change-Id: I1491fe3a3eba354e4dd4ebf2d3feae6759e3f87b Reviewed-on: https://chromium-review.googlesource.com/602394 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Albert J. Wong <ajwong@chromium.org> Cr-Commit-Position: refs/heads/master@{#493073}
qweri0p · Aug 9, 2017 · 78b7331 · 78b7331
1 parent d8faf36
commit 78b7331
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 54 deletions.
diff --git a/base/BUILD.gn b/base/BUILD.gn
@@ -1234,6 +1234,7 @@ component("base") {
       "base_paths_fuchsia.cc",
       "debug/stack_trace_fuchsia.cc",
       "files/file_path_watcher_fuchsia.cc",
+      "fuchsia/scoped_mx_handle.h",
       "memory/shared_memory_fuchsia.cc",
       "memory/shared_memory_handle_fuchsia.cc",
       "message_loop/message_pump_fuchsia.cc",

diff --git a/base/fuchsia/scoped_mx_handle.h b/base/fuchsia/scoped_mx_handle.h
@@ -0,0 +1,33 @@
+// 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.
+
+#ifndef BASE_FUCHSIA_SCOPED_MX_HANDLE_H_
+#define BASE_FUCHSIA_SCOPED_MX_HANDLE_H_
+
+#include <magenta/status.h>
+#include <magenta/syscalls.h>
+
+#include "base/logging.h"
+#include "base/scoped_generic.h"
+
+namespace base {
+
+namespace internal {
+
+struct ScopedMxHandleTraits {
+  static mx_handle_t InvalidValue() { return MX_HANDLE_INVALID; }
+  static void Free(mx_handle_t object) {
+    mx_status_t status = mx_handle_close(object);
+    CHECK_EQ(MX_OK, status) << mx_status_get_string(status);
+  }
+};
+
+}  // namespace internal
+
+using ScopedMxHandle =
+    ScopedGeneric<mx_handle_t, internal::ScopedMxHandleTraits>;
+
+}  // namespace base
+
+#endif  // BASE_FUCHSIA_SCOPED_MX_HANDLE_H_
diff --git a/base/memory/shared_memory_fuchsia.cc b/base/memory/shared_memory_fuchsia.cc
@@ -11,6 +11,7 @@
 #include <magenta/syscalls.h>
 
 #include "base/bits.h"
+#include "base/fuchsia/scoped_mx_handle.h"
 #include "base/logging.h"
 #include "base/memory/shared_memory_tracker.h"
 #include "base/process/process_metrics.h"
@@ -49,10 +50,10 @@ bool SharedMemory::CreateAndMapAnonymous(size_t size) {
 }
 
 bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
-  mx_handle_t vmo;
   requested_size_ = options.size;
   mapped_size_ = bits::Align(requested_size_, GetPageSize());
-  mx_status_t status = mx_vmo_create(mapped_size_, 0, &vmo);
+  ScopedMxHandle vmo;
+  mx_status_t status = mx_vmo_create(mapped_size_, 0, vmo.receive());
   if (status != MX_OK) {
     DLOG(ERROR) << "mx_vmo_create failed, status=" << status;
     return false;
@@ -61,15 +62,18 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   if (!options.executable) {
     // If options.executable isn't set, drop that permission by replacement.
     const int kNoExecFlags = MX_DEFAULT_VMO_RIGHTS & ~MX_RIGHT_EXECUTE;
-    status = mx_handle_replace(vmo, kNoExecFlags, &vmo);
+    ScopedMxHandle old_vmo(std::move(vmo));
+    status = mx_handle_replace(old_vmo.get(), kNoExecFlags, vmo.receive());
     if (status != MX_OK) {
-      DLOG(ERROR) << "mx_handle_replace failed, status=" << status;
-      mx_handle_close(vmo);
+      DLOG(ERROR) << "mx_handle_replace() failed: "
+                  << mx_status_get_string(status);
       return false;
     }
+    ignore_result(old_vmo.release());
   }
 
-  shm_ = SharedMemoryHandle(vmo, mapped_size_, UnguessableToken::Create());
+  shm_ = SharedMemoryHandle(vmo.release(), mapped_size_,
+                            UnguessableToken::Create());
   return true;
 }
 

diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
@@ -40,6 +40,7 @@
 #if defined(OS_FUCHSIA)
 #include <magenta/process.h>
 #include <magenta/syscalls.h>
+#include "base/fuchsia/scoped_mx_handle.h"
 #endif
 
 namespace base {
@@ -377,15 +378,14 @@ TEST(SharedMemoryTest, GetReadOnlyHandle) {
                                contents.size(), MX_VM_FLAG_PERM_WRITE, &addr))
       << "Shouldn't be able to map as writable.";
 
-  mx_handle_t duped_handle;
+  ScopedMxHandle duped_handle;
   EXPECT_NE(MX_OK, mx_handle_duplicate(handle.GetHandle(), MX_RIGHT_WRITE,
-                                       &duped_handle))
+                                       duped_handle.receive()))
       << "Shouldn't be able to duplicate the handle into a writable one.";
 
   EXPECT_EQ(MX_OK, mx_handle_duplicate(handle.GetHandle(), MX_RIGHT_READ,
-                                       &duped_handle))
+                                       duped_handle.receive()))
       << "Should be able to duplicate the handle into a readable one.";
-  EXPECT_EQ(MX_OK, mx_handle_close(duped_handle));
 #elif defined(OS_POSIX)
   int handle_fd = SharedMemory::GetFdFromSharedMemoryHandle(handle);
   EXPECT_EQ(O_RDONLY, fcntl(handle_fd, F_GETFL) & O_ACCMODE)

diff --git a/base/message_loop/message_pump_fuchsia.cc b/base/message_loop/message_pump_fuchsia.cc
@@ -33,7 +33,7 @@ bool MessagePumpFuchsia::FileDescriptorWatcher::StopWatchingFileDescriptor() {
   if (!weak_pump_ || handle_ == MX_HANDLE_INVALID)
     return true;
 
-  int result = mx_port_cancel(weak_pump_->port_, handle_, wait_key());
+  int result = mx_port_cancel(weak_pump_->port_.get(), handle_, wait_key());
   DLOG_IF(ERROR, result != MX_OK)
       << "mx_port_cancel(handle=" << handle_
       << ") failed: " << mx_status_get_string(result);
@@ -44,14 +44,7 @@ bool MessagePumpFuchsia::FileDescriptorWatcher::StopWatchingFileDescriptor() {
 
 MessagePumpFuchsia::MessagePumpFuchsia()
     : keep_running_(true), weak_factory_(this) {
-  CHECK_EQ(MX_OK, mx_port_create(0, &port_));
-}
-
-MessagePumpFuchsia::~MessagePumpFuchsia() {
-  mx_status_t status = mx_handle_close(port_);
-  if (status != MX_OK) {
-    DLOG(ERROR) << "mx_handle_close failed: " << mx_status_get_string(status);
-  }
+  CHECK_EQ(MX_OK, mx_port_create(0, port_.receive()));
 }
 
 bool MessagePumpFuchsia::WatchFileDescriptor(int fd,
@@ -103,12 +96,13 @@ bool MessagePumpFuchsia::FileDescriptorWatcher::WaitBegin() {
     return false;
   }
 
-  mx_status_t status = mx_object_wait_async(
-      handle_, weak_pump_->port_, wait_key(), signals, MX_WAIT_ASYNC_ONCE);
+  mx_status_t status =
+      mx_object_wait_async(handle_, weak_pump_->port_.get(), wait_key(),
+                           signals, MX_WAIT_ASYNC_ONCE);
   if (status != MX_OK) {
     DLOG(ERROR) << "mx_object_wait_async failed: "
                 << mx_status_get_string(status)
-                << " (port=" << weak_pump_->port_ << ")";
+                << " (port=" << weak_pump_->port_.get() << ")";
     return false;
   }
   return true;
@@ -157,7 +151,8 @@ void MessagePumpFuchsia::Run(Delegate* delegate) {
                              : delayed_work_time_.ToMXTime();
     mx_port_packet_t packet;
 
-    const mx_status_t wait_status = mx_port_wait(port_, deadline, &packet, 0);
+    const mx_status_t wait_status =
+        mx_port_wait(port_.get(), deadline, &packet, 0);
     if (wait_status != MX_OK && wait_status != MX_ERR_TIMED_OUT) {
       NOTREACHED() << "unexpected wait status: "
                    << mx_status_get_string(wait_status);
@@ -210,9 +205,9 @@ void MessagePumpFuchsia::ScheduleWork() {
   // wakes up.
   mx_port_packet_t packet = {};
   packet.type = MX_PKT_TYPE_USER;
-  mx_status_t status = mx_port_queue(port_, &packet, 0);
+  mx_status_t status = mx_port_queue(port_.get(), &packet, 0);
   DLOG_IF(ERROR, status != MX_OK)
-      << "mx_port_queue failed: " << status << " (port=" << port_ << ")";
+      << "mx_port_queue failed: " << status << " (port=" << port_.get() << ")";
 }
 
 void MessagePumpFuchsia::ScheduleDelayedWork(

diff --git a/base/message_loop/message_pump_fuchsia.h b/base/message_loop/message_pump_fuchsia.h
@@ -6,6 +6,7 @@
 #define BASE_MESSAGE_LOOP_MESSAGE_PUMP_FUCHSIA_H_
 
 #include "base/base_export.h"
+#include "base/fuchsia/scoped_mx_handle.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -98,7 +99,6 @@ class BASE_EXPORT MessagePumpFuchsia : public MessagePump {
   };
 
   MessagePumpFuchsia();
-  ~MessagePumpFuchsia() override;
 
   bool WatchFileDescriptor(int fd,
                            bool persistent,
@@ -116,7 +116,7 @@ class BASE_EXPORT MessagePumpFuchsia : public MessagePump {
   // This flag is set to false when Run should return.
   bool keep_running_;
 
-  mx_handle_t port_;
+  ScopedMxHandle port_;
 
   // The time at which we should call DoDelayedWork.
   TimeTicks delayed_work_time_;

diff --git a/base/process/process.h b/base/process/process.h
@@ -15,6 +15,10 @@
 #include "base/win/scoped_handle.h"
 #endif
 
+#if defined(OS_FUCHSIA)
+#include "base/fuchsia/scoped_mx_handle.h"
+#endif
+
 #if defined(OS_MACOSX)
 #include "base/feature_list.h"
 #include "base/process/port_provider_mac.h"
@@ -167,6 +171,8 @@ class BASE_EXPORT Process {
  private:
 #if defined(OS_WIN)
   win::ScopedHandle process_;
+#elif defined(OS_FUCHSIA)
+  ScopedMxHandle process_;
 #else
   ProcessHandle process_;
 #endif

diff --git a/base/process/process_fuchsia.cc b/base/process/process_fuchsia.cc
@@ -21,14 +21,13 @@ Process::~Process() {
 }
 
 Process::Process(Process&& other)
-    : process_(other.process_), is_current_process_(other.is_current_process_) {
-  other.process_ = kNullProcessHandle;
+    : process_(std::move(other.process_)),
+      is_current_process_(other.is_current_process_) {
   other.is_current_process_ = false;
 }
 
 Process& Process::operator=(Process&& other) {
-  process_ = other.process_;
-  other.process_ = kNullProcessHandle;
+  process_ = std::move(other.process_);
   is_current_process_ = other.is_current_process_;
   other.is_current_process_ = false;
   return *this;
@@ -48,14 +47,14 @@ Process Process::Open(ProcessId pid) {
 
   // While a process with object id |pid| might exist, the job returned by
   // mx_job_default() might not contain it, so this call can fail.
-  mx_handle_t handle;
-  mx_status_t status =
-      mx_object_get_child(mx_job_default(), pid, MX_RIGHT_SAME_RIGHTS, &handle);
+  ScopedMxHandle handle;
+  mx_status_t status = mx_object_get_child(
+      mx_job_default(), pid, MX_RIGHT_SAME_RIGHTS, handle.receive());
   if (status != MX_OK) {
     DLOG(ERROR) << "mx_object_get_child failed: " << status;
     return Process();
   }
-  return Process(handle);
+  return Process(handle.release());
 }
 
 // static
@@ -67,13 +66,14 @@ Process Process::OpenWithExtraPrivileges(ProcessId pid) {
 // static
 Process Process::DeprecatedGetProcessFromHandle(ProcessHandle handle) {
   DCHECK_NE(handle, GetCurrentProcessHandle());
-  mx_handle_t out;
-  if (mx_handle_duplicate(handle, MX_RIGHT_SAME_RIGHTS, &out) != MX_OK) {
+  ScopedMxHandle out;
+  if (mx_handle_duplicate(handle, MX_RIGHT_SAME_RIGHTS, out.receive()) !=
+      MX_OK) {
     DLOG(ERROR) << "mx_handle_duplicate failed: " << handle;
     return Process();
   }
 
-  return Process(out);
+  return Process(out.release());
 }
 
 // static
@@ -87,11 +87,11 @@ void Process::TerminateCurrentProcessImmediately(int exit_code) {
 }
 
 bool Process::IsValid() const {
-  return process_ != kNullProcessHandle || is_current();
+  return process_.is_valid() || is_current();
 }
 
 ProcessHandle Process::Handle() const {
-  return is_current_process_ ? mx_process_self() : process_;
+  return is_current_process_ ? mx_process_self() : process_.get();
 }
 
 Process Process::Duplicate() const {
@@ -101,18 +101,19 @@ Process Process::Duplicate() const {
   if (!IsValid())
     return Process();
 
-  mx_handle_t out;
-  if (mx_handle_duplicate(process_, MX_RIGHT_SAME_RIGHTS, &out) != MX_OK) {
-    DLOG(ERROR) << "mx_handle_duplicate failed: " << process_;
+  ScopedMxHandle out;
+  if (mx_handle_duplicate(process_.get(), MX_RIGHT_SAME_RIGHTS,
+                          out.receive()) != MX_OK) {
+    DLOG(ERROR) << "mx_handle_duplicate failed: " << process_.get();
     return Process();
   }
 
-  return Process(out);
+  return Process(out.release());
 }
 
 ProcessId Process::Pid() const {
   DCHECK(IsValid());
-  return GetProcId(process_);
+  return GetProcId(process_.get());
 }
 
 bool Process::is_current() const {
@@ -121,21 +122,17 @@ bool Process::is_current() const {
 
 void Process::Close() {
   is_current_process_ = false;
-  if (IsValid()) {
-    mx_status_t status = mx_handle_close(process_);
-    DCHECK_EQ(status, MX_OK);
-    process_ = kNullProcessHandle;
-  }
+  process_.reset();
 }
 
 bool Process::Terminate(int exit_code, bool wait) const {
   // exit_code isn't supportable.
-  mx_status_t status = mx_task_kill(process_);
+  mx_status_t status = mx_task_kill(process_.get());
   // TODO(scottmg): Put these LOG/CHECK back to DLOG/DCHECK after
   // https://crbug.com/750756 is diagnosed.
   if (status == MX_OK && wait) {
     mx_signals_t signals;
-    status = mx_object_wait_one(process_, MX_TASK_TERMINATED,
+    status = mx_object_wait_one(process_.get(), MX_TASK_TERMINATED,
                                 mx_deadline_after(MX_SEC(60)), &signals);
     if (status != MX_OK) {
       LOG(ERROR) << "Error waiting for process exit: " << status;
@@ -161,7 +158,7 @@ bool Process::WaitForExitWithTimeout(TimeDelta timeout, int* exit_code) const {
                            ? MX_TIME_INFINITE
                            : (TimeTicks::Now() + timeout).ToMXTime();
   mx_signals_t signals_observed = 0;
-  mx_status_t status = mx_object_wait_one(process_, MX_TASK_TERMINATED,
+  mx_status_t status = mx_object_wait_one(process_.get(), MX_TASK_TERMINATED,
                                           deadline, &signals_observed);
 
   // TODO(scottmg): Make these LOGs into DLOGs after https://crbug.com/750756 is
@@ -177,7 +174,7 @@ bool Process::WaitForExitWithTimeout(TimeDelta timeout, int* exit_code) const {
   }
 
   mx_info_process_t proc_info;
-  status = mx_object_get_info(process_, MX_INFO_PROCESS, &proc_info,
+  status = mx_object_get_info(process_.get(), MX_INFO_PROCESS, &proc_info,
                               sizeof(proc_info), nullptr, nullptr);
   if (status != MX_OK) {
     LOG(ERROR) << "mx_object_get_info failed, status=" << status;

diff --git a/base/scoped_generic.h b/base/scoped_generic.h
@@ -124,6 +124,13 @@ class ScopedGeneric {
     return old_generic;
   }
 
+  // Returns a raw pointer to the object storage, to allow the scoper to be used
+  // to receive and manage out-parameter values. Implies reset().
+  element_type* receive() WARN_UNUSED_RESULT {
+    reset();
+    return &data_.generic;
+  }
+
   const element_type& get() const { return data_.generic; }
 
   // Returns true if this object doesn't hold the special null value for the