ipc: Update MachPortMac ownership semantics.

This CL consists of a small refactor, a small change in ownership semantics, and
a lot of documentation.

The refactor removes a |const| qualifier from brokerable message attachments
that are passed to the attachment broker. This allows for an improvement to
ownership semantics for MachPortMac.

BUG=535711

Review URL: https://codereview.chromium.org/1385143002

Cr-Commit-Position: refs/heads/master@{#352938}

ipc/attachment_broker.h

@@ -63,7 +63,7 @@ class IPC_EXPORT AttachmentBroker : public Listener {
   // IPC::Channel to communicate with the broker process. This may be the same
   // IPC::Channel that is requesting the brokering of an attachment.
   // Returns true on success and false otherwise.
-  virtual bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  virtual bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                        base::ProcessId destination_process) = 0;
 
   // Returns whether the attachment was available. If the attachment was

ipc/attachment_broker_privileged_mac.cc

@@ -64,7 +64,7 @@ void AttachmentBrokerPrivilegedMac::SetPortProvider(
 }
 
 bool AttachmentBrokerPrivilegedMac::SendAttachmentToProcess(
-    const BrokerableAttachment* attachment,
+    BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::MACH_PORT: {

ipc/attachment_broker_privileged_mac.h

@@ -29,7 +29,7 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac
   void SetPortProvider(base::PortProvider* port_provider);
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.

ipc/attachment_broker_privileged_win.cc

@@ -19,7 +19,7 @@ AttachmentBrokerPrivilegedWin::AttachmentBrokerPrivilegedWin() {}
 AttachmentBrokerPrivilegedWin::~AttachmentBrokerPrivilegedWin() {}
 
 bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess(
-    const BrokerableAttachment* attachment,
+    BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::WIN_HANDLE: {

ipc/attachment_broker_privileged_win.h

@@ -20,7 +20,7 @@ class IPC_EXPORT AttachmentBrokerPrivilegedWin
   ~AttachmentBrokerPrivilegedWin() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.

ipc/attachment_broker_privileged_win_unittest.cc

@@ -277,7 +277,7 @@ class MockBroker : public IPC::AttachmentBrokerUnprivilegedWin {
  public:
   MockBroker() {}
   ~MockBroker() override {}
-  bool SendAttachmentToProcess(const IPC::BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(IPC::BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override {
     return IPC::AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
         attachment, base::Process::Current().Pid());

ipc/attachment_broker_unprivileged_mac.cc

@@ -50,16 +50,19 @@ AttachmentBrokerUnprivilegedMac::AttachmentBrokerUnprivilegedMac() {}
 AttachmentBrokerUnprivilegedMac::~AttachmentBrokerUnprivilegedMac() {}
 
 bool AttachmentBrokerUnprivilegedMac::SendAttachmentToProcess(
-    const BrokerableAttachment* attachment,
+    BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::MACH_PORT: {
-      const internal::MachPortAttachmentMac* mach_port_attachment =
-          static_cast<const internal::MachPortAttachmentMac*>(attachment);
+      internal::MachPortAttachmentMac* mach_port_attachment =
+          static_cast<internal::MachPortAttachmentMac*>(attachment);
       internal::MachPortAttachmentMac::WireFormat format =
           mach_port_attachment->GetWireFormat(destination_process);
-      return get_sender()->Send(
-          new AttachmentBrokerMsg_DuplicateMachPort(format));
+      bool success =
+          get_sender()->Send(new AttachmentBrokerMsg_DuplicateMachPort(format));
+      if (success)
+        mach_port_attachment->reset_mach_port_ownership();
+      return success;
     }
     default:
       NOTREACHED();

ipc/attachment_broker_unprivileged_mac.h

@@ -22,7 +22,7 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedMac
   ~AttachmentBrokerUnprivilegedMac() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.

ipc/attachment_broker_unprivileged_win.cc

@@ -17,7 +17,7 @@ AttachmentBrokerUnprivilegedWin::AttachmentBrokerUnprivilegedWin() {}
 AttachmentBrokerUnprivilegedWin::~AttachmentBrokerUnprivilegedWin() {}
 
 bool AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
-    const BrokerableAttachment* attachment,
+    BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::WIN_HANDLE: {

ipc/attachment_broker_unprivileged_win.h

@@ -22,7 +22,7 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedWin
   ~AttachmentBrokerUnprivilegedWin() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.

ipc/ipc_channel_posix.cc

@@ -697,8 +697,8 @@ bool ChannelPosix::ProcessMessageForDelivery(Message* message) {
   if (message->HasBrokerableAttachments()) {
     DCHECK(GetAttachmentBroker());
     DCHECK(peer_pid_ != base::kNullProcessId);
-    for (const BrokerableAttachment* attachment :
-         message->attachment_set()->PeekBrokerableAttachments()) {
+    for (BrokerableAttachment* attachment :
+         message->attachment_set()->GetBrokerableAttachments()) {
       if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment,
                                                           peer_pid_)) {
         delete message;

ipc/ipc_channel_reader.cc

@@ -213,8 +213,8 @@ ChannelReader::AttachmentIdSet ChannelReader::GetBrokeredAttachments(
 
 #if USE_ATTACHMENT_BROKER
   MessageAttachmentSet* set = msg->attachment_set();
-  std::vector<const BrokerableAttachment*> brokerable_attachments_copy =
-      set->PeekBrokerableAttachments();
+  std::vector<BrokerableAttachment*> brokerable_attachments_copy =
+      set->GetBrokerableAttachments();
   for (const BrokerableAttachment* attachment : brokerable_attachments_copy) {
     if (attachment->NeedsBrokering()) {
       AttachmentBroker* broker = GetAttachmentBroker();

ipc/ipc_channel_reader_unittest.cc

@@ -42,7 +42,7 @@ class MockAttachmentBroker : public AttachmentBroker {
  public:
   typedef std::set<scoped_refptr<BrokerableAttachment>> AttachmentSet;
 
-  bool SendAttachmentToProcess(const BrokerableAttachment* attachment,
+  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
                                base::ProcessId destination_process) override {
     return false;
   }

ipc/ipc_channel_win.cc

@@ -108,8 +108,8 @@ bool ChannelWin::ProcessMessageForDelivery(Message* message) {
   if (message->HasBrokerableAttachments()) {
     DCHECK(GetAttachmentBroker());
     DCHECK(peer_pid_ != base::kNullProcessId);
-    for (const BrokerableAttachment* attachment :
-         message->attachment_set()->PeekBrokerableAttachments()) {
+    for (BrokerableAttachment* attachment :
+         message->attachment_set()->GetBrokerableAttachments()) {
       if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment,
                                                           peer_pid_)) {
         delete message;

ipc/ipc_message.cc

@@ -144,8 +144,8 @@ Message::NextMessageInfo::~NextMessageInfo() {}
 Message::SerializedAttachmentIds
 Message::SerializedIdsOfBrokerableAttachments() {
   DCHECK(HasBrokerableAttachments());
-  std::vector<const BrokerableAttachment*> attachments =
-      attachment_set_->PeekBrokerableAttachments();
+  std::vector<BrokerableAttachment*> attachments =
+      attachment_set_->GetBrokerableAttachments();
   CHECK_LE(attachments.size(), std::numeric_limits<size_t>::max() /
                                    BrokerableAttachment::kNonceSize);
   size_t size = attachments.size() * BrokerableAttachment::kNonceSize;

ipc/ipc_message_attachment_set.cc

@@ -131,9 +131,9 @@ void MessageAttachmentSet::CommitAll() {
   consumed_descriptor_highwater_ = 0;
 }
 
-std::vector<const BrokerableAttachment*>
-MessageAttachmentSet::PeekBrokerableAttachments() const {
-  std::vector<const BrokerableAttachment*> output;
+std::vector<BrokerableAttachment*>
+MessageAttachmentSet::GetBrokerableAttachments() const {
+  std::vector<BrokerableAttachment*> output;
   for (const scoped_refptr<MessageAttachment>& attachment : attachments_) {
     if (attachment->GetType() ==
         MessageAttachment::TYPE_BROKERABLE_ATTACHMENT) {

ipc/ipc_message_attachment_set.h

@@ -59,7 +59,7 @@ class IPC_EXPORT MessageAttachmentSet
   void CommitAll();
 
   // Returns a vector of all brokerable attachments.
-  std::vector<const BrokerableAttachment*> PeekBrokerableAttachments() const;
+  std::vector<BrokerableAttachment*> GetBrokerableAttachments() const;
 
   // Replaces a placeholder brokerable attachment with |attachment|, matching
   // them by their id.

ipc/mach_port_attachment_mac.cc

@@ -4,21 +4,40 @@
 
 #include "ipc/mach_port_attachment_mac.h"
 
+#include "base/mac/mach_logging.h"
+
 namespace IPC {
 namespace internal {
 
 MachPortAttachmentMac::MachPortAttachmentMac(mach_port_t mach_port)
-    : mach_port_(mach_port) {}
+    : mach_port_(mach_port), owns_mach_port_(true) {
+  if (mach_port != MACH_PORT_NULL) {
+    kern_return_t kr = mach_port_mod_refs(mach_task_self(), mach_port,
+                                          MACH_PORT_RIGHT_SEND, 1);
+    MACH_LOG_IF(ERROR, kr != KERN_SUCCESS, kr)
+        << "MachPortAttachmentMac mach_port_mod_refs";
+  }
+}
 
 MachPortAttachmentMac::MachPortAttachmentMac(const WireFormat& wire_format)
     : BrokerableAttachment(wire_format.attachment_id),
-      mach_port_(static_cast<mach_port_t>(wire_format.mach_port)) {}
+      mach_port_(static_cast<mach_port_t>(wire_format.mach_port)),
+      owns_mach_port_(false) {}
 
 MachPortAttachmentMac::MachPortAttachmentMac(
     const BrokerableAttachment::AttachmentId& id)
-    : BrokerableAttachment(id), mach_port_(MACH_PORT_NULL) {}
-
-MachPortAttachmentMac::~MachPortAttachmentMac() {}
+    : BrokerableAttachment(id),
+      mach_port_(MACH_PORT_NULL),
+      owns_mach_port_(false) {}
+
+MachPortAttachmentMac::~MachPortAttachmentMac() {
+  if (mach_port_ != MACH_PORT_NULL && owns_mach_port_) {
+    kern_return_t kr = mach_port_mod_refs(mach_task_self(), mach_port_,
+                                          MACH_PORT_RIGHT_SEND, -1);
+    MACH_LOG_IF(ERROR, kr != KERN_SUCCESS, kr)
+        << "~MachPortAttachmentMac mach_port_mod_refs";
+  }
+}
 
 MachPortAttachmentMac::BrokerableType MachPortAttachmentMac::GetBrokerableType()
     const {

ipc/mach_port_attachment_mac.h

@@ -45,7 +45,13 @@ class IPC_EXPORT MachPortAttachmentMac : public BrokerableAttachment {
     AttachmentId attachment_id;
   };
 
+  // This constructor increments the ref count of |mach_port_| and takes
+  // ownership of the result. Should only be called by the sender of a Chrome
+  // IPC message.
   explicit MachPortAttachmentMac(mach_port_t mach_port);
+
+  // These constructors do not take ownership of |mach_port|, and should only be
+  // called by the receiver of a Chrome IPC message.
   explicit MachPortAttachmentMac(const WireFormat& wire_format);
   explicit MachPortAttachmentMac(const BrokerableAttachment::AttachmentId& id);
 
@@ -56,9 +62,21 @@ class IPC_EXPORT MachPortAttachmentMac : public BrokerableAttachment {
 
   mach_port_t get_mach_port() const { return mach_port_; }
 
+  // The caller of this method has taken ownership of |mach_port_|.
+  void reset_mach_port_ownership() { owns_mach_port_ = false; }
+
  private:
   ~MachPortAttachmentMac() override;
   mach_port_t mach_port_;
+
+  // In the sender process, the attachment owns the Mach port of a newly created
+  // message. The attachment broker will eventually take ownership, and set
+  // this member to |false|.
+  // In the destination process, the attachment never owns the Mach port. The
+  // client code that receives the Chrome IPC message is always expected to take
+  // ownership.
+  bool owns_mach_port_;
+  DISALLOW_COPY_AND_ASSIGN(MachPortAttachmentMac);
 };
 
 }  // namespace internal

ipc/mach_port_mac.h

@@ -7,24 +7,65 @@
 
 #include <mach/mach.h>
 
+#include "base/macros.h"
 #include "ipc/ipc_export.h"
 #include "ipc/ipc_message_macros.h"
 
 namespace IPC {
 
-// MachPortMac is a wrapper around an OSX mach_port_t that can be transported
-// across Chrome IPC channels that support attachment brokering. The mach_port_t
-// will be duplicated into the destination process by the attachment broker.
+// MachPortMac is a wrapper around an OSX Mach port that can be transported
+// across Chrome IPC channels that support attachment brokering. The send right
+// to the Mach port will be duplicated into the destination process by the
+// attachment broker. If needed, attachment brokering can be trivially extended
+// to support duplication of other types of rights.
 class IPC_EXPORT MachPortMac {
  public:
   MachPortMac() : mach_port_(MACH_PORT_NULL) {}
-  explicit MachPortMac(const mach_port_t& mach_port) : mach_port_(mach_port) {}
+
+  explicit MachPortMac(mach_port_t mach_port) {};
 
   mach_port_t get_mach_port() const { return mach_port_; }
+
+  // This method should only be used by ipc/ translation code.
   void set_mach_port(mach_port_t mach_port) { mach_port_ = mach_port; }
 
  private:
+  // The ownership semantics of |mach_port_| cannot be easily expressed with a
+  // C++ scoped object. This is partly due to the mechanism by which Mach ports
+  // are brokered, and partly due to the architecture of Chrome IPC.
+  //
+  // The broker for Mach ports may live in a different process than the sender
+  // of the original Chrome IPC. In this case, it is signalled asynchronously,
+  // and ownership of the Mach port passes from the sender process into the
+  // broker process.
+  //
+  // Chrome IPC is written with the assumption that translation is a stateless
+  // process. There is no good mechanism to convey the semantics of ownership
+  // transfer from the Chrome IPC stack into the client code that receives the
+  // translated message. As a result, Chrome IPC code assumes that every message
+  // has a handler, and that the handler will take ownership of the Mach port.
+  // Note that the same holds true for POSIX fds and Windows HANDLEs.
+  //
+  // When used by client code in the sender process, this class is just a
+  // wrapper. The client code calls Send(new Message(MachPortMac(mach_port)))
+  // and continues on its merry way. Behind the scenes, a MachPortAttachmentMac
+  // takes ownership of the Mach port. When the attachment broker sends the name
+  // of the Mach port to the broker process, it also releases
+  // MachPortAttachmentMac's reference to the Mach port, as ownership has
+  // effectively been transferred to the broker process.
+  //
+  // The broker process receives the name, duplicates the Mach port into the
+  // destination process, and then decrements the ref count in the original
+  // process.
+  //
+  // In the destination process, the attachment broker is responsible for
+  // coupling the Mach port (inserted by the broker process) with Chrome IPC in
+  // the form of a MachPortAttachmentMac. Due to the Chrome IPC translation
+  // semantics discussed above, this MachPortAttachmentMac does not take
+  // ownership of the Mach port, and assumes that the client code which receives
+  // the callback will take ownership of the Mach port.
   mach_port_t mach_port_;
+  DISALLOW_COPY_AND_ASSIGN(MachPortMac);
 };
 
 template <>