Fix transaction hang when downloading certain FTP files.

TEST=Covered by net_unittests.
http://crbug.com/19855

Review URL: http://codereview.chromium.org/174428

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

4 files changed, 105 insertions, 4 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 6b6b8c4..09d8d90 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -133,6 +133,9 @@ LoadState FtpNetworkTransaction::GetLoadState() const {
   if (next_state_ == STATE_DATA_READ_COMPLETE)
     return LOAD_STATE_READING_RESPONSE;
 
+  if (command_sent_ == COMMAND_RETR && read_data_buf_.get())
+    return LOAD_STATE_READING_RESPONSE;
+
   if (command_sent_ == COMMAND_QUIT)
     return LOAD_STATE_IDLE;
 
@@ -808,7 +811,10 @@ int FtpNetworkTransaction::ProcessResponseRETR(
     const FtpCtrlResponse& response) {
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
-      next_state_ = STATE_CTRL_READ;
+      // We want the client to start reading the response at this point.
+      // It got here either through Start or RestartWithAuth. We want that
+      // method to complete. Not setting next state here will make DoLoop exit
+      // and in turn make Start/RestartWithAuth complete.
       break;
     case ERROR_CLASS_OK:
       next_state_ = STATE_CTRL_WRITE_QUIT;
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index 032227e..aa143c0 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -208,6 +208,29 @@ class FtpMockControlSocketFileDownload : public FtpMockControlSocket {
   DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketFileDownload);
 };
 
+class FtpMockControlSocketFileDownloadAcceptedDataConnection
+    : public FtpMockControlSocketFileDownload {
+ public:
+  FtpMockControlSocketFileDownloadAcceptedDataConnection() {
+  }
+
+  virtual MockWriteResult OnWrite(const std::string& data) {
+    if (InjectFault())
+      return MockWriteResult(true, data.length());
+    switch (state()) {
+      case PRE_RETR:
+        return Verify("RETR /file\r\n", data, PRE_QUIT,
+                      "150 Accepted Data Connection\r\n");
+      default:
+        return FtpMockControlSocketFileDownload::OnWrite(data);
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(
+      FtpMockControlSocketFileDownloadAcceptedDataConnection);
+};
+
 class FtpMockControlSocketFileDownloadTransferStarting
     : public FtpMockControlSocketFileDownload {
  public:
@@ -410,6 +433,52 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionShortReads5) {
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK);
 }
 
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionAcceptedDataConnection) {
+  FtpMockControlSocketFileDownloadAcceptedDataConnection ctrl_socket;
+  std::string mock_data("mock-data");
+  MockRead data_reads[] = {
+    MockRead(mock_data.c_str()),
+  };
+  StaticMockSocket data_socket1(data_reads, NULL);
+  mock_socket_factory_.AddMockSocket(&ctrl_socket);
+  mock_socket_factory_.AddMockSocket(&data_socket1);
+  FtpRequestInfo request_info = GetRequestInfo("ftp://host/file");
+
+  // Start the transaction.
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Start(&request_info, &callback_, NULL));
+  EXPECT_EQ(OK, callback_.WaitForResult());
+
+  // The transaction fires the callback when we can start reading data.
+  EXPECT_EQ(FtpMockControlSocket::PRE_QUIT, ctrl_socket.state());
+  EXPECT_EQ(LOAD_STATE_SENDING_REQUEST, transaction_.GetLoadState());
+  scoped_refptr<IOBuffer> io_buffer(new IOBuffer(kBufferSize));
+  memset(io_buffer->data(), 0, kBufferSize);
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Read(io_buffer.get(), kBufferSize, &callback_));
+  EXPECT_EQ(LOAD_STATE_READING_RESPONSE, transaction_.GetLoadState());
+  EXPECT_EQ(static_cast<int>(mock_data.length()),
+            callback_.WaitForResult());
+  EXPECT_EQ(LOAD_STATE_READING_RESPONSE, transaction_.GetLoadState());
+  EXPECT_EQ(mock_data, std::string(io_buffer->data(), mock_data.length()));
+
+  // FTP server should disconnect the data socket. It is also a signal for the
+  // FtpNetworkTransaction that the data transfer is finished.
+  ClientSocket* data_socket = mock_socket_factory_.GetMockTCPClientSocket(1);
+  ASSERT_TRUE(data_socket);
+  data_socket->Disconnect();
+
+  // We should issue Reads until one returns EOF...
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Read(io_buffer.get(), kBufferSize, &callback_));
+
+  // Make sure the transaction finishes cleanly.
+  EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
+  EXPECT_EQ(0, callback_.WaitForResult());
+  EXPECT_EQ(FtpMockControlSocket::QUIT, ctrl_socket.state());
+  EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
+}
+
 TEST_F(FtpNetworkTransactionTest, DownloadTransactionTransferStarting) {
   FtpMockControlSocketFileDownloadTransferStarting ctrl_socket;
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK);
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index b1af59f..4b6a3ad 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -290,17 +290,32 @@ void MockClientSocketFactory::ResetNextMockIndexes() {
   mock_ssl_sockets_.ResetNextIndex();
 }
 
