[mojo-edk] Add explicit message object APIs

[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoAllocMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoFreeMessage: Frees a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379
Committed: https://crrev.com/72d05a46b2636d9d8cbf9a75b1c5d2bc2d8abe15
Cr-Commit-Position: refs/heads/master@{#389859}

[Ken Rockot(use gerrit already), 2016-04-13 02:56:46]

rockot@chromium.org changed reviewers:
+ amistry@chromium.org

[Ken Rockot(use gerrit already), 2016-04-13 02:56:47]

Please take a look. Some assorted rationale below:

Having a separate MojoGetMessageBuffer API buys us some freedom in message
object implementation. I originally tried to be more clever and derive buffer
metadata location from the payload pointer, but that gets really gnarly and
limits our freedom down the line.

I introduced both MojoWriteMessageNew and MojoReadMessageNew for now. I think
these should completely replace the original APIs, I just don't want to do that
in this CL. I don't see a good reason to keep the old APIs. They're marginally
simpler to use, but who cares? WDYT?

In any case I've implemented the old APIs in terms of the new ones. This means a
very little bit of extra overhead (alloc and free on a MessageForTransit object)
for current users.

One caveat is that it's hard to detect misuse (i.e. leaks or double frees) of
these opaque message objects. I toyed with adding some magic to
MessageForTransit to allow for some additional sanity checks (beyond non-null)
in DestroyMessage et al. Ultimately the overhead didn't seem worth it because
direct use of the API should be extremely limited. Perhaps it's sufficient to
eventually add a nice C++ API for scoped message objects.

[Anand Mistry (off Chromium), 2016-04-14 10:25:03]

A couple of high-level comments before I review the implementation further.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
File mojo/edk/embedder/entrypoints.cc (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
mojo/edk/embedder/entrypoints.cc:57: const MojoHandle* handles,
I know why you have this, but I'm really not a fan of exposing this. And
besides, the common case will be 0 handles and I think we can afford another
malloc/memcpy in the case you have a non-zero number of handles. Or, when you do
have handles, it'll likely be 1, so you could always reserve room for 1 handle.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
mojo/edk/embedder/entrypoints.cc:68: MojoResult MojoGetMessageBuffer(void*
message, void** buffer) {
Sorry, but not a fan. Two reasons:
1. Exposes internal details to the user. But also, a "Buffer API" should expose
just the buffer. The message itself is an implementation detail.
2. I'm concerned about the security aspect of making it easier to access the
message metadata. If an attacker compromises the process, it's easier for them
to re-route the message if they have easy access to the header.

Something to think about is crbug.com/590980. Having separate buffers for the
header and payload would make things simple. Even if you don't, you can always
map buffer address to message internally.

[Ken Rockot(use gerrit already), 2016-04-14 13:03:42]

On Apr 14, 2016 3:25 AM, <amistry@chromium.org> wrote:
>
> A couple of high-level comments before I review the implementation
further.
>
>
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> File mojo/edk/embedder/entrypoints.cc (right):
>
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> mojo/edk/embedder/entrypoints.cc:57: const MojoHandle* handles,
> I know why you have this, but I'm really not a fan of exposing this. And
> besides, the common case will be 0 handles and I think we can afford
> another malloc/memcpy in the case you have a non-zero number of handles.
> Or, when you do have handles, it'll likely be 1, so you could always
> reserve room for 1 handle.

Why does it matter that we expose this? To be clear, what you suggest is
how I started, with the same reasoning. Including the handles in the
message at creation time ended up feeling like a better API to me, and it's
less code (and more efficient code) overall.

>
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> mojo/edk/embedder/entrypoints.cc:68: MojoResult
> MojoGetMessageBuffer(void* message, void** buffer) {
> Sorry, but not a fan. Two reasons:
> 1. Exposes internal details to the user. But also, a "Buffer API" should
> expose just the buffer. The message itself is an implementation detail.

I disagree. It only seems like an internal detail if you know it's an
internal detail. As an API consumer it seems reasonable to believe Mojo
messages are objects with several properties, one of which being user
payload.

We could replace the void* with a new handle type or something, but the
difference would either only be aesthetic or it would add unnecessary
complexity.

> 2. I'm concerned about the security aspect of making it easier to access
> the message metadata. If an attacker compromises the process, it's
> easier for them to re-route the message if they have easy access to the
> header.

If an attacker compromises the process they have many options for tampering
with messages. I'm not sure how much this should influence our API design.

>
> Something to think about is crbug.com/590980. Having separate buffers
> for the header and payload would make things simple. Even if you don't,
> you can always map buffer address to message internally.

Yeah I thought about it. Seems more painful to implement than this API
though. We could always revisit that approach later when performance
improvements are less urgent.

>
> https://codereview.chromium.org/1880823005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-04-14 13:09:42]

On 2016/04/14 at 13:03:42, Ken Rockot wrote:
> On Apr 14, 2016 3:25 AM, <amistry@chromium.org> wrote:
> >
> > A couple of high-level comments before I review the implementation
> further.
> >
> >
> >
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> > File mojo/edk/embedder/entrypoints.cc (right):
> >
> >
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> > mojo/edk/embedder/entrypoints.cc:57: const MojoHandle* handles,
> > I know why you have this, but I'm really not a fan of exposing this. And
> > besides, the common case will be 0 handles and I think we can afford
> > another malloc/memcpy in the case you have a non-zero number of handles.
> > Or, when you do have handles, it'll likely be 1, so you could always
> > reserve room for 1 handle.
> 
> Why does it matter that we expose this? To be clear, what you suggest is
> how I started, with the same reasoning. Including the handles in the
> message at creation time ended up feeling like a better API to me, and it's
> less code (and more efficient code) overall.
> 
> >
> >
>
https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/embedder/entry...
> > mojo/edk/embedder/entrypoints.cc:68: MojoResult
> > MojoGetMessageBuffer(void* message, void** buffer) {
> > Sorry, but not a fan. Two reasons:
> > 1. Exposes internal details to the user. But also, a "Buffer API" should
> > expose just the buffer. The message itself is an implementation detail.
> 
> I disagree. It only seems like an internal detail if you know it's an
> internal detail. As an API consumer it seems reasonable to believe Mojo
> messages are objects with several properties, one of which being user
> payload.
> 
> We could replace the void* with a new handle type or something, but the
> difference would either only be aesthetic or it would add unnecessary
> complexity.
> 
> > 2. I'm concerned about the security aspect of making it easier to access
> > the message metadata. If an attacker compromises the process, it's
> > easier for them to re-route the message if they have easy access to the
> > header.
> 
> If an attacker compromises the process they have many options for tampering
> with messages. I'm not sure how much this should influence our API design.
> 
> >
> > Something to think about is crbug.com/590980. Having separate buffers
> > for the header and payload would make things simple. Even if you don't,
> > you can always map buffer address to message internally.
> 
> Yeah I thought about it. Seems more painful to implement than this API
> though. We could always revisit that approach later when performance
> improvements are less urgent.
> 
> >
> > https://codereview.chromium.org/1880823005/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

Also, I'd be OK with limiting CreateMessage to bytes-only if you feel strongly
about it. The common case would still be efficient, and the handles case could
be improved later with scatter-gather. WDYT?

[Ken Rockot(use gerrit already), 2016-04-18 15:09:34]

Ping? Any further comments/suggestions for how you want me to modify this CL
before you're willing to review it?

[Anand Mistry (off Chromium), 2016-04-19 12:57:35]

amistry@chromium.org changed reviewers:
+ sammc@chromium.org

[Anand Mistry (off Chromium), 2016-04-19 12:57:36]

+sam to get his opinions.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:8: #include <stdint.h>
#include <memory> for unique_ptr

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:71: std::unique_ptr<PortsMessage>*
message();
Returning a pointer to a unique_ptr seems weird. Why not just return
PortsMessage*, and not const? If it's to release the PortsMessage, add an
explicit ReleaseMessage which returns the unique_ptr.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:191: [num_handles, num_bytes,
&no_space, &may_discard, &ignore_num_bytes](
why did you swap bytes and handles? And why is ignore_num_bytes passed by
reference? and what is ignore_num_bytes anyways? This is kinda confusing.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:196:
sizeof(MessageForTransit::MessageHeader));
You could stick a "using MessageForTransit::MessageHeader" at the top of the
function and reduce the verbosity.

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:132: // DEPRECATED: This function will be
removed. Use |MojoWriteMessageNew()|.
I'd prefer if these weren't marked as deprecated just yet. As we discussed, the
new API might change significantly and I don't think it's wise to discourage the
current API just yet.

I know, there are very few users of the EDK's API, but still...

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:297: void** message);  // Out
Since you're not meant to directly read/write the message memory, it's really
just an opaque handle to a messgae and I'd prefer if that was more explicit by
having this be a handle type instead of a void*.

i.e:
typedef uintptr_t MessageHandle;

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:310: MOJO_SYSTEM_EXPORT MojoResult
MojoDestroyMessage(void* message);
nit: In my experience (mainly media APIs), buffer APIs generally "release" and
"return" buffers instead of "destroying" them.

[Ken Rockot(use gerrit already), 2016-04-19 18:11:26]

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:8: #include <stdint.h>
On 2016/04/19 at 12:57:35, Anand Mistry wrote:
> #include <memory> for unique_ptr

Done

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:71: std::unique_ptr<PortsMessage>*
message();
On 2016/04/19 at 12:57:35, Anand Mistry wrote:
> Returning a pointer to a unique_ptr seems weird. Why not just return
PortsMessage*, and not const? If it's to release the PortsMessage, add an
explicit ReleaseMessage which returns the unique_ptr.

The rationale was that a caller may or may not want to take ownership. In
practice though it doesn't matter and it's fine to always take ownership, so
changed it to TakePortsMessage which returns a unique_ptr.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
File mojo/edk/system/message_pipe_dispatcher.cc (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:191: [num_handles, num_bytes,
&no_space, &may_discard, &ignore_num_bytes](
On 2016/04/19 at 12:57:35, Anand Mistry wrote:
> why did you swap bytes and handles? And why is ignore_num_bytes passed by
reference? and what is ignore_num_bytes anyways? This is kinda confusing.

Didn't notice I had done the swap - probably from rewriting this line. Reverted.
Also changed the name of "ignore_num_bytes" to "read_any_size" and included some
extra commentary for it.

https://codereview.chromium.org/1880823005/diff/20001/mojo/edk/system/message...
mojo/edk/system/message_pipe_dispatcher.cc:196:
sizeof(MessageForTransit::MessageHeader));
On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> You could stick a "using MessageForTransit::MessageHeader" at the top of the
function and reduce the verbosity.

Done

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:132: // DEPRECATED: This function will be
removed. Use |MojoWriteMessageNew()|.
On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> I'd prefer if these weren't marked as deprecated just yet. As we discussed,
the new API might change significantly and I don't think it's wise to discourage
the current API just yet.
> 
> I know, there are very few users of the EDK's API, but still...

OK - Removed DEPRECATED notices and TODOs for now.

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:297: void** message);  // Out
On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> Since you're not meant to directly read/write the message memory, it's really
just an opaque handle to a messgae and I'd prefer if that was more explicit by
having this be a handle type instead of a void*.
> 
> i.e:
> typedef uintptr_t MessageHandle;

Done.

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:310: MOJO_SYSTEM_EXPORT MojoResult
MojoDestroyMessage(void* message);
On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> nit: In my experience (mainly media APIs), buffer APIs generally "release" and
"return" buffers instead of "destroying" them.

I left the names unchanged for now. In general I think Acquire and Release would
be good complementary verbs which better than Create and Destroy. However, I
don't like the idea of using "Acquire" - I think it would be confusing. I do
like "Alloc" or "Allocate", but then a better complementary verb just by
convention would be "Free" IMHO.

So I wanted to get your opinion before making any changes: How do you feel about
MojoAllocMessage (or MojoAllocateMessage) and MojoFreeMessage? Alternatively,
can you think of a verb better than Acquire to complement Release?

[Ken Rockot(use gerrit already), 2016-04-19 19:46:10]

Description was changed from

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoCreateMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoDestroyMessage: Destroys a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

The new read/write APIs will eventually replace the
old ones.

BUG=597379
==========

to

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoCreateMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoDestroyMessage: Destroys a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379
==========

[Anand Mistry (off Chromium), 2016-04-21 12:19:43]

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:310: MOJO_SYSTEM_EXPORT MojoResult
MojoDestroyMessage(void* message);
On 2016/04/19 18:11:25, Ken Rockot wrote:
> On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> > nit: In my experience (mainly media APIs), buffer APIs generally "release"
and
> "return" buffers instead of "destroying" them.
> 
> I left the names unchanged for now. In general I think Acquire and Release
would
> be good complementary verbs which better than Create and Destroy. However, I
> don't like the idea of using "Acquire" - I think it would be confusing. I do
> like "Alloc" or "Allocate", but then a better complementary verb just by
> convention would be "Free" IMHO.
> 
> So I wanted to get your opinion before making any changes: How do you feel
about
> MojoAllocMessage (or MojoAllocateMessage) and MojoFreeMessage? Alternatively,
> can you think of a verb better than Acquire to complement Release?

I like Alloc/Free better than Create/Destroy. I don't have a strong opinion on
alloc versus allocate.

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.cc (right):

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.cc:33: // is not a leak.
I don't believe this logic is correct. Actually, the whole ownership between
this, WriteMessage, and WriteMessageNew is confusing me.

Consider the case where you're using CreateMessage with a
PlatformHandleDispatcher. When the message creation succeeds,
PlatformHandleDispatcher::CompleteTransitAndClose() runs and releases it's
ownership of the PlatformHandle, effectively transferring ownership to this
message. Therefore, if the message send fails, the PlatformHandle leaks because
this doesn't close it.

However, if you look at WriteMessage, it only calls CompleteTransitAndClose() if
the write succeeds.

Now, where it's more confusing is that this message DOES take ownership of
handles, but then releases it if nothing takes over message_. But also, if the
MessagePipeDispatcher::WriteMessage fails after taking over message_, ownership
of handles reverts to the PlatformHandleDispatcher via CancelTransit. But
PortsMessage also owns its handles resulting in a double-free.

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:58: MessageForTransit** message,
WDYT about:
unique_ptr<MessageForTransit>* message,
instead? I think it makes ownership clear.

[Ken Rockot(use gerrit already), 2016-04-21 16:47:40]

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/20001/mojo/public/c/system/me...
mojo/public/c/system/message_pipe.h:310: MOJO_SYSTEM_EXPORT MojoResult
MojoDestroyMessage(void* message);
On 2016/04/21 at 12:19:42, Anand Mistry wrote:
> On 2016/04/19 18:11:25, Ken Rockot wrote:
> > On 2016/04/19 at 12:57:36, Anand Mistry wrote:
> > > nit: In my experience (mainly media APIs), buffer APIs generally "release"
and
> > "return" buffers instead of "destroying" them.
> > 
> > I left the names unchanged for now. In general I think Acquire and Release
would
> > be good complementary verbs which better than Create and Destroy. However, I
> > don't like the idea of using "Acquire" - I think it would be confusing. I do
> > like "Alloc" or "Allocate", but then a better complementary verb just by
> > convention would be "Free" IMHO.
> > 
> > So I wanted to get your opinion before making any changes: How do you feel
about
> > MojoAllocMessage (or MojoAllocateMessage) and MojoFreeMessage?
Alternatively,
> > can you think of a verb better than Acquire to complement Release?
> 
> I like Alloc/Free better than Create/Destroy. I don't have a strong opinion on
alloc versus allocate.

Done. MojoAllocMessage and MojoFreeMessage.

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.cc (right):

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.cc:33: // is not a leak.
On 2016/04/21 at 12:19:42, Anand Mistry wrote:
> I don't believe this logic is correct. Actually, the whole ownership between
this, WriteMessage, and WriteMessageNew is confusing me.
> 
> Consider the case where you're using CreateMessage with a
PlatformHandleDispatcher. When the message creation succeeds,
PlatformHandleDispatcher::CompleteTransitAndClose() runs and releases it's
ownership of the PlatformHandle, effectively transferring ownership to this
message. Therefore, if the message send fails, the PlatformHandle leaks because
this doesn't close it.
> 
> However, if you look at WriteMessage, it only calls CompleteTransitAndClose()
if the write succeeds.
> 
> Now, where it's more confusing is that this message DOES take ownership of
handles, but then releases it if nothing takes over message_. But also, if the
MessagePipeDispatcher::WriteMessage fails after taking over message_, ownership
of handles reverts to the PlatformHandleDispatcher via CancelTransit. But
PortsMessage also owns its handles resulting in a double-free.

Yes, this was all broken and confusing because I was trying to maintain the
original behavior of WriteMessage while implementing it in terms of message
objects. This was really only to preserve behavior expected by tests though
(namely that a failed WriteMessage leaves sent handles open), so I've decided to
change that behavior and update the test expectations. Bindings don't rely on
being able to recover from WriteMessage failure so it seems like a pointless
behavior to preserve.

WriteMessage is simpler now and the lifetime of handles should be clear.

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/80001/mojo/edk/system/message...
mojo/edk/system/message_for_transit.h:58: MessageForTransit** message,
On 2016/04/21 at 12:19:42, Anand Mistry wrote:
> WDYT about:
> unique_ptr<MessageForTransit>* message,
> instead? I think it makes ownership clear.

Done

[Ken Rockot(use gerrit already), 2016-04-22 00:02:00]

Description was changed from

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoCreateMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoDestroyMessage: Destroys a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379
==========

to

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoAllocMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoFreeMessage: Frees a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379
==========

[Ken Rockot(use gerrit already), 2016-04-22 00:02:04]

Any chance this could get another review pass before the weekend? :D

[Ken Rockot(use gerrit already), 2016-04-25 16:24:16]

ping.

[Anand Mistry (off Chromium), 2016-04-26 07:38:17]

lgtm

[Sam McNally, 2016-04-26 08:20:43]

lgtm

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_for_transit.h:67: const void* bytes() const;
Why not inline these?

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_pipe_dispatcher.cc (left):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_pipe_dispatcher.cc:378: // Either |*num_bytes| or
|*num_handles| wasn't sufficient to hold this
This still applies sometimes.

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_pipe_dispatcher.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_pipe_dispatcher.h:63: bool read_any_size) override;
This should be named the same as in Dispatcher.

https://codereview.chromium.org/1880823005/diff/140001/mojo/public/c/system/m...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/public/c/system/m...
mojo/public/c/system/message_pipe.h:219: // Reads the next message from a
message pipe and returns an address to a
s/an address to//

[Ken Rockot(use gerrit already), 2016-04-26 15:28:23]

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_for_transit.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_for_transit.h:67: const void* bytes() const;
On 2016/04/26 at 08:20:42, Sam McNally wrote:
> Why not inline these?

Done

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_pipe_dispatcher.cc (left):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_pipe_dispatcher.cc:378: // Either |*num_bytes| or
|*num_handles| wasn't sufficient to hold this
On 2016/04/26 at 08:20:42, Sam McNally wrote:
> This still applies sometimes.

Fixed

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
File mojo/edk/system/message_pipe_dispatcher.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/edk/system/messag...
mojo/edk/system/message_pipe_dispatcher.h:63: bool read_any_size) override;
On 2016/04/26 at 08:20:43, Sam McNally wrote:
> This should be named the same as in Dispatcher.

Done

https://codereview.chromium.org/1880823005/diff/140001/mojo/public/c/system/m...
File mojo/public/c/system/message_pipe.h (right):

https://codereview.chromium.org/1880823005/diff/140001/mojo/public/c/system/m...
mojo/public/c/system/message_pipe.h:219: // Reads the next message from a
message pipe and returns an address to a
On 2016/04/26 at 08:20:43, Sam McNally wrote:
> s/an address to//

Done

[Ken Rockot(use gerrit already), 2016-04-26 15:28:36]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-04-26 15:28:36]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1880823005/#ps160001
(title: " ")

[commit-bot: I haz the power, 2016-04-26 15:29:05]

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

[Ken Rockot(use gerrit already), 2016-04-26 16:11:42]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-04-26 16:18:09]

Patchset #9 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-04-26 16:18:13]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-04-26 16:18:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1880823005/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-04-26 16:18:29]

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

[commit-bot: I haz the power, 2016-04-26 17:56:33]

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

[commit-bot: I haz the power, 2016-04-26 17:56:35]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-04-26 18:14:47]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-04-26 18:15:08]

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

