ipc: Update MachPortMac ownership semantics.

ipc/mach_port_mac.h

 // Copyright 2015 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 IPC_MACH_PORT_MAC_H_
 #define IPC_MACH_PORT_MAC_H_
 
 #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
+// MachPortMac is a wrapper around an OSX Mach port that can be transported
+// across Chrome IPC channels that support attachment brokering. The Mach port
 // will be duplicated into the destination process by the attachment broker.
+// For now, this class only supports Mach ports with a send right. If needed,

[Tom Sepez, 2015/10/06 16:09:09]

Nit: Forgive my limited understanding, but I thought send rights were a property
of a given process, not the port itself, so the phrase "Mach ports with a send
right" sounds weird.  Do you mean that the wrapper only passes on the send
rights during duplication and that other rights are dropped?

[erikchen, 2015/10/06 20:40:43]

I do mean what you wrote, and I updated my comment to be more precise.

+// attachment brokering can be trivially extended to support Mach ports with
+// other rights.
 class IPC_EXPORT MachPortMac {
  public:
   MachPortMac() : mach_port_(MACH_PORT_NULL) {}
-  explicit MachPortMac(const mach_port_t& mach_port) : mach_port_(mach_port) {}
+
+  // This constructor increments the ref count of |mach_port|.
+  explicit MachPortMac(const mach_port_t& mach_port);

[Tom Sepez, 2015/10/06 16:09:09]

nit: is it really const if its being modified?

[erikchen, 2015/10/06 20:40:42]

mach_port_t is typedefed to int - I changed the parameter to just be
mach_port_t.

 
   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; }

[Tom Sepez, 2015/10/06 16:09:09]

nit: do you want to pass by const reference here?

[erikchen, 2015/10/06 20:40:43]

Nah, I got rid of the other const ref as well.

 
  private:
+  // The ownership semantics of |mach_port_| are a little confusing, and cannot

[Tom Sepez, 2015/10/06 16:09:09]

nit: s/are a little confusing, and//.

[erikchen, 2015/10/06 20:40:43]

Done.

+  // 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.
+  //
+  // From the sender process's perspective, this class is just a wrapper. It

[Tom Sepez, 2015/10/06 16:09:09]

nit: what are the other perspectives? hmm.  I get what you're saying: the class
is just a wrapper that allows the sender to call Send(...) without worrying
about the implementation details of ports and their ref counts.

[erikchen, 2015/10/06 20:40:43]

Right - I removed the word "perspective" from the comment chain.

+  // calls Send(new Message(MachPortMac(mach_port))) and continues on its merry
+  // way. Behind the scenes, this class increments the ref count on |mach_port_|
+  // to ensure that the object stays alive.

[Tom Sepez, 2015/10/06 16:09:09]

nit: which object is that?  I'm confused.

[erikchen, 2015/10/06 20:40:43]

I got rid of this comment. 

+  //
+  // The broker process receives a reference to the name of this Mach port. It

[Tom Sepez, 2015/10/06 16:09:09]

nit: when does this happen? first time we mention a name, who sets it?  The same
magic that does ref-counting?

[erikchen, 2015/10/06 20:40:42]

I updated the comments to add slightly more detail about who sends the "name".

+  // copies the right into the destination process, and then decrements the ref

[Tom Sepez, 2015/10/06 16:09:09]

nit: send right

[erikchen, 2015/10/06 20:40:43]

I updated the top level class comment to use "send right". In an attempt to save
myself from unnecessary verbosity, I've used the phrase "ref count of the Mach
port" in this comment in place of "ref count of the send right of the Mach
port".

Note: Most of this confusion stems from Mach itself - in kernel code,
mach_port_t is typedefed to a pointer to a struct, and the concept of a "Mach
port" generally refers to this object in the kernel. In user code, mach_port_t
is typedefed to an int, and the concept of a "Mach port" generally refers to
this int, which is an opaque identifier that references a kernel Mach port. In
user land, the name of a Mach port is the Mach port (both are typedefed to int),
but mach_port_t typically implies stronger semantics (a message can be sent to
one), whereas mach_port_name_t is just a raw int that can be passed between
processes. 

+  // count in the original process. Note that a different process is

[Tom Sepez, 2015/10/06 16:09:09]

nit: why is the note important?  The OS takes care of synchrnonization, right?

[erikchen, 2015/10/06 20:40:42]

A different process is decrementing the ref count, which is unusual.

+  // decrementing the ref count!
+  //
+  // From the receiver process's perspective, a Mach port was magically inserted
+  // into its name space. The attachment broker is responsible for coupling the
+  // Mach port with Chrome IPC in the form of a MessageAttachment. When the
+  // client code receives a callback with the parameter MachPortMac, it then
+  // assumes ownership of the Mach port.
   mach_port_t mach_port_;
+  DISALLOW_COPY_AND_ASSIGN(MachPortMac);
 };
 
 template <>
 struct IPC_EXPORT ParamTraits<MachPortMac> {
   typedef MachPortMac param_type;
   static void Write(Message* m, const param_type& p);
   static bool Read(const Message* m, base::PickleIterator* iter, param_type* p);
   static void Log(const param_type& p, std::string* l);
 };
 
 }  // namespace IPC
 
 #endif  // IPC_MACH_PORT_MAC_H_