Access ChromotingHost::clients_ only on network thread.

Previously ChromotingHost was doing some work on the main thread and
some on the network thread. |clients_| and some other members were
accessed without lock on both of these threads. Moved most of the
ChromotingHost activity to the network thread to avoid possible
race conditions.

BUG=96325
TEST=Chromoting works

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

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

5 files changed, 27 insertions, 27 deletions

diff --git a/remoting/protocol/connection_to_client.cc b/remoting/protocol/connection_to_client.cc
index 6b2bbd1..5e3c411 100644
--- a/remoting/protocol/connection_to_client.cc
+++ b/remoting/protocol/connection_to_client.cc
@@ -34,8 +34,8 @@ ConnectionToClient::ConnectionToClient(base::MessageLoopProxy* message_loop,
 }
 
 ConnectionToClient::~ConnectionToClient() {
-  // TODO(hclam): When we shut down the viewer we may have to close the
-  // connection.
+  if (session_.get())
+    message_loop_->DeleteSoon(FROM_HERE, session_.release());
 }
 
 void ConnectionToClient::SetEventHandler(EventHandler* event_handler) {
@@ -48,18 +48,21 @@ protocol::Session* ConnectionToClient::session() {
 }
 
 void ConnectionToClient::Disconnect() {
-  // This method can be called from main thread so perform threading switching.
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(
-        FROM_HERE, base::Bind(&ConnectionToClient::Disconnect, this));
-    return;
-  }
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
   CloseChannels();
 
-  // If there is a session then release it, causing it to close.
-  if (session_.get())
-    session_.reset();
+  DCHECK(session_.get());
+  Session* session = session_.release();
+
+  // It may not be safe to delete |session_| here becase this method
+  // may be invoked in resonse to a libjingle event and libjingle's
+  // sigslot doesn't handle it properly, so postpone the deletion.
+  message_loop_->DeleteSoon(FROM_HERE, session);
+
+  // This should trigger OnConnectionClosed() event and this object
+  // may be destroyed as the result.
+  session->Close();
 }
 
 void ConnectionToClient::UpdateSequenceNumber(int64 sequence_number) {
diff --git a/remoting/protocol/connection_to_client.h b/remoting/protocol/connection_to_client.h
index 3f34702..8723045 100644
--- a/remoting/protocol/connection_to_client.h
+++ b/remoting/protocol/connection_to_client.h
@@ -66,10 +66,7 @@ class ConnectionToClient :
   // Returns the connection in use.
   virtual Session* session();
 
-  // Disconnect the client connection. This method is allowed to be called
-  // more than once and calls after the first one will be ignored.
-  //
-  // After this method is called all the send method calls will be ignored.
+  // Disconnect the client connection.
   virtual void Disconnect();
 
   // Update the sequence number when received from the client. EventHandler
diff --git a/remoting/protocol/connection_to_client_unittest.cc b/remoting/protocol/connection_to_client_unittest.cc
index d3d1aed..81ea1f0 100644
--- a/remoting/protocol/connection_to_client_unittest.cc
+++ b/remoting/protocol/connection_to_client_unittest.cc
@@ -43,6 +43,11 @@ class ConnectionToClientTest : public testing::Test {
     message_loop_.RunAllPending();
   }
 
+  virtual void TearDown() OVERRIDE {
+    viewer_ = NULL;
+    message_loop_.RunAllPending();
+  }
+
   MessageLoop message_loop_;
   MockConnectionToClientEventHandler handler_;
   MockHostStub host_stub_;
@@ -101,13 +106,5 @@ TEST_F(ConnectionToClientTest, StateChange) {
   message_loop_.RunAllPending();
 }
 
-// Test that we can close client connection more than once.
-TEST_F(ConnectionToClientTest, Close) {
-  viewer_->Disconnect();
-  message_loop_.RunAllPending();
-  viewer_->Disconnect();
-  message_loop_.RunAllPending();
-}
-
 }  // namespace protocol
 }  // namespace remoting
diff --git a/remoting/protocol/jingle_session_manager.cc b/remoting/protocol/jingle_session_manager.cc
index cf85d0f..e1a935d 100644
--- a/remoting/protocol/jingle_session_manager.cc
+++ b/remoting/protocol/jingle_session_manager.cc
@@ -38,6 +38,8 @@ JingleSessionManager::JingleSessionManager(
 }
 
 JingleSessionManager::~JingleSessionManager() {
+  // Session manager can be destroyed only after all sessions are destroyed.
+  DCHECK(sessions_.empty());
   Close();
 }
 
@@ -111,13 +113,9 @@ void JingleSessionManager::Init(
 void JingleSessionManager::Close() {
   DCHECK(CalledOnValidThread());
 
-  // Close() can be called only after all sessions are destroyed.
-  DCHECK(sessions_.empty());
-
   if (!closed_) {
     cricket_session_manager_->RemoveClient(kChromotingXmlNamespace);
     jingle_signaling_connector_.reset();
-    cricket_session_manager_.reset();
     closed_ = true;
   }
 }
diff --git a/remoting/protocol/session.h b/remoting/protocol/session.h
index 27558d7..432b948 100644
--- a/remoting/protocol/session.h
+++ b/remoting/protocol/session.h
@@ -25,6 +25,11 @@ namespace protocol {
 // Generic interface for Chromotocol connection used by both client and host.
 // Provides access to the connection channels, but doesn't depend on the
 // protocol used for each channel.
+//
+// Because libjingle's sigslot class doesn't handle deletion properly
+// while it is being invoked all Session instances must be deleted
+// with a clean stack, i.e. not from event handlers, when sigslot may
+// be present in the stack.
 class Session : public base::NonThreadSafe {
  public:
   enum State {