[Chromoting] Don't use a job object to control the me2me host process on Windows XP.

[Chromoting] Don't use a job object to control the me2me host process on Windows XP.

The host binary specifies uiAccess='true' in its manifest that requires ShellExecute() to be used to launch it. Since ShellExecute() does not work across the session boundary we use a helper process to launch the me2me host. A job object is used to control both the helper process and the host process, so that they can be terminated if needed.

Unfortunately, the helper process spawned by CreateRemoteSessionProcess() function cannot be assigned to a job object on Windows XP/2K3. This CL works it around by using two different kaunchers for XP and Vista+:

  - XP launches the host process directly using CreateRemoteSessionProcess(). uiAccess='true' is ignored on XP and ShellExecute() call is not necessary;
  - Vista+ used the helper process to launch the host and assigns boths processes to a job object.

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

[alexeypa (please no reviews), 2012-09-12 21:48:51]

PTAL.

[Wez, 2012-09-14 23:36:18]

LGTM w/ nits.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
File remoting/host/win/wts_session_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:90: // Implements
|WorkerProcessLauncher::Delegate| that starts the host process
nit: host process -> specified process?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:92: // handle to control it.
nit: ... to monitor and close it. ?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:96: const FilePath& binary,
nit: binary_path

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:116: // A waiting handle that
becomes signalled once all process associated with
nit: Handle that becomes ... once the process has terminated.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:129: // in a different session
and uses job object to control it. This object
nit: You mention it uses [a] job object, and then start the next sentence with
"This object"; which I think you mean InJobLauncher rather than the job object?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:133: class InJobLauncher :
public WtsProcessLauncherImpl,
nit: ShellExecuteLauncher?

Then you can describe this as a Delegate that launches the specified process via
ShellExecute() and document the use of helper process and job objects in
comments on the class members/method implementations.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:159: // notifications has been
received.
typo: has -> have

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:163: void
DrainJobNotificationsCompleted();
Does this method trigger a notification, or is it called when a notification is
received? The name suggests the latter, in which case start the comment
"Notified when..."?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:169: // Notifies that the job
object initialization is complete.
nit: See above re "Notified when..."

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:172: void
OnJobNotification(DWORD message, DWORD pid);
nit: What's this for?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:176: // The main service
message loop.
Can you re-describe this in terms of what this class uses the loop for?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:192: base::win::ScopedHandle
worker_process_;
nit: Handle to the worker process, if launched?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:209: // Create the host
process command line passing the name of the IPC channel
nit: host process command line -> command line?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:210: // to use and copying
known switches from the service's command line.
nit: service's -> caller's?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:271: bool
SingleProcessLauncher::SetSessionToken(HANDLE session_token) {
nit: Rename this ResetUserToken() and have it take ownership of a supplied
ScopedHandle& to save duplicating Duplicate'ing code?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:300: // To receive job object
notifications it is registered with the completion
nit: What is "it"? You mean this object?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:644: impl_->Stop();
nit: Stop() doesn't seem to actually stop anything; rename it Cleanup(),
perhaps?

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
File remoting/host/win/wts_session_process_launcher.h (right):

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.h:34: class
WtsProcessLauncherImpl;
nit: WtsSessionProcessLauncherImpl

[alexeypa (please no reviews), 2012-09-18 16:58:06]

FYI

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
File remoting/host/win/wts_session_process_launcher.cc (right):

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:90: // Implements
|WorkerProcessLauncher::Delegate| that starts the host process
On 2012/09/14 23:36:18, Wez wrote:
> nit: host process -> specified process?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:92: // handle to control it.
On 2012/09/14 23:36:18, Wez wrote:
> nit: ... to monitor and close it. ?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:96: const FilePath& binary,
On 2012/09/14 23:36:18, Wez wrote:
> nit: binary_path

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:116: // A waiting handle that
becomes signalled once all process associated with
On 2012/09/14 23:36:18, Wez wrote:
> nit: Handle that becomes ... once the process has terminated.

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:129: // in a different session
and uses job object to control it. This object
On 2012/09/14 23:36:18, Wez wrote:
> nit: You mention it uses [a] job object, and then start the next sentence with
> "This object"; which I think you mean InJobLauncher rather than the job
object?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:133: class InJobLauncher :
public WtsProcessLauncherImpl,
On 2012/09/14 23:36:18, Wez wrote:
> nit: ShellExecuteLauncher?
> 
> Then you can describe this as a Delegate that launches the specified process
via
> ShellExecute() and document the use of helper process and job objects in
> comments on the class members/method implementations.

Thinking about what this class is actually doing, I called it
ElevatedProcessLauncher.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:159: // notifications has been
received.
On 2012/09/14 23:36:18, Wez wrote:
> typo: has -> have

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:163: void
DrainJobNotificationsCompleted();
On 2012/09/14 23:36:18, Wez wrote:
> Does this method trigger a notification, or is it called when a notification
is
> received? The name suggests the latter, in which case start the comment
> "Notified when..."?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:169: // Notifies that the job
object initialization is complete.
On 2012/09/14 23:36:18, Wez wrote:
> nit: See above re "Notified when..."

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:172: void
OnJobNotification(DWORD message, DWORD pid);
On 2012/09/14 23:36:18, Wez wrote:
> nit: What's this for?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:176: // The main service
message loop.
On 2012/09/14 23:36:18, Wez wrote:
> Can you re-describe this in terms of what this class uses the loop for?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:192: base::win::ScopedHandle
worker_process_;
On 2012/09/14 23:36:18, Wez wrote:
> nit: Handle to the worker process, if launched?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:209: // Create the host
process command line passing the name of the IPC channel
On 2012/09/14 23:36:18, Wez wrote:
> nit: host process command line -> command line?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:210: // to use and copying
known switches from the service's command line.
On 2012/09/14 23:36:18, Wez wrote:
> nit: service's -> caller's?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:271: bool
SingleProcessLauncher::SetSessionToken(HANDLE session_token) {
On 2012/09/14 23:36:18, Wez wrote:
> nit: Rename this ResetUserToken() and have it take ownership of a supplied
> ScopedHandle& to save duplicating Duplicate'ing code?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:300: // To receive job object
notifications it is registered with the completion
On 2012/09/14 23:36:18, Wez wrote:
> nit: What is "it"? You mean this object?

Done.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.cc:644: impl_->Stop();
On 2012/09/14 23:36:18, Wez wrote:
> nit: Stop() doesn't seem to actually stop anything; rename it Cleanup(),
> perhaps?

It does "stop" job object notification. I'd like to call it Stop for parity with
other asynchronous objects.

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
File remoting/host/win/wts_session_process_launcher.h (right):

https://chromiumcodereview.appspot.com/10911267/diff/5001/remoting/host/win/w...
remoting/host/win/wts_session_process_launcher.h:34: class
WtsProcessLauncherImpl;
On 2012/09/14 23:36:18, Wez wrote:
> nit: WtsSessionProcessLauncherImpl

Done.

[commit-bot: I haz the power, 2012-09-18 16:59:13]

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

[commit-bot: I haz the power, 2012-09-18 18:50:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-18 23:02:41]

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

[commit-bot: I haz the power, 2012-09-19 04:03:56]

Change committed as 157490