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 ec24dcb762deb08e4e428a1ca8ba17a6ce504e44..8065e03c68b5d1ee15013b98ce9e0c090642eddc 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -14,8 +14,10 @@
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/mock_entropy_provider.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -140,6 +142,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 {
@@ -172,6 +179,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().
@@ -188,10 +199,15 @@ class ReadBufferingStreamSocket : public WrappedStreamSocket {
     STATE_READ_COMPLETE,
   };
 
-  int DoLoop(int result);
-  int DoRead();
-  int DoReadComplete(int result);
-  void OnReadCompleted(int result);
+  // Routine shared between Read() and ReadIfReady().
+  int ReadCommon(IOBuffer* buf,
+                 int buf_len,
+                 const CompletionCallback& callback,
+                 bool use_read_if_ready);
+  int DoLoop(int result, bool use_read_if_ready);
+  int DoRead(bool use_read_if_ready);
+  int DoReadComplete(int result, bool use_read_if_ready);
+  void OnReadCompleted(bool use_read_if_ready, int result);
 
   State state_;
   scoped_refptr<GrowableIOBuffer> read_buffer_;
@@ -218,31 +234,55 @@ int ReadBufferingStreamSocket::Read(IOBuffer* buf,
                                     const CompletionCallback& callback) {
   if (buffer_size_ == 0)
     return transport_->Read(buf, buf_len, callback);
+  return ReadCommon(buf, buf_len, callback, false);
+}
+
+int ReadBufferingStreamSocket::ReadIfReady(IOBuffer* buf,
+                                           int buf_len,
+                                           const CompletionCallback& callback) {

[davidben, 2017/02/01 22:25:58]

Similar comment. I think you can structure this like:

- Read calls ReadIfReady and, if ERR_IO_PENDING, stashes user_read_buf_.
- ReadCommon inlines into ReadIfReady and doesn't set user_read_buf_.
- No need for boolean.
- DoRead can just call transport_->Read; the tests only care about the external
behavior of ReadBufferingStreamSocket and leaving it alone is simpler.
- DoReadComplete checks user_read_buf_ and does the ReadIfReady (just return OK)
or Read (memcpy and return capacity) thing accordingly.

[xunjieli, 2017/02/03 16:35:33]

Done.

+  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();
+  }
+  int rv = ReadCommon(buf, buf_len, callback, true);
+  user_read_buf_ = nullptr;
+  return rv;
+}
+
+int ReadBufferingStreamSocket::ReadCommon(IOBuffer* buf,
+                                          int buf_len,
+                                          const CompletionCallback& callback,
+                                          bool use_read_if_ready) {
+  DCHECK(!user_read_buf_);
 
   if (buf_len < buffer_size_)
     return ERR_UNEXPECTED;
 
   state_ = STATE_READ;
   user_read_buf_ = buf;
-  int result = DoLoop(OK);
+  int result = DoLoop(OK, use_read_if_ready);
   if (result == ERR_IO_PENDING)
     user_read_callback_ = callback;
   else
-    user_read_buf_ = NULL;
+    user_read_buf_ = nullptr;
   return result;
 }
 
