BufferSend needs to call memio_GetWriteParams() and transport_->Write()

twice because the circular memio buffer may have two contiguous parts of
data.

In the SSLClientSocket unit tests, we should compare the result of
sock->Write() with the expected value whether it completes synchronously
or asynchronously.

R=dank
BUG=29815
TEST=a new unit test that issue a hanging Read and then issue a Write.
Review URL: http://codereview.chromium.org/464082

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

2 files changed, 95 insertions, 23 deletions

diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 4968823..5c95ee0 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -747,25 +747,35 @@ bool SSLClientSocketNSS::DoTransportIO() {
 int SSLClientSocketNSS::BufferSend(void) {
   if (transport_send_busy_) return ERR_IO_PENDING;
 
-  const char *buf;
-  int nb = memio_GetWriteParams(nss_bufs_, &buf);
-  EnterFunction(nb);
+  int nsent = 0;
+  EnterFunction("");
+  // nss_bufs_ is a circular buffer.  It may have two contiguous parts
+  // (before and after the wrap).  So this for loop needs two iterations.
+  for (int i = 0; i < 2; ++i) {
+    const char* buf;
+    int nb = memio_GetWriteParams(nss_bufs_, &buf);
+    if (!nb)
+      break;
 
-  int rv;
-  if (!nb) {
-    rv = OK;
-  } else {
     scoped_refptr<IOBuffer> send_buffer = new IOBuffer(nb);
     memcpy(send_buffer->data(), buf, nb);
-    rv = transport_->Write(send_buffer, nb, &buffer_send_callback_);
-    if (rv == ERR_IO_PENDING)
+    int rv = transport_->Write(send_buffer, nb, &buffer_send_callback_);
+    if (rv == ERR_IO_PENDING) {
       transport_send_busy_ = true;
-    else
+      break;
+    } else {
       memio_PutWriteResult(nss_bufs_, MapErrorToNSS(rv));
+      if (rv < 0) {
+        // Return the error even if the previous Write succeeded.
+        nsent = rv;
+        break;
+      }
+      nsent += rv;
+    }
   }
 
-  LeaveFunction(rv);
-  return rv;
+  LeaveFunction(nsent);
+  return nsent;
 }
 
 void SSLClientSocketNSS::BufferSendComplete(int result) {
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index e360136..2fca322 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -237,10 +237,9 @@ TEST_F(SSLClientSocketTest, Read) {
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
-  if (rv == net::ERR_IO_PENDING) {
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
-  }
+  EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
 
   scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(4096);
   for (;;) {
@@ -256,6 +255,71 @@ TEST_F(SSLClientSocketTest, Read) {
   }
 }
 
+// Test the full duplex mode, with Read and Write pending at the same time.
+// This test also serves as a regression test for http://crbug.com/29815.
+TEST_F(SSLClientSocketTest, Read_FullDuplex) {
+  StartOKServer();
+
+  net::AddressList addr;
+  TestCompletionCallback callback;  // Used for everything except Write.
+  TestCompletionCallback callback2;  // Used for Write only.
+
+  net::HostResolver::RequestInfo info(server_.kHostName, server_.kOKHTTPSPort);
+  int rv = resolver_->Resolve(info, &addr, &callback, NULL, NULL);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  net::ClientSocket *transport = new net::TCPClientSocket(addr);
+  rv = transport->Connect(&callback, NULL);
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  scoped_ptr<net::SSLClientSocket> sock(
+      socket_factory_->CreateSSLClientSocket(transport,
+                                             server_.kHostName,
+                                             kDefaultSSLConfig));
+
+  rv = sock->Connect(&callback, NULL);
+  if (rv != net::OK) {
+    ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+    rv = callback.WaitForResult();
+    EXPECT_EQ(net::OK, rv);
+  }
+  EXPECT_TRUE(sock->IsConnected());
+
+  // Issue a "hanging" Read first.
+  scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(4096);
+  rv = sock->Read(buf, 4096, &callback);
+  // We haven't written the request, so there should be no response yet.
+  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+
+  // Write the request.
+  // The request is padded with a User-Agent header to a size that causes the
+  // memio circular buffer (4k bytes) in SSLClientSocketNSS to wrap around.
+  // This tests the fix for http://crbug.com/29815.
+  std::string request_text = "GET / HTTP/1.1\r\nUser-Agent: long browser name ";
+  for (int i = 0; i < 3800; ++i)
+    request_text.push_back('*');
+  request_text.append("\r\n\r\n");
+  scoped_refptr<net::IOBuffer> request_buffer =
+      new net::StringIOBuffer(request_text);
+
+  rv = sock->Write(request_buffer, request_text.size(), &callback2);
+  EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
+
+  if (rv == net::ERR_IO_PENDING)
+    rv = callback2.WaitForResult();
+  EXPECT_EQ(static_cast<int>(request_text.size()), rv);
+
+  // Now get the Read result.
+  rv = callback.WaitForResult();
+  EXPECT_GT(rv, 0);
+}
+
 TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   StartOKServer();
 
@@ -292,10 +356,9 @@ TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
-  if (rv == net::ERR_IO_PENDING) {
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
-  }
+  EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
 
   scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(1);
   for (;;) {
@@ -347,18 +410,17 @@ TEST_F(SSLClientSocketTest, Read_Interrupted) {
   rv = sock->Write(request_buffer, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
-  if (rv == net::ERR_IO_PENDING) {
+  if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
-    EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
-  }
+  EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
 
   // Do a partial read and then exit.  This test should not crash!
   scoped_refptr<net::IOBuffer> buf = new net::IOBuffer(512);
   rv = sock->Read(buf, 512, &callback);
-  EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
+  EXPECT_TRUE(rv > 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
 
-  EXPECT_NE(rv, 0);
+  EXPECT_GT(rv, 0);
 }