Various cleanups FTP-related.

- use better name for FTP LIST parsing code in about:credits
- don't open a second data socket
- add a comment explaining why we close the data socket at one point

TEST=Covered by net_unittests.
BUG=none

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

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

5 files changed, 24 insertions, 42 deletions

diff --git a/chrome/browser/resources/about_credits.html b/chrome/browser/resources/about_credits.html
index 346edc9..cbdc9ca 100644
--- a/chrome/browser/resources/about_credits.html
+++ b/chrome/browser/resources/about_credits.html
@@ -1323,13 +1323,13 @@ along with FFmpeg; if not, write to:</p>
 </div>
 </div>
 
-<!-- parseftp -->
+<!-- ParseFTPList -->
 <div class="product">
-<span class="title">parseftp</span>
+<span class="title">ParseFTPList</span>
 <a class="show" href="#" onclick="return toggle(this);">show license</a>
-<span class="homepage"><a href="http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.h">homepage</a></span>
+<span class="homepage"><a href="http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/ParseFTPList.cpp">homepage</a></span>
 <div class="licence">
-<h3>parseftp is licensed as follows:</h3>
+<h3>ParseFTPList is licensed as follows:</h3>
 <p>The contents of this file are subject to the Mozilla Public License Version
 1.1 (the "License"); you may not use this file except in compliance with
 the License. You may obtain a copy of the License at
diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 753aee2..e1a4114caf 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -865,7 +865,10 @@ int FtpNetworkTransaction::ProcessResponseRETR(
 
       DCHECK(!retr_failed_);  // Should not get here twice.
       retr_failed_ = true;
-      next_state_ = STATE_CTRL_WRITE_PASV;
+
+      // It's possible that RETR failed because the path is a directory.
+      // Try CWD next, to see if that's the case.
+      next_state_ = STATE_CTRL_WRITE_CWD;
       break;
     default:
       NOTREACHED();
@@ -1023,7 +1026,8 @@ int FtpNetworkTransaction::DoDataRead() {
 
   if (data_socket_ == NULL || !data_socket_->IsConnected()) {
     // If we don't destroy the data socket completely, some servers will wait
-    // for us (http://crbug.com/21127).
+    // for us (http://crbug.com/21127). The half-closed TCP connection needs
+    // to be closed on our side too.
     data_socket_.reset();
 
     // No more data so send QUIT Command now and wait for response.
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index 95b1e8e..140a37d 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -47,7 +47,6 @@ class FtpMockControlSocket : public DynamicMockSocket {
     PRE_MDTM,
     PRE_LIST,
     PRE_RETR,
-    PRE_PASV2,
     PRE_CWD,
     PRE_QUIT,
     QUIT
@@ -83,7 +82,7 @@ class FtpMockControlSocket : public DynamicMockSocket {
                       "200 TYPE is now 8-bit binary\r\n");
       case PRE_PASV:
         return Verify("PASV\r\n", data, PRE_SIZE,
-                      "227 Entering Passive Mode (127,0,0,1,123,456)\r\n");
+                      "227 Entering Passive Mode 127,0,0,1,123,456\r\n");
       case PRE_QUIT:
         return Verify("QUIT\r\n", data, QUIT, "221 Goodbye.\r\n");
       default:
@@ -171,12 +170,8 @@ class FtpMockControlSocketDirectoryListing : public FtpMockControlSocket {
         return Verify("MDTM /\r\n", data, PRE_RETR,
                       "213 20070221112533\r\n");
       case PRE_RETR:
-        return Verify("RETR /\r\n", data, PRE_PASV2,
+        return Verify("RETR /\r\n", data, PRE_CWD,
                       "550 Can't download directory\r\n");
-      case PRE_PASV2:
-        // Parser should also accept format without parentheses.
-        return Verify("PASV\r\n", data, PRE_CWD,
-                      "227 Entering Passive Mode 127,0,0,1,123,456\r\n");
       case PRE_CWD:
         return Verify("CWD /\r\n", data, PRE_LIST, "200 OK\r\n");
       case PRE_LIST:
@@ -298,9 +293,6 @@ class FtpMockControlSocketFileDownloadRetrFail
     if (InjectFault())
       return MockWriteResult(true, data.length());
     switch (state()) {
-      case PRE_PASV2:
-        return Verify("PASV\r\n", data, PRE_CWD,
-                      "227 Entering Passive Mode (127,0,0,1,123,456)\r\n");
       case PRE_CWD:
         return Verify("CWD /file\r\n", data, PRE_QUIT,
                       "550 file is a directory\r\n");
@@ -391,12 +383,9 @@ class FtpNetworkTransactionTest : public PlatformTest {
     MockRead data_reads[] = {
       MockRead(mock_data.c_str()),
     };
-    // TODO(phajdan.jr): FTP transaction should not open two data sockets.
-    StaticMockSocket data_socket1(data_reads, NULL);
-    StaticMockSocket data_socket2(data_reads, NULL);
+    StaticMockSocket data_socket(data_reads, NULL);
     mock_socket_factory_.AddMockSocket(ctrl_socket);
-    mock_socket_factory_.AddMockSocket(&data_socket1);
-    mock_socket_factory_.AddMockSocket(&data_socket2);
+    mock_socket_factory_.AddMockSocket(&data_socket);
     FtpRequestInfo request_info = GetRequestInfo(request);
     EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
     ASSERT_EQ(ERR_IO_PENDING,
@@ -774,16 +763,6 @@ TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailMdtm) {
                         OK);
 }
 
-TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailPasv2) {
-  FtpMockControlSocketDirectoryListing ctrl_socket;
-  TransactionFailHelper(&ctrl_socket,
-                        "ftp://host",
-                        FtpMockControlSocket::PRE_PASV2,
-                        FtpMockControlSocket::PRE_QUIT,
-                        "500 failed pasv\r\n",
-                        ERR_FAILED);
-}
-
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailCwd) {
   FtpMockControlSocketDirectoryListing ctrl_socket;
   TransactionFailHelper(&ctrl_socket,
@@ -899,7 +878,7 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionFileNotFound) {
   TransactionFailHelper(&ctrl_socket,
                         "ftp://host/file",
                         FtpMockControlSocket::PRE_RETR,
-                        FtpMockControlSocket::PRE_PASV2,
+                        FtpMockControlSocket::PRE_CWD,
                         "550 cannot open file\r\n",
                         ERR_FILE_NOT_FOUND);
 }
diff --git a/net/url_request/url_request_new_ftp_job.cc b/net/url_request/url_request_new_ftp_job.cc
index 40b23c4..7755503 100644
--- a/net/url_request/url_request_new_ftp_job.cc
+++ b/net/url_request/url_request_new_ftp_job.cc
@@ -300,7 +300,12 @@ int URLRequestNewFtpJob::ProcessFtpDir(net::IOBuffer *buf,
         break;
     }
   }
-  LogFtpServerType(state);
+
+  // We can't recognize server type based on empty directory listings. Only log
+  // server type when we have enough data to recognize one.
+  if (state.parsed_one)
+    LogFtpServerType(state.lstyle);
+
   directory_html_.append(file_entry);
   size_t bytes_to_copy = std::min(static_cast<size_t>(buf_size),
                                   directory_html_.length());
@@ -311,14 +316,8 @@ int URLRequestNewFtpJob::ProcessFtpDir(net::IOBuffer *buf,
   return bytes_to_copy;
 }
 
-void URLRequestNewFtpJob::LogFtpServerType(
-    const struct net::list_state& list_state) {
-  // We can't recognize server type based on empty directory listings. Don't log
-  // that as unknown, it's misleading.
-  if (!list_state.parsed_one)
-    return;
-
-  switch (list_state.lstyle) {
+void URLRequestNewFtpJob::LogFtpServerType(char server_type) {
+  switch (server_type) {
     case 'E':
       net::UpdateFtpServerTypeHistograms(net::SERVER_EPLF);
       break;
diff --git a/net/url_request/url_request_new_ftp_job.h b/net/url_request/url_request_new_ftp_job.h
index fade74d..e29687f 100644
--- a/net/url_request/url_request_new_ftp_job.h
+++ b/net/url_request/url_request_new_ftp_job.h
@@ -56,7 +56,7 @@ class URLRequestNewFtpJob : public URLRequestJob {
 
   int ProcessFtpDir(net::IOBuffer *buf, int buf_size, int bytes_read);
 
-  void LogFtpServerType(const struct net::list_state& list_state);
+  void LogFtpServerType(char server_type);
 
   net::FtpRequestInfo request_info_;
   scoped_ptr<net::FtpTransaction> transaction_;