remoting: Add AttachmentBroker for daemon and desktop processes

remoting: Add AttachmentBroker for daemon and desktop processes

Daemon and desktop processes didn't have attachment brokers, and so the
desktop process was failing when trying to pass shared buffer to the
network process.

BUG=581174
Committed: https://crrev.com/77a444dc8136e3efb8542d6965b3e8f9439c5266
Cr-Commit-Position: refs/heads/master@{#371858}

[Sergey Ulanov, 2016-01-26 21:34:11]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-01-26 21:35:01]

sergeyu@chromium.org changed reviewers:
+ erikchen@chromium.org, joedow@chromium.org

[erikchen, 2016-01-26 21:40:36]

lgtm

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/win/unpri...
File remoting/host/win/unprivileged_process_delegate.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/win/unpri...
remoting/host/win/unprivileged_process_delegate.cc:410: channel_.reset();
Should this go through CloseChannel()?

[joedow, 2016-01-26 21:42:51]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
It looks like this method has different args for OSX and I believe this file is
compiled into our mac host.  Does this need a conditional around it for OSX vs.
other host builds?

[erikchen, 2016-01-26 21:47:00]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
On 2016/01/26 21:42:51, joedow wrote:
> It looks like this method has different args for OSX and I believe this file
is
> compiled into our mac host.  Does this need a conditional around it for OSX
vs.
> other host builds?

Oh, yes. The Chrome browser process, on Mac, has a Port Provider
(base::PortProvider) which provides the task port of its child processes. This
is also required for attachment brokering. I'm surprised this hasn't broken on
Mac since we've been passing Mach SharedMemoryHandles on Mac since M49.

[Sergey Ulanov, 2016-01-27 01:38:39]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
On 2016/01/26 21:42:51, joedow wrote:
> It looks like this method has different args for OSX and I believe this file
is
> compiled into our mac host.  Does this need a conditional around it for OSX
vs.
> other host builds?

There are some files like this one that are compiled only on platform that don't
use multi-process hosts (i.e. OSX and Linux). This is mainly so that this code
can be unittested on all platforms and to make it easier to enabled
multi-process when necessary.
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded() doesn't do anything on
Linux, but we can still call it. OSX does need attachment broker.

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/win/unpri...
File remoting/host/win/unprivileged_process_delegate.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/win/unpri...
remoting/host/win/unprivileged_process_delegate.cc:410: channel_.reset();
On 2016/01/26 21:40:36, erikchen wrote:
> Should this go through CloseChannel()?

Yes, thanks for catching this.

[Sergey Ulanov, 2016-01-27 01:43:02]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
On 2016/01/26 21:47:00, erikchen wrote:
> On 2016/01/26 21:42:51, joedow wrote:
> > It looks like this method has different args for OSX and I believe this file
> is
> > compiled into our mac host.  Does this need a conditional around it for OSX
> vs.
> > other host builds?
> 
> Oh, yes. The Chrome browser process, on Mac, has a Port Provider
> (base::PortProvider) which provides the task port of its child processes. This
> is also required for attachment brokering. I'm surprised this hasn't broken on
> Mac since we've been passing Mach SharedMemoryHandles on Mac since M49.

We are not using this code on OSX.

Updated this code to compile on OSX now.

Erik, is it possible to update CreateBrokerIfNeeded() to
callMachBroker::GetInstance()? then initialization here wouldn't need to be
platform-specific

[Sergey Ulanov, 2016-01-27 01:43:10]

Patchset #2 (id:40001) has been deleted

[Sergey Ulanov, 2016-01-27 01:43:19]

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

[commit-bot: I haz the power, 2016-01-27 01:43:48]

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

[commit-bot: I haz the power, 2016-01-27 01:58:20]

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

[commit-bot: I haz the power, 2016-01-27 01:58:21]

Dry run: 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_...)

