HttpStreamParser: Don't reuse sockets which receive unparsed data.

net/http/http_network_transaction_unittest.cc

Index: net/http/http_network_transaction_unittest.cc
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 3ebe1cb4e08c14493fdb51e155d268b8c12640ed..f8d59943f65b3a908517fa048b882af4cc710bcc 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -52,6 +52,7 @@
 #include "net/http/http_network_session.h"
 #include "net/http/http_network_session_peer.h"
 #include "net/http/http_request_headers.h"
+#include "net/http/http_response_info.h"
 #include "net/http/http_server_properties_impl.h"
 #include "net/http/http_stream.h"
 #include "net/http/http_stream_factory.h"
@@ -1971,6 +1972,316 @@ TEST_P(HttpNetworkTransactionTest, KeepAliveAfterUnreadBody) {
   EXPECT_EQ("hello", response_data);
 }
 
+// Sockets that receive extra data after a response is complete should not be
+// reused.
+TEST_P(HttpNetworkTransactionTest, KeepAliveWithUnusedData1) {
+  scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  MockWrite data_writes1[] = {
+      MockWrite("HEAD / HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads1[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Connection: keep-alive\r\n"
+               "Content-Length: 22\r\n\r\n"
+               "This server is borked."),
+  };
+
+  MockWrite data_writes2[] = {
+      MockWrite("GET /foo HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads2[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Content-Length: 3\r\n\r\n"
+               "foo"),
+  };
+  StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
+                                 data_writes1, arraysize(data_writes1));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
+                                 data_writes2, arraysize(data_writes2));
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  TestCompletionCallback callback;
+  HttpRequestInfo request1;
+  request1.method = "HEAD";
+  request1.url = GURL("http://www.borked.com/");
+
+  scoped_ptr<HttpTransaction> trans1(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  int rv = trans1->Start(&request1, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1);
+  ASSERT_TRUE(response1->headers);
+  EXPECT_EQ(200, response1->headers->response_code());
+  EXPECT_TRUE(response1->headers->IsKeepAlive());
+
+  std::string response_data1;
+  EXPECT_EQ(OK, ReadTransaction(trans1.get(), &response_data1));
+  EXPECT_EQ("", response_data1);
+  // Deleting the transaction attempts to release the socket back into the
+  // socket pool.
+  trans1.reset();
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("http://www.borked.com/foo");
+
+  scoped_ptr<HttpTransaction> trans2(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  rv = trans2->Start(&request2, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response2 = trans2->GetResponseInfo();
+  ASSERT_TRUE(response2);
+  ASSERT_TRUE(response2->headers);
+  EXPECT_EQ(200, response2->headers->response_code());
+
+  std::string response_data2;
+  EXPECT_EQ(OK, ReadTransaction(trans2.get(), &response_data2));
+  EXPECT_EQ("foo", response_data2);
+}
+
+TEST_P(HttpNetworkTransactionTest, KeepAliveWithUnusedData2) {
+  scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  MockWrite data_writes1[] = {
+      MockWrite("GET / HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads1[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Connection: keep-alive\r\n"
+               "Content-Length: 22\r\n\r\n"
+               "This server is borked."
+               "Bonus data!"),
+  };
+
+  MockWrite data_writes2[] = {
+      MockWrite("GET /foo HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads2[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Content-Length: 3\r\n\r\n"
+               "foo"),
+  };
+  StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
+                                 data_writes1, arraysize(data_writes1));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
+                                 data_writes2, arraysize(data_writes2));
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  TestCompletionCallback callback;
+  HttpRequestInfo request1;
+  request1.method = "GET";
+  request1.url = GURL("http://www.borked.com/");
+
+  scoped_ptr<HttpTransaction> trans1(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  int rv = trans1->Start(&request1, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1);
+  ASSERT_TRUE(response1->headers);
+  EXPECT_EQ(200, response1->headers->response_code());
+  EXPECT_TRUE(response1->headers->IsKeepAlive());
+
+  std::string response_data1;
+  EXPECT_EQ(OK, ReadTransaction(trans1.get(), &response_data1));
+  EXPECT_EQ("This server is borked.", response_data1);
+  // Deleting the transaction attempts to release the socket back into the
+  // socket pool.
+  trans1.reset();
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("http://www.borked.com/foo");
+
+  scoped_ptr<HttpTransaction> trans2(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  rv = trans2->Start(&request2, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response2 = trans2->GetResponseInfo();
+  ASSERT_TRUE(response2);
+  ASSERT_TRUE(response2->headers);
+  EXPECT_EQ(200, response2->headers->response_code());
+
+  std::string response_data2;
+  EXPECT_EQ(OK, ReadTransaction(trans2.get(), &response_data2));
+  EXPECT_EQ("foo", response_data2);
+}
+
+TEST_P(HttpNetworkTransactionTest, KeepAliveWithUnusedData3) {
+  scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  MockWrite data_writes1[] = {
+      MockWrite("GET / HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads1[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Connection: keep-alive\r\n"
+               "Transfer-Encoding: chunked\r\n\r\n"),
+      MockRead("16\r\nThis server is borked.\r\n"),
+      MockRead("0\r\n\r\nBonus data!"),
+  };
+
+  MockWrite data_writes2[] = {
+      MockWrite("GET /foo HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads2[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Content-Length: 3\r\n\r\n"
+               "foo"),
+  };
+  StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
+                                 data_writes1, arraysize(data_writes1));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
+                                 data_writes2, arraysize(data_writes2));
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  TestCompletionCallback callback;
+  HttpRequestInfo request1;
+  request1.method = "GET";
+  request1.url = GURL("http://www.borked.com/");
+
+  scoped_ptr<HttpTransaction> trans1(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  int rv = trans1->Start(&request1, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1);
+  ASSERT_TRUE(response1->headers);
+  EXPECT_EQ(200, response1->headers->response_code());
+  EXPECT_TRUE(response1->headers->IsKeepAlive());
+
+  std::string response_data1;
+  EXPECT_EQ(OK, ReadTransaction(trans1.get(), &response_data1));
+  EXPECT_EQ("This server is borked.", response_data1);
+  // Deleting the transaction attempts to release the socket back into the
+  // socket pool.
+  trans1.reset();
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("http://www.borked.com/foo");
+
+  scoped_ptr<HttpTransaction> trans2(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  rv = trans2->Start(&request2, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response2 = trans2->GetResponseInfo();
+  ASSERT_TRUE(response2);
+  ASSERT_TRUE(response2->headers);
+  EXPECT_EQ(200, response2->headers->response_code());
+
+  std::string response_data2;
+  EXPECT_EQ(OK, ReadTransaction(trans2.get(), &response_data2));
+  EXPECT_EQ("foo", response_data2);
+}
+
+// This is a little different from the others - it tests the case that the
+// HttpStreamParser doesn't know if there's extra data on a socket or not when
+// the HttpNetworkTransaction is torn down, because the response body hasn't
+// been read from yet, but the request goes through the HttpResponseBodyDrainer.
+TEST_P(HttpNetworkTransactionTest, KeepAliveWithUnusedData4) {
+  scoped_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  MockWrite data_writes1[] = {
+      MockWrite("GET / HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads1[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Connection: keep-alive\r\n"
+               "Transfer-Encoding: chunked\r\n\r\n"),
+      MockRead("16\r\nThis server is borked.\r\n"),
+      MockRead("0\r\n\r\nBonus data!"),
+  };
+
+  MockWrite data_writes2[] = {
+      MockWrite("GET /foo HTTP/1.1\r\n"
+                "Host: www.borked.com\r\n"
+                "Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads2[] = {
+      MockRead("HTTP/1.1 200 OK\r\n"
+               "Content-Length: 3\r\n\r\n"
+               "foo"),
+  };
+  StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
+                                 data_writes1, arraysize(data_writes1));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
+                                 data_writes2, arraysize(data_writes2));
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  TestCompletionCallback callback;
+  HttpRequestInfo request1;
+  request1.method = "GET";
+  request1.url = GURL("http://www.borked.com/");
+
+  scoped_ptr<HttpTransaction> trans1(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  int rv = trans1->Start(&request1, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response1 = trans1->GetResponseInfo();
+  ASSERT_TRUE(response1);
+  ASSERT_TRUE(response1->headers);
+  EXPECT_EQ(200, response1->headers->response_code());
+  EXPECT_TRUE(response1->headers->IsKeepAlive());
+
+  // Deleting the transaction creates an HttpResponseBodyDrainer to read the
+  // response body.
+  trans1.reset();
+
+  // Let the HttpResponseBodyDrainer drain the socket.  It should determine the
+  // socket can't be reused, rather than returning it to the socket pool.
+  base::RunLoop().RunUntilIdle();
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("http://www.borked.com/foo");
+
+  scoped_ptr<HttpTransaction> trans2(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+  rv = trans2->Start(&request2, callback.callback(), BoundNetLog());
+  EXPECT_EQ(OK, callback.GetResult(rv));
+
+  const HttpResponseInfo* response2 = trans2->GetResponseInfo();
+  ASSERT_TRUE(response2);
+  ASSERT_TRUE(response2->headers);
+  EXPECT_EQ(200, response2->headers->response_code());
+
+  std::string response_data2;
+  EXPECT_EQ(OK, ReadTransaction(trans2.get(), &response_data2));
+  EXPECT_EQ("foo", response_data2);
+}
+
 // Test the request-challenge-retry sequence for basic auth.
 // (basic auth is the easiest to mock, because it has no randomness).
 TEST_P(HttpNetworkTransactionTest, BasicAuth) {
@@ -7081,7 +7392,7 @@ TEST_P(HttpNetworkTransactionTest, ResetStateForRestart) {
   trans->request_headers_.SetHeader("Authorization", "NTLM");
 
   // Setup state in response_
-  HttpResponseInfo* response = &trans->response_;
+  HttpResponseInfo* response = trans->response_.get();
   response->auth_challenge = new AuthChallengeInfo();
   response->ssl_info.cert_status = static_cast<CertStatus>(-1);  // Nonsensical.
   response->response_time = base::Time::Now();
@@ -14528,7 +14839,10 @@ class FakeStream : public HttpStream,
     return ERR_NOT_IMPLEMENTED;
   }
 
-  void Drain(HttpNetworkSession* session) override { ADD_FAILURE(); }
+  void Drain(HttpNetworkSession* session,
+             scoped_ptr<HttpResponseInfo> response_info) override {
+    ADD_FAILURE();
+  }
 
   void PopulateNetErrorDetails(NetErrorDetails* details) override { return; }
 
@@ -14765,7 +15079,10 @@ class FakeWebSocketBasicHandshakeStream : public WebSocketHandshakeStreamBase {
     return ERR_NOT_IMPLEMENTED;
   }
 
-  void Drain(HttpNetworkSession* session) override { NOTREACHED(); }
+  void Drain(HttpNetworkSession* session,
+             scoped_ptr<HttpResponseInfo> response_info) override {
+    NOTREACHED();
+  }
 
   void PopulateNetErrorDetails(NetErrorDetails* details) override { return; }