[Chromoting] The desktop process now creates a pre-connected pipe and passes (with some help of the…

[Chromoting] The desktop process now creates a pre-connected pipe and passes (with some help of the daemon) its client-end handle to the network process. The network process will request screen/audio capturing and input injection services using the created channel.

Accompanying changes:
- The daemon can request other processes to crash in response to critical protocol errors.
- Both network and desktop processes exit if the connection to the daemon has been disconnected.
- CreateConnectedIpcChannel() function moved to remoting/host/win/launch_process_with_token.h
- IPC::Channel::GenerateUniqueRandomChannelID() is used to generate a unique channel name.
- Added unit tests for DesktopProcess.
- Made 'remote_desktop' compilable on all platforms.

BUG=134694
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164006

[alexeypa (please no reviews), 2012-10-22 22:33:57]

PTAL.

[alexeypa (please no reviews), 2012-10-23 19:38:01]

I added unit tests and made 'remote_desktop' target compilable on all platforms.
This way the code gets more cross-platfom coverage. PTAL.

[Sergey Ulanov, 2012-10-24 20:12:01]

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process.h:71: virtual bool OnDesktopSessionAgentAttached(
Does this method need to be public? Maybe make it protected since we don't
expect it to be called from outside.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
File remoting/host/daemon_process_unittest.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process_unittest.cc:65:
MOCK_METHOD3(OnDesktopSessionAgentAttached, bool(
nit: move "bool(" to the next line.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
File remoting/host/daemon_process_win.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process_win.cc:105: bool
DaemonProcessWin::OnDesktopSessionAgentAttached(
Did you mean to implement this method in DaemonProcess class?
If not then you don't need posix specific code in here.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process.cc:56: DCHECK(MessageLoop::current()->type() ==
MessageLoop::TYPE_UI);
DCHECK_EQ

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process.h:39: // request.
Not sure I understand why we need this. Is it because daemon process can't send
crash reports? Please explain in the comment.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process_main.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process_main.cc:44: void usage(const FilePath&
program_name) {
nit: Usage()

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process_main.cc:46: LOG(INFO) <<
StringPrintf(kUsageMessage, program_name.value().c_str());
AsUTF8Unsafe() instead of value() and you won't need ifdef.
Or you can use LossyDisplayName().

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent.cc:58:
DCHECK(caller_task_runner_->BelongsToCurrentThread());
It might be better to use base::ThreadTaskRunnerHandle::Get() here to get
current task runner - that way you won't need two arguments.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent.h:18: }  // base
nit: namespace base

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent_posix.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:58: if (fcntl(pipe_fds[0], F_SETFL,
O_NONBLOCK) == -1 ||
Looks like this duplicates code from SocketPair() in
src/ipc/ipc_channel_posix.cc . Can we avoid duplicating it?

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:70: std::string socket_name =
base::StringPrintf("/var/tmp/DesktopSessionAgent%s",
Not sure why we need this. We pass IPC::ChannelProxy a file descriptor for the
socket anyway, so it will use this name only in logs, but will not be creating
named socket, right? In that case the name doesn't need to be an actual path,
and there is no need to make it unique. Something like "nameless _socket" should
work just fine.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:76: scoped_ptr<IPC::ChannelProxy>
server(new IPC::ChannelProxy(
nit: server_out->reset()?

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent_win.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_win.cc:19: const char
kSelfSecurityDescriptorFormat[] = "O:%1$sG:%1$sD:(A;;GA;;;%1$s)";
static or anonymous namespace. Or maybe move it inline in
DoCreateNetworkChannel() - it's used only in one place anyway.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_win.cc:56: // presubmit: allow wstring
remove this comment?

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.h:19: };
  // namespace base
and remove semicolon

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.h:24: };
same here.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/worker_pr...
File remoting/host/win/worker_process_launcher.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/worker_pr...
remoting/host/win/worker_process_launcher.cc:233: scoped_refptr<Core> self =
this;
How does that make any difference? This method returns after calling delegate
anyway.

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp#newco...
remoting/remoting.gyp:981: 'target_name': 'remoting_desktop',
Do we need to compile this to a separate binary for desktop process? I think we
should use to apply the same approach that chrome uses: one single binary is
shared between all chrome processes. This will allow to reduce downloadable
package size and also reduce memory consumption by the daemons.

[alexeypa (please no reviews), 2012-10-24 21:41:50]

FYI

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_process.h
File remoting/host/daemon_process.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process.h:71: virtual bool OnDesktopSessionAgentAttached(
On 2012/10/24 20:12:01, sergeyu wrote:
> Does this method need to be public? Maybe make it protected since we don't
> expect it to be called from outside.

It is called by DesktopSessionWin::OnDesktopSessionAgentAttached().

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
File remoting/host/daemon_process_unittest.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process_unittest.cc:65:
MOCK_METHOD3(OnDesktopSessionAgentAttached, bool(
On 2012/10/24 20:12:01, sergeyu wrote:
> nit: move "bool(" to the next line.

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
File remoting/host/daemon_process_win.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/daemon_proces...
remoting/host/daemon_process_win.cc:105: bool
DaemonProcessWin::OnDesktopSessionAgentAttached(
On 2012/10/24 20:12:01, sergeyu wrote:
> Did you mean to implement this method in DaemonProcess class?
> If not then you don't need posix specific code in here.

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process.cc:56: DCHECK(MessageLoop::current()->type() ==
MessageLoop::TYPE_UI);
On 2012/10/24 20:12:01, sergeyu wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process.h:39: // request.
On 2012/10/24 20:12:01, sergeyu wrote:
> Not sure I understand why we need this. Is it because daemon process can't
send
> crash reports? Please explain in the comment.

It is explained at the declaration of ChromotingDaemonDesktopMsg_Crash message.
I added a reference to the comment.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
File remoting/host/desktop_process_main.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process_main.cc:44: void usage(const FilePath&
program_name) {
On 2012/10/24 20:12:01, sergeyu wrote:
> nit: Usage()

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_proce...
remoting/host/desktop_process_main.cc:46: LOG(INFO) <<
StringPrintf(kUsageMessage, program_name.value().c_str());
On 2012/10/24 20:12:01, sergeyu wrote:
> AsUTF8Unsafe() instead of value() and you won't need ifdef.
> Or you can use LossyDisplayName().

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent.cc:58:
DCHECK(caller_task_runner_->BelongsToCurrentThread());
On 2012/10/24 20:12:01, sergeyu wrote:
> It might be better to use base::ThreadTaskRunnerHandle::Get() here to get
> current task runner - that way you won't need two arguments.

base::ThreadTaskRunnerHandle::Get() will give me a pointer to message_loop_proxy
exposed by the message loop. I, however, need AutoThreadTaskRunner that makes
sure that the message loop will continue run until all references to
AutoThreadTaskRunner are gone.

I updated a bunch of base::SingleThreadTaskRunner to be AutoThreadTaskRunners.
I'll gradually update others.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent.h:18: }  // base
On 2012/10/24 20:12:01, sergeyu wrote:
> nit: namespace base

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent_posix.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:58: if (fcntl(pipe_fds[0], F_SETFL,
O_NONBLOCK) == -1 ||
On 2012/10/24 20:12:01, sergeyu wrote:
> Looks like this duplicates code from SocketPair() in
> src/ipc/ipc_channel_posix.cc . Can we avoid duplicating it?

I'll follow up with a separate CL.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:70: std::string socket_name =
base::StringPrintf("/var/tmp/DesktopSessionAgent%s",
On 2012/10/24 20:12:01, sergeyu wrote:
> Not sure why we need this. We pass IPC::ChannelProxy a file descriptor for the
> socket anyway, so it will use this name only in logs, but will not be creating
> named socket, right? In that case the name doesn't need to be an actual path,
> and there is no need to make it unique. Something like "nameless _socket"
should
> work just fine.

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_posix.cc:76: scoped_ptr<IPC::ChannelProxy>
server(new IPC::ChannelProxy(
On 2012/10/24 20:12:01, sergeyu wrote:
> nit: server_out->reset()?

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
File remoting/host/desktop_session_agent_win.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_win.cc:19: const char
kSelfSecurityDescriptorFormat[] = "O:%1$sG:%1$sD:(A;;GA;;;%1$s)";
On 2012/10/24 20:12:01, sergeyu wrote:
> static or anonymous namespace.

It is a constant. It is limited to the file scope by default.

> Or maybe move it inline in DoCreateNetworkChannel() - it's used only
> in one place anyway.

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/desktop_sessi...
remoting/host/desktop_session_agent_win.cc:56: // presubmit: allow wstring
On 2012/10/24 20:12:01, sergeyu wrote:
> remove this comment?

It is necessary to suppress the presubmit warning. It basically says that
std::wstring below should be allowed because Windows only API requires it to be
used.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
File remoting/host/win/launch_process_with_token.h (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.h:19: };
On 2012/10/24 20:12:01, sergeyu wrote:
>   // namespace base
> and remove semicolon

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/launch_pr...
remoting/host/win/launch_process_with_token.h:24: };
On 2012/10/24 20:12:01, sergeyu wrote:
> same here.

Done.

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/worker_pr...
File remoting/host/win/worker_process_launcher.cc (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/host/win/worker_pr...
remoting/host/win/worker_process_launcher.cc:233: scoped_refptr<Core> self =
this;
On 2012/10/24 20:12:01, sergeyu wrote:
> How does that make any difference? This method returns after calling delegate
> anyway.

Yeah, this is probably not needed.

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp#newco...
remoting/remoting.gyp:981: 'target_name': 'remoting_desktop',
On 2012/10/24 20:12:01, sergeyu wrote:
> Do we need to compile this to a separate binary for desktop process?

Yes. It should have "level='requireAdministrator'" in the binary's manifest
which means that the binary cannot be launched using CreateProcess.

> I think we
> should use to apply the same approach that chrome uses: one single binary is
> shared between all chrome processes. This will allow to reduce downloadable
> package size and also reduce memory consumption by the daemons.

I agree that having a single binary is preferred to multiple different binaries.
Right now however I cannot really merge them. Once refactoring is done, I will
be able to merge daemon and host/network binaries. The desktop one will have to
be separate.

Another thing that we can do is to have a couple of launchers and a single DLL
loaded by the launchers. We still need two launchers because of the manifest
issue above.

[Sergey Ulanov, 2012-10-24 23:46:27]

lgtm

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp#newco...
remoting/remoting.gyp:981: 'target_name': 'remoting_desktop',
On 2012/10/24 21:41:51, alexeypa wrote:
> On 2012/10/24 20:12:01, sergeyu wrote:
> > Do we need to compile this to a separate binary for desktop process?
> 
> Yes. It should have "level='requireAdministrator'" in the binary's manifest
> which means that the binary cannot be launched using CreateProcess.
> 
> > I think we
> > should use to apply the same approach that chrome uses: one single binary is
> > shared between all chrome processes. This will allow to reduce downloadable
> > package size and also reduce memory consumption by the daemons.
> 
> I agree that having a single binary is preferred to multiple different
binaries.
> Right now however I cannot really merge them. Once refactoring is done, I will
> be able to merge daemon and host/network binaries. The desktop one will have
to
> be separate.

Ok, maybe add TODO somewhere for now?

> 
> Another thing that we can do is to have a couple of launchers and a single DLL
> loaded by the launchers. We still need two launchers because of the manifest
> issue above.

I see. Is it possible to have one small shim binary with that attribute in the
manifest that would just launch the main binary that doesn't have that
attribute?

[alexeypa (please no reviews), 2012-10-24 23:58:19]

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/11231060/diff/5006/remoting/remoting.gyp#newco...
remoting/remoting.gyp:981: 'target_name': 'remoting_desktop',
On 2012/10/24 23:46:28, sergeyu wrote:
> Ok, maybe add TODO somewhere for now?

http://crbug.com/157727

> I see. Is it possible to have one small shim binary with that attribute in the
> manifest that would just launch the main binary that doesn't have that
> attribute?

Maybe. I'm not sure how SendSAS() function checks the manifest. This will also
make us to spawn three processes in order to launch the desktop process in a
session.

[commit-bot: I haz the power, 2012-10-25 00:35:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/11231060/16007

[commit-bot: I haz the power, 2012-10-25 03:25:44]

Change committed as 164006