+ClientSocket* MockClientSocketFactory::GetMockTCPClientSocket(int index) const {
+  return tcp_client_sockets_[index];
+}
+
+SSLClientSocket* MockClientSocketFactory::GetMockSSLClientSocket(
+    int index) const {
+  return ssl_client_sockets_[index];
+}
+
 ClientSocket* MockClientSocketFactory::CreateTCPClientSocket(
     const AddressList& addresses) {
-  return new MockTCPClientSocket(addresses, mock_sockets_.GetNext());
+  ClientSocket* socket =
+      new MockTCPClientSocket(addresses, mock_sockets_.GetNext());
+  tcp_client_sockets_.push_back(socket);
+  return socket;
 }
 
 SSLClientSocket* MockClientSocketFactory::CreateSSLClientSocket(
     ClientSocket* transport_socket,
     const std::string& hostname,
     const SSLConfig& ssl_config) {
-  return new MockSSLClientSocket(transport_socket, hostname, ssl_config,
-                                 mock_ssl_sockets_.GetNext());
+  SSLClientSocket* socket =
+      new MockSSLClientSocket(transport_socket, hostname, ssl_config,
+                              mock_ssl_sockets_.GetNext());
+  ssl_client_sockets_.push_back(socket);
+  return socket;
 }
 
 int TestSocketRequest::WaitForResult() {
diff --git a/net/socket/socket_test_util.h b/net/socket/socket_test_util.h
index 76d4df1f..75f3a37 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -200,6 +200,13 @@ class MockClientSocketFactory : public ClientSocketFactory {
   void AddMockSSLSocket(MockSSLSocket* socket);
   void ResetNextMockIndexes();
 
+  // Return |index|-th ClientSocket (starting from 0) that the factory created.
+  ClientSocket* GetMockTCPClientSocket(int index) const;
+
+  // Return |index|-th SSLClientSocket (starting from 0) that the factory
+  // created.
+  SSLClientSocket* GetMockSSLClientSocket(int index) const;
+
   // ClientSocketFactory
   virtual ClientSocket* CreateTCPClientSocket(const AddressList& addresses);
   virtual SSLClientSocket* CreateSSLClientSocket(
@@ -210,6 +217,10 @@ class MockClientSocketFactory : public ClientSocketFactory {
  private:
   MockSocketArray<MockSocket> mock_sockets_;
   MockSocketArray<MockSSLSocket> mock_ssl_sockets_;
+
+  // Store pointers to handed out sockets in case the test wants to get them.
+  std::vector<ClientSocket*> tcp_client_sockets_;
+  std::vector<SSLClientSocket*> ssl_client_sockets_;
 };
 
 class MockClientSocket : public net::SSLClientSocket {