Retry requests on reused sockets that receive ERR_EMPTY_RESPONSE.

We retry requests on ERR_CONNECTION_CLOSED in case of a close/reuse race, but
ERR_CONNECTION_CLOSED is converted to ERR_EMPTY_RESPONSE if this is a socket's
first request. Such a socket is normally not reused unless it was a preconnect
miss.

To avoid test flakiness, make the UNUSED vs UNUSED_IDLE determination not
timing-sensitive. The existing logic is compares idle_time to 0, so it's
dependent on clock granularity rather than any intentional timeout.

Add equivalent tests to HttpNetworkTransactionTest.KeepAliveConnection for
preconnect misses.

BUG=352156

Review URL: https://codereview.chromium.org/197283012

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

10 files changed, 144 insertions, 33 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 4c6be63..ba27cb7 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1443,7 +1443,11 @@ int HttpNetworkTransaction::HandleIOError(int error) {
     // likely happen when trying to retrieve its IP address.
     // See http://crbug.com/105824 for more details.
     case ERR_SOCKET_NOT_CONNECTED:
-      if (ShouldResendRequest(error)) {
+    // If a socket is closed on its initial request, HttpStreamParser returns
+    // ERR_EMPTY_RESPONSE. This may still be close/reuse race if the socket was
+    // preconnected but failed to be used before the server timed it out.
+    case ERR_EMPTY_RESPONSE:
+      if (ShouldResendRequest()) {
         net_log_.AddEventWithNetErrorCode(
             NetLog::TYPE_HTTP_TRANSACTION_RESTART_AFTER_ERROR, error);
         ResetConnectionAndRequestForResend();
@@ -1494,7 +1498,7 @@ HttpResponseHeaders* HttpNetworkTransaction::GetResponseHeaders() const {
   return response_.headers.get();
 }
 
-bool HttpNetworkTransaction::ShouldResendRequest(int error) const {
+bool HttpNetworkTransaction::ShouldResendRequest() const {
   bool connection_is_proven = stream_->IsConnectionReused();
   bool has_received_headers = GetResponseHeaders() != NULL;
 
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index b10e155..24ab95b 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -210,9 +210,9 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
   // Gets the response headers from the HttpStream.
   HttpResponseHeaders* GetResponseHeaders() const;
 
-  // Called when we reached EOF or got an error.  Returns true if we should
-  // resend the request.  |error| is OK when we reached EOF.
-  bool ShouldResendRequest(int error) const;
+  // Called when the socket is unexpectedly closed.  Returns true if the request
+  // should be resent in case of a socket reuse/close race.
+  bool ShouldResendRequest() const;
 
   // Resets the connection and the request headers for resend.  Called when
   // ShouldResendRequest() is true.
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 0703350..13cef37 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -16,6 +16,7 @@
 #include "base/json/json_writer.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_file_util.h"
@@ -283,6 +284,13 @@ class HttpNetworkTransactionTest
   void KeepAliveConnectionResendRequestTest(const MockWrite* write_failure,
                                             const MockRead* read_failure);
 
+  // Either |write_failure| specifies a write failure or |read_failure|
+  // specifies a read failure when using a reused socket.  In either case, the
+  // failure should cause the network transaction to resend the request, and the
+  // other argument should be NULL.
+  void PreconnectErrorResendRequestTest(const MockWrite* write_failure,
+                                        const MockRead* read_failure);
+
   SimpleGetHelperResult SimpleGetHelperForData(StaticSocketDataProvider* data[],
                                                size_t data_count) {
     SimpleGetHelperResult out;
@@ -1239,7 +1247,7 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest(
   };
 
   if (write_failure) {
-    ASSERT_TRUE(!read_failure);
+    ASSERT_FALSE(read_failure);
     data1_writes[1] = *write_failure;
   } else {
     ASSERT_TRUE(read_failure);
@@ -1298,6 +1306,90 @@ void HttpNetworkTransactionTest::KeepAliveConnectionResendRequestTest(
   }
 }
 
+void HttpNetworkTransactionTest::PreconnectErrorResendRequestTest(
+    const MockWrite* write_failure,
+    const MockRead* read_failure) {
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.foo.com/");
+  request.load_flags = 0;
+
+  CapturingNetLog net_log;
+  session_deps_.net_log = &net_log;
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+
+  // Written data for successfully sending a request.
+  MockWrite data1_writes[] = {
+    MockWrite("GET / HTTP/1.1\r\n"
+              "Host: www.foo.com\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+  };
+
+  // Read results for the first request.
+  MockRead data1_reads[] = {
+    MockRead(ASYNC, OK),
+  };
+
+  if (write_failure) {
+    ASSERT_FALSE(read_failure);
+    data1_writes[0] = *write_failure;
+  } else {
+    ASSERT_TRUE(read_failure);
+    data1_reads[0] = *read_failure;
+  }
+
+  StaticSocketDataProvider data1(data1_reads, arraysize(data1_reads),
+                                 data1_writes, arraysize(data1_writes));
+  session_deps_.socket_factory->AddSocketDataProvider(&data1);
+
+  MockRead data2_reads[] = {
+    MockRead("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"),
+    MockRead("hello"),
+    MockRead(ASYNC, OK),
+  };
+  StaticSocketDataProvider data2(data2_reads, arraysize(data2_reads), NULL, 0);
+  session_deps_.socket_factory->AddSocketDataProvider(&data2);
+
+  // Preconnect a socket.
+  net::SSLConfig ssl_config;
+  session->ssl_config_service()->GetSSLConfig(&ssl_config);
+  if (session->http_stream_factory()->has_next_protos())
+    ssl_config.next_protos = session->http_stream_factory()->next_protos();
+  session->http_stream_factory()->PreconnectStreams(
+      1, request, DEFAULT_PRIORITY, ssl_config, ssl_config);
+  // Wait for the preconnect to complete.
+  // TODO(davidben): Some way to wait for an idle socket count might be handy.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, GetIdleSocketCountInTransportSocketPool(session.get()));
+
+  // Make the request.
+  TestCompletionCallback callback;
+
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  LoadTimingInfo load_timing_info;
+  EXPECT_TRUE(trans->GetLoadTimingInfo(&load_timing_info));
+  TestLoadTimingNotReused(load_timing_info, CONNECT_TIMING_HAS_DNS_TIMES);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  ASSERT_TRUE(response != NULL);
+
+  EXPECT_TRUE(response->headers.get() != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
+
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ("hello", response_data);
+}
+
 TEST_P(HttpNetworkTransactionTest,
        KeepAliveConnectionNotConnectedOnWrite) {
   MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED);
@@ -1314,6 +1406,22 @@ TEST_P(HttpNetworkTransactionTest, KeepAliveConnectionEOF) {
   KeepAliveConnectionResendRequestTest(NULL, &read_failure);
 }
 
+TEST_P(HttpNetworkTransactionTest,
+       PreconnectErrorNotConnectedOnWrite) {
+  MockWrite write_failure(ASYNC, ERR_SOCKET_NOT_CONNECTED);
+  PreconnectErrorResendRequestTest(&write_failure, NULL);
+}
+
+TEST_P(HttpNetworkTransactionTest, PreconnectErrorReset) {
+  MockRead read_failure(ASYNC, ERR_CONNECTION_RESET);
+  PreconnectErrorResendRequestTest(NULL, &read_failure);
+}
+
+TEST_P(HttpNetworkTransactionTest, PreconnectErrorEOF) {
+  MockRead read_failure(SYNCHRONOUS, OK);  // EOF
+  PreconnectErrorResendRequestTest(NULL, &read_failure);
+}
+
 TEST_P(HttpNetworkTransactionTest, NonKeepAliveConnectionReset) {
   HttpRequestInfo request;
   request.method = "GET";
diff --git a/net/http/http_pipelined_connection_impl.cc b/net/http/http_pipelined_connection_impl.cc
index 2ee9c29..ebd73f2 100644
--- a/net/http/http_pipelined_connection_impl.cc
+++ b/net/http/http_pipelined_connection_impl.cc
@@ -650,7 +650,7 @@ bool HttpPipelinedConnectionImpl::IsConnectionReused(int pipeline_id) const {
 
 void HttpPipelinedConnectionImpl::SetConnectionReused(int pipeline_id) {
   CHECK(ContainsKey(stream_info_map_, pipeline_id));
-  connection_->set_is_reused(true);
+  connection_->set_reuse_type(ClientSocketHandle::REUSED_IDLE);
 }
 
 int64 HttpPipelinedConnectionImpl::GetTotalReceivedBytes(
diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc
index 7037616..e2e46660 100644
--- a/net/http/http_proxy_client_socket.cc
+++ b/net/http/http_proxy_client_socket.cc
@@ -276,7 +276,7 @@ int HttpProxyClientSocket::PrepareForAuthRestart() {
 int HttpProxyClientSocket::DidDrainBodyForAuthRestart(bool keep_alive) {
   if (keep_alive && transport_->socket()->IsConnectedAndIdle()) {
     next_state_ = STATE_GENERATE_AUTH_TOKEN;
-    transport_->set_is_reused(true);
+    transport_->set_reuse_type(ClientSocketHandle::REUSED_IDLE);
   } else {
     // This assumes that the underlying transport socket is a TCP socket,
     // since only TCP sockets are restartable.
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index 0821c84..9b787da 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -904,7 +904,7 @@ bool HttpStreamParser::IsConnectionReused() const {
 }
 
 void HttpStreamParser::SetConnectionReused() {
-  connection_->set_is_reused(true);
+  connection_->set_reuse_type(ClientSocketHandle::REUSED_IDLE);
 }
 
 bool HttpStreamParser::IsConnectionReusable() const {
diff --git a/net/socket/client_socket_handle.cc b/net/socket/client_socket_handle.cc
index e42e9fc..53bcd77 100644
--- a/net/socket/client_socket_handle.cc
+++ b/net/socket/client_socket_handle.cc
@@ -19,7 +19,7 @@ ClientSocketHandle::ClientSocketHandle()
     : is_initialized_(false),
       pool_(NULL),
       higher_pool_(NULL),
-      is_reused_(false),
+      reuse_type_(ClientSocketHandle::UNUSED),
       callback_(base::Bind(&ClientSocketHandle::OnIOComplete,
                            base::Unretained(this))),
       is_ssl_error_(false) {}
@@ -58,7 +58,7 @@ void ClientSocketHandle::ResetInternal(bool cancel) {
   is_initialized_ = false;
   socket_.reset();
   group_name_.clear();
-  is_reused_ = false;
+  reuse_type_ = ClientSocketHandle::UNUSED;
   user_callback_.Reset();
   if (higher_pool_)
     RemoveHigherLayeredPool(higher_pool_);
diff --git a/net/socket/client_socket_handle.h b/net/socket/client_socket_handle.h
index 30b7c03..0899d9a 100644
--- a/net/socket/client_socket_handle.h
+++ b/net/socket/client_socket_handle.h
@@ -126,7 +126,7 @@ class NET_EXPORT ClientSocketHandle {
   // SetSocket() may also be used if this handle is used as simply for
   // socket storage (e.g., http://crbug.com/37810).
   void SetSocket(scoped_ptr<StreamSocket> s);
-  void set_is_reused(bool is_reused) { is_reused_ = is_reused; }
+  void set_reuse_type(SocketReuseType reuse_type) { reuse_type_ = reuse_type; }
   void set_idle_time(base::TimeDelta idle_time) { idle_time_ = idle_time; }
   void set_pool_id(int id) { pool_id_ = id; }
   void set_is_ssl_error(bool is_ssl_error) { is_ssl_error_ = is_ssl_error; }
@@ -161,17 +161,9 @@ class NET_EXPORT ClientSocketHandle {
   // These may only be used if is_initialized() is true.
   const std::string& group_name() const { return group_name_; }
   int id() const { return pool_id_; }
-  bool is_reused() const { return is_reused_; }
+  bool is_reused() const { return reuse_type_ == REUSED_IDLE; }
   base::TimeDelta idle_time() const { return idle_time_; }
-  SocketReuseType reuse_type() const {
-    if (is_reused()) {
-      return REUSED_IDLE;
-    } else if (idle_time() == base::TimeDelta()) {
-      return UNUSED;
-    } else {
-      return UNUSED_IDLE;
-    }
-  }
+  SocketReuseType reuse_type() const { return reuse_type_; }
   const LoadTimingInfo::ConnectTiming& connect_timing() const {
     return connect_timing_;
   }
@@ -200,7 +192,7 @@ class NET_EXPORT ClientSocketHandle {
   HigherLayeredPool* higher_pool_;
   scoped_ptr<StreamSocket> socket_;
   std::string group_name_;
-  bool is_reused_;
+  SocketReuseType reuse_type_;
   CompletionCallback callback_;
   CompletionCallback user_callback_;
   base::TimeDelta idle_time_;
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 1c79923..e20b355 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -15,7 +15,6 @@
 #include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
-#include "net/socket/client_socket_handle.h"
 
 using base::TimeDelta;
 
@@ -401,7 +400,7 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
   if (rv == OK) {
     LogBoundConnectJobToRequest(connect_job->net_log().source(), request);
     if (!preconnecting) {
-      HandOutSocket(connect_job->PassSocket(), false /* not reused */,
+      HandOutSocket(connect_job->PassSocket(), ClientSocketHandle::UNUSED,
                     connect_job->connect_timing(), handle, base::TimeDelta(),
                     group, request.net_log());
     } else {
@@ -427,7 +426,7 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
       error_socket = connect_job->PassSocket();
     }
     if (error_socket) {
-      HandOutSocket(error_socket.Pass(), false /* not reused */,
+      HandOutSocket(error_socket.Pass(), ClientSocketHandle::UNUSED,
                     connect_job->connect_timing(), handle, base::TimeDelta(),
                     group, request.net_log());
     } else if (group->IsEmpty()) {
@@ -476,9 +475,16 @@ bool ClientSocketPoolBaseHelper::AssignIdleSocketToRequest(
         base::TimeTicks::Now() - idle_socket_it->start_time;
     IdleSocket idle_socket = *idle_socket_it;
     idle_sockets->erase(idle_socket_it);
+    // TODO(davidben): If |idle_time| is under some low watermark, consider
+    // treating as UNUSED rather than UNUSED_IDLE. This will avoid
+    // HttpNetworkTransaction retrying on some errors.
+    ClientSocketHandle::SocketReuseType reuse_type =
+        idle_socket.socket->WasEverUsed() ?
+            ClientSocketHandle::REUSED_IDLE :
+            ClientSocketHandle::UNUSED_IDLE;
     HandOutSocket(
         scoped_ptr<StreamSocket>(idle_socket.socket),
-        idle_socket.socket->WasEverUsed(),
+        reuse_type,
         LoadTimingInfo::ConnectTiming(),
         request.handle(),
         idle_time,
@@ -878,7 +884,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
     if (request) {
       LogBoundConnectJobToRequest(job_log.source(), *request);
       HandOutSocket(
-          socket.Pass(), false /* unused socket */, connect_timing,
+          socket.Pass(), ClientSocketHandle::UNUSED, connect_timing,
           request->handle(), base::TimeDelta(), group, request->net_log());
       request->net_log().EndEvent(NetLog::TYPE_SOCKET_POOL);
       InvokeUserCallbackLater(request->handle(), request->callback(), result);
@@ -898,7 +904,7 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
       RemoveConnectJob(job, group);
       if (socket.get()) {
         handed_out_socket = true;
-        HandOutSocket(socket.Pass(), false /* unused socket */,
+        HandOutSocket(socket.Pass(), ClientSocketHandle::UNUSED,
                       connect_timing, request->handle(), base::TimeDelta(),
                       group, request->net_log());
       }
@@ -963,7 +969,7 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest(
 
 void ClientSocketPoolBaseHelper::HandOutSocket(
     scoped_ptr<StreamSocket> socket,
-    bool reused,
+    ClientSocketHandle::SocketReuseType reuse_type,
     const LoadTimingInfo::ConnectTiming& connect_timing,
     ClientSocketHandle* handle,
     base::TimeDelta idle_time,
@@ -971,12 +977,12 @@ void ClientSocketPoolBaseHelper::HandOutSocket(
     const BoundNetLog& net_log) {
   DCHECK(socket);
   handle->SetSocket(socket.Pass());
-  handle->set_is_reused(reused);
+  handle->set_reuse_type(reuse_type);
   handle->set_idle_time(idle_time);
   handle->set_pool_id(pool_generation_number_);
   handle->set_connect_timing(connect_timing);
 
-  if (reused) {
+  if (handle->is_reused()) {
     net_log.AddEvent(
         NetLog::TYPE_SOCKET_POOL_REUSED_AN_EXISTING_SOCKET,
         NetLog::IntegerCallback(
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 2c2ddb5..8a0a1f3 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -46,6 +46,7 @@
 #include "net/base/network_change_notifier.h"
 #include "net/base/priority_queue.h"
 #include "net/base/request_priority.h"
+#include "net/socket/client_socket_handle.h"
 #include "net/socket/client_socket_pool.h"
 #include "net/socket/stream_socket.h"
 
@@ -528,7 +529,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper
 
   // Assigns |socket| to |handle| and updates |group|'s counters appropriately.
   void HandOutSocket(scoped_ptr<StreamSocket> socket,
-                     bool reused,
+                     ClientSocketHandle::SocketReuseType reuse_type,
                      const LoadTimingInfo::ConnectTiming& connect_timing,
                      ClientSocketHandle* handle,
                      base::TimeDelta time_idle,