Add Socket::ReadIfReady()

net/socket/ssl_client_socket_unittest.cc

Index: net/socket/ssl_client_socket_unittest.cc
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 3b1f266032ea7d1307b8ac91dd57d57f54df8365..13acfb53c61fd8eb9d98847db9d037d1fdc3ccd5 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <errno.h>
 #include <string.h>
+#include <algorithm>

[davidben, 2017/02/10 23:33:48]

Nit: Swap this and the blank line below.

[xunjieli, 2017/02/13 20:28:18]

Done.

 
 #include <utility>
 
@@ -17,6 +18,8 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/stl_util.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_scheduler.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ -142,6 +145,11 @@ class WrappedStreamSocket : public StreamSocket {
            const CompletionCallback& callback) override {
     return transport_->Read(buf, buf_len, callback);
   }
+  int ReadIfReady(IOBuffer* buf,
+                  int buf_len,
+                  const CompletionCallback& callback) override {
+    return transport_->ReadIfReady(buf, buf_len, callback);
+  }
   int Write(IOBuffer* buf,
             int buf_len,
             const CompletionCallback& callback) override {
@@ -174,6 +182,10 @@ class ReadBufferingStreamSocket : public WrappedStreamSocket {
            int buf_len,
            const CompletionCallback& callback) override;
 
+  int ReadIfReady(IOBuffer* buf,
+                  int buf_len,
+                  const CompletionCallback& callback) override;
+
   // Sets the internal buffer to |size|. This must not be greater than
   // the largest value supplied to Read() - that is, it does not handle
   // having "leftovers" at the end of Read().
@@ -210,7 +222,7 @@ ReadBufferingStreamSocket::ReadBufferingStreamSocket(
       buffer_size_(0) {}
 
 void ReadBufferingStreamSocket::SetBufferSize(int size) {
-  DCHECK(!user_read_buf_.get());
+  DCHECK(!user_read_buf_);
   buffer_size_ = size;
   read_buffer_->SetCapacity(size);
 }
@@ -218,20 +230,36 @@ void ReadBufferingStreamSocket::SetBufferSize(int size) {
 int ReadBufferingStreamSocket::Read(IOBuffer* buf,
                                     int buf_len,
                                     const CompletionCallback& callback) {
+  DCHECK(!user_read_buf_);
   if (buffer_size_ == 0)
     return transport_->Read(buf, buf_len, callback);
+  int rv = ReadIfReady(buf, buf_len, callback);
+  if (rv == ERR_IO_PENDING)
+    user_read_buf_ = buf;
+  return rv;
+}
+
+int ReadBufferingStreamSocket::ReadIfReady(IOBuffer* buf,
+                                           int buf_len,
+                                           const CompletionCallback& callback) {
+  DCHECK(!user_read_buf_);
+  if (buffer_size_ == 0)
+    return transport_->ReadIfReady(buf, buf_len, callback);
+
+  if (read_buffer_->RemainingCapacity() == 0) {
+    memcpy(buf->data(), read_buffer_->StartOfBuffer(),
+           read_buffer_->capacity());
+    read_buffer_->set_offset(0);
+    return read_buffer_->capacity();
+  }
 
   if (buf_len < buffer_size_)
     return ERR_UNEXPECTED;
-
   state_ = STATE_READ;
-  user_read_buf_ = buf;
-  int result = DoLoop(OK);
-  if (result == ERR_IO_PENDING)
+  int rv = DoLoop(OK);
+  if (rv == ERR_IO_PENDING)
     user_read_callback_ = callback;
-  else
-    user_read_buf_ = NULL;
-  return result;
+  return rv;
 }
 
 int ReadBufferingStreamSocket::DoLoop(int result) {
@@ -258,16 +286,15 @@ int ReadBufferingStreamSocket::DoLoop(int result) {
 
 int ReadBufferingStreamSocket::DoRead() {
   state_ = STATE_READ_COMPLETE;
-  int rv =
-      transport_->Read(read_buffer_.get(),
-                       read_buffer_->RemainingCapacity(),
-                       base::Bind(&ReadBufferingStreamSocket::OnReadCompleted,
-                                  base::Unretained(this)));
-  return rv;
+  return transport_->Read(
+      read_buffer_.get(), read_buffer_->RemainingCapacity(),
+      base::Bind(&ReadBufferingStreamSocket::OnReadCompleted,
+                 base::Unretained(this)));
 }
 
 int ReadBufferingStreamSocket::DoReadComplete(int result) {
   state_ = STATE_NONE;
+
   if (result <= 0)
     return result;
 
@@ -277,6 +304,10 @@ int ReadBufferingStreamSocket::DoReadComplete(int result) {
     return OK;
   }
 
+  // If ReadIfReady() is used.
+  if (user_read_buf_ == nullptr)
+    return OK;
+
   memcpy(user_read_buf_->data(),
          read_buffer_->StartOfBuffer(),
          read_buffer_->capacity());
@@ -285,11 +316,13 @@ int ReadBufferingStreamSocket::DoReadComplete(int result) {
 }
 
 void ReadBufferingStreamSocket::OnReadCompleted(int result) {
+  DCHECK_NE(ERR_IO_PENDING, result);
+  DCHECK(user_read_callback_);
+
   result = DoLoop(result);
   if (result == ERR_IO_PENDING)
     return;
-
-  user_read_buf_ = NULL;
+  user_read_buf_ = nullptr;
   base::ResetAndReturn(&user_read_callback_).Run(result);
 }
 
@@ -304,6 +337,9 @@ class SynchronousErrorStreamSocket : public WrappedStreamSocket {
   int Read(IOBuffer* buf,
            int buf_len,
            const CompletionCallback& callback) override;
+  int ReadIfReady(IOBuffer* buf,
+                  int buf_len,
+                  const CompletionCallback& callback) override;
   int Write(IOBuffer* buf,
             int buf_len,
             const CompletionCallback& callback) override;
@@ -346,6 +382,15 @@ int SynchronousErrorStreamSocket::Read(IOBuffer* buf,
   return transport_->Read(buf, buf_len, callback);
 }
 
+int SynchronousErrorStreamSocket::ReadIfReady(
+    IOBuffer* buf,
+    int buf_len,
+    const CompletionCallback& callback) {
+  if (have_read_error_)
+    return pending_read_error_;
+  return transport_->ReadIfReady(buf, buf_len, callback);
+}
+
 int SynchronousErrorStreamSocket::Write(IOBuffer* buf,
                                         int buf_len,
                                         const CompletionCallback& callback) {
@@ -368,6 +413,9 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   int Read(IOBuffer* buf,
            int buf_len,
            const CompletionCallback& callback) override;
+  int ReadIfReady(IOBuffer* buf,
+                  int buf_len,
+                  const CompletionCallback& callback) override;
   int Write(IOBuffer* buf,
             int buf_len,
             const CompletionCallback& callback) override;
@@ -411,6 +459,13 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   // True if read callbacks are blocked.
   bool should_block_read_ = false;
 
+  // Used to buffer result returned by a synchronously completed ReadIfReady()
+  // which needs to be blocked.
+  std::string blocked_sync_read_if_ready_data_;

[davidben, 2017/02/10 23:33:48]

Hah! Writing into a buffer like that hadn't occured to me. Good idea.

[xunjieli, 2017/02/13 20:28:18]

Acknowledged.

+  // True if a synchronous ReadIfReady() has been blocked. Reset to false when
+  // the data from the blocked sync ReadIfReady() is consumed.
+  bool has_blocked_sync_read_if_ready_ = false;
+
   // The buffer for the pending read, or NULL if not consumed.
   scoped_refptr<IOBuffer> pending_read_buf_;
 
@@ -451,8 +506,9 @@ int FakeBlockingStreamSocket::Read(IOBuffer* buf,
   DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
   DCHECK(!callback.is_null());
 
-  int rv = transport_->Read(buf, len, base::Bind(
-      &FakeBlockingStreamSocket::OnReadCompleted, base::Unretained(this)));
+  int rv = transport_->Read(
+      buf, len, base::Bind(&FakeBlockingStreamSocket::OnReadCompleted,
+                           base::Unretained(this)));
   if (rv == ERR_IO_PENDING || should_block_read_) {
     // Save the callback to be called later.
     pending_read_buf_ = buf;
@@ -467,6 +523,60 @@ int FakeBlockingStreamSocket::Read(IOBuffer* buf,
   return rv;
 }
 
+int FakeBlockingStreamSocket::ReadIfReady(IOBuffer* buf,
+                                          int len,
+                                          const CompletionCallback& callback) {
+  if (has_blocked_sync_read_if_ready_) {

[davidben, 2017/02/10 23:33:48]

I think this doesn't quite work if should_block_read_ is true. Consider this
scenario:

  BlockReadResult()
  ReadIfReady(10) => get 10 bytes, but should_block_read_ so it goes to a
buffer.
  UnblockReadResult() => ReadIfReady completes with OK.
  ReadIfReady(5) => okay, we consume 5 bytes from the stashed buffer.
  BlockReadResult()
  ReadIfReady(5) => we have bytes in the stashed buffer, but need to resolve it
later.

But this is also a test socket and I think we'd never do that? We can probably
just stick a bunch of CHECKs and simplify this. See comment below.

[xunjieli, 2017/02/13 20:28:18]

Acknowledged.

+    DCHECK(!pending_read_callback_);
+    DCHECK(pending_read_buf_);
+    DCHECK_NE(ERR_IO_PENDING, pending_read_result_);
+    if (pending_read_result_ > 0) {
+      DCHECK_LE(0, pending_read_buf_len_);
+      int bytes_read = std::min(len, pending_read_buf_len_);
+      memcpy(buf->data(), pending_read_buf_->data(), bytes_read);
+      blocked_sync_read_if_ready_data_.erase(0, bytes_read);
+      if (!blocked_sync_read_if_ready_data_.empty()) {
+        pending_read_buf_ =
+            new StringIOBuffer(blocked_sync_read_if_ready_data_);
+        pending_read_buf_len_ = blocked_sync_read_if_ready_data_.length();
+        pending_read_result_ = pending_read_buf_len_;
+        return bytes_read;
+      }
+    }
+    int rv = pending_read_result_;
+    has_blocked_sync_read_if_ready_ = false;
+    pending_read_buf_ = nullptr;
+    pending_read_buf_len_ = -1;
+    pending_read_result_ = ERR_IO_PENDING;
+    return rv;
+  }
+  DCHECK(!pending_read_buf_);
+  DCHECK(pending_read_callback_.is_null());
+  DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
+  DCHECK(!callback.is_null());
+
+  int rv = transport_->ReadIfReady(
+      buf, len, base::Bind(&FakeBlockingStreamSocket::OnReadCompleted,
+                           base::Unretained(this)));
+  if (rv == ERR_IO_PENDING) {
+    pending_read_callback_ = callback;
+    return rv;
+  }
+  if (should_block_read_) {
+    // Save the data to be called later.
+    if (rv >= 0)
+      blocked_sync_read_if_ready_data_.append(buf->data(), rv);
+    has_blocked_sync_read_if_ready_ = true;
+    // Save the callback to be called later.
+    pending_read_buf_ = new StringIOBuffer(blocked_sync_read_if_ready_data_);

[davidben, 2017/02/10 23:33:48]

Do you need both the std::string and the StringIOBuffer? This looks a little
funny. StringIOBuffer makes a copy of its input, so if you modify things later
with ReplaceReadResult, it doesn't actually notice.

I like the idea to redirect it to another buffer though. What if we implemented
ReadIfReady using Read?

  CompletionCallback read_if_ready_callback_;
  string read_if_ready_buf_;

int FBSS::ReadIfReady(buf, len, callback) {
  if (!read_if_ready_buf_.empty()) {
    // This is possible if the caller calls ReadIfReady with a smaller
    // buffer size than previously or if BlockReadResult is called
    // after ReadIfReady reports there is data available but before the
    // data is drained.
    CHECK(!should_block_read_);
    CHECK(len >= read_if_ready_buf_.size());

    int rv = read_if_read_buf_.size();
    memcpy(buf->data(), read_if_read_buf_.data(), rv);
    read_if_ready_buf_.clear();
    return rv;
  }

  buf_copy = new IOBuffer(len);
  int rv = Read(read_if_ready_buf_, len, Bind(CompleteReadIfReady, buf_copy));
  if (rv > 0)
    memcpy(buf->data(), buf_copy->data(), rv);
  if (rv == ERR_IO_PENDING)
    read_if_ready_callback_ = callback;
  return rv;
}

void FBSS::CompleteReadIfReady(IOBuffer* buf, int rv) {
  DCHECK(read_if_ready_buf_.empty());
  DCHECK(!should_block_read_);

  if (rv > 0) {
    read_if_ready_buf_ = string(buf->data(), buf->data() + rv);
    rv = OK;
  }

  ResetAndReturn(read_if_ready_callback_, rv);
}

(If one of those CHECKs crashes, you might need to make this a little more
complicated. Notably the read_if_ready_buf_ + should_block_read_ case will
probably require UnblockReadResult to be aware of read_if_ready_callback_
whereas right now should_block_read_ is almost entirely hidden from
ReadIfReady.)

[xunjieli, 2017/02/13 20:28:19]

Done. Neat! I somehow though StringIOBuffer doesn't own the underlying buffer or
do a string copy

+    pending_read_buf_len_ = len;
+    pending_read_callback_ = callback;
+    OnReadCompleted(rv);
+    rv = ERR_IO_PENDING;
+  }
+  return rv;
+}
+
 int FakeBlockingStreamSocket::Write(IOBuffer* buf,
                                     int len,
                                     const CompletionCallback& callback) {
@@ -507,13 +617,18 @@ void FakeBlockingStreamSocket::UnblockReadResult() {
 
 bool FakeBlockingStreamSocket::ReplaceReadResult(const std::string& data) {
   DCHECK(should_block_read_);
-  DCHECK_NE(ERR_IO_PENDING, pending_read_result_);
   DCHECK(pending_read_buf_);
   DCHECK_NE(-1, pending_read_buf_len_);
 
   if (static_cast<size_t>(pending_read_buf_len_) < data.size())
     return false;
 
+  if (has_blocked_sync_read_if_ready_)
+    blocked_sync_read_if_ready_data_ = data;
+
+  DCHECK_NE(ERR_IO_PENDING, pending_read_result_);
+  DCHECK(pending_read_buf_);
+  DCHECK_NE(-1, pending_read_buf_len_);
   memcpy(pending_read_buf_->data(), data.data(), data.size());
   pending_read_result_ = data.size();
   return true;
@@ -586,6 +701,10 @@ void FakeBlockingStreamSocket::OnReadCompleted(int result) {
 
 void FakeBlockingStreamSocket::ReturnReadResult() {
   int result = pending_read_result_;
+  if (has_blocked_sync_read_if_ready_) {
+    base::ResetAndReturn(&pending_read_callback_).Run(result > 0 ? OK : result);
+    return;
+  }
   pending_read_result_ = ERR_IO_PENDING;
   pending_read_buf_ = nullptr;
   pending_read_buf_len_ = -1;
@@ -858,6 +977,53 @@ class SSLClientSocketTest : public PlatformTest {
   AddressList addr_;
 };
 
+// If GetParam(), try ReadIfReady() and fall back to Read() if needed.
+class SSLClientSocketReadTest : public SSLClientSocketTest,
+                                public ::testing::WithParamInterface<bool> {
+ protected:
+  void SetUp() override {
+    if (GetParam())
+      scoped_feature_list_.InitAndEnableFeature(Socket::kReadIfReadyExperiment);
+  }
+
+  // Convienient wrapper to call Read() or ReadIfReady() depending on
+  // GetParam().
+  int Read(StreamSocket* socket,
+           IOBuffer* buf,
+           int buf_len,
+           const CompletionCallback& callback) {
+    if (GetParam())
+      return socket->ReadIfReady(buf, buf_len, callback);
+    return socket->Read(buf, buf_len, callback);
+  }
+
+  // Calls Read()/ReadIfReady() and waits for it to return data.
+  int ReadAndWaitForCompletion(StreamSocket* socket,
+                               IOBuffer* buf,
+                               int buf_len) {
+    TestCompletionCallback callback;
+    if (GetParam()) {
+      int rv = socket->ReadIfReady(buf, buf_len, callback.callback());
+      if (rv == ERR_IO_PENDING) {
+        rv = callback.GetResult(rv);
+        if (rv == OK) {
+          rv = socket->ReadIfReady(buf, buf_len, callback.callback());
+          DCHECK_NE(ERR_IO_PENDING, rv);
+        }
+      }
+      return rv;

[davidben, 2017/02/10 23:33:48]

It's legal for ReadIfReady to signal OK before it's actually ready to complete.
(SSLClientSocketImpl will do this since we don't keep perfect track of what is
blocking on what.) I suspect this needs to be a loop to avoid flakiness.
Something like:

  int rv;
  do {
    int rv = socket->ReadIfReady(...);
    if (rv != ERR_IO_PENDING)
      break;
    rv = callback.GetResult(rv);
  } while (rv == OK);

-----

Edit: Actually, I see you sometimes split things up above. You might need
something like:

int Read(StreamSocket* socket, IOBuffer* buf, int buf_len,
         TestCompletionCallback* callback) {
  if (GetParam())
    return socket->ReadIfReady(buf, buf_len, callback->callback());
  return socket->Read(buf, buf_len, callback->callback());
}

int WaitForReadCompletion(StreamSocket* socket, IOBuffer* buf, int buf_len,
                          TestCompletionCallback* callback, int rv) {
  if (!GetParam())
    return callback->GetResult(rv);

  while (rv == ERR_IO_PENDING) {
    rv = callback->GetResult(rv);
    if (rv != OK)
      return rv;
    rv = socket->ReadIfReady(buf, buf_len, callback.callback())
  }

  return rv;
}

int ReadAndWaitForCompletion(StreamSocket* socket,
                             IOBuffer* buf,
                             int buf_len) {
  TestCompletionCallback callback;
  int rv = Read(socket, buf, buf_len, &callback);
  return WaitForReadCompletion(socket, buf, buf_len, &callback, rv);
}

[xunjieli, 2017/02/13 20:28:18]

Done.

+    }
+    return callback.GetResult(socket->Read(buf, buf_len, callback.callback()));
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+INSTANTIATE_TEST_CASE_P(/* no prefix */,
+                        SSLClientSocketReadTest,
+                        ::testing::Bool());
+
 // Verifies the correctness of GetSSLCertRequestInfo.
 class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
  protected:
@@ -1206,7 +1372,7 @@ TEST_F(SSLClientSocketTest, ConnectClientAuthSendNullCert) {
 
 // Tests that the socket can be read from successfully. Also test that a peer's
 // close_notify alert is successfully processed without error.
-TEST_F(SSLClientSocketTest, Read) {
+TEST_P(SSLClientSocketReadTest, Read) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1242,7 +1408,7 @@ TEST_F(SSLClientSocketTest, Read) {
   int64_t unencrypted_bytes_read = 0;
   int64_t network_bytes_read_during_handshake = sock->GetTotalReceivedBytes();
   do {
-    rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+    rv = ReadAndWaitForCompletion(sock.get(), buf.get(), 4096);
     EXPECT_GE(rv, 0);
     if (rv >= 0) {
       unencrypted_bytes_read += rv;
@@ -1293,7 +1459,7 @@ TEST_F(SSLClientSocketTest, Connect_WithSynchronousError) {
 // synchronously returns an error code - such as if an intermediary terminates
 // the socket connection uncleanly.
 // This is a regression test for http://crbug.com/238536
-TEST_F(SSLClientSocketTest, Read_WithSynchronousError) {
+TEST_P(SSLClientSocketReadTest, Read_WithSynchronousError) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1335,7 +1501,7 @@ TEST_F(SSLClientSocketTest, Read_WithSynchronousError) {
   // Note: This test will hang if this bug has regressed. Simply checking that
   // rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING is a legitimate
   // result when using a dedicated task runner for NSS.
-  rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+  rv = Read(sock.get(), buf.get(), 4096, callback.callback());
   EXPECT_THAT(rv, IsError(ERR_CONNECTION_RESET));
 }
 
@@ -1470,7 +1636,7 @@ TEST_F(SSLClientSocketTest, Write_WithSynchronousErrorNoRead) {
 
 // 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) {
+TEST_P(SSLClientSocketReadTest, Read_FullDuplex) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -1480,7 +1646,7 @@ TEST_F(SSLClientSocketTest, Read_FullDuplex) {
   // Issue a "hanging" Read first.
   TestCompletionCallback callback;
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  rv = sock_->Read(buf.get(), 4096, callback.callback());
+  rv = Read(sock_.get(), buf.get(), 4096, callback.callback());
   // We haven't written the request, so there should be no response yet.
   ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
 
@@ -1501,6 +1667,10 @@ TEST_F(SSLClientSocketTest, Read_FullDuplex) {
 
   // Now get the Read result.
   rv = callback.WaitForResult();
+  if (GetParam()) {
+    EXPECT_THAT(rv, IsOk());
+    rv = Read(sock_.get(), buf.get(), 4096, callback.callback());

[davidben, 2017/02/10 23:33:48]

Given the issue with ReadIfReady always needing to be driven to completion in a
loop, perhaps we need a helper function for completing a Read|ReadIfReady
result.

(See amended comment above.)

[xunjieli, 2017/02/13 20:28:18]

Done.

+  }
   EXPECT_GT(rv, 0);
 }
 
@@ -1510,7 +1680,7 @@ TEST_F(SSLClientSocketTest, Read_FullDuplex) {
 // Read() and Write() callbacks. If the socket is deleted by the Read()
 // callback, the Write() callback should not be invoked.
 // Regression test for http://crbug.com/232633
-TEST_F(SSLClientSocketTest, Read_DeleteWhilePendingFullDuplex) {
+TEST_P(SSLClientSocketReadTest, Read_DeleteWhilePendingFullDuplex) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1559,7 +1729,7 @@ TEST_F(SSLClientSocketTest, Read_DeleteWhilePendingFullDuplex) {
   SSLClientSocket* raw_sock = sock.get();
   DeleteSocketCallback read_callback(sock.release());
   scoped_refptr<IOBuffer> read_buf(new IOBuffer(4096));
-  rv = raw_sock->Read(read_buf.get(), 4096, read_callback.callback());
+  rv = Read(raw_sock, read_buf.get(), 4096, read_callback.callback());
 
   // Ensure things didn't complete synchronously, otherwise |sock| is invalid.
   ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
@@ -1589,7 +1759,7 @@ TEST_F(SSLClientSocketTest, Read_DeleteWhilePendingFullDuplex) {
 // transport socket after a failing write. This can occur if we have a Write
 // error in a SPDY socket.
 // Regression test for http://crbug.com/335557
-TEST_F(SSLClientSocketTest, Read_WithWriteError) {
+TEST_P(SSLClientSocketReadTest, Read_WithWriteError) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1634,7 +1804,7 @@ TEST_F(SSLClientSocketTest, Read_WithWriteError) {
   TestCompletionCallback read_callback;
   raw_transport->BlockReadResult();
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  rv = sock->Read(buf.get(), 4096, read_callback.callback());
+  rv = Read(sock.get(), buf.get(), 4096, read_callback.callback());
   EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
 
   // Perform another write, but have it fail. Write a request larger than the
@@ -1702,7 +1872,7 @@ TEST_F(SSLClientSocketTest, Connect_WithZeroReturn) {
 // Tests that SSLClientSocket returns a Read of size 0 if the underlying socket
 // is cleanly closed, but the peer does not send close_notify.
 // This is a regression test for https://crbug.com/422246
-TEST_F(SSLClientSocketTest, Read_WithZeroReturn) {
+TEST_P(SSLClientSocketReadTest, Read_WithZeroReturn) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1728,14 +1898,14 @@ TEST_F(SSLClientSocketTest, Read_WithZeroReturn) {
 
   raw_transport->SetNextReadError(0);
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+  rv = ReadAndWaitForCompletion(sock.get(), buf.get(), 4096);
   EXPECT_EQ(0, rv);
 }
 
 // Tests that SSLClientSocket cleanly returns a Read of size 0 if the
 // underlying socket is cleanly closed asynchronously.
 // This is a regression test for https://crbug.com/422246
-TEST_F(SSLClientSocketTest, Read_WithAsyncZeroReturn) {
+TEST_P(SSLClientSocketReadTest, Read_WithAsyncZeroReturn) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1765,17 +1935,22 @@ TEST_F(SSLClientSocketTest, Read_WithAsyncZeroReturn) {
   raw_error_socket->SetNextReadError(0);
   raw_transport->BlockReadResult();
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  rv = sock->Read(buf.get(), 4096, callback.callback());
+  TestCompletionCallback read_callback;
+  rv = Read(sock.get(), buf.get(), 4096, read_callback.callback());
   EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
 
   raw_transport->UnblockReadResult();
-  rv = callback.GetResult(rv);
+  rv = read_callback.GetResult(rv);
+  if (GetParam()) {
+    EXPECT_EQ(OK, rv);
+    rv = Read(sock.get(), buf.get(), 4096, read_callback.callback());
+  }
   EXPECT_EQ(0, rv);
 }
 
 // Tests that fatal alerts from the peer are processed. This is a regression
 // test for https://crbug.com/466303.
-TEST_F(SSLClientSocketTest, Read_WithFatalAlert) {
+TEST_P(SSLClientSocketReadTest, Read_WithFatalAlert) {
   SpawnedTestServer::SSLOptions ssl_options;
   ssl_options.alert_after_handshake = true;
   ASSERT_TRUE(StartTestServer(ssl_options));
@@ -1787,11 +1962,11 @@ TEST_F(SSLClientSocketTest, Read_WithFatalAlert) {
   // Receive the fatal alert.
   TestCompletionCallback callback;
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR, callback.GetResult(sock_->Read(
-                                        buf.get(), 4096, callback.callback())));
+  EXPECT_EQ(ERR_SSL_PROTOCOL_ERROR,
+            ReadAndWaitForCompletion(sock_.get(), buf.get(), 4096));
 }
 
-TEST_F(SSLClientSocketTest, Read_SmallChunks) {
+TEST_P(SSLClientSocketReadTest, Read_SmallChunks) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -1810,12 +1985,12 @@ TEST_F(SSLClientSocketTest, Read_SmallChunks) {
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(1));
   do {
-    rv = callback.GetResult(sock_->Read(buf.get(), 1, callback.callback()));
+    rv = ReadAndWaitForCompletion(sock_.get(), buf.get(), 1);
     EXPECT_GE(rv, 0);
   } while (rv > 0);
 }
 
-TEST_F(SSLClientSocketTest, Read_ManySmallRecords) {
+TEST_P(SSLClientSocketReadTest, Read_ManySmallRecords) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1858,11 +2033,11 @@ TEST_F(SSLClientSocketTest, Read_ManySmallRecords) {
   raw_transport->SetBufferSize(15000);
 
   scoped_refptr<IOBuffer> buffer(new IOBuffer(8192));
-  rv = callback.GetResult(sock->Read(buffer.get(), 8192, callback.callback()));
+  rv = ReadAndWaitForCompletion(sock.get(), buffer.get(), 8192);
   ASSERT_EQ(rv, 8192);
 }
 
-TEST_F(SSLClientSocketTest, Read_Interrupted) {
+TEST_P(SSLClientSocketReadTest, Read_Interrupted) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -1881,11 +2056,11 @@ TEST_F(SSLClientSocketTest, Read_Interrupted) {
 
   // Do a partial read and then exit.  This test should not crash!
   scoped_refptr<IOBuffer> buf(new IOBuffer(512));
-  rv = callback.GetResult(sock_->Read(buf.get(), 512, callback.callback()));
+  rv = ReadAndWaitForCompletion(sock_.get(), buf.get(), 512);
   EXPECT_GT(rv, 0);
 }
 
-TEST_F(SSLClientSocketTest, Read_FullLogging) {
+TEST_P(SSLClientSocketReadTest, Read_FullLogging) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1921,7 +2096,7 @@ TEST_F(SSLClientSocketTest, Read_FullLogging) {
 
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
   for (;;) {
-    rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+    rv = ReadAndWaitForCompletion(sock.get(), buf.get(), 4096);
     EXPECT_GE(rv, 0);
     if (rv <= 0)
       break;
@@ -3593,7 +3768,7 @@ TEST_F(SSLClientSocketTest, AccessDeniedClientCerts) {
 }
 
 // Basic test for dumping memory stats.
-TEST_F(SSLClientSocketTest, DumpMemoryStats) {
+TEST_P(SSLClientSocketReadTest, DumpMemoryStats) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -3609,16 +3784,28 @@ TEST_F(SSLClientSocketTest, DumpMemoryStats) {
   // Read the response without writing a request, so the read will be pending.
   TestCompletionCallback read_callback;
   scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
-  rv = sock_->Read(buf.get(), 4096, read_callback.callback());
+  rv = Read(sock_.get(), buf.get(), 4096, read_callback.callback());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   // Dump memory again and check that |buffer_size| contain the read buffer.
   StreamSocket::SocketMemoryStats stats2;
   sock_->DumpMemoryStats(&stats2);
-  EXPECT_EQ(17 * 1024u, stats2.buffer_size);
+
+  bool buffer_released = false;
+  if (GetParam()) {
+#if defined(OS_POSIX)
+    buffer_released = true;

[davidben, 2017/02/10 23:33:48]

Probably wants a comment for what's happening here. (That you only implemented
it for POSIX and not Windows I assume.)

[xunjieli, 2017/02/13 20:28:18]

Done.

+#endif
+  }
+  if (buffer_released) {
+    EXPECT_EQ(0u, stats2.buffer_size);
+    EXPECT_EQ(stats.serialized_cert_size, stats2.total_size);
+  } else {
+    EXPECT_EQ(17 * 1024u, stats2.buffer_size);
+    EXPECT_LT(17 * 1024u, stats2.total_size);
+  }
   EXPECT_EQ(1u, stats2.cert_count);
   EXPECT_LT(0u, stats2.serialized_cert_size);
-  EXPECT_LT(17 * 1024u, stats2.total_size);
 }
 
 }  // namespace net