Minor cleanups in JingleSession and JingleSessionManager.

BUG=None
TEST=None

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

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

remoting/host/chromoting_host.cc

@@ -223,7 +223,7 @@ void ChromotingHost::OnStateChange(
 
     // Create and start session manager.
     protocol::JingleSessionManager* server =
-        new protocol::JingleSessionManager(NULL, NULL, NULL);
+        protocol::JingleSessionManager::CreateNotSandboxed();
     // TODO(ajwong): Make this a command switch when we're more stable.
     server->set_allow_local_ips(true);
 

remoting/jingle_glue/javascript_signal_strategy.cc

@@ -22,6 +22,7 @@ JavascriptSignalStrategy::JavascriptSignalStrategy(const std::string& your_jid)
 
 JavascriptSignalStrategy::~JavascriptSignalStrategy() {
   DCHECK(listener_ == NULL);
+  Close();
 }
 
 void JavascriptSignalStrategy::AttachXmppProxy(

remoting/jingle_glue/xmpp_signal_strategy.cc

@@ -29,10 +29,8 @@ XmppSignalStrategy::XmppSignalStrategy(JingleThread* jingle_thread,
 }
 
 XmppSignalStrategy::~XmppSignalStrategy() {
-  if (xmpp_client_)
-    xmpp_client_->engine()->RemoveStanzaHandler(this);
-
   DCHECK(listener_ == NULL);
+  Close();
 }
 
 void XmppSignalStrategy::Init(StatusObserver* observer) {

remoting/protocol/connection_to_client.cc

@@ -58,11 +58,9 @@ void ConnectionToClient::Disconnect() {
 
   CloseChannels();
 
-  // If there is a channel then close it and release the reference.
-  if (session_.get()) {
-    session_->Close();
+  // If there is a session then release it, causing it to close.
+  if (session_.get())
     session_.reset();
-  }
 }
 
 void ConnectionToClient::UpdateSequenceNumber(int64 sequence_number) {

remoting/protocol/connection_to_host.cc

@@ -86,20 +86,14 @@ void ConnectionToHost::Disconnect(const base::Closure& shutdown_task) {
 
   CloseChannels();
 
-  if (session_.get()) {
-    session_->Close();
+  if (session_.get())
     session_.reset();
-  }
 
-  if (session_manager_.get()) {
-    session_manager_->Close();
+  if (session_manager_.get())
     session_manager_.reset();
-  }
 
-  if (signal_strategy_.get()) {
-    signal_strategy_->Close();
+  if (signal_strategy_.get())
     signal_strategy_.reset();
-  }
 
   shutdown_task.Run();
 }
@@ -109,9 +103,9 @@ void ConnectionToHost::InitSession() {
 
   // Initialize chromotocol |session_manager_|.
   JingleSessionManager* session_manager =
-      new JingleSessionManager(network_manager_.release(),
-                               socket_factory_.release(),
-                               port_allocator_session_factory_.release());
+      JingleSessionManager::CreateSandboxed(
+          network_manager_.release(), socket_factory_.release(),
+          port_allocator_session_factory_.release());
 
   // TODO(ajwong): Make this a command switch when we're more stable.
   session_manager->set_allow_local_ips(true);

remoting/protocol/jingle_session.cc

@@ -159,7 +159,9 @@ JingleSession::JingleSession(
 }
 
 JingleSession::~JingleSession() {
-  DCHECK(closed_);
+  // Reset the callback so that it's not called from Close().
+  state_change_callback_.reset();
+  Close();
   jingle_session_manager_->SessionDestroyed(this);
 }
 
@@ -212,8 +214,10 @@ void JingleSession::CloseInternal(int result, bool failed) {
     if (video_rtcp_channel_.get())
       video_rtcp_channel_->Close(result);
 
-    if (cricket_session_)
+    if (cricket_session_) {
       cricket_session_->Terminate();
+      cricket_session_->SignalState.disconnect(this);
+    }
 
     if (failed)
       SetState(FAILED);

remoting/protocol/jingle_session_manager.cc

@@ -26,6 +26,20 @@ using buzz::XmlElement;
 namespace remoting {
 namespace protocol {
 
+// static
+JingleSessionManager* JingleSessionManager::CreateNotSandboxed() {
+  return new JingleSessionManager(NULL, NULL, NULL);
+}
+
+// static
+JingleSessionManager* JingleSessionManager::CreateSandboxed(
+    talk_base::NetworkManager* network_manager,
+    talk_base::PacketSocketFactory* socket_factory,
+    PortAllocatorSessionFactory* port_allocator_session_factory) {
+  return new JingleSessionManager(network_manager, socket_factory,
+                                  port_allocator_session_factory);
+}
+
 JingleSessionManager::JingleSessionManager(
     talk_base::NetworkManager* network_manager,
     talk_base::PacketSocketFactory* socket_factory,
@@ -42,7 +56,7 @@ JingleSessionManager::JingleSessionManager(
 }
 
 JingleSessionManager::~JingleSessionManager() {
-  DCHECK(closed_);
+  Close();
 }
 
 void JingleSessionManager::Init(
@@ -127,21 +141,6 @@ Session* JingleSessionManager::Connect(
   jingle_session->set_candidate_config(candidate_config);
   jingle_session->set_receiver_token(receiver_token);
 
-  // TODO(sergeyu): Does this need to be asynchronous?
-  MessageLoop::current()->PostTask(FROM_HERE, task_factory_.NewRunnableMethod(
-      &JingleSessionManager::DoConnect, jingle_session, host_jid,
-      host_public_key, receiver_token, state_change_callback));
-  return jingle_session;
-}
-
-void JingleSessionManager::DoConnect(
-    JingleSession* jingle_session,
-    const std::string& host_jid,
-    const std::string& host_public_key,
-    const std::string& receiver_token,
-    Session::StateChangeCallback* state_change_callback) {
-  DCHECK(CalledOnValidThread());
-
   cricket::Session* cricket_session = cricket_session_manager_->CreateSession(
       local_jid_, kChromotingXmlNamespace);
 
@@ -153,6 +152,8 @@ void JingleSessionManager::DoConnect(
   cricket_session->Initiate(host_jid, CreateClientSessionDescription(
       jingle_session->candidate_config()->Clone(), receiver_token,
       jingle_session->GetEncryptedMasterKey()));
+
+  return jingle_session;
 }
 
 void JingleSessionManager::OnSessionCreate(
@@ -261,7 +262,7 @@ void JingleSessionManager::OnJingleInfo(
     const std::vector<talk_base::SocketAddress>& stun_hosts) {
   DCHECK(CalledOnValidThread());
 
-  if (port_allocator_.get()) {
+  if (http_port_allocator_) {
     // TODO(ajwong): Avoid string processing if log-level is low.
     std::string stun_servers;
     for (size_t i = 0; i < stun_hosts.size(); ++i) {

remoting/protocol/jingle_session_manager.h

@@ -37,11 +37,13 @@ class JingleSessionManager
     : public SessionManager,
       public cricket::SessionClient {
  public:
-  JingleSessionManager(
+  virtual ~JingleSessionManager();
+
+  static JingleSessionManager* CreateNotSandboxed();
+  static JingleSessionManager* CreateSandboxed(
       talk_base::NetworkManager* network_manager,
       talk_base::PacketSocketFactory* socket_factory,
       PortAllocatorSessionFactory* port_allocator_session_factory);
-  virtual ~JingleSessionManager();
 
   // SessionManager interface.
   virtual void Init(const std::string& local_jid,
@@ -76,6 +78,11 @@ class JingleSessionManager
  private:
   friend class JingleSession;
 
+  JingleSessionManager(
+      talk_base::NetworkManager* network_manager,
+      talk_base::PacketSocketFactory* socket_factory,
+      PortAllocatorSessionFactory* port_allocator_session_factory);
+
   // Called by JingleSession when a new connection is
   // initiated. Returns true if session is accepted.
   bool AcceptConnection(JingleSession* jingle_session,
@@ -84,13 +91,6 @@ class JingleSessionManager
   // Called by JingleSession when it is being destroyed.
   void SessionDestroyed(JingleSession* jingle_session);
 
-  void DoConnect(
-      JingleSession* jingle_session,
-      const std::string& host_jid,
-      const std::string& host_public_key,
-      const std::string& client_token,
-      Session::StateChangeCallback* state_change_callback);
-
   // Callback for JingleInfoRequest.
   void OnJingleInfo(
       const std::string& token,

remoting/protocol/jingle_session_unittest.cc

@@ -174,7 +174,7 @@ class JingleSessionTest : public testing::Test {
     FakeSignalStrategy::Connect(host_signal_strategy_.get(),
                                 client_signal_strategy_.get());
 
-    host_server_.reset(new JingleSessionManager(NULL, NULL, NULL));
+    host_server_.reset(JingleSessionManager::CreateNotSandboxed());
     host_server_->set_allow_local_ips(true);
     host_server_->Init(
         kHostJid, host_signal_strategy_.get(),
@@ -183,7 +183,7 @@ class JingleSessionTest : public testing::Test {
         private_key.release(),
         cert);
 
-    client_server_.reset(new JingleSessionManager(NULL, NULL, NULL));
+    client_server_.reset(JingleSessionManager::CreateNotSandboxed());
     client_server_->set_allow_local_ips(true);
     client_server_->Init(
         kClientJid, client_signal_strategy_.get(),

remoting/protocol/protocol_test_client.cc

@@ -220,7 +220,7 @@ void ProtocolTestClient::Run(const std::string& username,
       new XmppSignalStrategy(&jingle_thread, username, auth_token,
                              auth_service));
   signal_strategy_->Init(this);
-  session_manager_.reset(new JingleSessionManager(NULL, NULL, NULL));
+  session_manager_.reset(JingleSessionManager::CreateNotSandboxed());
 
   host_jid_ = host_jid;
 

remoting/protocol/session_manager.h

@@ -2,16 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// The purprose of SessionManager is to facilitate creation of chromotocol
+// The purpose of SessionManager is to facilitate creation of chromotocol
 // sessions. Both host and client use it to establish chromotocol
 // sessions. JingleChromotocolServer implements this inteface using
 // libjingle.
 //
 // OUTGOING SESSIONS
 // Connect() must be used to create new session to a remote host. The
-// returned sessionion is initially in INITIALIZING state. Later state is
-// changed to CONNECTED if the session is accepted by the host or CLOSED
-// if the session is rejected.
+// returned session is initially in INITIALIZING state. Later state is
+// changed to CONNECTED if the session is accepted by the host or
+// CLOSED if the session is rejected.
 //
 // INCOMING SESSIONS
 // The IncomingSessionCallback is called when a client attempts to connect.
@@ -19,12 +19,11 @@
 // rejected.
 //
 // SESSION OWNERSHIP AND SHUTDOWN
-// SessionManager owns all Chromotocol Session it creates. The server
-// must not be closed while sessions created by the server are still in use.
-// When shutting down the Close() method for the sessionion and the server
-// objects must be called in the following order: Session,
-// SessionManager, JingleClient. The same order must be followed in the case
-// of rejected and failed sessions.
+// SessionManager owns all Sessions it creates. The manager must not
+// be closed or destroyed before all sessions created by that
+// SessionManager are destroyed. Caller owns Sessions created by a
+// SessionManager (except rejected sessions). Sessions must outlive
+// SessionManager, and SignalStrategy must outlive SessionManager.
 //
 // PROTOCOL VERSION NEGOTIATION
 // When client connects to a host it sends a session-initiate stanza with list
@@ -70,6 +69,9 @@ class SignalStrategy;
 namespace protocol {
 
 // Generic interface for Chromoting session manager.
+//
+// TODO(sergeyu): Split this into two separate interfaces: one for the
+// client side and one for the host side.
 class SessionManager : public base::NonThreadSafe {
  public:
   SessionManager() { }
@@ -88,18 +90,16 @@ class SessionManager : public base::NonThreadSafe {
   // has incompatible configuration, and cannot be accepted.  If the
   // callback accepts session then it must also set configuration for
   // the new session using Session::set_config(). The callback must
-  // take ownership of the session if it accepts connection.
+  // take ownership of the session if it ACCEPTs it.
   typedef Callback2<Session*, IncomingSessionResponse*>::Type
       IncomingSessionCallback;
 
-  // Initializes the session client. Doesn't accept ownership of the
-  // |signal_strategy|. Close() must be called _before_ the |session_manager|
-  // is destroyed.
-  // If this object is used in server mode, then |private_key| and
-  // |certificate| are used to establish a secured communication with the
-  // client. It will also take ownership of these objects.
-  // In case this is used in client mode, pass in NULL for both private key and
-  // certificate.
+  // Initializes the session client. Caller retains ownership of the
+  // |signal_strategy|. If this object is used in server mode, then
+  // |private_key| and |certificate| are used to establish a secured
+  // communication with the client. It will also take ownership of
+  // these objects. In case this is used in client mode, pass in NULL
+  // for both private key and certificate.
   virtual void Init(const std::string& local_jid,
                     SignalStrategy* signal_strategy,
                     IncomingSessionCallback* incoming_session_callback,
@@ -114,9 +114,6 @@ class SessionManager : public base::NonThreadSafe {
   // |config| contains the session configurations that the client supports.
   // |state_change_callback| is called when the connection state changes.
   //
-  // This function may be called from any thread. The |state_change_callback|
-  // is invoked on the network thread.
-  //
   // Ownership of the |config| is passed to the new session.
   virtual Session* Connect(
       const std::string& host_jid,
@@ -125,8 +122,9 @@ class SessionManager : public base::NonThreadSafe {
       CandidateSessionConfig* config,
       Session::StateChangeCallback* state_change_callback) = 0;
 
-  // Close session manager and all current sessions. No callbacks are
-  // called after this method returns.
+  // Close session manager. Can be called only after all corresponding
+  // sessions are destroyed. No callbacks are called after this method
+  // returns.
   virtual void Close() = 0;
 
  private: