Fix JingleSession to respect net::StreamSocket Read/Write semantics.

Remove some unnecessary state checks in PseudoTcpAdapter.

BUG=
TEST=Unit tests and Remoting components continue to work.


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

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

2 files changed, 114 insertions, 69 deletions

diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index 3370118e..3d89f14 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -140,7 +140,6 @@ JingleSession::JingleSession(
       closed_(false),
       closing_(false),
       cricket_session_(NULL),
-      ssl_connections_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(connect_callback_(
           this, &JingleSession::OnConnect)),
       ALLOW_THIS_IN_INITIALIZER_LIST(ssl_connect_callback_(
@@ -183,18 +182,24 @@ void JingleSession::CloseInternal(int result, bool failed) {
 
     if (control_channel_.get())
       control_channel_->Close(result);
-    if (control_ssl_socket_.get())
-      control_ssl_socket_->Disconnect();
+    control_socket_.reset();
+    control_ssl_socket_.reset();
+    if (control_socket_wrapper_.get())
+      control_socket_wrapper_->Disconnect();
 
     if (event_channel_.get())
       event_channel_->Close(result);
-    if (event_ssl_socket_.get())
-      event_ssl_socket_->Disconnect();
+    event_socket_.reset();
+    event_ssl_socket_.reset();
+    if (event_socket_wrapper_.get())
+      event_socket_wrapper_->Disconnect();
 
     if (video_channel_.get())
       video_channel_->Close(result);
-    if (video_ssl_socket_.get())
-      video_ssl_socket_->Disconnect();
+    video_socket_.reset();
+    video_ssl_socket_.reset();
+    if (video_socket_wrapper_.get())
+      video_socket_wrapper_->Disconnect();
 
     if (video_rtp_channel_.get())
       video_rtp_channel_->Close(result);
@@ -239,18 +244,18 @@ void JingleSession::SetStateChangeCallback(StateChangeCallback* callback) {
 
 net::Socket* JingleSession::control_channel() {
   DCHECK_EQ(jingle_session_manager_->message_loop(), MessageLoop::current());
-  return control_ssl_socket_.get();
+  return control_socket_wrapper_.get();
 }
 
 net::Socket* JingleSession::event_channel() {
   DCHECK_EQ(jingle_session_manager_->message_loop(), MessageLoop::current());
-  return event_ssl_socket_.get();
+  return event_socket_wrapper_.get();
 }
 
 // TODO(sergeyu): Remove this method after we switch to RTP.
 net::Socket* JingleSession::video_channel() {
   DCHECK_EQ(jingle_session_manager_->message_loop(), MessageLoop::current());
-  return video_ssl_socket_.get();
+  return video_socket_wrapper_.get();
 }
 
 net::Socket* JingleSession::video_rtp_channel() {
@@ -428,7 +433,8 @@ void JingleSession::OnInitiate() {
       new jingle_glue::TransportChannelSocketAdapter(raw_event_channel_));
 
   // Create video channel.
-  // TODO(sergeyu): Remove video channel when we are ready to switch to RTP.
+  // TODO(wez): When we have RTP video support, we'll need to negotiate the
+  // type of video channel to allocate, for legacy compatibility.
   raw_video_channel_ =
       cricket_session_->CreateChannel(content_name, kVideoChannelName);
   video_channel_.reset(
@@ -443,21 +449,26 @@ void JingleSession::OnInitiate() {
   }
 }
 
-bool JingleSession::EstablishSSLConnection(
-    net::Socket* channel, scoped_ptr<SocketWrapper>* ssl_socket) {
-  jingle_glue::PseudoTcpAdapter* pseudotcp =
-      new jingle_glue::PseudoTcpAdapter(channel);
-  pseudotcp->Connect(&connect_callback_);
+bool JingleSession::EstablishPseudoTcp(
+    net::Socket* channel,
+    scoped_ptr<net::StreamSocket>* stream) {
+  stream->reset(new jingle_glue::PseudoTcpAdapter(channel));
+  int result = (*stream)->Connect(&connect_callback_);
+  return (result == net::OK) || (result == net::ERR_IO_PENDING);
+}
 
-  // TODO(wez): We shouldn't try to start SSL until the socket we're
-  // starting it on is connected.
+bool JingleSession::EstablishSSLConnection(
+    net::StreamSocket* socket,
+    scoped_ptr<net::StreamSocket>* ssl_socket) {
+  DCHECK(socket);
+  DCHECK(socket->IsConnected());
   if (cricket_session_->initiator()) {
     // Create client SSL socket.
-    net::SSLClientSocket* socket = CreateSSLClientSocket(
-        pseudotcp, remote_cert_, cert_verifier_.get());
-    ssl_socket->reset(new SocketWrapper(socket));
+    net::SSLClientSocket* ssl_client_socket = CreateSSLClientSocket(
+        socket, remote_cert_, cert_verifier_.get());
+    ssl_socket->reset(ssl_client_socket);
 
-    int ret = socket->Connect(&ssl_connect_callback_);
+    int ret = ssl_client_socket->Connect(&ssl_connect_callback_);
     if (ret == net::ERR_IO_PENDING) {
       return true;
     } else if (ret != net::OK) {
@@ -468,11 +479,11 @@ bool JingleSession::EstablishSSLConnection(
   } else {
     // Create server SSL socket.
     net::SSLConfig ssl_config;
-    net::SSLServerSocket* socket = net::CreateSSLServerSocket(
-        pseudotcp, local_cert_, local_private_key_.get(), ssl_config);
-    ssl_socket->reset(new SocketWrapper(socket));
+    net::SSLServerSocket* ssl_server_socket = net::CreateSSLServerSocket(
+        socket, local_cert_, local_private_key_.get(), ssl_config);
+    ssl_socket->reset(ssl_server_socket);
 
-    int ret = socket->Handshake(&ssl_connect_callback_);
+    int ret = ssl_server_socket->Handshake(&ssl_connect_callback_);
     if (ret == net::ERR_IO_PENDING) {
       return true;
     } else if (ret != net::OK) {
@@ -518,27 +529,8 @@ bool JingleSession::InitializeConfigFromDescription(
   return true;
 }
 
-bool JingleSession::InitializeSSL() {
-  if (!EstablishSSLConnection(control_channel_.release(),
-                              &control_ssl_socket_)) {
-    LOG(ERROR) << "Establish control channel failed";
-    return false;
-  }
-  if (!EstablishSSLConnection(event_channel_.release(),
-                              &event_ssl_socket_)) {
-    LOG(ERROR) << "Establish event channel failed";
-    return false;
-  }
-  if (!EstablishSSLConnection(video_channel_.release(),
-                              &video_ssl_socket_)) {
-    LOG(ERROR) << "Establish video channel failed";
-    return false;
-  }
-  return true;
-}
-
 void JingleSession::InitializeChannels() {
-  // Disable incoming connections on the host so that we don't travers
+  // Disable incoming connections on the host so that we don't traverse
   // the firewall.
   if (!cricket_session_->initiator()) {
     raw_control_channel_->GetP2PChannel()->set_incoming_only(true);
@@ -548,7 +540,13 @@ void JingleSession::InitializeChannels() {
     raw_video_rtcp_channel_->GetP2PChannel()->set_incoming_only(true);
   }
 
-  if (!InitializeSSL()) {
+  // Create the Control, Event and Video connections on the channels.
+  if (!EstablishPseudoTcp(control_channel_.release(),
+                           &control_socket_) ||
+      !EstablishPseudoTcp(event_channel_.release(),
+                           &event_socket_) ||
+      !EstablishPseudoTcp(video_channel_.release(),
+                           &video_socket_)) {
     CloseInternal(net::ERR_CONNECTION_FAILED, true);
     return;
   }
@@ -584,6 +582,32 @@ void JingleSession::OnConnect(int result) {
   if (result != net::OK) {
     LOG(ERROR) << "PseudoTCP connection failed: " << result;
     CloseInternal(result, true);
+    return;
+  }
+
+  if (control_socket_.get() && control_socket_->IsConnected()) {
+    if (!EstablishSSLConnection(control_socket_.release(),
+                                &control_ssl_socket_)) {
+      LOG(ERROR) << "Establish control channel failed";
+      CloseInternal(net::ERR_CONNECTION_FAILED, true);
+      return;
+    }
+  }
+  if (event_socket_.get() && event_socket_->IsConnected()) {
+    if (!EstablishSSLConnection(event_socket_.release(),
+                                &event_ssl_socket_)) {
+      LOG(ERROR) << "Establish control event failed";
+      CloseInternal(net::ERR_CONNECTION_FAILED, true);
+      return;
+    }
+  }
+  if (video_socket_.get() && video_socket_->IsConnected()) {
+    if (!EstablishSSLConnection(video_socket_.release(),
+                                &video_ssl_socket_)) {
+      LOG(ERROR) << "Establish control video failed";
+      CloseInternal(net::ERR_CONNECTION_FAILED, true);
+      return;
+    }
   }
 }
 
@@ -595,14 +619,24 @@ void JingleSession::OnSSLConnect(int result) {
     return;
   }
 
-  // Number of channels for a jingle session.
-  const int kChannels = 3;
+  if (control_ssl_socket_.get() && control_ssl_socket_->IsConnected()) {
+    control_socket_wrapper_.reset(
+        new SocketWrapper(control_ssl_socket_.release()));
+  }
+  if (event_ssl_socket_.get() && event_ssl_socket_->IsConnected()) {
+    event_socket_wrapper_.reset(
+        new SocketWrapper(event_ssl_socket_.release()));
+  }
+  if (video_ssl_socket_.get() && video_ssl_socket_->IsConnected()) {
+    video_socket_wrapper_.reset(
+        new SocketWrapper(video_ssl_socket_.release()));
+  }
 
-  // Set the state to connected only of all SSL sockets are connected.
-  if (++ssl_connections_ == kChannels) {
+  if (event_socket_wrapper_.get() &&
+      control_socket_wrapper_.get() &&
+      video_socket_wrapper_.get()) {
     SetState(CONNECTED);
   }
-  CHECK(ssl_connections_ <= kChannels) << "Unexpected SSL connect callback";
 }
 
 void JingleSession::SetState(State new_state) {
diff --git a/remoting/protocol/jingle_session.h b/remoting/protocol/jingle_session.h
index a48dc10..0abe34e 100644
--- a/remoting/protocol/jingle_session.h
+++ b/remoting/protocol/jingle_session.h
@@ -110,15 +110,15 @@ class JingleSession : public protocol::Session,
   // Configures channels and calls InitializeSSL().
   void InitializeChannels();
 
-  // Initialize PseudoTCP + SSL on each of the video, control and input
-  // channels. The channels must have been created before this is called.
-  bool InitializeSSL();
+  // Helper method to initialize PseudoTcp over the datagram-oriented |channel|.
+  // If things don't immediately fail then a new StreamSocket is placed in
+  // |pseudo_tcp|.
+  bool EstablishPseudoTcp(net::Socket* channel,
+                          scoped_ptr<net::StreamSocket>* pseudo_tcp);
 
-  // Helper method to create and initialize PseudoTCP + SSL socket on
-  // top of the provided |channel|. The resultant SSL socket is
-  // written to |ssl_socket|. Return true if successful.
-  bool EstablishSSLConnection(net::Socket* channel,
-                              scoped_ptr<SocketWrapper>* ssl_socket);
+  // Helper method to initialize SSL over a StreamSocket.
+  bool EstablishSSLConnection(net::StreamSocket* socket,
+                              scoped_ptr<net::StreamSocket>* ssl_socket);
 
   // Used for Session.SignalState sigslot.
   void OnSessionState(cricket::BaseSession* session,
@@ -178,23 +178,34 @@ class JingleSession : public protocol::Session,
   // These data members are only set on the receiving side.
   scoped_ptr<const CandidateSessionConfig> candidate_config_;
 
-  // |control_channel_| holds a channel until SSL socket is
-  // created. After that |control_ssl_socket_| owns the channel. The
-  // same is the case fo |event_channel_| and |video_channel_|.
+  // |raw_foo_channel_| is a reference to the Cricket transport channel foo,
+  // which is owned by the Cricket session.
+  // |foo_channel_| owns the Socket adapter for that channel.
+  // |foo_socket_| takes ownership of the Socket from |foo_channel_| and wraps
+  // it with PseudoTcp.  |foo_ssl_socket_| takes ownership from |foo_socket_|
+  // and runs the SSL protocol over it.  Finally |foo_socket_wrapper_| takes
+  // ownership of |foo_ssl_socket_|, wrapping it with a SocketWrapper to work
+  // around issues with callers of the resulting Sockets continuing to access
+  // them after CloseInternal() has completed.
+  // TODO(wez): Fix the tear-down issue and remove SocketWrapper.
+
   cricket::TransportChannel* raw_control_channel_;
   scoped_ptr<jingle_glue::TransportChannelSocketAdapter> control_channel_;
-  scoped_ptr<SocketWrapper> control_ssl_socket_;
+  scoped_ptr<net::StreamSocket> control_socket_;
+  scoped_ptr<net::StreamSocket> control_ssl_socket_;
+  scoped_ptr<SocketWrapper> control_socket_wrapper_;
 
   cricket::TransportChannel* raw_event_channel_;
   scoped_ptr<jingle_glue::TransportChannelSocketAdapter> event_channel_;
-  scoped_ptr<SocketWrapper> event_ssl_socket_;
+  scoped_ptr<net::StreamSocket> event_socket_;
+  scoped_ptr<net::StreamSocket> event_ssl_socket_;
+  scoped_ptr<SocketWrapper> event_socket_wrapper_;
 
   cricket::TransportChannel* raw_video_channel_;
   scoped_ptr<jingle_glue::TransportChannelSocketAdapter> video_channel_;
-  scoped_ptr<SocketWrapper> video_ssl_socket_;
-
-  // Count the number of SSL connections esblished.
-  int ssl_connections_;
+  scoped_ptr<net::StreamSocket> video_socket_;
+  scoped_ptr<net::StreamSocket> video_ssl_socket_;
+  scoped_ptr<SocketWrapper> video_socket_wrapper_;
 
   // Used to verify the certificate received in SSLClientSocket.
   scoped_ptr<net::CertVerifier> cert_verifier_;