Add ChromotingHost::RemoveStatusObserver().

Now HostStatusObservers can be added or removed at any time.
ObserverList<> is used to store the list of observers. This allows deletion of status observers before host is destroyed. It was previously causing a crash described in http://crbug.com/110196 .

BUG=110196

Review URL: https://chromiumcodereview.appspot.com/9231002

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

12 files changed, 63 insertions, 45 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index f8afcd4..d3747d6a 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -118,8 +118,12 @@ void ChromotingHost::Shutdown(const base::Closure& shutdown_task) {
 
 void ChromotingHost::AddStatusObserver(HostStatusObserver* observer) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kInitial);
-  status_observers_.push_back(observer);
+  status_observers_.AddObserver(observer);
+}
+
+void ChromotingHost::RemoveStatusObserver(HostStatusObserver* observer) {
+  DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
+  status_observers_.RemoveObserver(observer);
 }
 
 void ChromotingHost::SetAuthenticatorFactory(
@@ -164,20 +168,16 @@ void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
 
   // Notify observers that there is at least one authenticated client.
   const std::string& jid = client->connection()->session()->jid();
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnClientAuthenticated(jid);
-  }
+  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
+                    OnClientAuthenticated(jid));
 }
 
 void ChromotingHost::OnSessionAuthenticationFailed(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
   // Notify observers.
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnAccessDenied();
-  }
+  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
+                    OnAccessDenied());
 }
 
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
@@ -193,10 +193,8 @@ void ChromotingHost::OnSessionClosed(ClientSession* client) {
     recorder_->RemoveConnection(client->connection());
   }
 
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnClientDisconnected(client->client_jid());
-  }
+  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
+                    OnClientDisconnected(client->client_jid()));
 
   if (recorder_.get()) {
     // Currently we don't allow more than one similtaneous connection,
@@ -365,10 +363,8 @@ void ChromotingHost::ShutdownFinish() {
   scoped_refptr<ChromotingHost> self(this);
 
   // Notify observers.
-  for (StatusObserverList::iterator it = status_observers_.begin();
-       it != status_observers_.end(); ++it) {
-    (*it)->OnShutdown();
-  }
+  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
+                    OnShutdown());
 
   for (std::vector<base::Closure>::iterator it = shutdown_tasks_.begin();
        it != shutdown_tasks_.end(); ++it) {
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index ff20fe8..09ca0a9 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
 #include "base/threading/thread.h"
 #include "remoting/base/encoder.h"
 #include "remoting/host/capturer.h"
@@ -83,11 +84,10 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   // called after shutdown is completed.
   void Shutdown(const base::Closure& shutdown_task);
 
-  // Adds |observer| to the list of status observers. Doesn't take
-  // ownership of |observer|, so |observer| must outlive this
-  // object. All status observers must be added before the host is
-  // started.
+  // Add/Remove |observer| to/from the list of status observers. Both
+  // methods can be called on the network thread only.
   void AddStatusObserver(HostStatusObserver* observer);
+  void RemoveStatusObserver(HostStatusObserver* observer);
 
   // Sets the authenticator factory to use for incoming
   // connections. Incoming connections are rejected until
@@ -138,7 +138,6 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   friend class base::RefCountedThreadSafe<ChromotingHost>;
   friend class ChromotingHostTest;
 
-  typedef std::vector<HostStatusObserver*> StatusObserverList;
   typedef std::vector<ClientSession*> ClientList;
 
   enum State {
@@ -188,8 +187,8 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   SignalStrategy* signal_strategy_;
   scoped_ptr<protocol::SessionManager> session_manager_;
 
-  // StatusObserverList is thread-safe and can be used on any thread.
-  StatusObserverList status_observers_;
+  // Must be used on the network thread only.
+  ObserverList<HostStatusObserver> status_observers_;
 
   // The connections to remote clients.
   ClientList clients_;
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 5ba4370..185be71 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -103,8 +103,7 @@ class ChromotingHostTest : public testing::Test {
     it2me_host_user_interface_.reset(new It2MeHostUserInterface(host_,
                                                                 &context_));
     it2me_host_user_interface_->InitFrom(disconnect_window_, continue_window_,
-                              local_input_monitor_);
-    host_->AddStatusObserver(it2me_host_user_interface_.get());
+                                         local_input_monitor_);
 
     session_ = new MockSession();
     session2_ = new MockSession();
diff --git a/remoting/host/host_event_logger.cc b/remoting/host/host_event_logger.cc
index 9284cff..c72b7df 100644
--- a/remoting/host/host_event_logger.cc
+++ b/remoting/host/host_event_logger.cc
@@ -6,7 +6,13 @@
 
 #if defined(OS_LINUX)
 #include <syslog.h>
-#endif
+// Following defines from syslog.h conflict with constants in
+// base/logging.h.
+#undef LOG_INFO
+#undef LOG_WARNING
+#endif  // defined(OS_LINUX)
+
+#include "remoting/host/chromoting_host.h"
 
 namespace {
 
@@ -21,13 +27,16 @@ void Log(const std::string& message) {
 
 namespace remoting {
 
-HostEventLogger::HostEventLogger() {
+HostEventLogger::HostEventLogger(ChromotingHost* host)
+    : host_(host) {
 #if defined(OS_LINUX)
   openlog("chromoting_host", 0, LOG_USER);
 #endif
+  host_->AddStatusObserver(this);
 }
 
 HostEventLogger::~HostEventLogger() {
+  host_->RemoveStatusObserver(this);
 }
 
 void HostEventLogger::OnClientAuthenticated(const std::string& jid) {
diff --git a/remoting/host/host_event_logger.h b/remoting/host/host_event_logger.h
index 27d43ff..4a4de5a 100644
--- a/remoting/host/host_event_logger.h
+++ b/remoting/host/host_event_logger.h
@@ -8,13 +8,16 @@
 #include <string>
 
 #include "base/compiler_specific.h"
+#include "base/memory/ref_counted.h"
 #include "remoting/host/host_status_observer.h"
 
 namespace remoting {
 
+class ChromotingHost;
+
 class HostEventLogger : public HostStatusObserver {
  public:
-  HostEventLogger();
+  HostEventLogger(ChromotingHost* host);
   virtual ~HostEventLogger();
 
   // HostStatusObserver implementation.  These methods will be called from the
@@ -23,6 +26,11 @@ class HostEventLogger : public HostStatusObserver {
   virtual void OnClientDisconnected(const std::string& jid) OVERRIDE;
   virtual void OnAccessDenied() OVERRIDE;
   virtual void OnShutdown() OVERRIDE;
+
+ private:
+  scoped_refptr<ChromotingHost> host_;
+
+  DISALLOW_COPY_AND_ASSIGN(HostEventLogger);
 };
 
 }
diff --git a/remoting/host/it2me_host_user_interface.cc b/remoting/host/it2me_host_user_interface.cc
index 8157374..8556d19 100644
--- a/remoting/host/it2me_host_user_interface.cc
+++ b/remoting/host/it2me_host_user_interface.cc
@@ -50,6 +50,7 @@ void It2MeHostUserInterface::InitFrom(DisconnectWindow* disconnect_window,
   disconnect_window_.reset(disconnect_window);
   continue_window_.reset(continue_window);
   local_input_monitor_.reset(monitor);
+  host_->AddStatusObserver(this);
 }
 
 void It2MeHostUserInterface::OnClientAuthenticated(const std::string& jid) {
@@ -76,6 +77,9 @@ void It2MeHostUserInterface::OnAccessDenied() {
 }
 
 void It2MeHostUserInterface::OnShutdown() {
+  // Host status observers must be removed on the network thread, so
+  // it must happen here instead of in the destructor.
+  host_->RemoveStatusObserver(this);
 }
 
 void It2MeHostUserInterface::Shutdown() {
diff --git a/remoting/host/log_to_server.cc b/remoting/host/log_to_server.cc
index 5fc6fed..c80ee88 100644
--- a/remoting/host/log_to_server.cc
+++ b/remoting/host/log_to_server.cc
@@ -24,13 +24,21 @@ namespace {
 const char kLogCommand[] = "log";
 }  // namespace
 
-LogToServer::LogToServer(SignalStrategy* signal_strategy)
-    : signal_strategy_(signal_strategy) {
+LogToServer::LogToServer(ChromotingHost* host,
+                         SignalStrategy* signal_strategy)
+    : host_(host),
+      signal_strategy_(signal_strategy) {
   signal_strategy_->AddListener(this);
+
+  // |host| may be NULL in tests.
+  if (host_)
+    host_->AddStatusObserver(this);
 }
 
 LogToServer::~LogToServer() {
   signal_strategy_->RemoveListener(this);
+  if (host_)
+    host_->RemoveStatusObserver(this);
 }
 
 void LogToServer::LogSessionStateChange(bool connected) {
diff --git a/remoting/host/log_to_server.h b/remoting/host/log_to_server.h
index 4d5d77d7..76fba6c 100644
--- a/remoting/host/log_to_server.h
+++ b/remoting/host/log_to_server.h
@@ -34,7 +34,8 @@ class LogToServer : public base::NonThreadSafe,
                     public HostStatusObserver,
                     public SignalStrategy::Listener {
  public:
-  explicit LogToServer(SignalStrategy* signal_strategy);
+  explicit LogToServer(ChromotingHost* host,
+                       SignalStrategy* signal_strategy);
   virtual ~LogToServer();
 
   // Logs a session state change. Currently, this is either
diff --git a/remoting/host/log_to_server_unittest.cc b/remoting/host/log_to_server_unittest.cc
index 60e7c2f5..f7b8004 100644
--- a/remoting/host/log_to_server_unittest.cc
+++ b/remoting/host/log_to_server_unittest.cc
@@ -32,7 +32,7 @@ class LogToServerTest : public testing::Test {
   virtual void SetUp() OVERRIDE {
     message_loop_proxy_ = base::MessageLoopProxy::current();
     EXPECT_CALL(signal_strategy_, AddListener(_));
-    log_to_server_.reset(new LogToServer(&signal_strategy_));
+    log_to_server_.reset(new LogToServer(NULL, &signal_strategy_));
     EXPECT_CALL(signal_strategy_, RemoveListener(_));
   }
 
diff --git a/remoting/host/plugin/host_script_object.cc b/remoting/host/plugin/host_script_object.cc
index b0eab71..4380cf8 100644
--- a/remoting/host/plugin/host_script_object.cc
+++ b/remoting/host/plugin/host_script_object.cc
@@ -374,6 +374,7 @@ void HostNPScriptObject::OnShutdown() {
   register_request_.reset();
   log_to_server_.reset();
   signal_strategy_.reset();
+  host_->RemoveStatusObserver(this);
   host_ = NULL;
 
   if (state_ != kDisconnected) {
@@ -514,13 +515,11 @@ void HostNPScriptObject::FinishConnectNetworkThread(
       &host_context_, signal_strategy_.get(), desktop_environment_.get(),
       protocol::NetworkSettings(nat_traversal_enabled_));
   host_->AddStatusObserver(this);
-  log_to_server_.reset(new LogToServer(signal_strategy_.get()));
-  host_->AddStatusObserver(log_to_server_.get());
+  log_to_server_.reset(new LogToServer(host_, signal_strategy_.get()));
   host_->set_it2me(true);
   it2me_host_user_interface_.reset(new It2MeHostUserInterface(host_.get(),
                                                               &host_context_));
   it2me_host_user_interface_->Init();
-  host_->AddStatusObserver(it2me_host_user_interface_.get());
 
   {
     base::AutoLock auto_lock(ui_strings_lock_);
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index bd397a7..1e4c4ae 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -173,11 +173,8 @@ class HostProcess {
     heartbeat_sender_.reset(
         new HeartbeatSender(host_id_, signal_strategy_.get(), &key_pair_));
 
-    log_to_server_.reset(new LogToServer(signal_strategy_.get()));
-    host_->AddStatusObserver(log_to_server_.get());
-
-    host_event_logger_.reset(new HostEventLogger());
-    host_->AddStatusObserver(host_event_logger_.get());
+    log_to_server_.reset(new LogToServer(host_, signal_strategy_.get()));
+    host_event_logger_.reset(new HostEventLogger(host_));
 
     host_->Start();
 
diff --git a/remoting/host/simple_host_process.cc b/remoting/host/simple_host_process.cc
index f06ba79..270d114 100644
--- a/remoting/host/simple_host_process.cc
+++ b/remoting/host/simple_host_process.cc
@@ -217,14 +217,12 @@ class SimpleHost {
                                desktop_environment_.get(), network_settings_);
     host_->set_it2me(is_it2me_);
 
-    log_to_server_.reset(new LogToServer(signal_strategy_.get()));
-    host_->AddStatusObserver(log_to_server_.get());
+    log_to_server_.reset(new LogToServer(host_, signal_strategy_.get()));
 
     if (is_it2me_) {
       it2me_host_user_interface_.reset(
           new It2MeHostUserInterface(host_, &context_));
       it2me_host_user_interface_->Init();
-      host_->AddStatusObserver(it2me_host_user_interface_.get());
     }
 
     if (protocol_config_.get()) {