-int ReadBufferingStreamSocket::DoLoop(int result) {
+int ReadBufferingStreamSocket::DoLoop(int result, bool use_read_if_ready) {
   int rv = result;
   do {
     State current_state = state_;
     state_ = STATE_NONE;
     switch (current_state) {
       case STATE_READ:
-        rv = DoRead();
+        rv = DoRead(use_read_if_ready);
         break;
       case STATE_READ_COMPLETE:
-        rv = DoReadComplete(rv);
+        rv = DoReadComplete(rv, use_read_if_ready);
         break;
       case STATE_NONE:
       default:
@@ -254,17 +294,23 @@ int ReadBufferingStreamSocket::DoLoop(int result) {
   return rv;
 }
 
-int ReadBufferingStreamSocket::DoRead() {
+int ReadBufferingStreamSocket::DoRead(bool use_read_if_ready) {
   state_ = STATE_READ_COMPLETE;
-  int rv =
-      transport_->Read(read_buffer_.get(),
-                       read_buffer_->RemainingCapacity(),
-                       base::Bind(&ReadBufferingStreamSocket::OnReadCompleted,
-                                  base::Unretained(this)));
-  return rv;
+  if (use_read_if_ready) {
+    return transport_->ReadIfReady(
+        read_buffer_.get(), read_buffer_->RemainingCapacity(),
+        base::Bind(&ReadBufferingStreamSocket::OnReadCompleted,
+                   base::Unretained(this), true));
+  } else {
+    return transport_->Read(
+        read_buffer_.get(), read_buffer_->RemainingCapacity(),
+        base::Bind(&ReadBufferingStreamSocket::OnReadCompleted,
+                   base::Unretained(this), false));
+  }
 }
 
-int ReadBufferingStreamSocket::DoReadComplete(int result) {
+int ReadBufferingStreamSocket::DoReadComplete(int result,
+                                              bool use_read_if_ready) {
   state_ = STATE_NONE;
   if (result <= 0)
     return result;
@@ -275,6 +321,9 @@ int ReadBufferingStreamSocket::DoReadComplete(int result) {
     return OK;
   }
 
+  if (use_read_if_ready)
+    return OK;
+
   memcpy(user_read_buf_->data(),
          read_buffer_->StartOfBuffer(),
          read_buffer_->capacity());
@@ -282,11 +331,16 @@ int ReadBufferingStreamSocket::DoReadComplete(int result) {
   return read_buffer_->capacity();
 }
 
-void ReadBufferingStreamSocket::OnReadCompleted(int result) {
-  result = DoLoop(result);
+void ReadBufferingStreamSocket::OnReadCompleted(bool use_read_if_ready,
+                                                int result) {
+  DCHECK_NE(ERR_IO_PENDING, result);
+
+  if (use_read_if_ready && result == 0)
+    state_ = STATE_READ;
+
+  result = DoLoop(result, use_read_if_ready);
   if (result == ERR_IO_PENDING)
     return;
-
   user_read_buf_ = NULL;
   base::ResetAndReturn(&user_read_callback_).Run(result);
 }
@@ -302,6 +356,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;
@@ -344,6 +401,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) {
@@ -366,6 +432,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;
@@ -400,6 +469,13 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
   void WaitForWrite();
 
  private:
+  // Shared between Read() and ReadIfReady(). If |use_read_if_ready| is true,
+  // ReadIfReady() will be used.
+  int ReadCommon(IOBuffer* buf,
+                 int buf_len,
+                 const CompletionCallback& callback,
+                 bool use_read_if_ready);
+
   // Handles completion from the underlying transport read.
   void OnReadCompleted(int result);
 
@@ -444,24 +520,15 @@ class FakeBlockingStreamSocket : public WrappedStreamSocket {
 int FakeBlockingStreamSocket::Read(IOBuffer* buf,
                                    int len,
                                    const CompletionCallback& callback) {
-  DCHECK(!pending_read_buf_);
-  DCHECK(pending_read_callback_.is_null());
-  DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
-  DCHECK(!callback.is_null());
+  return ReadCommon(buf, len, callback, false);
+}
 
-  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;
-    pending_read_buf_len_ = len;
-    pending_read_callback_ = callback;
-    // Save the read result.
-    if (rv != ERR_IO_PENDING) {
-      OnReadCompleted(rv);
-      rv = ERR_IO_PENDING;
-    }
-  }
+int FakeBlockingStreamSocket::ReadIfReady(IOBuffer* buf,
+                                          int len,
+                                          const CompletionCallback& callback) {
+  int rv = ReadCommon(buf, len, callback, true);
+  pending_read_buf_ = nullptr;
+  pending_read_buf_len_ = 0;
   return rv;
 }
 
@@ -566,6 +633,39 @@ void FakeBlockingStreamSocket::WaitForWrite() {
   DCHECK(pending_write_buf_.get());
 }
 
+int FakeBlockingStreamSocket::ReadCommon(IOBuffer* buf,
+                                         int len,
+                                         const CompletionCallback& callback,
+                                         bool use_read_if_ready) {
+  DCHECK(!pending_read_buf_);
+  DCHECK(pending_read_callback_.is_null());
+  DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
+  DCHECK(!callback.is_null());
+
+  int rv;
+  if (!use_read_if_ready) {
+    rv = transport_->Read(buf, len,
+                          base::Bind(&FakeBlockingStreamSocket::OnReadCompleted,
+                                     base::Unretained(this)));
+  } else {
+    rv = transport_->ReadIfReady(
+        buf, len, base::Bind(&FakeBlockingStreamSocket::OnReadCompleted,
+                             base::Unretained(this)));

[davidben, 2017/02/01 22:25:57]

Hrm. This one's a little interesting. If ReadIfReady returns synchronously while
should_block_read_ is true, I think things will get confused here. Because
you've actually written to the buffer and taken data out of the socket, but
you're not returning it yet which means the caller thinks it can throw that data
away. (This socket blocks read *results* rather than read *calls*. I think there
was some awkward False Start test synchronizing reason why we needed that.
Though arguably we care less about those tests now that the code is actually in
BoringSSL.)

I'm actually not sure how FakeBlockingStreamSocket could implement ReadIfReady +
BlockReadResult + WaitForReadResult. Also ReplaceReadResult. Without those two
functions, I think BlockRead and BlockReadResult are the same though.

[xunjieli, 2017/02/03 16:35:33]

Ah, sorry I missed that. Good catch! How about we create a temp buffer to hold
the result? That way, if caller throws away |buf|, we can still unblock/replace
correctly? I uploaded a new patch.

+  }
+  if (rv == ERR_IO_PENDING || should_block_read_) {
+    // Save the callback to be called later.
+    pending_read_buf_ = buf;
+    pending_read_buf_len_ = len;
+    pending_read_callback_ = callback;
+    // Save the read result.
+    if (rv != ERR_IO_PENDING) {
+      OnReadCompleted(rv);
+      rv = ERR_IO_PENDING;
+    }
+  }
+  return rv;
+}
+
 void FakeBlockingStreamSocket::OnReadCompleted(int result) {
   DCHECK_EQ(ERR_IO_PENDING, pending_read_result_);
   DCHECK(!pending_read_callback_.is_null());
@@ -856,6 +956,62 @@ 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()) {
+      field_trial_list_ = base::MakeUnique<base::FieldTrialList>(
+          base::MakeUnique<base::MockEntropyProvider>());
+      base::FieldTrialList::CreateFieldTrial(Socket::kReadIfReadyTrialName,
+                                             "enable");
+    }
+  }
+
+  // Convienient wrapper to call Read() or ReadIfReady() depending on
+  // GetParam().
+  int Read(StreamSocket* socket,
+           IOBuffer* buf,
+           int buf_len,
+           const CompletionCallback& callback) {
+    if (GetParam()) {
+      int rv = socket->ReadIfReady(buf, buf_len, callback);
+      if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED)
+        return rv;

[davidben, 2017/02/01 22:25:58]

Since this is test code, do we want to have this check? Otherwise if
SSLClientSocket just didn't bother implementing ReadIfReady, I think it'd pass
all the tests.

[xunjieli, 2017/02/03 16:35:33]

Done.

+    }
+    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) {

[davidben, 2017/02/01 22:25:58]

Since you pass in and wait on callback in the same function, I think you don't
need to pass it in and can instead just create your own one in this function.

[xunjieli, 2017/02/03 16:35:33]

Done.

+    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);
+        }
+      }
+      if (rv != ERR_READ_IF_READY_NOT_IMPLEMENTED)

[davidben, 2017/02/01 22:25:58]

Ditto re ERR_READ_IF_READY_NOT_IMPLEMENTED.

[xunjieli, 2017/02/03 16:35:33]

Done.

+        return rv;
+    }
+    return callback->GetResult(
+        socket->Read(buf, buf_len, callback->callback()));
+  }
+
+ private:
+  std::unique_ptr<base::FieldTrialList> field_trial_list_;
+};
+
+INSTANTIATE_TEST_CASE_P(/* no prefix */,
+                        SSLClientSocketReadTest,
+                        ::testing::Bool());
+
 // Verifies the correctness of GetSSLCertRequestInfo.
 class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
  protected:
@@ -1200,7 +1356,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;
@@ -1236,7 +1392,8 @@ 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()));
+    TestCompletionCallback read_callback;
+    rv = ReadAndWaitForCompletion(sock.get(), buf.get(), 4096, &read_callback);
     EXPECT_GE(rv, 0);
     if (rv >= 0) {
       unencrypted_bytes_read += rv;
@@ -1287,7 +1444,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;
@@ -1329,7 +1486,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));
 }
 
