Fix SSLSocketAdapter to handle asynchronous writes appropriately.

remoting/jingle_glue/ssl_socket_adapter.cc

Index: remoting/jingle_glue/ssl_socket_adapter.cc
diff --git a/remoting/jingle_glue/ssl_socket_adapter.cc b/remoting/jingle_glue/ssl_socket_adapter.cc
index b6b530dc5e367d45c54d90fb28ea8a9fe9097201..32afec09175284a5afa8f036fb773f9a8cef2b06 100644
--- a/remoting/jingle_glue/ssl_socket_adapter.cc
+++ b/remoting/jingle_glue/ssl_socket_adapter.cc
@@ -28,8 +28,9 @@ SSLSocketAdapter::SSLSocketAdapter(AsyncSocket* socket)
       cert_verifier_(net::CertVerifier::CreateDefault()),
       ssl_state_(SSLSTATE_NONE),
       read_state_(IOSTATE_NONE),
-      write_state_(IOSTATE_NONE),
-      data_transferred_(0) {
+      read_buffer_size_(0),
+      read_buffer_position_(0),
+      write_state_(IOSTATE_NONE) {
   transport_socket_ = new TransportSocket(socket, this);
 }
 
@@ -83,20 +84,24 @@ int SSLSocketAdapter::BeginSSL() {
 }
 
 int SSLSocketAdapter::Send(const void* buf, size_t len) {
-  if (ssl_state_ != SSLSTATE_CONNECTED) {
+  if (ssl_state_ == SSLSTATE_ERROR) {
+    SetError(EINVAL);

[Wez, 2012/06/11 21:21:35]

You're avoiding just returning whatever error caused SSLSTATE_ERROR because it
might not be something that makes sense for a Send() call to return?

[Sergey Ulanov, 2012/06/11 23:06:39]

Correct. Send() returns one of the error codes defined in net/base/net_errors.h.
We would have to convert it to POSIX error code.

+    return -1;
+  } else if (ssl_state_ != SSLSTATE_CONNECTED) {

[Wez, 2012/06/11 21:21:35]

nit: Using "else" here makes this harder to read.

[Sergey Ulanov, 2012/06/11 23:06:39]

Done.

     return AsyncSocketAdapter::Send(buf, len);

[Wez, 2012/06/11 21:21:35]

Add a comment to explain why we propagate the Send() to the underlying
implementation if we're not connected yet.

[Sergey Ulanov, 2012/06/11 23:06:39]

Done.

-  } else {
-    scoped_refptr<net::IOBuffer> transport_buf(new net::IOBuffer(len));
-    memcpy(transport_buf->data(), buf, len);
+  }
 
-    int result = ssl_socket_->Write(transport_buf, len,
-                                    net::CompletionCallback());
-    if (result == net::ERR_IO_PENDING) {
-      SetError(EWOULDBLOCK);
-    }
-    transport_buf = NULL;
-    return result;
+  if (write_state_ == IOSTATE_PENDING) {
+    SetError(EWOULDBLOCK);
+    return -1;
   }
+
+  write_buffer_ = new net::DrainableIOBuffer(new net::IOBuffer(len), len);
+  memcpy(write_buffer_->data(), buf, len);
+
+  DoWrite();
+
+  return len;
 }
 
 int SSLSocketAdapter::Recv(void* buf, size_t len) {
@@ -111,12 +116,12 @@ int SSLSocketAdapter::Recv(void* buf, size_t len) {
     case SSLSTATE_CONNECTED:
       switch (read_state_) {
         case IOSTATE_NONE: {
-          transport_buf_ = new net::IOBuffer(len);
+          read_buffer_ = new net::IOBuffer(len);
           int result = ssl_socket_->Read(
-              transport_buf_, len,
+              read_buffer_, len,
               base::Bind(&SSLSocketAdapter::OnRead, base::Unretained(this)));
           if (result >= 0) {
-            memcpy(buf, transport_buf_->data(), len);
+            memcpy(buf, read_buffer_->data(), len);
           }
 
           if (result == net::ERR_IO_PENDING) {
@@ -124,10 +129,11 @@ int SSLSocketAdapter::Recv(void* buf, size_t len) {
             SetError(EWOULDBLOCK);
           } else {
             if (result < 0) {
-              SetError(result);
-              VLOG(1) << "Socket error " << result;
+              SetError(EINVAL);
+              ssl_state_ = SSLSTATE_ERROR;
+              LOG(ERROR) << "Error when reading from SSL socket " << result;

[Wez, 2012/06/11 21:21:35]

nit: not need for "when" here

[Sergey Ulanov, 2012/06/11 23:06:39]

Done.

             }
-            transport_buf_ = NULL;
+            read_buffer_ = NULL;
           }
           return result;
         }
@@ -136,11 +142,20 @@ int SSLSocketAdapter::Recv(void* buf, size_t len) {
           return -1;
 
         case IOSTATE_COMPLETE:
-          memcpy(buf, transport_buf_->data(), len);
-          transport_buf_ = NULL;
-          read_state_ = IOSTATE_NONE;
-          return data_transferred_;
+          int size = std::min(read_buffer_size_ - read_buffer_position_,
+                              static_cast<int>(len));

[Wez, 2012/06/11 21:21:35]

You'll need {} in this case, since you're declaring case-scoped variables.

[Wez, 2012/06/11 21:21:35]

nit: If len > read_buffer_size_ - read_buffer_position_ then we should ideally
schedule a Read() to get more data, rather than wait til we hit the end of the
current buffer, to maximize throughput.

[Sergey Ulanov, 2012/06/11 23:06:39]

Done.

+          memcpy(buf, read_buffer_->data() + read_buffer_position_, size);
+          read_buffer_position_ += size;

[Wez, 2012/06/11 21:21:35]

Why use a buffer position rather than creating a DrainableIOBuffer when we first
hit the IOSTATE_COMPLETE state?

[Sergey Ulanov, 2012/06/11 23:06:39]

Drainable buffer doesn't allow truncating it. Replaced it with GrowableIOBuffer.

+          if (read_buffer_position_ == read_buffer_size_) {
+            read_buffer_ = NULL;
+            read_state_ = IOSTATE_NONE;
+          }
+          return size;
       }
+
+    case SSLSTATE_ERROR:
+      SetError(EINVAL);
+      return -1;
   }
 
   NOTREACHED();
@@ -158,18 +173,51 @@ void SSLSocketAdapter::OnConnected(int result) {
 
 void SSLSocketAdapter::OnRead(int result) {
   DCHECK(read_state_ == IOSTATE_PENDING);
-  read_state_ = IOSTATE_COMPLETE;
-  data_transferred_ = result;
+  if (result > 0) {
+    read_state_ = IOSTATE_COMPLETE;
+    read_buffer_size_ = result;
+    read_buffer_position_ = 0;
+  } else {
+    read_state_ = IOSTATE_NONE;
+    if (result < 0) {
+      ssl_state_ = SSLSTATE_ERROR;

[Wez, 2012/06/11 21:21:35]

nit: It's confusing that you have read_state_, write_state_ and ssl_state_, but
that ssl_state_ is the only one that contains the ERROR flag for all three...

[Sergey Ulanov, 2012/06/11 23:06:39]

Repalaced write_state_ and read_state_ with write_pending_ and read_pending_
flags.

+    }
+  }
   AsyncSocketAdapter::OnReadEvent(this);
 }
 
-void SSLSocketAdapter::OnWrite(int result) {
+void SSLSocketAdapter::OnWritten(int result) {
   DCHECK(write_state_ == IOSTATE_PENDING);
   write_state_ = IOSTATE_COMPLETE;
-  data_transferred_ = result;
+  if (result >= 0) {
+    write_buffer_->DidConsume(result);
+    if (!write_buffer_->BytesRemaining()) {
+      write_buffer_ = NULL;
+    } else {
+      DoWrite();
+    }
+  } else {
+    ssl_state_ = SSLSTATE_ERROR;
+  }
   AsyncSocketAdapter::OnWriteEvent(this);
 }
 
+void SSLSocketAdapter::DoWrite() {
+  DCHECK_GT(write_buffer_->BytesRemaining(), 0);

[Wez, 2012/06/11 21:21:35]

nit: DCHECK |write_state_| is not IO_PENDING?

[Sergey Ulanov, 2012/06/11 23:06:39]

Done.

+  int result = ssl_socket_->Write(
+      write_buffer_, write_buffer_->BytesRemaining(),
+      base::Bind(&SSLSocketAdapter::OnWritten, base::Unretained(this)));
+
+  if (result == net::ERR_IO_PENDING) {
+    write_state_ = IOSTATE_PENDING;
+  } else if (result < 0) {
+    SetError(EINVAL);
+    ssl_state_ = SSLSTATE_ERROR;
+  } else {
+    write_buffer_ = NULL;
+  }
+}
+
 void SSLSocketAdapter::OnConnectEvent(talk_base::AsyncSocket* socket) {
   if (ssl_state_ != SSLSTATE_WAIT) {
     AsyncSocketAdapter::OnConnectEvent(socket);