[Chromoting] Refactored worker process launching code and speeded up the desktop process launch.

[Chromoting] Refactored worker process launching code and speeded up the desktop process launch.

This CL reduces time needed to start the desktop process in the user's session. The speed up is archieved by:
- Not using elevation on Win7 and below. Elevation is still required on Win8 in order to be able to inject Alt+Tab.
- If a launch attempt failed because of the job object not being ready, it is retried immediately after the job object is fully initialized.
- The initial delay when retrying a failed launch attempt reduced to 100ms.

Refactoring changes:
- Converted WorkerProcessLauncher classes into a single-threaded, synchronous class.
- Refactored and cleaned up the WorkerProcessLauncher::Delegate interface. The delegate can now launch the worker process asynchronously. Asynchronous events are reported by calling methods of WorkerProcessLauncher.
- WtsSessionProcessDelegate now uses GetNamedPipeClientProcessId() function to obtain the client's PID. The job object is still used to contain worker processes if there are more than one.
- WorkerProcessLauncher now limits time the worker process should start and connect back to 5 seconds.
- Handling of permanent exits codes was moved to WorkerProcessLauncher.
- WorkerProcessLauncher::Delegate now exposes the worker's handle instead of PID.

BUG=238821
TEST=remoting_unittests.WorkerProcessLauncherTest.*

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

[alexeypa (please no reviews), 2013-05-13 21:44:49]

PTAL.

[alexeypa (please no reviews), 2013-05-13 22:47:05]

Cris, could you please take a look at named pipes connection code. Specifically
at wts_session_process_delegate.cc,
WtsSessionProcessDelegate::Core::OnChannelConnected(). This is how it works.

The daemon process running under SYSTEM creates a server end of the pipe (look
for CreateIpcChannel() call in the same file). A single instance of the pipe is
created and FILE_FLAG_FIRST_PIPE_INSTANCE flag is passed. The security
descriptor on the pipe allows only SYSTEM to connect.

When the client process connects OnChannelConnected() is called. At this point
the daemon uses GetNamedPipeClientProcessId() to get PID of the client and
assumes the connected process is a trusted one.

We do it this way because we cannot launch the client directly. (It has to be
launched in a different session with elevation). I think it is safe, because we
protect from spoofing of the pipe's endpoint and only SYSTEM can connect to it.
WDYT?

[Wez, 2013-05-14 03:45:22]

