Made ClientSession a non ref-counted class.

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

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

6 files changed, 46 insertions, 76 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index fc642e2..3b66a96 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -4,6 +4,8 @@
 
 #include "remoting/host/chromoting_host.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
@@ -184,18 +186,17 @@ void ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
 
   login_backoff_.Reset();
 
-  // Disconnect all other clients.
-  // Iterate over a copy of the list of clients, to avoid mutating the list
-  // while iterating over it.
-  ClientList clients_copy(clients_);
-  for (ClientList::const_iterator other_client = clients_copy.begin();
-       other_client != clients_copy.end(); ++other_client) {
-    if (other_client->get() != client) {
-      (*other_client)->Disconnect();
-    }
+  // Disconnect all other clients. |it| should be advanced before Disconnect()
+  // is called to avoid it becoming invalid when the client is removed from
+  // the list.
+  ClientList::iterator it = clients_.begin();
+  while (it != clients_.end()) {
+    ClientSession* other_client = *it++;
+    if (other_client != client)
+      other_client->Disconnect();
   }
 
-  // Disconnects above must have destroyed all other clients and |recorder_|.
+  // Disconnects above must have destroyed all other clients.
   DCHECK_EQ(clients_.size(), 1U);
 
   // Notify observers that there is at least one authenticated client.
@@ -232,12 +233,7 @@ void ChromotingHost::OnSessionAuthenticationFailed(ClientSession* client) {
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
-  ClientList::iterator it = clients_.begin();
-  for (; it != clients_.end(); ++it) {
-    if (it->get() == client) {
-      break;
-    }
-  }
+  ClientList::iterator it = std::find(clients_.begin(), clients_.end(), client);
   CHECK(it != clients_.end());
 
   if (client->is_authenticated()) {
@@ -245,8 +241,8 @@ void ChromotingHost::OnSessionClosed(ClientSession* client) {
                       OnClientDisconnected(client->client_jid()));
   }
 
-  client->Stop();
   clients_.erase(it);
+  delete client;
 
   if (state_ == kStopping && clients_.empty())
     ShutdownFinish();
@@ -311,7 +307,7 @@ void ChromotingHost::OnIncomingSession(
   // Create a client object.
   scoped_ptr<protocol::ConnectionToClient> connection(
       new protocol::ConnectionToClient(session));
-  scoped_refptr<ClientSession> client = new ClientSession(
+  ClientSession* client = new ClientSession(
       this,
       audio_task_runner_,
       input_task_runner_,
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index 9aa9afd..6a574a8 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -5,8 +5,8 @@
 #ifndef REMOTING_HOST_CHROMOTING_HOST_H_
 #define REMOTING_HOST_CHROMOTING_HOST_H_
 
+#include <list>
 #include <string>
-#include <vector>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/ref_counted.h"
@@ -157,7 +157,7 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   friend class base::RefCountedThreadSafe<ChromotingHost>;
   friend class ChromotingHostTest;
 
-  typedef std::vector<scoped_refptr<ClientSession> > ClientList;
+  typedef std::list<ClientSession*> ClientList;
 
   enum State {
     kInitial,
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 80b3fa0..f53caf8 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -237,7 +237,7 @@ class ChromotingHostTest : public testing::Test {
         ((connection_index == 0) ? owned_connection1_ : owned_connection2_).
         PassAs<protocol::ConnectionToClient>();
     protocol::ConnectionToClient* connection_ptr = connection.get();
-    scoped_refptr<ClientSession> client = new ClientSession(
+    scoped_ptr<ClientSession> client(new ClientSession(
         host_.get(),
         ui_task_runner_,  // Audio
         ui_task_runner_,  // Input
@@ -247,31 +247,33 @@ class ChromotingHostTest : public testing::Test {
         ui_task_runner_,  // UI
         connection.Pass(),
         desktop_environment_factory_.get(),
-        base::TimeDelta());
-    connection_ptr->set_host_stub(client);
+        base::TimeDelta()));
+
+    ClientSession* client_raw = client.get();
+    connection_ptr->set_host_stub(client_raw);
 
     ui_task_runner_->PostTask(
         FROM_HERE, base::Bind(&ChromotingHostTest::AddClientToHost,
-                              host_, client));
+                              host_, base::Passed(&client)));
 
     if (authenticate) {
       ui_task_runner_->PostTask(
           FROM_HERE, base::Bind(&ClientSession::OnConnectionAuthenticated,
-                                client, connection_ptr));
+                                base::Unretained(client_raw), connection_ptr));
       if (!reject) {
         ui_task_runner_->PostTask(
             FROM_HERE,
             base::Bind(&ClientSession::OnConnectionChannelsConnected,
-                       client, connection_ptr));
+                       base::Unretained(client_raw), connection_ptr));
       }
     } else {
       ui_task_runner_->PostTask(
           FROM_HERE, base::Bind(&ClientSession::OnConnectionClosed,
-                                client, connection_ptr,
+                                base::Unretained(client_raw), connection_ptr,
                                 protocol::AUTHENTICATION_FAILED));
     }
 
-    get_client(connection_index) = client;
+    get_client(connection_index) = client_raw;
   }
 
   virtual void TearDown() OVERRIDE {
@@ -360,8 +362,9 @@ class ChromotingHostTest : public testing::Test {
   }
 
   static void AddClientToHost(scoped_refptr<ChromotingHost> host,
-                              ClientSession* session) {
-    host->clients_.push_back(session);
+                              scoped_ptr<ClientSession> client) {
+    // |host| is responsible for deleting |client| from now on.
+    host->clients_.push_back(client.release());
   }
 
   void ShutdownHost() {
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index a150066..11a380e 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -89,6 +89,16 @@ ClientSession::ClientSession(
 #endif  // defined(OS_WIN)
 }
 
+ClientSession::~ClientSession() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!audio_scheduler_);
+  DCHECK(!event_executor_);
+  DCHECK(!session_controller_);
+  DCHECK(!video_scheduler_);
+
+  connection_.reset();
+}
+
 void ClientSession::NotifyClientResolution(
     const protocol::ClientResolution& resolution) {
   if (resolution.has_dips_width() && resolution.has_dips_height()) {
@@ -267,16 +277,6 @@ void ClientSession::Disconnect() {
   connection_->Disconnect();
 }
 
-void ClientSession::Stop() {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!audio_scheduler_);
-  DCHECK(!event_executor_);
-  DCHECK(!session_controller_);
-  DCHECK(!video_scheduler_);
-
-  connection_.reset();
-}
-
 void ClientSession::LocalMouseMoved(const SkIPoint& mouse_pos) {
   DCHECK(CalledOnValidThread());
   remote_input_filter_.LocalMouseMoved(mouse_pos);
@@ -292,14 +292,6 @@ void ClientSession::SetDisableInputs(bool disable_inputs) {
   disable_clipboard_filter_.set_enabled(!disable_inputs);
 }
 
-ClientSession::~ClientSession() {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!audio_scheduler_);
-  DCHECK(!event_executor_);
-  DCHECK(!session_controller_);
-  DCHECK(!video_scheduler_);
-}
-
 scoped_ptr<protocol::ClipboardStub> ClientSession::CreateClipboardProxy() {
   DCHECK(CalledOnValidThread());
 
@@ -342,9 +334,4 @@ scoped_ptr<AudioEncoder> ClientSession::CreateAudioEncoder(
   return scoped_ptr<AudioEncoder>(NULL);
 }
 
-// static
-void ClientSessionTraits::Destruct(const ClientSession* client) {
-  client->network_task_runner_->DeleteSoon(FROM_HERE, client);
-}
-
 }  // namespace remoting
diff --git a/remoting/host/client_session.h b/remoting/host/client_session.h
index b6d3a6a..f0d1e51 100644
--- a/remoting/host/client_session.h
+++ b/remoting/host/client_session.h
@@ -34,7 +34,6 @@ namespace remoting {
 
 class AudioEncoder;
 class AudioScheduler;
-struct ClientSessionTraits;
 class DesktopEnvironment;
 class DesktopEnvironmentFactory;
 class EventExecutor;
@@ -45,8 +44,7 @@ class VideoScheduler;
 // A ClientSession keeps a reference to a connection to a client, and maintains
 // per-client state.
 class ClientSession
-    : public base::RefCountedThreadSafe<ClientSession, ClientSessionTraits>,
-      public protocol::HostStub,
+    : public protocol::HostStub,
       public protocol::ConnectionToClient::EventHandler,
       public base::NonThreadSafe {
  public:
@@ -96,6 +94,7 @@ class ClientSession
       scoped_ptr<protocol::ConnectionToClient> connection,
       DesktopEnvironmentFactory* desktop_environment_factory,
       const base::TimeDelta& max_duration);
+  virtual ~ClientSession();
 
   // protocol::HostStub interface.
   virtual void NotifyClientResolution(
@@ -124,10 +123,6 @@ class ClientSession
   // method returns.
   void Disconnect();
 
-  // Stops the ClientSession. The caller can safely release its reference to
-  // the client session once Stop() returns.
-  void Stop();
-
   protocol::ConnectionToClient* connection() const {
     return connection_.get();
   }
@@ -146,10 +141,6 @@ class ClientSession
   void SetDisableInputs(bool disable_inputs);
 
  private:
-  friend class base::DeleteHelper<ClientSession>;
-  friend struct ClientSessionTraits;
-  virtual ~ClientSession();
-
   // Creates a proxy for sending clipboard events to the client.
   scoped_ptr<protocol::ClipboardStub> CreateClipboardProxy();
 
@@ -230,11 +221,6 @@ class ClientSession
   DISALLOW_COPY_AND_ASSIGN(ClientSession);
 };
 
-// Destroys |ClienSession| instances on the network thread.
-struct ClientSessionTraits {
-  static void Destruct(const ClientSession* client);
-};
-
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_CLIENT_SESSION_H_
diff --git a/remoting/host/client_session_unittest.cc b/remoting/host/client_session_unittest.cc
index a7893a4..33277f5 100644
--- a/remoting/host/client_session_unittest.cc
+++ b/remoting/host/client_session_unittest.cc
@@ -100,7 +100,7 @@ class ClientSessionTest : public testing::Test {
   MessageLoop message_loop_;
 
   // ClientSession instance under test.
-  scoped_refptr<ClientSession> client_session_;
+  scoped_ptr<ClientSession> client_session_;
 
   // ClientSession::EventHandler mock for use in tests.
   MockClientSessionEventHandler session_event_handler_;
@@ -160,7 +160,7 @@ void ClientSessionTest::SetUp() {
   EXPECT_CALL(*connection, Disconnect());
   connection_ = connection.get();
 
-  client_session_ = new ClientSession(
+  client_session_.reset(new ClientSession(
       &session_event_handler_,
       ui_task_runner, // Audio thread.
       ui_task_runner, // Input thread.
@@ -170,12 +170,12 @@ void ClientSessionTest::SetUp() {
       ui_task_runner, // UI thread.
       connection.PassAs<protocol::ConnectionToClient>(),
       desktop_environment_factory_.get(),
-      base::TimeDelta());
+      base::TimeDelta()));
 }
 
 void ClientSessionTest::TearDown() {
   // Verify that the client session has been stopped.
-  EXPECT_TRUE(client_session_.get() == NULL);
+  EXPECT_TRUE(!client_session_);
 }
 
 void ClientSessionTest::DisconnectClientSession() {
@@ -186,9 +186,7 @@ void ClientSessionTest::DisconnectClientSession() {
 }
 
 void ClientSessionTest::StopClientSession() {
-  // MockClientSessionEventHandler won't trigger Stop, so fake it.
-  client_session_->Stop();
-  client_session_ = NULL;
+  client_session_.reset();
 
   desktop_environment_factory_.reset();
 }