@@ -1464,7 +1621,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;
@@ -1474,7 +1631,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));
 
@@ -1495,6 +1652,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());
+  }
   EXPECT_GT(rv, 0);
 }
 
@@ -1504,7 +1665,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;
@@ -1553,7 +1714,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));
@@ -1583,7 +1744,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;
@@ -1628,7 +1789,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
@@ -1696,7 +1857,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;
@@ -1722,14 +1883,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, &callback);
   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;
@@ -1759,17 +1920,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));
@@ -1781,11 +1947,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, &callback));
 }
 
-TEST_F(SSLClientSocketTest, Read_SmallChunks) {
+TEST_P(SSLClientSocketReadTest, Read_SmallChunks) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -1804,12 +1970,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, &callback);
     EXPECT_GE(rv, 0);
   } while (rv > 0);
 }
 
-TEST_F(SSLClientSocketTest, Read_ManySmallRecords) {
+TEST_P(SSLClientSocketReadTest, Read_ManySmallRecords) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1852,11 +2018,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, &callback);
   ASSERT_EQ(rv, 8192);
 }
 
-TEST_F(SSLClientSocketTest, Read_Interrupted) {
+TEST_P(SSLClientSocketReadTest, Read_Interrupted) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -1875,11 +2041,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, &callback);
   EXPECT_GT(rv, 0);
 }
 
