After draining the body of a 401/407 response, verify that

the keep-alive connection is still connected and idle before
reusing it for authentication restart.  An impatient server
may have closed the connection while waiting for the user to
enter the username and password.

In socket_test_util.cc, return the mock ERR_UNEXPECTED error
synchronously.

R=eroman
BUG=21675
TEST=new unit test
Review URL: http://codereview.chromium.org/389007

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

4 files changed, 127 insertions, 11 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index ddb74d9..58dd97f 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -276,8 +276,11 @@ void HttpNetworkTransaction::PrepareForAuthRestart(HttpAuth::Target target) {
 }
 
 void HttpNetworkTransaction::DidDrainBodyForAuthRestart(bool keep_alive) {
-  if (keep_alive) {
+  if (keep_alive && connection_.socket()->IsConnectedAndIdle()) {
+    // We should call connection_.set_idle_time(), but this doesn't occur
+    // often enough to be worth the trouble.
     next_state_ = STATE_SEND_REQUEST;
+    connection_.set_is_reused(true);
     reused_socket_ = true;
   } else {
     next_state_ = STATE_INIT_CONNECTION;
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index c06e21a4..f12fd1f 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -877,15 +877,10 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAliveNoBody) {
               "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"),
   };
 
-  // Respond with 5 kb of response body.
-  std::string large_body_string("Unauthorized");
-  large_body_string.append(5 * 1024, ' ');
-  large_body_string.append("\r\n");
-
   MockRead data_reads1[] = {
     MockRead("HTTP/1.1 401 Unauthorized\r\n"),
     MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
-    MockRead("Content-Length: 0\r\n\r\n"),
+    MockRead("Content-Length: 0\r\n\r\n"),  // No response body.
 
     // Lastly, the server responds with the actual content.
     MockRead("HTTP/1.1 200 OK\r\n"),
@@ -1010,6 +1005,95 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAliveLargeBody) {
 }
 
 // Test the request-challenge-retry sequence for basic auth, over a keep-alive
+// connection, but the server gets impatient and closes the connection.
+TEST_F(HttpNetworkTransactionTest, BasicAuthKeepAliveImpatientServer) {
+  SessionDependencies session_deps;
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(CreateSession(&session_deps)));
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.google.com/");
+  request.load_flags = 0;
+
+  MockWrite data_writes1[] = {
+    MockWrite("GET / HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n\r\n"),
+    // This simulates the seemingly successful write to a closed connection
+    // if the bug is not fixed.
+    MockWrite("GET / HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n"
+              "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"),
+  };
+
+  MockRead data_reads1[] = {
+    MockRead("HTTP/1.1 401 Unauthorized\r\n"),
+    MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
+    MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"),
+    MockRead("Content-Length: 14\r\n\r\n"),
+    // Tell MockTCPClientSocket to simulate the server closing the connection.
+    MockRead(false, ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ),
+    MockRead("Unauthorized\r\n"),
+    MockRead(false, OK),  // The server closes the connection.
+  };
+
+  // After calling trans->RestartWithAuth(), this is the request we should
+  // be issuing -- the final header line contains the credentials.
+  MockWrite data_writes2[] = {
+    MockWrite("GET / HTTP/1.1\r\n"
+              "Host: www.google.com\r\n"
+              "Connection: keep-alive\r\n"
+              "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"),
+  };
+
+  // Lastly, the server responds with the actual content.
+  MockRead data_reads2[] = {
+    MockRead("HTTP/1.1 200 OK\r\n"),
+    MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"),
+    MockRead("Content-Length: 100\r\n\r\n"),
+    MockRead(false, OK),
+  };
+
+  StaticSocketDataProvider data1(data_reads1, data_writes1);
+  StaticSocketDataProvider data2(data_reads2, data_writes2);
+  session_deps.socket_factory.AddSocketDataProvider(&data1);
+  session_deps.socket_factory.AddSocketDataProvider(&data2);
+
+  TestCompletionCallback callback1;
+
+  int rv = trans->Start(&request, &callback1, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback1.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_FALSE(response == NULL);
+
+  // The password prompt info should have been set in response->auth_challenge.
+  EXPECT_FALSE(response->auth_challenge.get() == NULL);
+
+  EXPECT_EQ(L"www.google.com:80", response->auth_challenge->host_and_port);
+  EXPECT_EQ(L"MyRealm1", response->auth_challenge->realm);
+  EXPECT_EQ(L"basic", response->auth_challenge->scheme);
+
+  TestCompletionCallback callback2;
+
+  rv = trans->RestartWithAuth(L"foo", L"bar", &callback2);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback2.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  response = trans->GetResponseInfo();
+  ASSERT_FALSE(response == NULL);
+  EXPECT_TRUE(response->auth_challenge.get() == NULL);
+  EXPECT_EQ(100, response->headers->GetContentLength());
+}
+
+// Test the request-challenge-retry sequence for basic auth, over a keep-alive
 // proxy connection, when setting up an SSL tunnel.
 TEST_F(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   // Configure against proxy server "myproxy:70".
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index 239a81d..ac9411e 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -66,8 +66,9 @@ MockTCPClientSocket::MockTCPClientSocket(const net::AddressList& addresses,
     : addresses_(addresses),
       data_(data),
       read_offset_(0),
-      read_data_(true, net::ERR_UNEXPECTED),
+      read_data_(false, net::ERR_UNEXPECTED),
       need_read_data_(true),
+      peer_closed_connection_(false),
       pending_buf_(NULL),
       pending_buf_len_(0),
       pending_callback_(NULL) {
@@ -87,9 +88,13 @@ int MockTCPClientSocket::Connect(net::CompletionCallback* callback,
   return data_->connect_data().result;
 }
 
+bool MockTCPClientSocket::IsConnected() const {
+  return connected_ && !peer_closed_connection_;
+}
+
 int MockTCPClientSocket::Read(net::IOBuffer* buf, int buf_len,
                               net::CompletionCallback* callback) {
-  if (!IsConnected())
+  if (!connected_)
     return net::ERR_UNEXPECTED;
 
   // If the buffer is already in use, a read is already in progress!
@@ -102,6 +107,12 @@ int MockTCPClientSocket::Read(net::IOBuffer* buf, int buf_len,
 
   if (need_read_data_) {
     read_data_ = data_->GetNextRead();
+    if (read_data_.result == ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ) {
+      // This MockRead is just a marker to instruct us to set
+      // peer_closed_connection_.  Skip it and get the next one.
+      read_data_ = data_->GetNextRead();
+      peer_closed_connection_ = true;
+    }
     // ERR_IO_PENDING means that the SocketDataProvider is taking responsibility
     // to complete the async IO manually later (via OnReadComplete).
     if (read_data_.result == ERR_IO_PENDING) {
@@ -119,7 +130,7 @@ int MockTCPClientSocket::Write(net::IOBuffer* buf, int buf_len,
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
 
-  if (!IsConnected())
+  if (!connected_)
     return net::ERR_UNEXPECTED;
 
   std::string data(buf->data(), buf_len);
@@ -304,7 +315,7 @@ DynamicSocketDataProvider::DynamicSocketDataProvider()
 
 MockRead DynamicSocketDataProvider::GetNextRead() {
   if (reads_.empty())
-    return MockRead(true, ERR_UNEXPECTED);
+    return MockRead(false, ERR_UNEXPECTED);
   MockRead result = reads_.front();
   if (short_read_limit_ == 0 || result.data_len <= short_read_limit_) {
     reads_.pop_front();
diff --git a/net/socket/socket_test_util.h b/net/socket/socket_test_util.h
index c369435..d16dcb4 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -25,6 +25,15 @@
 
 namespace net {
 
+enum {
+  // A private network error code used by the socket test utility classes.
+  // If the |result| member of a MockRead is
+  // ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ, that MockRead is just a
+  // marker that indicates the peer will close the connection after the next
+  // MockRead.  The other members of that MockRead are ignored.
+  ERR_TEST_PEER_CLOSE_AFTER_NEXT_MOCK_READ = -10000,
+};
+
 class ClientSocket;
 class LoadLog;
 class MockClientSocket;
@@ -285,6 +294,8 @@ class MockClientSocket : public net::SSLClientSocket {
   void RunCallback(net::CompletionCallback*, int result);
 
   ScopedRunnableMethodFactory<MockClientSocket> method_factory_;
+
+  // True if Connect completed successfully and Disconnect hasn't been called.
   bool connected_;
 };
 
@@ -296,6 +307,8 @@ class MockTCPClientSocket : public MockClientSocket {
   // ClientSocket methods:
   virtual int Connect(net::CompletionCallback* callback,
                       LoadLog* load_log);
+  virtual bool IsConnected() const;
+  virtual bool IsConnectedAndIdle() const { return IsConnected(); }
 
   // Socket methods:
   virtual int Read(net::IOBuffer* buf, int buf_len,
@@ -317,6 +330,11 @@ class MockTCPClientSocket : public MockClientSocket {
   net::MockRead read_data_;
   bool need_read_data_;
 
+  // True if the peer has closed the connection.  This allows us to simulate
+  // the recv(..., MSG_PEEK) call in the IsConnectedAndIdle method of the real
+  // TCPClientSocket.
+  bool peer_closed_connection_;
+
   // While an asynchronous IO is pending, we save our user-buffer state.
   net::IOBuffer* pending_buf_;
   int pending_buf_len_;