Made the ChromotingHost class not ref-counted.

Made the ChromotingHost class not ref-counted.

ChromotingHost becomes a non thread-safe class should should live on the network thread. The CL removes ChromotingHost::Shutdown() allowing ChromotingHost to be destroyed synchronously closing all existing connections. It also updates HostStatusObserver::OnShutdown() handler to not destroy the host since it is the host's owner responsibility now.

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

[alexeypa (please no reviews), 2013-04-02 19:03:08]

PTAL.

[Wez, 2013-04-02 19:58:33]

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:99: DCHECK(CalledOnValidThread());
Can we also DCHECK that Start() is only called from the Initial state?  Do we
still need the separate Stopped state?

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:117: DCHECK(CalledOnValidThread());
nit: Can we also DCHECK that Shutdown is never called from the Initial or
Stopped states?

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:139: if (state_ == kStopping) {
How can we ever end up not in the kStopping state at this point?  Even if
DisconnectSession() triggers another Shutdown() we'll ignore that via the
kStopping case.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.h:92: void Shutdown();
Rename this Stop() to match Start().

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.h:138: // Disconnects all active clients.
Clarify the semantics, e.g. might sub-components asynchronously shutdown in the
background?

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host_unittest.cc:90: task_runner_,   // Audio
nit: In codereview these indents look different from the ones in
SimulateClientConnection?

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host_unittest.cc:215: get_client(connection_index) =
client.get();
Ick.  Assigning to a getter doesn't seem right...

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:145: // Shuts down |host_| on the
network thread.
nit: ... and posts ShutdownOnUiThread() to shut down UI thread resources.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:261: ShutdownOnUiThread();
nit: Call ShutdownOnNetworkThread() here and let that post on to
ShutdownOnUiThread(), so that the post-to-UI-thread can go in
ShutdownOnNetworkThread for clarity.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:285: // ChromotingHost::Shutdown()
later.
nit: Move this comment into ShutdownOnNetworkThread, and do the
deferred-shutdown there, i.e. 

if (host_) {
  network->PostTask(Shutdown);
  network->DeleteSoon(host_.release());
}

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:422: if
(!host_context_->ui_task_runner()->BelongsToCurrentThread()) {
Rather than thread-hop here, can you move the post to ShutdownOnNetworkThread()?
 Shutdown should always be starting off from the network thread, so that
shouldn't add any extra thread-hop overhead.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:428: // Note that
ShutdownOnUiThread() may be called more than once.
Is this still true?

[alexeypa (please no reviews), 2013-04-08 23:28:17]

PTAL

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:99: DCHECK(CalledOnValidThread());
On 2013/04/02 19:58:33, Wez wrote:
> Can we also DCHECK that Start() is only called from the Initial state?  Do we
> still need the separate Stopped state?

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:117: DCHECK(CalledOnValidThread());
On 2013/04/02 19:58:33, Wez wrote:
> nit: Can we also DCHECK that Shutdown is never called from the Initial or
> Stopped states?

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.cc:139: if (state_ == kStopping) {
On 2013/04/02 19:58:33, Wez wrote:
> How can we ever end up not in the kStopping state at this point?  Even if
> DisconnectSession() triggers another Shutdown() we'll ignore that via the
> kStopping case.

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.h:92: void Shutdown();
On 2013/04/02 19:58:33, Wez wrote:
> Rename this Stop() to match Start().

I moved the logic to the destructor.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host.h:138: // Disconnects all active clients.
On 2013/04/02 19:58:33, Wez wrote:
> Clarify the semantics, e.g. might sub-components asynchronously shutdown in
the
> background?

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
File remoting/host/chromoting_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host_unittest.cc:90: task_runner_,   // Audio
On 2013/04/02 19:58:33, Wez wrote:
> nit: In codereview these indents look different from the ones in
> SimulateClientConnection?

They don't have to be the same. There is additional "));" here.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/chrom...
remoting/host/chromoting_host_unittest.cc:215: get_client(connection_index) =
client.get();
On 2013/04/02 19:58:33, Wez wrote:
> Ick.  Assigning to a getter doesn't seem right...

Let's fix it in a separate CL.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
File remoting/host/plugin/host_script_object.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:145: // Shuts down |host_| on the
network thread.
On 2013/04/02 19:58:33, Wez wrote:
> nit: ... and posts ShutdownOnUiThread() to shut down UI thread resources.

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:261: ShutdownOnUiThread();
On 2013/04/02 19:58:33, Wez wrote:
> nit: Call ShutdownOnNetworkThread() here and let that post on to
> ShutdownOnUiThread(), so that the post-to-UI-thread can go in
> ShutdownOnNetworkThread for clarity.

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:285: // ChromotingHost::Shutdown()
later.
On 2013/04/02 19:58:33, Wez wrote:
> nit: Move this comment into ShutdownOnNetworkThread, and do the
> deferred-shutdown there, i.e. 
> 
> if (host_) {
>   network->PostTask(Shutdown);
>   network->DeleteSoon(host_.release());
> }

It will not work because HostNPScriptObject::It2MeImpl should destroy
|signal_strategy_| only after the host has been shutdown. See
HostNPScriptObject::It2MeImpl::OnShutdown().

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:422: if
(!host_context_->ui_task_runner()->BelongsToCurrentThread()) {
On 2013/04/02 19:58:33, Wez wrote:
> Rather than thread-hop here, can you move the post to
ShutdownOnNetworkThread()?
>  Shutdown should always be starting off from the network thread, so that
> shouldn't add any extra thread-hop overhead.

Done.

https://chromiumcodereview.appspot.com/13466014/diff/2001/remoting/host/plugi...
remoting/host/plugin/host_script_object.cc:428: // Note that
ShutdownOnUiThread() may be called more than once.
On 2013/04/02 19:58:33, Wez wrote:
> Is this still true?

Done.

[Wez, 2013-04-09 01:08:20]

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.cc:102: // Destroy session manager.
nit: Why does this need destroying explicitly rather than leaving the
scoped_ptr<> to do it?

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.cc:106: // ensure we don't try to touch them after
running shutdown tasks
Is there any risk of that, if we're being deleted?  Should this comment read "to
ensure that we don't try to use them as a result of OnShutdown() activity"?

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.h:148: enum State {
No longer used? :D

[alexeypa (please no reviews), 2013-04-12 18:22:26]

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
File remoting/host/chromoting_host.cc (right):

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.cc:102: // Destroy session manager.
On 2013/04/09 01:08:21, Wez wrote:
> nit: Why does this need destroying explicitly rather than leaving the
> scoped_ptr<> to do it?

Done.

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.cc:106: // ensure we don't try to touch them after
running shutdown tasks
On 2013/04/09 01:08:21, Wez wrote:
> Is there any risk of that, if we're being deleted?  Should this comment read
"to
> ensure that we don't try to use them as a result of OnShutdown() activity"?

Done.

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/13466014/diff/9001/remoting/host/chrom...
remoting/host/chromoting_host.h:148: enum State {
On 2013/04/09 01:08:21, Wez wrote:
> No longer used? :D

Done.

[Wez, 2013-04-15 19:50:04]

LGTM but please update the CL description, since Shutdown() no longer exists.

https://chromiumcodereview.appspot.com/13466014/diff/16001/remoting/host/chro...
File remoting/host/chromoting_host.h (right):

https://chromiumcodereview.appspot.com/13466014/diff/16001/remoting/host/chro...
remoting/host/chromoting_host.h:134: // Disconnects all active clients
synchronously. Sub-components might shutdown
nit: synchronously implies blocking.  Suggest:

"Immediately disconnects all active clients.  Host-internal components may
shutdown asynchronously, but the caller is guaranteed not to receive callbacks
for disconnected clients after this call returns."

[alexeypa (please no reviews), 2013-04-15 20:04:42]

https://codereview.chromium.org/13466014/diff/16001/remoting/host/chromoting_...
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/13466014/diff/16001/remoting/host/chromoting_...
remoting/host/chromoting_host.h:134: // Disconnects all active clients
synchronously. Sub-components might shutdown
On 2013/04/15 19:50:04, Wez wrote:
> nit: synchronously implies blocking.  Suggest:
> 
> "Immediately disconnects all active clients.  Host-internal components may
> shutdown asynchronously, but the caller is guaranteed not to receive callbacks
> for disconnected clients after this call returns."

Done.

[commit-bot: I haz the power, 2013-04-24 21:19:48]

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

[commit-bot: I haz the power, 2013-04-24 21:39:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
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, 2013-04-24 21:53:17]

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

[commit-bot: I haz the power, 2013-04-25 01:05:23]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-04-25 03:36:33]

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

[commit-bot: I haz the power, 2013-04-25 07:19:42]

Change committed as 196343