ipc: Update MachPortMac ownership semantics.

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
Committed: https://crrev.com/3722a32d89d1fa27b7510d82d14e94f0ea68aa9a
Cr-Commit-Position: refs/heads/master@{#352938}

[erikchen, 2015-10-06 01:12:44]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2015-10-06 01:12:44]

mark: Please review.

Note that these updated semantics are not yet reflected in
attachment_broker_privileged_mac.cc, which needs to be updated to decrement the
ref count in the sender process. I will do so after you review this CL. Ideally,
this won't block your next pass on that CL, as that upcoming change is minimal.

[Mark Mentovai, 2015-10-06 13:43:04]

LGTM

[erikchen, 2015-10-06 15:54:48]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2015-10-06 15:54:49]

tsepez: Please review.

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

Code is fine, I just want to see if we can't make your comments clearer to a
Mach novice like me.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h
File ipc/mach_port_mac.h (right):

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:19: // For now, this class only supports Mach ports with a
send right. If needed,
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?

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:27: explicit MachPortMac(const mach_port_t& mach_port);
nit: is it really const if its being modified?

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:32: void set_mach_port(mach_port_t mach_port) { mach_port_ =
mach_port; }
nit: do you want to pass by const reference here?

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:35: // The ownership semantics of |mach_port_| are a little
confusing, and cannot
nit: s/are a little confusing, and//.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:40: // From the sender process's perspective, this class is
just a wrapper. It
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.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:43: // to ensure that the object stays alive.
nit: which object is that?  I'm confused.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:45: // The broker process receives a reference to the name
of this Mach port. It
nit: when does this happen? first time we mention a name, who sets it?  The same
magic that does ref-counting?

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:46: // copies the right into the destination process, and
then decrements the ref
nit: send right

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:47: // count in the original process. Note that a different
process is
nit: why is the note important?  The OS takes care of synchrnonization, right?

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

tsepez: Please review.

As I rewrote/expanded the documentation in MachPortMac, I decided that I wanted
to improve the ownership semantics for the sender. With this new patch set, the
only place where the ownership semantics break down is at the receiver, due to
the architecture of Chrome IPC translation, which I describe in more detail in
the documentation for MachPortMac.

I wanted to keep all the documentation in a single place, and MachPortMac is an
exported class, so I decided to leave it all in MachPortMac rather than
MachPortAttachmentMac.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h
File ipc/mach_port_mac.h (right):

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:19: // For now, this class only supports Mach ports with a
send right. If needed,
On 2015/10/06 16:09:09, Tom Sepez wrote:
> 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?

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

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:27: explicit MachPortMac(const mach_port_t& mach_port);
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: is it really const if its being modified?

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

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:32: void set_mach_port(mach_port_t mach_port) { mach_port_ =
mach_port; }
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: do you want to pass by const reference here?

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

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:35: // The ownership semantics of |mach_port_| are a little
confusing, and cannot
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: s/are a little confusing, and//.

Done.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:40: // From the sender process's perspective, this class is
just a wrapper. It
On 2015/10/06 16:09:09, Tom Sepez wrote:
> 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.

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

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:43: // to ensure that the object stays alive.
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: which object is that?  I'm confused.

I got rid of this comment.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:45: // The broker process receives a reference to the name
of this Mach port. It
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: when does this happen? first time we mention a name, who sets it?  The
same
> magic that does ref-counting?

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

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:46: // copies the right into the destination process, and
then decrements the ref
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: send right

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.

https://codereview.chromium.org/1385143002/diff/20001/ipc/mach_port_mac.h#new...
ipc/mach_port_mac.h:47: // count in the original process. Note that a different
process is
On 2015/10/06 16:09:09, Tom Sepez wrote:
> nit: why is the note important?  The OS takes care of synchrnonization, right?

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

[Tom Sepez, 2015-10-07 17:28:02]

lgtm

[erikchen, 2015-10-07 17:53:32]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-07 17:53:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1385143002/#ps60001
(title: "Refactor, update ownership semantics.")

[commit-bot: I haz the power, 2015-10-07 17:55:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385143002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385143002/60001

[commit-bot: I haz the power, 2015-10-07 18:14:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-07 18:14:20]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[erikchen, 2015-10-07 19:27:43]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-10-07 19:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1385143002/#ps100001
(title: "Fix rebase errors.")

[commit-bot: I haz the power, 2015-10-07 19:29:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385143002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385143002/100001

[commit-bot: I haz the power, 2015-10-07 20:52:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-07 20:53:08]

Patchset 6 (id:??) landed as
https://crrev.com/3722a32d89d1fa27b7510d82d14e94f0ea68aa9a
Cr-Commit-Position: refs/heads/master@{#352938}