The Chromoting service should not start automatically unless it was configured from the webapp to d…

The Chromoting service should not start automatically unless it was configured from the webapp to do so. This way the service will not try to load unconfigured host after reboot. The installer should also respect this preserving the service start type when upgrading the installation.

The service should also stop automatically if a configuration error was detected. The UI will restart the service once configuration is recreated.


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

[alexeypa (please no reviews), 2012-04-10 20:27:27]

Please take a look.

[Wez, 2012-04-10 21:26:20]

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
File remoting/host/elevated_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
remoting/host/elevated_controller_win.cc:232: SERVICE_WIN32_OWN_PROCESS,
nit: Use SERVICE_NO_CHANGE here, so the installer controls the type.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
remoting/host/elevated_controller_win.cc:272: SERVICE_WIN32_OWN_PROCESS,
nit: Same here.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
File remoting/host/host_service_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:121: // Stop the service if there is no more
observers.
typo: is -> are

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:122: if
(!console_observers_.might_have_observers()) {
might_have_observers is reliable because our observers share the same thread,
right? Perhaps clarify that here?

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:373: io_thread.message_loop_proxy());
Are these related to this CL?  If so then mention them in the description.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
File remoting/host/installer/chromoting.wxs (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
remoting/host/installer/chromoting.wxs:291: set to 'manual'. This becomes a
problem when upgrading an exitsing
typo: existing

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
remoting/host/installer/chromoting.wxs:295: before any changes to the system
were made. After the installation is
nit: This wording could be clearer; we check the state before we make changes,
and restore the startup state after making changes, it looks like?

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:51: // The process exit codes.
nit: This comment doesn't really add anything.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:335: base::Unretained(this),
object));
How to do you ensure there are no tasks posted to this object when it's deleted?

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:345: // Stop trying to restart
the host process exited due misconfiguration.
typo: missing "if it", I think.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:356:
monitor_->RemoveWtsConsoleObserver(this);
nit: Mention that once all observers are gone the service will stop itself.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.h:45: // be launched in the
console session.
nit: Update this comment.

[alexeypa (please no reviews), 2012-04-10 22:18:16]

PTAL.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
File remoting/host/elevated_controller_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
remoting/host/elevated_controller_win.cc:232: SERVICE_WIN32_OWN_PROCESS,
On 2012/04/10 21:26:20, Wez wrote:
> nit: Use SERVICE_NO_CHANGE here, so the installer controls the type.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/elevated...
remoting/host/elevated_controller_win.cc:272: SERVICE_WIN32_OWN_PROCESS,
On 2012/04/10 21:26:20, Wez wrote:
> nit: Same here.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
File remoting/host/host_service_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:121: // Stop the service if there is no more
observers.
On 2012/04/10 21:26:20, Wez wrote:
> typo: is -> are

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:122: if
(!console_observers_.might_have_observers()) {
On 2012/04/10 21:26:20, Wez wrote:
> might_have_observers is reliable because our observers share the same thread,
> right? Perhaps clarify that here?

I added a DCHECK.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/host_ser...
remoting/host/host_service_win.cc:373: io_thread.message_loop_proxy());
On 2012/04/10 21:26:20, Wez wrote:
> Are these related to this CL?  If so then mention them in the description.

It is already mentioned. I have to pass message_loop_->message_loop_proxy() to
make the change work. Otherwise AddWtsConsoleObserver/RemoveWtsConsoleObserver
are not synchronized.

Once message_loop_->message_loop_proxy() is passed, not passing io_thread is a
matter of style.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
File remoting/host/installer/chromoting.wxs (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
remoting/host/installer/chromoting.wxs:291: set to 'manual'. This becomes a
problem when upgrading an exitsing
On 2012/04/10 21:26:20, Wez wrote:
> typo: existing

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/installe...
remoting/host/installer/chromoting.wxs:295: before any changes to the system
were made. After the installation is
On 2012/04/10 21:26:20, Wez wrote:
> nit: This wording could be clearer; we check the state before we make changes,
> and restore the startup state after making changes, it looks like?

Well, the wording above is incorrect.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/remoting...
File remoting/host/remoting_me2me_host.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/remoting...
remoting/host/remoting_me2me_host.cc:51: // The process exit codes.
On 2012/04/10 21:26:20, Wez wrote:
> nit: This comment doesn't really add anything.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:335: base::Unretained(this),
object));
On 2012/04/10 21:26:20, Wez wrote:
> How to do you ensure there are no tasks posted to this object when it's
deleted?