[erikchen, 2016-01-27 15:16:41]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
On 2016/01/27 01:43:02, Sergey Ulanov wrote:
> On 2016/01/26 21:47:00, erikchen wrote:
> > On 2016/01/26 21:42:51, joedow wrote:
> > > It looks like this method has different args for OSX and I believe this
file
> > is
> > > compiled into our mac host.  Does this need a conditional around it for
OSX
> > vs.
> > > other host builds?
> > 
> > Oh, yes. The Chrome browser process, on Mac, has a Port Provider
> > (base::PortProvider) which provides the task port of its child processes.
This
> > is also required for attachment brokering. I'm surprised this hasn't broken
on
> > Mac since we've been passing Mach SharedMemoryHandles on Mac since M49.
> 
> We are not using this code on OSX.
> 
> Updated this code to compile on OSX now.
> 
> Erik, is it possible to update CreateBrokerIfNeeded() to
> callMachBroker::GetInstance()? then initialization here wouldn't need to be
> platform-specific

If this code isn't being used on OSX, why is it being compiled on OSX? I'm
hesitant to make changes that make it easier to incorrectly use an API. It
doesn't make sense to instantiate a privileged AttachmentBroker without a Mach
broker.

Can we just wrap this in !defined(OS_MACOSX)?

[Sergey Ulanov, 2016-01-27 18:29:12]

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
File remoting/host/daemon_process.cc (right):

https://codereview.chromium.org/1634933007/diff/20001/remoting/host/daemon_pr...
remoting/host/daemon_process.cc:188:
IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
On 2016/01/27 15:16:41, erikchen wrote:
> On 2016/01/27 01:43:02, Sergey Ulanov wrote:
> > On 2016/01/26 21:47:00, erikchen wrote:
> > > On 2016/01/26 21:42:51, joedow wrote:
> > > > It looks like this method has different args for OSX and I believe this
> file
> > > is
> > > > compiled into our mac host.  Does this need a conditional around it for
> OSX
> > > vs.
> > > > other host builds?
> > > 
> > > Oh, yes. The Chrome browser process, on Mac, has a Port Provider
> > > (base::PortProvider) which provides the task port of its child processes.
> This
> > > is also required for attachment brokering. I'm surprised this hasn't
broken
> on
> > > Mac since we've been passing Mach SharedMemoryHandles on Mac since M49.
> > 
> > We are not using this code on OSX.
> > 
> > Updated this code to compile on OSX now.
> > 
> > Erik, is it possible to update CreateBrokerIfNeeded() to
> > callMachBroker::GetInstance()? then initialization here wouldn't need to be
> > platform-specific
> 
> If this code isn't being used on OSX, why is it being compiled on OSX? 

The goal is to implement multi-process host on OSX as well, and maybe eventually
on Linux.


> I'm
> hesitant to make changes that make it easier to incorrectly use an API. It
> doesn't make sense to instantiate a privileged AttachmentBroker without a Mach
> broker.
> 
> Can we just wrap this in !defined(OS_MACOSX)?

Done.

[Sergey Ulanov, 2016-01-27 18:29:32]

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

[commit-bot: I haz the power, 2016-01-27 18:29:50]

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

[erikchen, 2016-01-27 18:40:39]

still lgtm

[commit-bot: I haz the power, 2016-01-27 19:00:31]

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

[commit-bot: I haz the power, 2016-01-27 19:00:32]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-01-27 20:24:24]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-01-27 20:26:28]

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

[commit-bot: I haz the power, 2016-01-27 20:37:41]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-01-27 20:38:57]

Description was changed from

==========
remoting: Add AttachmentBroker for daemon and desktop processes

Daemon and desktop processes didn't have attachment brokers, and so the
desktop process was failing when trying to pass shared buffer to the
network process.

BUG=581174
==========

to

==========
remoting: Add AttachmentBroker for daemon and desktop processes

Daemon and desktop processes didn't have attachment brokers, and so the
desktop process was failing when trying to pass shared buffer to the
network process.

BUG=581174

Committed: https://crrev.com/77a444dc8136e3efb8542d6965b3e8f9439c5266
Cr-Commit-Position: refs/heads/master@{#371858}
==========

[commit-bot: I haz the power, 2016-01-27 20:38:58]

Patchset 3 (id:??) landed as
https://crrev.com/77a444dc8136e3efb8542d6965b3e8f9439c5266
Cr-Commit-Position: refs/heads/master@{#371858}