Use sockets with unread data if they've never been used before.

net/socket/client_socket_pool_base_unittest.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 "net/socket/client_socket_pool_base.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 98 matching lines @@
   ~TestSocketParams() {}
 
   const bool ignore_limits_;
 };
 typedef ClientSocketPoolBase<TestSocketParams> TestClientSocketPoolBase;
 
 class MockClientSocket : public StreamSocket {
  public:
   explicit MockClientSocket(net::NetLog* net_log)
       : connected_(false),
+        idle_(true),
         net_log_(BoundNetLog::Make(net_log, net::NetLog::SOURCE_SOCKET)),
         was_used_to_convey_data_(false) {
   }
 
+  void set_idle(bool idle) { idle_ = idle; }

[mmenke, 2014/03/18 15:16:55]

Think it's worth a comment just what this does (Lets 1 byte be "read")

[davidben, 2014/03/25 19:50:18]

Done.

+
   // Socket implementation.
   virtual int Read(
       IOBuffer* /* buf */, int len,
       const CompletionCallback& /* callback */) OVERRIDE {
+    if (!idle_ && len > 0) {
+      idle_ = true;
+      was_used_to_convey_data_ = true;
+      return 1;
+    }
     return ERR_UNEXPECTED;
   }
 
   virtual int Write(
       IOBuffer* /* buf */, int len,
       const CompletionCallback& /* callback */) OVERRIDE {
     was_used_to_convey_data_ = true;
     return len;
   }
   virtual bool SetReceiveBufferSize(int32 size) OVERRIDE { return true; }
   virtual bool SetSendBufferSize(int32 size) OVERRIDE { return true; }
 
   // StreamSocket implementation.
   virtual int Connect(const CompletionCallback& callback) OVERRIDE {
     connected_ = true;
     return OK;
   }
 
   virtual void Disconnect() OVERRIDE { connected_ = false; }
   virtual bool IsConnected() const OVERRIDE { return connected_; }
-  virtual bool IsConnectedAndIdle() const OVERRIDE { return connected_; }
+  virtual bool IsConnectedAndIdle() const OVERRIDE {
+    return connected_ && idle_;
+  }
 
   virtual int GetPeerAddress(IPEndPoint* /* address */) const OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
   virtual int GetLocalAddress(IPEndPoint* /* address */) const OVERRIDE {
     return ERR_UNEXPECTED;
   }
 
   virtual const BoundNetLog& NetLog() const OVERRIDE {
@@ skipping 11 matching lines @@
   }
   virtual NextProto GetNegotiatedProtocol() const OVERRIDE {
     return kProtoUnknown;
   }
   virtual bool GetSSLInfo(SSLInfo* ssl_info) OVERRIDE {
     return false;
   }
 
  private:
   bool connected_;
+  bool idle_;
   BoundNetLog net_log_;
   bool was_used_to_convey_data_;
 
   DISALLOW_COPY_AND_ASSIGN(MockClientSocket);
 };
 
 class TestConnectJob;
 
 class MockClientSocketFactory : public ClientSocketFactory {
  public:
@@ skipping 49 matching lines @@
   enum JobType {
     kMockJob,
     kMockFailingJob,
     kMockPendingJob,
     kMockPendingFailingJob,
     kMockWaitingJob,
     kMockRecoverableJob,
     kMockPendingRecoverableJob,
     kMockAdditionalErrorStateJob,
     kMockPendingAdditionalErrorStateJob,
+    kMockUnreadDataJob,
   };
 
   // The kMockPendingJob uses a slight delay before allowing the connect
   // to complete.
   static const int kPendingConnectDelay = 2;
 
   TestConnectJob(JobType job_type,
                  const std::string& group_name,
                  const TestClientSocketPoolBase::Request& request,
                  base::TimeDelta timeout_duration,
@@ skipping 107 matching lines @@
         store_additional_error_state_ = true;
         base::MessageLoop::current()->PostDelayedTask(
             FROM_HERE,
             base::Bind(base::IgnoreResult(&TestConnectJob::DoConnect),
                        weak_factory_.GetWeakPtr(),
                        false /* error */,
                        true  /* async */,
                        false /* recoverable */),
             base::TimeDelta::FromMilliseconds(2));
         return ERR_IO_PENDING;
+      case kMockUnreadDataJob: {
+        int ret = DoConnect(true /* successful */, false /* sync */,
+                            false /* recoverable */);
+        static_cast<MockClientSocket*>(socket())->set_idle(false);

[mmenke, 2014/03/18 15:16:55]

Can we just store the socket as a local and avoid the cast?

+        return ret;
+      }
       default:
         NOTREACHED();
         SetSocket(scoped_ptr<StreamSocket>());
         return ERR_FAILED;
     }
   }
 
   int DoConnect(bool succeed, bool was_async, bool recoverable) {
     int result = OK;
     if (succeed) {
@@ skipping 3149 matching lines @@
                                          callback1.callback(),
                                          pool_.get(),
                                          BoundNetLog()));
 
   EXPECT_EQ(1, pool_->NumConnectJobsInGroup("a"));
   EXPECT_EQ(0, pool_->NumUnassignedConnectJobsInGroup("a"));
   EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
 
   ASSERT_EQ(OK, callback1.WaitForResult());
 