Good question. The shutdown sequence is indeed screwed up. 

I added WtsConsoleMonitor::RequestRemoveWtsConsoleObserver that removes an
observer making sure that OnSessionDetach will be called. This will take care of
|process_watcher_|.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:345: // Stop trying to restart
the host process exited due misconfiguration.
On 2012/04/10 21:26:20, Wez wrote:
> typo: missing "if it", I think.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.cc:356:
monitor_->RemoveWtsConsoleObserver(this);
On 2012/04/10 21:26:20, Wez wrote:
> nit: Mention that once all observers are gone the service will stop itself.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/1/remoting/host/wts_sess...
remoting/host/wts_session_process_launcher_win.h:45: // be launched in the
console session.
On 2012/04/10 21:26:20, Wez wrote:
> nit: Update this comment.

Done.

[alexeypa (please no reviews), 2012-04-10 22:18:20]

PTAL.

[Wez, 2012-04-11 01:01:23]

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_c...
File remoting/host/wts_console_monitor_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_c...
remoting/host/wts_console_monitor_win.h:27: // Requests removal of a previously
registered observer.
What's the difference between unregistering and requesting removal?

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:341: // safe to ignore this
notiifcation.
typo: notiification

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:351: // Stop trying to restart
the host if its process exited due misconfiguration.
typo: ... due to misconfiguration.

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:45: // |monitor| should happen
on the same tthread as |main_message_loop| is.
typo: tthread

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:45: // |monitor| should happen
on the same tthread as |main_message_loop| is.
reword: "All interaction ... should happen on |main_message_loop|."

[alexeypa (please no reviews), 2012-04-11 17:39:39]

PTAL.

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_c...
File remoting/host/wts_console_monitor_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_c...
remoting/host/wts_console_monitor_win.h:27: // Requests removal of a previously
registered observer.
On 2012/04/11 01:01:23, Wez wrote:
> What's the difference between unregistering and requesting removal?

I've reworded the comment.

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.cc (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:341: // safe to ignore this
notiifcation.
On 2012/04/11 01:01:23, Wez wrote:
> typo: notiification

Done.

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.cc:351: // Stop trying to restart
the host if its process exited due misconfiguration.
On 2012/04/11 01:01:23, Wez wrote:
> typo: ... due to misconfiguration.

Done.

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
File remoting/host/wts_session_process_launcher_win.h (right):

https://chromiumcodereview.appspot.com/10048003/diff/2002/remoting/host/wts_s...
remoting/host/wts_session_process_launcher_win.h:45: // |monitor| should happen
on the same tthread as |main_message_loop| is.
On 2012/04/11 01:01:23, Wez wrote:
> reword: "All interaction ... should happen on |main_message_loop|."

Done.

[alexeypa (please no reviews), 2012-04-11 21:22:35]

Ping. This has to be in before Friday. The sooner the better actually.

[alexeypa (please no reviews), 2012-04-11 22:34:57]

On 2012/04/11 21:22:35, alexeypa wrote:
> Ping. This has to be in before Friday. The sooner the better actually.

Check the last patch set out.

[Wez, 2012-04-11 22:46:30]

LGTM!

[commit-bot: I haz the power, 2012-04-11 23:04:39]

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

[Wez, 2012-04-11 23:09:43]

On 2012/04/11 22:46:30, Wez wrote:
> LGTM!

Thanks for making those changes, BTW. :)

[commit-bot: I haz the power, 2012-04-11 23:28:11]

Try job failure for 10048003-13005 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-04-11 23:31:06]

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

[commit-bot: I haz the power, 2012-04-12 03:34:44]

Try job failure for 10048003-13005 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-12 08:27:15]

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

[commit-bot: I haz the power, 2012-04-12 11:32:00]

Try job failure for 10048003-13005 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-12 13:48:07]

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

[commit-bot: I haz the power, 2012-04-12 16:30:36]

Change committed as 131997