[commit-bot: I haz the power, 2016-04-26 19:57:04]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-04-26 19:58:18]

Description was changed from

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoAllocMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoFreeMessage: Frees a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379
==========

to

==========
[mojo-edk] Add explicit message object APIs

Adds several new system API methods:

  - MojoAllocMessage: Allocates a new message object
    for a fixed payload size and set of handles
  - MojoFreeMessage: Frees a message object
  - MojoGetMessageBuffer: Acquires a mutable buffer
    address to manipulate message object payload
  - MojoWriteMessageNew: Writes a message object to
    a message pipe, relinquishing ownership
  - MojoReadMessageNew: Reads a message object from a
    message pipe, taking ownership

Message objects themselves are respresented in the public
API by an opaque MojoMessageHandle.

The purpose of these APIs is to encapsulate ownership
transfer of message buffers, allowing us to avoid several
redundant copies that are forced by MojoWriteMessage()
and MojoReadMessage() design.

BUG=597379

Committed: https://crrev.com/72d05a46b2636d9d8cbf9a75b1c5d2bc2d8abe15
Cr-Commit-Position: refs/heads/master@{#389859}
==========

[commit-bot: I haz the power, 2016-04-26 19:58:19]

Patchset 9 (id:??) landed as
https://crrev.com/72d05a46b2636d9d8cbf9a75b1c5d2bc2d8abe15
Cr-Commit-Position: refs/heads/master@{#389859}