-  // Make sure if a preconneced socket is not fully connected when a request
+  // Make sure if a preconnected socket is not fully connected when a request
   // starts, it has a connect start time.
   TestLoadTimingInfoConnectedNotReused(handle1);
   handle1.Reset();
 
   EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
 }
 
 // Checks that fully connected preconnect jobs have no connect times, and are
 // marked as reused.
 TEST_F(ClientSocketPoolBaseTest, ConnectedPreconnectJobsHaveNoConnectTimes) {
@@ skipping 158 matching lines @@
   ASSERT_EQ(OK, callback.WaitForResult());
 
   // The hung connect job should still be there, but everything else should be
   // complete.
   EXPECT_EQ(1, pool_->NumConnectJobsInGroup("a"));
   EXPECT_EQ(0, pool_->NumUnassignedConnectJobsInGroup("a"));
   EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
   EXPECT_EQ(1, pool_->NumActiveSocketsInGroup("a"));
 }
 
+// Tests that a preconnect that starts out with unread data can still be used.
+// http://crbug.com/334467

[mmenke, 2014/03/18 15:16:55]

Suggest a test where a reused socket with unread data can't be used (Assuming we
don't have one here, since we didn't have one for the unused case.  If we do
have one, just say so).

+TEST_F(ClientSocketPoolBaseTest, PreconnectWithUnreadData) {
+  CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockUnreadDataJob);
+
+  pool_->RequestSockets("a", &params_, 1, BoundNetLog());
+
+  ASSERT_TRUE(pool_->HasGroup("a"));
+  EXPECT_EQ(0, pool_->NumConnectJobsInGroup("a"));
+  EXPECT_EQ(0, pool_->NumUnassignedConnectJobsInGroup("a"));
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
+
+  // Fail future jobs to be sure that handle receives the preconnected socket
+  // rather than closing it and making a new one.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockFailingJob);
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  EXPECT_EQ(OK, handle.Init("a",
+                             params_,
+                             DEFAULT_PRIORITY,
+                             callback.callback(),
+                             pool_.get(),
+                             BoundNetLog()));
+
+  ASSERT_TRUE(pool_->HasGroup("a"));
+  EXPECT_EQ(0, pool_->NumConnectJobsInGroup("a"));
+  EXPECT_EQ(0, pool_->NumUnassignedConnectJobsInGroup("a"));
+  EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
+
+  // Drain the pending read.
+  EXPECT_EQ(1, handle.socket()->Read(NULL, 1, CompletionCallback()));
+
+  TestLoadTimingInfoConnectedReused(handle);
+  handle.Reset();
+
+  // The socket should be usable now that it's idle again.
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
+}
+
 class MockLayeredPool : public HigherLayeredPool {
  public:
   MockLayeredPool(TestClientSocketPool* pool,
                   const std::string& group_name)
       : pool_(pool),
         group_name_(group_name),
         can_release_connection_(true) {
     pool_->AddHigherLayeredPool(this);
   }
 
@@ skipping 325 matching lines @@
   request(1)->handle()->Reset();
   ASSERT_EQ(1, pool_->NumConnectJobsInGroup("a"));
 
   EXPECT_EQ(OK, request(2)->WaitForResult());
   EXPECT_FALSE(request(1)->have_result());
 }
 
 }  // namespace
 
 }  // namespace net