Cleanup state transitions for Host plugin.

BUG=94105
TEST=See repro steps in the bug.

Review URL: http://codereview.chromium.org/7857011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@100489 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 154 insertions, 51 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index 2d578446..86ba1c0 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -617,6 +617,10 @@ void ChromotingHost::ShutdownFinish() {
     state_ = kStopped;
   }
 
+  // Keep reference to |this|, so that we don't get destroyed while
+  // sending notifications.
+  scoped_refptr<ChromotingHost> self(this);
+
   // Notify observers.
   for (StatusObserverList::iterator it = status_observers_.begin();
        it != status_observers_.end(); ++it) {
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index c017e62..2d5dd5b 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -32,10 +32,11 @@ namespace remoting {
 //
 // state: {
 //     DISCONNECTED,
+//     STARTING,
 //     REQUESTED_ACCESS_CODE,
 //     RECEIVED_ACCESS_CODE,
 //     CONNECTED,
-//     AFFIRMING_CONNECTION,
+//     DISCONNECTING,
 //     ERROR,
 // }
 //
@@ -62,10 +63,11 @@ const char* kFuncNameLocalize = "localize";
 
 // States.
 const char* kAttrNameDisconnected = "DISCONNECTED";
+const char* kAttrNameStarting = "STARTING";
 const char* kAttrNameRequestedAccessCode = "REQUESTED_ACCESS_CODE";
 const char* kAttrNameReceivedAccessCode = "RECEIVED_ACCESS_CODE";
 const char* kAttrNameConnected = "CONNECTED";
-const char* kAttrNameAffirmingConnection = "AFFIRMING_CONNECTION";
+const char* kAttrNameDisconnecting = "DISCONNECTING";
 const char* kAttrNameError = "ERROR";
 
 const int kMaxLoginAttempts = 5;
@@ -95,7 +97,9 @@ HostNPScriptObject::~HostNPScriptObject() {
 
   // Shutdown DesktopEnvironment first so that it doesn't try to post
   // tasks on the UI thread while we are stopping the host.
-  desktop_environment_->Shutdown();
+  if (desktop_environment_.get()) {
+    desktop_environment_->Shutdown();
+  }
 
   HostLogHandler::UnregisterLoggingScriptObject(this);
 
@@ -178,10 +182,11 @@ bool HostNPScriptObject::HasProperty(const std::string& property_name) {
           property_name == kAttrNameLogDebugInfo ||
           property_name == kAttrNameOnStateChanged ||
           property_name == kAttrNameDisconnected ||
+          property_name == kAttrNameStarting ||
           property_name == kAttrNameRequestedAccessCode ||
           property_name == kAttrNameReceivedAccessCode ||
           property_name == kAttrNameConnected ||
-          property_name == kAttrNameAffirmingConnection ||
+          property_name == kAttrNameDisconnecting ||
           property_name == kAttrNameError);
 }
 
@@ -204,9 +209,11 @@ bool HostNPScriptObject::GetProperty(const std::string& property_name,
     INT32_TO_NPVARIANT(state_, *result);
     return true;
   } else if (property_name == kAttrNameAccessCode) {
+    base::AutoLock auto_lock(access_code_lock_);
     *result = NPVariantFromString(access_code_);
     return true;
   } else if (property_name == kAttrNameAccessCodeLifetime) {
+    base::AutoLock auto_lock(access_code_lock_);
     INT32_TO_NPVARIANT(access_code_lifetime_.InSeconds(), *result);
     return true;
   } else if (property_name == kAttrNameClient) {
@@ -215,6 +222,9 @@ bool HostNPScriptObject::GetProperty(const std::string& property_name,
   } else if (property_name == kAttrNameDisconnected) {
     INT32_TO_NPVARIANT(kDisconnected, *result);
     return true;
+  } else if (property_name == kAttrNameStarting) {
+    INT32_TO_NPVARIANT(kStarting, *result);
+    return true;
   } else if (property_name == kAttrNameRequestedAccessCode) {
     INT32_TO_NPVARIANT(kRequestedAccessCode, *result);
     return true;
@@ -224,8 +234,8 @@ bool HostNPScriptObject::GetProperty(const std::string& property_name,
   } else if (property_name == kAttrNameConnected) {
     INT32_TO_NPVARIANT(kConnected, *result);
     return true;
-  } else if (property_name == kAttrNameAffirmingConnection) {
-    INT32_TO_NPVARIANT(kAffirmingConnection, *result);
+  } else if (property_name == kAttrNameDisconnecting) {
+    INT32_TO_NPVARIANT(kDisconnecting, *result);
     return true;
   } else if (property_name == kAttrNameError) {
     INT32_TO_NPVARIANT(kError, *result);
@@ -285,10 +295,11 @@ bool HostNPScriptObject::Enumerate(std::vector<std::string>* values) {
     kFuncNameDisconnect,
     kFuncNameLocalize,
     kAttrNameDisconnected,
+    kAttrNameStarting,
     kAttrNameRequestedAccessCode,
     kAttrNameReceivedAccessCode,
     kAttrNameConnected,
-    kAttrNameAffirmingConnection,
+    kAttrNameDisconnecting,
     kAttrNameError
   };
   for (size_t i = 0; i < arraysize(entries); ++i) {
@@ -314,25 +325,38 @@ void HostNPScriptObject::OnAccessDenied() {
 
 void HostNPScriptObject::OnClientAuthenticated(
     remoting::protocol::ConnectionToClient* client) {
-  DCHECK_NE(base::PlatformThread::CurrentId(), np_thread_id_);
+  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+
+  if (state_ == kDisconnecting) {
+    // Ignore the new connection if we are disconnecting.
+    return;
+  }
+
   client_username_ = client->session()->jid();
   size_t pos = client_username_.find('/');
   if (pos != std::string::npos)
     client_username_.replace(pos, std::string::npos, "");
   LOG(INFO) << "Client " << client_username_ << " connected.";
-  OnStateChanged(kConnected);
+  SetState(kConnected);
 }
 
 void HostNPScriptObject::OnClientDisconnected(
     remoting::protocol::ConnectionToClient* client) {
+  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+
   client_username_.clear();
-  OnStateChanged(kDisconnected);
+
+  // Disconnect the host when a client disconnects.
+  DisconnectInternal();
 }
 
 void HostNPScriptObject::OnShutdown() {
   DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
 
-  OnStateChanged(kDisconnected);
+  host_ = NULL;
+  if (state_ != kDisconnected) {
+    SetState(kDisconnected);
+  }
 }
 
 // string uid, string auth_token
@@ -348,6 +372,11 @@ bool HostNPScriptObject::Connect(const NPVariant* args,
     return false;
   }
 
+  if (state_ != kDisconnected) {
+    SetException("connect: can be called only when disconnected");
+    return false;
+  }
+
   std::string uid = StringFromNPVariant(args[0]);
   if (uid.empty()) {
     SetException("connect: bad uid argument");
@@ -380,6 +409,8 @@ void HostNPScriptObject::ReadPolicyAndConnect(const std::string& uid,
     return;
   }
 
+  SetState(kStarting);
+
   // Only proceed to FinishConnect() if at least one policy update has been
   // received.
   if (policy_received_) {
@@ -396,13 +427,13 @@ void HostNPScriptObject::FinishConnect(
     const std::string& uid,
     const std::string& auth_token,
     const std::string& auth_service) {
-  if (MessageLoop::current() != host_context_.main_message_loop()) {
-    host_context_.main_message_loop()->PostTask(
-        FROM_HERE, base::Bind(
-            &HostNPScriptObject::FinishConnect, base::Unretained(this),
-            uid, auth_token, auth_service));
+  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+
+  if (state_ != kStarting) {
+    // Host has been stopped while we were fetching policy.
     return;
   }
+
   // Store the supplied user ID and token to the Host configuration.
   scoped_refptr<MutableHostConfig> host_config = new InMemoryHostConfig();
   host_config->SetString(kXmppLoginConfigPath, uid);
@@ -427,14 +458,14 @@ void HostNPScriptObject::FinishConnect(
           base::Bind(&HostNPScriptObject::OnReceivedSupportID,
                      base::Unretained(this),
                      access_verifier.get()))) {
-    OnStateChanged(kError);
+    SetState(kError);
     return;
   }
 
   // Create DesktopEnvironment.
   desktop_environment_.reset(DesktopEnvironment::Create(&host_context_));
   if (desktop_environment_.get() == NULL) {
-    OnStateChanged(kError);
+    SetState(kError);
     return;
   }
 
@@ -459,7 +490,7 @@ void HostNPScriptObject::FinishConnect(
   // Start the Host.
   host_->Start();
 
-  OnStateChanged(kRequestedAccessCode);
+  SetState(kRequestedAccessCode);
   return;
 }
 
@@ -504,21 +535,31 @@ void HostNPScriptObject::DisconnectInternal() {
     return;
   }
 
-  if (!host_) {
-    disconnected_event_.Signal();
-    return;
-  }
+  switch (state_) {
+    case kDisconnected:
+      disconnected_event_.Signal();
+      return;
+
+    case kStarting:
+      SetState(kDisconnecting);
+      SetState(kDisconnected);
+      disconnected_event_.Signal();
+      return;
 
-  host_->Shutdown(
-      NewRunnableMethod(this, &HostNPScriptObject::OnShutdownFinished));
+    case kDisconnecting:
+      return;
+
+    default:
+      DCHECK(host_);
+      SetState(kDisconnecting);
+      host_->Shutdown(
+          NewRunnableMethod(this, &HostNPScriptObject::OnShutdownFinished));
+  }
 }
 
 void HostNPScriptObject::OnShutdownFinished() {
   DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
 
-  host_ = NULL;
-  register_request_.reset();
-  host_config_ = NULL;
   disconnected_event_.Signal();
 }
 
@@ -553,41 +594,85 @@ void HostNPScriptObject::OnReceivedSupportID(
     bool success,
     const std::string& support_id,
     const base::TimeDelta& lifetime) {
-  CHECK_NE(base::PlatformThread::CurrentId(), np_thread_id_);
+  DCHECK(host_context_.network_message_loop()->BelongsToCurrentThread());
 
   if (!success) {
-    // TODO(wez): Replace the success/fail flag with full error reporting.
-    OnStateChanged(kError);
+    host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+        &HostNPScriptObject::NotifyAccessCode, base::Unretained(this), false));
     DisconnectInternal();
     return;
   }
 
-  // Inform the AccessVerifier of our Support-Id, for authentication.
   access_verifier->OnIT2MeHostRegistered(success, support_id);
+  std::string access_code = support_id + access_verifier->host_secret();
+  host_->set_access_code(access_code);
 
-  // Combine the Support Id with the Host Id to make the Access Code.
-  // TODO(wez): Locking, anyone?
-  access_code_ = support_id + access_verifier->host_secret();
-  access_code_lifetime_ = lifetime;
+  {
+    base::AutoLock lock(access_code_lock_);
+    access_code_ = access_code;
+    access_code_lifetime_ = lifetime;
+  }
 
-  // Tell the ChromotingHost the access code, to use as shared-secret.
-  host_->set_access_code(access_code_);
+  host_context_.main_message_loop()->PostTask(FROM_HERE, base::Bind(
+      &HostNPScriptObject::NotifyAccessCode, base::Unretained(this), true));
+}
 
-  // Let the caller know that life is good.
-  OnStateChanged(kReceivedAccessCode);
+void HostNPScriptObject::NotifyAccessCode(bool success) {
+  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  if (state_ == kRequestedAccessCode) {
+    SetState(success ? kReceivedAccessCode : kError);
+  }
 }
 
-void HostNPScriptObject::OnStateChanged(State state) {
+void HostNPScriptObject::SetState(State state) {
+  DCHECK_EQ(MessageLoop::current(), host_context_.main_message_loop());
+  switch (state_) {
+    case kDisconnected:
+      DCHECK(state == kStarting ||
+             state == kError) << state;
+      break;
+    case kStarting:
+      DCHECK(state == kRequestedAccessCode ||
+             state == kDisconnecting ||
+             state == kError) << state;
+      break;
+    case kRequestedAccessCode:
+      DCHECK(state == kReceivedAccessCode ||
+             state == kDisconnecting ||
+             state == kError) << state;
+      break;
+    case kReceivedAccessCode:
+      DCHECK(state == kConnected ||
+             state == kDisconnecting ||
+             state == kError) << state;
+      break;
+    case kConnected:
+      DCHECK(state == kDisconnecting ||
+             state == kDisconnected ||
+             state == kError) << state;
+      break;
+    case kDisconnecting:
+      DCHECK(state == kDisconnected) << state;
+      break;
+    case kError:
+      DCHECK(state == kDisconnecting) << state;
+      break;
+  };
+  state_ = state;
+  NotifyStateChanged(state);
+}
+
+void HostNPScriptObject::NotifyStateChanged(State state) {
   if (!plugin_message_loop_proxy_->BelongsToCurrentThread()) {
     plugin_message_loop_proxy_->PostTask(
-        FROM_HERE, base::Bind(&HostNPScriptObject::OnStateChanged,
+        FROM_HERE, base::Bind(&HostNPScriptObject::NotifyStateChanged,
                               base::Unretained(this), state));
     return;
   }
-  state_ = state;
   if (on_state_changed_func_.get()) {
     VLOG(2) << "Calling state changed " << state;
-    bool is_good = InvokeAndIgnoreResult(on_state_changed_func_.get(), NULL, 0);
+    bool is_good = InvokeAndIgnoreResult(on_state_changed_func_.get(),
+                                         NULL, 0);
     LOG_IF(ERROR, !is_good) << "OnStateChanged failed";
   }
 }
diff --git a/remoting/host/plugin/host_script_object.h b/remoting/host/plugin/host_script_object.h
index 91688d3..c04a373 100644
--- a/remoting/host/plugin/host_script_object.h
+++ b/remoting/host/plugin/host_script_object.h
@@ -88,10 +88,11 @@ class HostNPScriptObject : public HostStatusObserver {
  private:
   enum State {
     kDisconnected,
+    kStarting,
     kRequestedAccessCode,
     kReceivedAccessCode,
     kConnected,
-    kAffirmingConnection,
+    kDisconnecting,
     kError
   };
 
@@ -109,8 +110,11 @@ class HostNPScriptObject : public HostStatusObserver {
   // No result.
   bool Localize(const NPVariant* args, uint32_t argCount, NPVariant* result);
 
-  // Call OnStateChanged handler if there is one.
-  void OnStateChanged(State state);
+  // Updates state of the host. Can be called only on the main thread.
+  void SetState(State state);
+
+  // Notifies OnStateChanged handler of a state change.
+  void NotifyStateChanged(State state);
 
   // Call LogDebugInfo handler if there is one.
   // This must be called on the correct thread.
@@ -121,6 +125,7 @@ class HostNPScriptObject : public HostStatusObserver {
                            bool success,
                            const std::string& support_id,
                            const base::TimeDelta& lifetime);
+  void NotifyAccessCode(bool success);
 
   // Helper functions that run on main thread. Can be called on any
   // other thread.
@@ -158,10 +163,13 @@ class HostNPScriptObject : public HostStatusObserver {
 
   NPP plugin_;
   NPObject* parent_;
-  int state_;
+  State state_;
+
+  base::Lock access_code_lock_;
   std::string access_code_;
-  std::string client_username_;
   base::TimeDelta access_code_lifetime_;
+
+  std::string client_username_;
   ScopedRefNPObject log_debug_info_func_;
   ScopedRefNPObject on_state_changed_func_;
   base::PlatformThreadId np_thread_id_;
diff --git a/remoting/webapp/me2mom/host_plugin_proto.js b/remoting/webapp/me2mom/host_plugin_proto.js
index 8b3a72a..59addc6 100644
--- a/remoting/webapp/me2mom/host_plugin_proto.js
+++ b/remoting/webapp/me2mom/host_plugin_proto.js
@@ -27,11 +27,12 @@ remoting.HostPlugin.prototype.localize = function(callback) {};
 
 /** @type {number} */ remoting.HostPlugin.prototype.state;
 
+/** @type {number} */ remoting.HostPlugin.prototype.STARTING;
 /** @type {number} */ remoting.HostPlugin.prototype.REQUESTED_ACCESS_CODE;
 /** @type {number} */ remoting.HostPlugin.prototype.RECEIVED_ACCESS_CODE;
 /** @type {number} */ remoting.HostPlugin.prototype.CONNECTED;
 /** @type {number} */ remoting.HostPlugin.prototype.DISCONNECTED;
-/** @type {number} */ remoting.HostPlugin.prototype.AFFIRMING_CONNECTION;
+/** @type {number} */ remoting.HostPlugin.prototype.DISCONNECTING;
 /** @type {number} */ remoting.HostPlugin.prototype.ERROR;
 
 /** @type {number} */ remoting.HostPlugin.prototype.accessCodeLifetime;
diff --git a/remoting/webapp/me2mom/remoting.js b/remoting/webapp/me2mom/remoting.js
index c7bea98..dde8cc7 100644
--- a/remoting/webapp/me2mom/remoting.js
+++ b/remoting/webapp/me2mom/remoting.js
@@ -304,7 +304,10 @@ function onStateChanged_() {
   var plugin = /** @type {remoting.HostPlugin} */
       document.getElementById(remoting.HOST_PLUGIN_ID);
   var state = plugin.state;
-  if (state == plugin.REQUESTED_ACCESS_CODE) {
+  if (state == plugin.STARTING) {
+    // Nothing to do here.
+    remoting.debug.log('Host plugin state: STARTING');
+  } else if (state == plugin.REQUESTED_ACCESS_CODE) {
     // Nothing to do here.
     remoting.debug.log('Host plugin state: REQUESTED_ACCESS_CODE');
   } else if (state == plugin.RECEIVED_ACCESS_CODE) {
@@ -340,6 +343,8 @@ function onStateChanged_() {
     l10n.localizeElement(element, client);
     remoting.setMode(remoting.AppMode.HOST_SHARED);
     disableTimeoutCountdown_();
+  } else if (state == plugin.DISCONNECTING) {
+    remoting.debug.log('Host plugin state: DISCONNECTING');
   } else if (state == plugin.DISCONNECTED) {
     remoting.debug.log('Host plugin state: DISCONNECTED');
     if (remoting.currentMode != remoting.AppMode.HOST_SHARE_FAILED) {