Some drive-by comments...

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
File remoting/host/win/worker_process_launcher.cc (right):

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:72: worker_delegate_ = NULL;
nit: You could turn stopping_ into a private stopping() helper that just tests
whether worker_delegate_ is NULL, to avoid duplicating state.

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:166: void
WorkerProcessLauncher::OnObjectSignaled(HANDLE object) {
nit: Check that |object| is the |worker_process_|?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:200: void
WorkerProcessLauncher::RecordSuccessfulLaunch() {
This method records both successful and unsuccessful launches, so call it
RecordLaunchResult?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:204: LOG(WARNING) << "A worker
process didn't start in "
nit: "... failed to start within ..."

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:213: // for a few seconds.
nit: typos

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.cc:245: // Stop monitoring the worker
process termination.
nit: lose "termination".

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
File remoting/host/win/worker_process_launcher.h (right):

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:86: // Delegate::LaunchProcess().
The implementation has to make sure that this
nit: The WorkerProcessLauncher implementation, or the Delegate?  If the former
then rephrase as "WorkerProcessLauncher ensures that..."

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:92: // delivered once OnFatalError()
has been called.
Same here.

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:95: // Mirrors methods of
IPC::Listener.
nit: Clarify why these need to be mirrored here.

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:103: //
base::win::ObjectWatcher::Delegate implementation.
nit: "... used to watch for the worker process exiting."?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:113: // Used to emulate
|start_process_timer_| expiration.
nit: Clarify what this method actually does that's different from
RecordSuccessfulLaunch() alone - seems to only call RecordSuccessfulLaunch iff
there's a launch currently in-progress?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:127: WorkerProcessIpcDelegate*
worker_delegate_;
nit: ipc_handler_?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:129: DWORD exit_code_;
nit: Add a comment to explain why we need to store this.

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:138: // The default timeout for
|kill_process_timer_|.
nit: Do you really mean "default", or "initial"?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:141: // Launch backoff state.
nit: Suggest "State used to backoff worker launch attempts on failure."

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:147: // Monitors
|process_exit_event_| to detect when the launched process
nit: There is no |process_exit_event| - do you mean |worker_process_|?

https://codereview.chromium.org/15077010/diff/1/remoting/host/win/worker_proc...
remoting/host/win/worker_process_launcher.h:152:
base::OneShotTimer<WorkerProcessLauncher> start_process_timer_;
nit: This is confusingly named wrt |launch_timer_| - consider
|launch_timeout_timer_|?

[alexeypa (please no reviews), 2013-05-14 18:31:10]

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:72: worker_delegate_ = NULL;
On 2013/05/14 03:45:22, Wez wrote:
> nit: You could turn stopping_ into a private stopping() helper that just tests
> whether worker_delegate_ is NULL, to avoid duplicating state.

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:166: void
WorkerProcessLauncher::OnObjectSignaled(HANDLE object) {
On 2013/05/14 03:45:22, Wez wrote:
> nit: Check that |object| is the |worker_process_|?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:200: void
WorkerProcessLauncher::RecordSuccessfulLaunch() {
On 2013/05/14 03:45:22, Wez wrote:
> This method records both successful and unsuccessful launches, so call it
> RecordLaunchResult?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:204: LOG(WARNING) << "A worker
process didn't start in "
On 2013/05/14 03:45:22, Wez wrote:
> nit: "... failed to start within ..."

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:213: // for a few seconds.
On 2013/05/14 03:45:22, Wez wrote:
> nit: typos

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.cc:245: // Stop monitoring the worker
process termination.
On 2013/05/14 03:45:22, Wez wrote:
> nit: lose "termination".

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:86: // Delegate::LaunchProcess().
The implementation has to make sure that this
On 2013/05/14 03:45:22, Wez wrote:
> nit: The WorkerProcessLauncher implementation, or the Delegate?  If the former
> then rephrase as "WorkerProcessLauncher ensures that..."

It is the delegate. I updated the comment.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:92: // delivered once OnFatalError()
has been called.
On 2013/05/14 03:45:22, Wez wrote:
> Same here.

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:95: // Mirrors methods of
IPC::Listener.
On 2013/05/14 03:45:22, Wez wrote:
> nit: Clarify why these need to be mirrored here.

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:103: //
base::win::ObjectWatcher::Delegate implementation.
On 2013/05/14 03:45:22, Wez wrote:
> nit: "... used to watch for the worker process exiting."?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:113: // Used to emulate
|start_process_timer_| expiration.
On 2013/05/14 03:45:22, Wez wrote:
> nit: Clarify what this method actually does that's different from
> RecordSuccessfulLaunch() alone - seems to only call RecordSuccessfulLaunch iff
> there's a launch currently in-progress?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:127: WorkerProcessIpcDelegate*
worker_delegate_;
On 2013/05/14 03:45:22, Wez wrote:
> nit: ipc_handler_?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:129: DWORD exit_code_;
On 2013/05/14 03:45:22, Wez wrote:
> nit: Add a comment to explain why we need to store this.

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:138: // The default timeout for
|kill_process_timer_|.
On 2013/05/14 03:45:22, Wez wrote:
> nit: Do you really mean "default", or "initial"?

I really meant "default".

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:141: // Launch backoff state.
On 2013/05/14 03:45:22, Wez wrote:
> nit: Suggest "State used to backoff worker launch attempts on failure."

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:147: // Monitors
|process_exit_event_| to detect when the launched process
On 2013/05/14 03:45:22, Wez wrote:
> nit: There is no |process_exit_event| - do you mean |worker_process_|?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/1/remoting/host/win/work...
remoting/host/win/worker_process_launcher.h:152:
base::OneShotTimer<WorkerProcessLauncher> start_process_timer_;
On 2013/05/14 03:45:22, Wez wrote:
> nit: This is confusingly named wrt |launch_timer_| - consider
> |launch_timeout_timer_|?

Done.

[garykac, 2013-05-15 23:24:30]

lgtm

lgtm, although I can't comment on best practices for the Win API calls.

(I'm glad we took the time to go over this code architecture earlier).

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/desk...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/desk...
remoting/host/desktop_session_win.cc:523: // Get the name of the executable the
desktop process will run.
Can you expand this comment to explain why elevated = 'Desktop' and not-elevated
= 'Host'?

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher.cc (left):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher.cc:274: // Verify |peer_pid| because
it is controlled by the client and cannot be
Could you add a comment in the code mentioning why we no longer verify the
|peer_id|?

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher.cc:26: 100,
Wahoo! 900ms speedup! ^_^

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher.h:90: // The delegate must guarantee
that no other notifications is delivered once
s/is/are/

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher_unittest.cc:334: void
WorkerProcessLauncherTest::DoLaunchProcess() {
Is is reasonable to Mockout the Windows version checks to add tests that verify
each platform is doing what we expect it to do?

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/wts_console_session_process_driver.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_console_session_process_driver.cc:93: // Launch elevated
on Win8 to be able to inject Alt+Tab.
Is it worth sharing this code with desktop_session_win.cc?

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_session_process_delegate.cc:161: if
(base::win::GetVersion() < base::win::VERSION_VISTA)
The other |launch_elevated| code is set when version >= Win8. Just checking that
< VISTA is intended here.

Also, a comment on why some versions need to be elevated but not others would be
nice.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_session_process_delegate.cc:165: //
GetNamedPipeClientProcessId() is available starting from Vista.
This seems to answer part of my question above. But what do we use instead of
GetNamedPipeClientProcessId pre-vista?

[alexeypa (please no reviews), 2013-05-16 16:47:49]

Cris, ping. Could you take a look? See
https://chromiumcodereview.appspot.com/15077010/#msg2.

[alexeypa (please no reviews), 2013-05-16 17:49:57]

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/desk...
File remoting/host/desktop_session_win.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/desk...
remoting/host/desktop_session_win.cc:523: // Get the name of the executable the
desktop process will run.
On 2013/05/15 23:24:30, garykac wrote:
> Can you expand this comment to explain why elevated = 'Desktop' and
not-elevated
> = 'Host'?

Done.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher.cc (left):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher.cc:274: // Verify |peer_pid| because
it is controlled by the client and cannot be
On 2013/05/15 23:24:30, garykac wrote:
> Could you add a comment in the code mentioning why we no longer verify the
> |peer_id|?

Added this comment to the declaration of
WorkerProcessLauncher::OnChannelConnected().

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher.h (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher.h:90: // The delegate must guarantee
that no other notifications is delivered once
On 2013/05/15 23:24:30, garykac wrote:
> s/is/are/

Done.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/worker_process_launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/worker_process_launcher_unittest.cc:334: void
WorkerProcessLauncherTest::DoLaunchProcess() {
On 2013/05/15 23:24:30, garykac wrote:
> Is is reasonable to Mockout the Windows version checks to add tests that
verify
> each platform is doing what we expect it to do?

That should be possible. I'd postpone it until we start using
WorkerProcessLauncher on platforms other than Windows though.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/wts_console_session_process_driver.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_console_session_process_driver.cc:93: // Launch elevated
on Win8 to be able to inject Alt+Tab.
On 2013/05/15 23:24:30, garykac wrote:
> Is it worth sharing this code with desktop_session_win.cc?

WtsConsoleSessionProcessDriver is not used any more and going away soon.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
File remoting/host/win/wts_session_process_delegate.cc (right):

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_session_process_delegate.cc:161: if
(base::win::GetVersion() < base::win::VERSION_VISTA)
On 2013/05/15 23:24:30, garykac wrote:
> The other |launch_elevated| code is set when version >= Win8. Just checking
that
> < VISTA is intended here.

Yes, |< VISTA| is the right thing to do here.

> Also, a comment on why some versions need to be elevated but not others would
be
> nice.

Done.

https://chromiumcodereview.appspot.com/15077010/diff/12001/remoting/host/win/...
remoting/host/win/wts_session_process_delegate.cc:165: //
GetNamedPipeClientProcessId() is available starting from Vista.
On 2013/05/15 23:24:30, garykac wrote:
> This seems to answer part of my question above. But what do we use instead of
> GetNamedPipeClientProcessId pre-vista?

We launch the process directly in that case. So there is no need to check the
PID.

[alexeypa (please no reviews), 2013-05-17 16:49:36]

jschuh@ could you please take a look at the pipe connection logic (see the
quoted explanation below)?

On 2013/05/13 22:47:05, alexeypa wrote:
> Cris, could you please take a look at named pipes connection code.
Specifically
> at wts_session_process_delegate.cc,
> WtsSessionProcessDelegate::Core::OnChannelConnected(). This is how it works.
> 
> The daemon process running under SYSTEM creates a server end of the pipe (look
> for CreateIpcChannel() call in the same file). A single instance of the pipe
is
> created and FILE_FLAG_FIRST_PIPE_INSTANCE flag is passed. The security
> descriptor on the pipe allows only SYSTEM to connect.
> 
> When the client process connects OnChannelConnected() is called. At this point
> the daemon uses GetNamedPipeClientProcessId() to get PID of the client and
> assumes the connected process is a trusted one.
> 
> We do it this way because we cannot launch the client directly. (It has to be
> launched in a different session with elevation). I think it is safe, because
we
> protect from spoofing of the pipe's endpoint and only SYSTEM can connect to
it.
> WDYT?

[DO NOT USE (jschuh), 2013-05-17 17:39:27]

lgtm. We use GenerateVerifiedChannelID elsewhere, which includes a shared secret
exchanged in the hello message. I'm not sure if that adds anything here given
that the ACL should already provide a strong guarantee. But if you're so
inclined you could investigate if it's easy to use and worth doing.


On 2013/05/17 16:49:36, alexeypa wrote:
> jschuh@ could you please take a look at the pipe connection logic (see the
> quoted explanation below)?
> 
> On 2013/05/13 22:47:05, alexeypa wrote:
> > Cris, could you please take a look at named pipes connection code.
> Specifically
> > at wts_session_process_delegate.cc,
> > WtsSessionProcessDelegate::Core::OnChannelConnected(). This is how it works.
> > 
> > The daemon process running under SYSTEM creates a server end of the pipe
(look
> > for CreateIpcChannel() call in the same file). A single instance of the pipe
> is
> > created and FILE_FLAG_FIRST_PIPE_INSTANCE flag is passed. The security
> > descriptor on the pipe allows only SYSTEM to connect.
> > 
> > When the client process connects OnChannelConnected() is called. At this
point
> > the daemon uses GetNamedPipeClientProcessId() to get PID of the client and
> > assumes the connected process is a trusted one.
> > 
> > We do it this way because we cannot launch the client directly. (It has to
be
> > launched in a different session with elevation). I think it is safe, because
> we
> > protect from spoofing of the pipe's endpoint and only SYSTEM can connect to
> it.
> > WDYT?

[commit-bot: I haz the power, 2013-05-17 18:54:58]

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

[commit-bot: I haz the power, 2013-05-18 01:35:48]

Change committed as 200942