[mojo-edk] Add support for transferring mach ports.

[mojo-edk] Add support for transferring mach ports.

This change adds the ability to transfer mach ports over Mojo. Mach ports can
either be wrapped using CreatePlatformHandleWrapper() or a Mojo shared buffer
can be created using CreateSharedBufferWrapper(). For now, Mojo shared buffers
created using MojoCreateSharedBuffer() (or MojoCreateDataPipe()) will still
use posix shared memory.

BUG=582468
Committed: https://crrev.com/ba362cd27ec5b4410bd992daccc88a164988384a
Cr-Commit-Position: refs/heads/master@{#381615}

[Anand Mistry (off Chromium), 2016-03-09 06:01:23]

Description was changed from

==========
Add Mach ports transport to Mojo

BUG=582468
==========

to

==========
[mojo-edk] Add support for transferring mach ports.

This change adds the ability to transfer mach ports over Mojo. Mach ports can
either be wrapped using CreatePlatformHandleWrapper() or a Mojo shared buffer
can be created using CreateSharedBufferWrapper(). For now, Mojo shared buffers
created using MojoCreateSharedBuffer() (or MojoCreateDataPipe()) will still
use posix shared memory.

BUG=582468
==========

[Anand Mistry (off Chromium), 2016-03-09 06:13:55]

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

[Anand Mistry (off Chromium), 2016-03-09 06:13:56]

Three points to make:
1. base::PortProvider::Observer doesn't currently do anything. That will be
implemented in a separate cl.
2. There's no support for forwarding task ports from grandchildren to roots.
I've implemented this locally and it works, but I'm focused on chrome's use case
first.
3. No Mojo shell integration because of #2.

For these reasons, I've left Mojo's shared buffer creation to use posix. I've
switched it locally and all tests pass (except
SharedBufferTest.PassHandleBetweenCousins, which passes when I implement shell
support and task port forwarding).

A big chunk of this change is copy-pasted from the attachment brokering code.

[Ken Rockot(use gerrit already), 2016-03-09 08:34:31]

Just nits and questions but lgtm overall. I defer to Erik for MachPortRelay
review and anything else he wants to look over.

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:627:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
I'm confused about why this (and really the AttachmentBrokerPrivileged above) is
created here instead of somewhere a bit more generic. If this is a
process-global thing, why is RPHI responsible for ensuring it exists, and why
does every RPHI instance do it? Why not BrowserMainLoop or some other very-early
initialization?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channel.h
File mojo/edk/system/channel.h (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.h:75: "mach_port_t must be smaller than uint32_t");
nit: must be no larger than uint32_t

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.h:160: MachPortsEntry* mach_ports_ = nullptr;
nit: this comment appears to be out of date since they're in the extra header
now

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
File mojo/edk/system/channel_posix.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel_posix.cc:141: // message.
nit: end of the message -> extra header

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/test/mojo_tes...
File mojo/edk/test/mojo_test_base.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/test/mojo_tes...
mojo/edk/test/mojo_test_base.cc:55: base::AutoLock
lock(g_mach_broker->GetLock());
nit: please document why you have to acquire this lock before starting the child

https://codereview.chromium.org/1712143002/diff/260001/mojo/mojo_edk.gyp
File mojo/mojo_edk.gyp (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/mojo_edk.gyp#newc...
mojo/mojo_edk.gyp:149: ['OS=="mac" and OS!="ios"', {
possibly just the code review UI but this indentation appears to be wrong

[erikchen, 2016-03-09 19:13:17]

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:627:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
On 2016/03/09 08:34:31, Ken Rockot wrote:
> I'm confused about why this (and really the AttachmentBrokerPrivileged above)
is
> created here instead of somewhere a bit more generic. If this is a
> process-global thing, why is RPHI responsible for ensuring it exists, and why
> does every RPHI instance do it? Why not BrowserMainLoop or some other
very-early
> initialization?

That would be cleaner.

Right now, we actually call this same logic in 3 different places (you'll have
to do the same for mojo::edk as well)
https://code.google.com/p/chromium/codesearch#search/&q=brokerprivileged::Cre...

The idea was that we should attempt to lazy initialize, and only perform this
setup when we need it (which is when the browser tries to make a child process).

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/embedder/embe...
File mojo/edk/embedder/embedder.h (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/embedder/embe...
mojo/edk/embedder/embedder.h:143: // called on any thread, but must be set in
the root process before any mach
s/mach/Mach

Mach is a proper noun.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
File mojo/edk/system/channel.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.cc:17: #if defined(OS_MACOSX)
it seems you're inconsistent on whether to include a "&& !defined(OS_IOS)".
Could you standardize?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.cc:114: #if !defined(OS_WIN) && (!defined(OS_MACOSX) ||
defined(OS_IOS))
could you rewrite as !(defined(OS_MACOSX) && !defined(OS_IOS))?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
File mojo/edk/system/mach_port_relay.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:37: kern_return_t SendMachPort(mach_port_t
endpoint,
Can we move everything in this anonymous namespace into base/? It looks pretty
much like a copy-paste from attachment_broker_privileged_mac.cc.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:152: base::mac::ScopedMachReceiveRight
message_port(handle->port);
This method takes ownership of handle->port, and replaces it with a new port. On
failure, it leaks a reference to the original port (bug?). 

Please add comments for public methods of this class. In general, I'd like to be
able to read the header and know: 1. what the method does. 2. how it behaves on
failure. 3. any funky ownership semantics that are not expressed by the
input/output parameters.

For this method, something like:
// See [foo] for the definition of intermediate and final Mach ports.
// This method interprets each Mach port in |handles| as an intermediate Mach
port. It replaces each handle->port with the final Mach port. This method takes
ownership of the original, intermediate Mach port, and releases ownership of the
final Mach port back to the caller.
// Returns |false| on failure, and makes no guarantees about whether each
handle->port is an intermediate, or final Mach port.

attempts to replace each Mach port

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:157: return false;
clear handle->port?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:160: num_received++;
this variable is never used.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:199: if (intermediate_port == MACH_PORT_NULL)
{
handle->port should be set to MACH_PORT_NULL?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:203: handle->port = intermediate_port;
we need some way of leaking this handle from the current process, right? Since
the destination process will take ownership?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:418: bool use_broker = false;
What happens if the message has mach ports but there is no broker? Should there
be a CHECK?

[Anand Mistry (off Chromium), 2016-03-11 07:44:18]

So, the major change in this version is that I've split the Mach port handling
in PlatformHandle into a "real" mach port, and a mach port "name". The idea
being that a name is a reference to a Mach port in another process, which
doesn't exist (or may point to a different port) in the current process. Let me
know if this is a terrible idea.

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:627:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
On 2016/03/09 19:13:16, erikchen wrote:
> On 2016/03/09 08:34:31, Ken Rockot wrote:
> > I'm confused about why this (and really the AttachmentBrokerPrivileged
above)
> is
> > created here instead of somewhere a bit more generic. If this is a
> > process-global thing, why is RPHI responsible for ensuring it exists, and
why
> > does every RPHI instance do it? Why not BrowserMainLoop or some other
> very-early
> > initialization?
> 
> That would be cleaner.

I agree, but think it's outside the scope of this CL.

> 
> Right now, we actually call this same logic in 3 different places (you'll have
> to do the same for mojo::edk as well)
>
https://code.google.com/p/chromium/codesearch#search/&q=brokerprivileged::Cre...
> 
> The idea was that we should attempt to lazy initialize, and only perform this
> setup when we need it (which is when the browser tries to make a child
process).
> 

I've added it to the one other place. The other (ChildProcessHostImpl) doesn't
actually create the broker on mac. Maybe that's a bug?

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/embedder/embe...
File mojo/edk/embedder/embedder.h (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/embedder/embe...
mojo/edk/embedder/embedder.h:143: // called on any thread, but must be set in
the root process before any mach
On 2016/03/09 19:13:17, erikchen wrote:
> s/mach/Mach
> 
> Mach is a proper noun.

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
File mojo/edk/system/channel.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.cc:17: #if defined(OS_MACOSX)
On 2016/03/09 19:13:17, erikchen wrote:
> it seems you're inconsistent on whether to include a "&& !defined(OS_IOS)".
> Could you standardize?

Oops. I thought I standardised these. I'll take another pass at the CL to make
sure I caught all these.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.cc:114: #if !defined(OS_WIN) && (!defined(OS_MACOSX) ||
defined(OS_IOS))
On 2016/03/09 19:13:17, erikchen wrote:
> could you rewrite as !(defined(OS_MACOSX) && !defined(OS_IOS))?

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channel.h
File mojo/edk/system/channel.h (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.h:75: "mach_port_t must be smaller than uint32_t");
On 2016/03/09 08:34:31, Ken Rockot wrote:
> nit: must be no larger than uint32_t

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel.h:160: MachPortsEntry* mach_ports_ = nullptr;
On 2016/03/09 08:34:31, Ken Rockot wrote:
> nit: this comment appears to be out of date since they're in the extra header
> now

Done, and updated the windows comment.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
File mojo/edk/system/channel_posix.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/channe...
mojo/edk/system/channel_posix.cc:141: // message.
On 2016/03/09 08:34:31, Ken Rockot wrote:
> nit: end of the message -> extra header

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
File mojo/edk/system/mach_port_relay.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:37: kern_return_t SendMachPort(mach_port_t
endpoint,
On 2016/03/09 19:13:17, erikchen wrote:
> Can we move everything in this anonymous namespace into base/? It looks pretty
> much like a copy-paste from attachment_broker_privileged_mac.cc.

Done. Actually done in https://codereview.chromium.org/1778203002/, but once
that's submitted and I rebase, the diff in this change will go away. So ignore
anything in //base or //ipc in this cl.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:152: base::mac::ScopedMachReceiveRight
message_port(handle->port);
On 2016/03/09 19:13:17, erikchen wrote:
> This method takes ownership of handle->port, and replaces it with a new port.
On
> failure, it leaks a reference to the original port (bug?). 
> 
> Please add comments for public methods of this class. In general, I'd like to
be
> able to read the header and know: 1. what the method does. 2. how it behaves
on
> failure. 3. any funky ownership semantics that are not expressed by the
> input/output parameters.
> 
> For this method, something like:
> // See [foo] for the definition of intermediate and final Mach ports.
> // This method interprets each Mach port in |handles| as an intermediate Mach
> port. It replaces each handle->port with the final Mach port. This method
takes
> ownership of the original, intermediate Mach port, and releases ownership of
the
> final Mach port back to the caller.
> // Returns |false| on failure, and makes no guarantees about whether each
> handle->port is an intermediate, or final Mach port.
> 
> attempts to replace each Mach port 

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:157: return false;
On 2016/03/09 19:13:17, erikchen wrote:
> clear handle->port?

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:160: num_received++;
On 2016/03/09 19:13:17, erikchen wrote:
> this variable is never used. 

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:199: if (intermediate_port == MACH_PORT_NULL)
{
On 2016/03/09 19:13:17, erikchen wrote:
> handle->port should be set to MACH_PORT_NULL?

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/mach_p...
mojo/edk/system/mach_port_relay.cc:203: handle->port = intermediate_port;
On 2016/03/09 19:13:17, erikchen wrote:
> we need some way of leaking this handle from the current process, right? Since
> the destination process will take ownership?

This is tricky because there's a mixture of "real" mach ports and port names
here. When a port is sent, the port is replaced with a name which as I
understand it, isn't a valid mach port in the current process.

To solve this, I've extended the PlatformHandle abstraction to also contain Mach
port names which do not get closed (since they're not really open).

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:418: bool use_broker = false;
On 2016/03/09 19:13:17, erikchen wrote:
> What happens if the message has mach ports but there is no broker? Should
there
> be a CHECK?

That's covered below. The child startup process is async and so we could be
sending messages before there's a connection to the broker (root process in this
context). If that connection doesn't exist, the messages are queued.

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/test/mojo_tes...
File mojo/edk/test/mojo_test_base.cc (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/edk/test/mojo_tes...
mojo/edk/test/mojo_test_base.cc:55: base::AutoLock
lock(g_mach_broker->GetLock());
On 2016/03/09 08:34:31, Ken Rockot wrote:
> nit: please document why you have to acquire this lock before starting the
child

Done.

https://codereview.chromium.org/1712143002/diff/260001/mojo/mojo_edk.gyp
File mojo/mojo_edk.gyp (right):

https://codereview.chromium.org/1712143002/diff/260001/mojo/mojo_edk.gyp#newc...
mojo/mojo_edk.gyp:149: ['OS=="mac" and OS!="ios"', {
On 2016/03/09 08:34:31, Ken Rockot wrote:
> possibly just the code review UI but this indentation appears to be wrong

It's because my vim was inserting tabs instead of spaces. Sigh!

[erikchen, 2016-03-11 18:09:40]

mojo/edk/system/mach_port_relay.{h,cc} lgtm

In the future, please create a local branch A, which includes your first set of
changes, associated with CL A_CL.

Then create a local branch B (for this CL), whose upstream is A. Rietveld will
"do the right thing" and create a dependent patchset, which will be pulled in
for CQ runs, but not for review.

[Anand Mistry (off Chromium), 2016-03-12 06:10:28]

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

[Anand Mistry (off Chromium), 2016-03-12 06:10:29]

sievers@chromium.org: Please review changes in //content

[Anand Mistry (off Chromium), 2016-03-14 22:05:54]

amistry@chromium.org changed reviewers:
+ jam@chromium.org
- sievers@chromium.org

[Anand Mistry (off Chromium), 2016-03-14 22:05:55]

-sievers, +jam who works on mojo

[jam, 2016-03-15 16:27:42]

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:145:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
it doesn't make sense that this code is run each time a child process is
created, given that it's a single time initialization. i realize this just
copies what the previous block does, but i think that's wrong. they should both
be done once in content's browser initialization code, i.e. in
BrowserMainLoop::PostMainMessageLoopStart()

[Anand Mistry (off Chromium), 2016-03-15 20:26:10]

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:145:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
On 2016/03/15 16:27:42, jam wrote:
> it doesn't make sense that this code is run each time a child process is
> created, given that it's a single time initialization. i realize this just
> copies what the previous block does, but i think that's wrong. they should
both
> be done once in content's browser initialization code, i.e. in
> BrowserMainLoop::PostMainMessageLoopStart()

I agree, but I think it should be done to both Mojo and IPC, not just one (it
would be confusing to have IPC here and Mojo elsewhere). I would like to keep
this here for this change, and do a follow up that moves both to
BrowserMainLoop.

[jam, 2016-03-15 23:13:09]

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:145:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
On 2016/03/15 20:26:10, Anand Mistry wrote:
> On 2016/03/15 16:27:42, jam wrote:
> > it doesn't make sense that this code is run each time a child process is
> > created, given that it's a single time initialization. i realize this just
> > copies what the previous block does, but i think that's wrong. they should
> both
> > be done once in content's browser initialization code, i.e. in
> > BrowserMainLoop::PostMainMessageLoopStart()
> 
> I agree, but I think it should be done to both Mojo and IPC, not just one (it
> would be confusing to have IPC here and Mojo elsewhere). I would like to keep
> this here for this change, and do a follow up that moves both to
> BrowserMainLoop.

yep I meant do both. can they just be moved in this cl? that should be a small
change

[Anand Mistry (off Chromium), 2016-03-15 23:30:56]

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
content/browser/browser_child_process_host_impl.cc:145:
mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
On 2016/03/15 23:13:09, jam wrote:
> On 2016/03/15 20:26:10, Anand Mistry wrote:
> > On 2016/03/15 16:27:42, jam wrote:
> > > it doesn't make sense that this code is run each time a child process is
> > > created, given that it's a single time initialization. i realize this just
> > > copies what the previous block does, but i think that's wrong. they should
> > both
> > > be done once in content's browser initialization code, i.e. in
> > > BrowserMainLoop::PostMainMessageLoopStart()
> > 
> > I agree, but I think it should be done to both Mojo and IPC, not just one
(it
> > would be confusing to have IPC here and Mojo elsewhere). I would like to
keep
> > this here for this change, and do a follow up that moves both to
> > BrowserMainLoop.
> 
> yep I meant do both. can they just be moved in this cl? that should be a small
> change

It is small, but this change is already big and complicated. I'd rather leave it
to a separate cl.

[Anand Mistry (off Chromium), 2016-03-16 00:09:05]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-16 00:09:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712143002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712143002/320001

[jam, 2016-03-16 00:12:44]

On 2016/03/15 23:30:56, Anand Mistry wrote:
>
https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
> File content/browser/browser_child_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
> content/browser/browser_child_process_host_impl.cc:145:
> mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
> On 2016/03/15 23:13:09, jam wrote:
> > On 2016/03/15 20:26:10, Anand Mistry wrote:
> > > On 2016/03/15 16:27:42, jam wrote:
> > > > it doesn't make sense that this code is run each time a child process is
> > > > created, given that it's a single time initialization. i realize this
just
> > > > copies what the previous block does, but i think that's wrong. they
should
> > > both
> > > > be done once in content's browser initialization code, i.e. in
> > > > BrowserMainLoop::PostMainMessageLoopStart()
> > > 
> > > I agree, but I think it should be done to both Mojo and IPC, not just one
> (it
> > > would be confusing to have IPC here and Mojo elsewhere). I would like to
> keep
> > > this here for this change, and do a follow up that moves both to
> > > BrowserMainLoop.
> > 
> > yep I meant do both. can they just be moved in this cl? that should be a
small
> > change
> 
> It is small, but this change is already big and complicated. I'd rather leave
it
> to a separate cl.

ok can the existing code be moved first, and then this land? it just seems wrong
to add new code that we agree should run once, but is instead running per
process launch.

[Anand Mistry (off Chromium), 2016-03-16 00:24:11]

Well, I would really like to land this, but I'm about to get on a flight.
I'm just trying to land this without delaying too much since I have some
follow up stuff I need to do.

On Wednesday, 16 March 2016, <jam@chromium.org> wrote:

> On 2016/03/15 23:30:56, Anand Mistry wrote:
> >
>
>
https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
> > File content/browser/browser_child_process_host_impl.cc (right):
> >
> >
>
>
https://codereview.chromium.org/1712143002/diff/300001/content/browser/browse...
> > content/browser/browser_child_process_host_impl.cc:145:
> > mojo::edk::SetMachPortProviderIfNeeded(MachBroker::GetInstance());
> > On 2016/03/15 23:13:09, jam wrote:
> > > On 2016/03/15 20:26:10, Anand Mistry wrote:
> > > > On 2016/03/15 16:27:42, jam wrote:
> > > > > it doesn't make sense that this code is run each time a child
> process is
> > > > > created, given that it's a single time initialization. i realize
> this
> just
> > > > > copies what the previous block does, but i think that's wrong. they
> should
> > > > both
> > > > > be done once in content's browser initialization code, i.e. in
> > > > > BrowserMainLoop::PostMainMessageLoopStart()
> > > >
> > > > I agree, but I think it should be done to both Mojo and IPC, not
> just one
> > (it
> > > > would be confusing to have IPC here and Mojo elsewhere). I would
> like to
> > keep
> > > > this here for this change, and do a follow up that moves both to
> > > > BrowserMainLoop.
> > >
> > > yep I meant do both. can they just be moved in this cl? that should be
> a
> small
> > > change
> >
> > It is small, but this change is already big and complicated. I'd rather
> leave
> it
> > to a separate cl.
>
> ok can the existing code be moved first, and then this land? it just seems
> wrong
> to add new code that we agree should run once, but is instead running per
> process launch.
>
> https://codereview.chromium.org/1712143002/
>

-- 
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.

[commit-bot: I haz the power, 2016-03-16 01:18:55]

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

[commit-bot: I haz the power, 2016-03-16 01:18:56]

Dry run: This issue passed the CQ dry run.

[Anand Mistry (off Chromium), 2016-03-16 04:22:01]

On 2016/03/16 00:24:11, Anand Mistry wrote:
> Well, I would really like to land this, but I'm about to get on a flight.
> I'm just trying to land this without delaying too much since I have some
> follow up stuff I need to do.
> 

And in case you're wondering why I'm pushing back so much, it's because I can't
reasonably do local builds (on a MacBook Pro). Any change outside of //mojo is
going to be a very long round-trip, so I want to isolate those changes as much
as possible. Basically, my workflow has been: do change, upload cl, run tryjob.

[Anand Mistry (off Chromium), 2016-03-16 22:17:11]

jam: PTAL. I've moved the setup code for mojo to browser startup, and I've also
removed the "IfNeeded" from the function name since it's no longer relevant.

[jam, 2016-03-16 22:33:36]

On 2016/03/16 22:17:11, Anand Mistry wrote:
> jam: PTAL. I've moved the setup code for mojo to browser startup, and I've
also
> removed the "IfNeeded" from the function name since it's no longer relevant.

lgtm

is there a bug filed for the other stuff, if you're not going to do it?

[Anand Mistry (off Chromium), 2016-03-16 23:11:45]

On 2016/03/16 22:33:36, jam wrote:
> On 2016/03/16 22:17:11, Anand Mistry wrote:
> > jam: PTAL. I've moved the setup code for mojo to browser startup, and I've
> also
> > removed the "IfNeeded" from the function name since it's no longer relevant.
> 
> lgtm
> 
> is there a bug filed for the other stuff, if you're not going to do it?

I am going to do it. It's just a cleanup so I haven't filed a bug.

[Anand Mistry (off Chromium), 2016-03-16 23:13:40]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-03-16 23:13:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1712143002/#ps340001
(title: "Move setup code")

[commit-bot: I haz the power, 2016-03-16 23:14:21]

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

[commit-bot: I haz the power, 2016-03-16 23:27:29]

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

[commit-bot: I haz the power, 2016-03-16 23:27:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Anand Mistry (off Chromium), 2016-03-16 23:33:34]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-03-16 23:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
erikchen@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1712143002/#ps360001
(title: "Fix build")

[commit-bot: I haz the power, 2016-03-16 23:34:03]

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

[commit-bot: I haz the power, 2016-03-17 00:53:57]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-17 00:55:06]

Description was changed from

==========
[mojo-edk] Add support for transferring mach ports.

This change adds the ability to transfer mach ports over Mojo. Mach ports can
either be wrapped using CreatePlatformHandleWrapper() or a Mojo shared buffer
can be created using CreateSharedBufferWrapper(). For now, Mojo shared buffers
created using MojoCreateSharedBuffer() (or MojoCreateDataPipe()) will still
use posix shared memory.

BUG=582468
==========

to

==========
[mojo-edk] Add support for transferring mach ports.

This change adds the ability to transfer mach ports over Mojo. Mach ports can
either be wrapped using CreatePlatformHandleWrapper() or a Mojo shared buffer
can be created using CreateSharedBufferWrapper(). For now, Mojo shared buffers
created using MojoCreateSharedBuffer() (or MojoCreateDataPipe()) will still
use posix shared memory.

BUG=582468

Committed: https://crrev.com/ba362cd27ec5b4410bd992daccc88a164988384a
Cr-Commit-Position: refs/heads/master@{#381615}
==========

[commit-bot: I haz the power, 2016-03-17 00:55:07]

Patchset 19 (id:??) landed as
https://crrev.com/ba362cd27ec5b4410bd992daccc88a164988384a
Cr-Commit-Position: refs/heads/master@{#381615}