Fix SSLSocketAdapter to handle asynchronous writes appropriately.

remoting/jingle_glue/ssl_socket_adapter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/jingle_glue/ssl_socket_adapter.h"
 
 #include "base/base64.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
 #include "jingle/glue/utils.h"
@@ skipping 10 matching lines @@
 SSLSocketAdapter* SSLSocketAdapter::Create(AsyncSocket* socket) {
   return new SSLSocketAdapter(socket);
 }
 
 SSLSocketAdapter::SSLSocketAdapter(AsyncSocket* socket)
     : SSLAdapter(socket),
       ignore_bad_cert_(false),
       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);
 }
 
 SSLSocketAdapter::~SSLSocketAdapter() {
 }
 
 int SSLSocketAdapter::StartSSL(const char* hostname, bool restartable) {
   DCHECK(!restartable);
   hostname_ = hostname;
 
@@ skipping 33 matching lines @@
 
   if (result == net::ERR_IO_PENDING || result == net::OK) {
     return 0;
   } else {
     LOG(ERROR) << "Could not start SSL: " << net::ErrorToString(result);
     return result;
   }
 }
 
 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) {
   switch (ssl_state_) {
     case SSLSTATE_NONE:
       return AsyncSocketAdapter::Recv(buf, len);
 
     case SSLSTATE_WAIT:
       SetError(EWOULDBLOCK);
       return -1;
 
     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) {
             read_state_ = IOSTATE_PENDING;
             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;
         }
         case IOSTATE_PENDING:
           SetError(EWOULDBLOCK);
           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();
   return -1;
 }
 
 void SSLSocketAdapter::OnConnected(int result) {
   if (result == net::OK) {
     ssl_state_ = SSLSTATE_CONNECTED;
     OnConnectEvent(this);
   } else {
     LOG(WARNING) << "OnConnected failed with error " << 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);
   } else {
     ssl_state_ = SSLSTATE_NONE;
     int result = BeginSSL();
     if (0 != result) {
       // TODO(zork): Handle this case gracefully.
       LOG(WARNING) << "BeginSSL() failed with " << result;
     }
@@ skipping 185 matching lines @@
         write_buffer_len_ = buffer_len;
         return;
       }
     }
     was_used_to_convey_data_ = true;
     callback.Run(result);
   }
 }
 
 }  // namespace remoting