-TEST_F(SSLClientSocketTest, Read_FullLogging) {
+TEST_P(SSLClientSocketReadTest, Read_FullLogging) {
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   TestCompletionCallback callback;
@@ -1915,7 +2081,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, &callback);
     EXPECT_GE(rv, 0);
     if (rv <= 0)
       break;
@@ -3588,6 +3754,12 @@ TEST_F(SSLClientSocketTest, AccessDeniedClientCerts) {
 
 // Basic test for dumping memory stats.
 TEST_F(SSLClientSocketTest, DumpMemoryStats) {
+  std::unique_ptr<base::FieldTrialList> field_trial_list =
+      base::MakeUnique<base::FieldTrialList>(
+          base::MakeUnique<base::MockEntropyProvider>());
+
+  base::FieldTrialList::CreateFieldTrial(Socket::kReadIfReadyTrialName,
+                                         "enable");
   ASSERT_TRUE(StartTestServer(SpawnedTestServer::SSLOptions()));
 
   int rv;
@@ -3603,16 +3775,26 @@ 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));
+#if defined(OS_POSIX)

[davidben, 2017/02/01 22:25:58]

Not a TEST_P?

[xunjieli, 2017/02/03 16:35:33]

Done.

+  rv = sock_->ReadIfReady(buf.get(), 4096, read_callback.callback());
+#else
   rv = sock_->Read(buf.get(), 4096, read_callback.callback());
+#endif
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   // Dump memory again and check that |buffer_size| contain the read buffer.
   StreamSocket::SocketMemoryStats stats2;
   sock_->DumpMemoryStats(&stats2);
+
+#if defined(OS_POSIX)
+  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);
+#endif
   EXPECT_EQ(1u, stats2.cert_count);
   EXPECT_LT(0u, stats2.serialized_cert_size);
-  EXPECT_LT(17 * 1024u, stats2.total_size);
 }
 
 }  // namespace net