Added support of Secure Attention Sequence in multiprocess mode.

Added support of Secure Attention Sequence in multiprocess mode.

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

[alexeypa (please no reviews), 2012-12-06 00:40:50]

Cris, please take a look at remoting/host/chromoting_messages.h. The added
message is sent between two processes running under SYSTEM account in different
sessions.

Jamie, please review everything else.

[Cris Neckar, 2012-12-06 01:10:28]

ipc changes lgtm

[Jamie, 2012-12-06 21:42:24]

I'm not particularly familiar with this code, so I might not be the best choice
of reviewer, but here are some initial comments anyway.

Also, is any of this code unit-testable?

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent.h (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.h:81: // Stop() is called.
Would it be possible/sensible to use refcounting to avoid this restriction? It
feels like a likely source of future bugs.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent_posix.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent_posix.cc:34: virtual bool
CreateChannelForNetworkProcess(
Add "Overriden from..." comment?

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent_win.cc:113: }
Should there be an error log if there's no delegate?

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_win.cc:138: return;
This check seems unnecessary, since you're not using launcher in this function.

[alexeypa (please no reviews), 2012-12-06 21:56:01]

> Also, is any of this code unit-testable?

No, not really. It requires integration-style tests.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent.h (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent.h:81: // Stop() is called.
On 2012/12/06 21:42:25, Jamie wrote:
> Would it be possible/sensible to use refcounting to avoid this restriction? It
> feels like a likely source of future bugs.

Using Start()/Stop() methods with a requirement to retain some object alive is a
common case in our code. Let me see if we could use a weak pointer here.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent_posix.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent_posix.cc:34: virtual bool
CreateChannelForNetworkProcess(
On 2012/12/06 21:42:25, Jamie wrote:
> Add "Overriden from..." comment?

The only case of "Overriden from ..." comment in our code is in CurtainMode
code. Elsewhere we only comment interface implementations as far as I can see.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_agent_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_agent_win.cc:113: }
On 2012/12/06 21:42:25, Jamie wrote:
> Should there be an error log if there's no delegate?

Nope, this case happen when shutting down DesktopSessionAgent instance.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_win.cc:138: return;
On 2012/12/06 21:42:25, Jamie wrote:
> This check seems unnecessary, since you're not using launcher in this
function.

This check is required for the case when OnInjectSas arrived, but suddenly the
desktop process dies for any reason.

[Jamie, 2012-12-06 22:07:08]

lgtm once you've added a comment, and the WeakPtr idea sounds like it's worth
looking into as well.

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_win.cc:138: return;
On 2012/12/06 21:56:02, alexeypa wrote:
> On 2012/12/06 21:42:25, Jamie wrote:
> > This check seems unnecessary, since you're not using launcher in this
> function.
> 
> This check is required for the case when OnInjectSas arrived, but suddenly the
> desktop process dies for any reason.

Okay, maybe a comment would be useful to indicate what a NULL launcher_
indicates, and why it makes sense not to inject SAS in this case.

[alexeypa (please no reviews), 2012-12-06 22:38:51]

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/11447021/diff/1/remoting/host/desktop_...
remoting/host/desktop_session_win.cc:138: return;
On 2012/12/06 22:07:09, Jamie wrote:
> On 2012/12/06 21:56:02, alexeypa wrote:
> > On 2012/12/06 21:42:25, Jamie wrote:
> > > This check seems unnecessary, since you're not using launcher in this
> > function.
> > 
> > This check is required for the case when OnInjectSas arrived, but suddenly
the
> > desktop process dies for any reason.
> 
> Okay, maybe a comment would be useful to indicate what a NULL launcher_
> indicates, and why it makes sense not to inject SAS in this case.

Done.

[commit-bot: I haz the power, 2012-12-06 22:42:04]

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

[commit-bot: I haz the power, 2012-12-07 01:08:14]

Change committed as 171664