Do not send PASV until RETR or LIST

net/ftp/ftp_network_transaction_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ftp/ftp_network_transaction.h"
 
 #include "build/build_config.h"
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
@@ skipping 23 matching lines @@
 class FtpSocketDataProvider : public DynamicSocketDataProvider {
  public:
   enum State {
     NONE,
     PRE_USER,
     PRE_PASSWD,
     PRE_SYST,
     PRE_PWD,
     PRE_TYPE,
     PRE_SIZE,
-    PRE_EPSV,
-    PRE_PASV,
+    PRE_LIST_EPSV,
+    PRE_LIST_PASV,
     PRE_LIST,
     PRE_RETR,
     PRE_RETR_EPSV,
     PRE_RETR_PASV,
-    PRE_CWD_EPSV,
-    PRE_CWD_PASV,
     PRE_CWD,
     PRE_QUIT,
     PRE_NOPASV,
     QUIT
   };
 
   FtpSocketDataProvider()
       : failure_injection_state_(NONE),
         multiline_welcome_(false),
         use_epsv_(true),
@@ skipping 15 matching lines @@
           return Verify("PASS chrome@example.com\r\n", data, PRE_SYST,
                         multiline_welcome_ ? response_multi : response_one);
         }
       case PRE_SYST:
         return Verify("SYST\r\n", data, PRE_PWD, "215 UNIX\r\n");
       case PRE_PWD:
         return Verify("PWD\r\n", data, PRE_TYPE,
                       "257 \"/\" is your current location\r\n");
       case PRE_TYPE:
         return Verify(std::string("TYPE ") + data_type_ + "\r\n", data,
-                      use_epsv_ ? PRE_EPSV : PRE_PASV,
-                      "200 TYPE set successfully\r\n");
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_SIZE,
+                      PRE_SIZE, "200 TYPE set successfully\r\n");
+      case PRE_LIST_EPSV:
+        return Verify("EPSV\r\n", data, PRE_LIST,
                       "227 Entering Extended Passive Mode (|||31744|)\r\n");
-      case PRE_CWD_EPSV:
-        return Verify("EPSV\r\n", data, PRE_CWD,
-                      "227 Entering Extended Passive Mode (|||31744|)\r\n");
+      case PRE_LIST_PASV:
+        return Verify("PASV\r\n", data, PRE_LIST,
+                      "227 Entering Passive Mode 127,0,0,1,123,456\r\n");
       case PRE_RETR_EPSV:
         return Verify("EPSV\r\n", data, PRE_RETR,
                       "227 Entering Extended Passive Mode (|||31744|)\r\n");
-      case PRE_CWD_PASV:
-        return Verify("PASV\r\n", data, PRE_CWD,
-                      "227 Entering Passive Mode 127,0,0,1,123,456\r\n");
       case PRE_RETR_PASV:
         return Verify("PASV\r\n", data, PRE_RETR,
                       "227 Entering Passive Mode 127,0,0,1,123,456\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");
       case PRE_NOPASV:
         // Use unallocated 599 FTP error code to make sure it falls into the
         // generic ERR_FTP_FAILED bucket.
         return Verify("PASV\r\n", data, PRE_QUIT,
                       "599 fail\r\n");
       case PRE_QUIT:
         return Verify("QUIT\r\n", data, QUIT, "221 Goodbye.\r\n");
       default:
         NOTREACHED() << "State not handled " << state();
         return MockWriteResult(ASYNC, ERR_UNEXPECTED);
@@ skipping 86 matching lines @@
 class FtpSocketDataProviderDirectoryListing : public FtpSocketDataProvider {
  public:
   FtpSocketDataProviderDirectoryListing() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SIZE:
-        return Verify("SIZE /\r\n", data,
-                      use_epsv() ? PRE_CWD_EPSV : PRE_CWD_PASV,
+        return Verify("SIZE /\r\n", data, PRE_CWD,
                       "550 I can only retrieve regular files\r\n");
       case PRE_CWD:
-        return Verify("CWD /\r\n", data, PRE_LIST, "200 OK\r\n");
+        return Verify("CWD /\r\n", data,
+                      use_epsv() ? PRE_LIST_EPSV : PRE_LIST_PASV, "200 OK\r\n");

[eroman, 2015/01/26 23:00:38]

Is there a test which covers the circumstances of 450687 ?

[xunjieli, 2015/01/27 14:42:02]

Unfortunately there isn't one. I considered adding one, but I don't think we can
emulate the server behavior, which is no longer relevant. In 450687, we receive
a not-directory error, and we throw away the first data connection and establish
a second data connection by sending a second PASV. Then the server got confused
and closed the second data connection. 
But after this change, there's no longer a second data connection. We establish
the first data connection after we know whether it's a file or directory. We
never attempt to send PASV again. So I think the situation in 450687 can no
longer be simulated in the test. With the removal of the second data connection,
the situation in 450687 is just a normal file retrieval covered in
FtpNetworkTransactionTest.DownloadTransaction.

[davidben, 2015/01/27 19:59:00]

I think we're implicitly testing it by asserting that only one PASV is sent.
(And we were arguably simulating the old situation before by asserting our old
trace. We just didn't account for some server barfing on it.)

       case PRE_LIST:
         return Verify("LIST -l\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderDirectoryListing);
 };
 
 class FtpSocketDataProviderDirectoryListingWithPasvFallback
     : public FtpSocketDataProviderDirectoryListing {
  public:
   FtpSocketDataProviderDirectoryListingWithPasvFallback() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_PASV,
+      case PRE_LIST_EPSV:
+        return Verify("EPSV\r\n", data, PRE_LIST_PASV,
                       "500 no EPSV for you\r\n");
       case PRE_SIZE:
-        return Verify("SIZE /\r\n", data, PRE_CWD_PASV,
+        return Verify("SIZE /\r\n", data, PRE_CWD,
                       "550 I can only retrieve regular files\r\n");
       default:
         return FtpSocketDataProviderDirectoryListing::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(
       FtpSocketDataProviderDirectoryListingWithPasvFallback);
 };
@@ skipping 26 matching lines @@
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SYST:
         return Verify("SYST\r\n", data, PRE_PWD, "215 VMS\r\n");
       case PRE_PWD:
         return Verify("PWD\r\n", data, PRE_TYPE,
                       "257 \"ANONYMOUS_ROOT:[000000]\"\r\n");
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_PASV, "500 Invalid command\r\n");
+      case PRE_LIST_EPSV:
+        return Verify("EPSV\r\n", data, PRE_LIST_PASV,
+                      "500 Invalid command\r\n");
       case PRE_SIZE:
-        return Verify("SIZE ANONYMOUS_ROOT:[000000]dir\r\n", data, PRE_CWD_PASV,
+        return Verify("SIZE ANONYMOUS_ROOT:[000000]dir\r\n", data, PRE_CWD,
                       "550 I can only retrieve regular files\r\n");
       case PRE_CWD:
-        return Verify("CWD ANONYMOUS_ROOT:[dir]\r\n", data, PRE_LIST,
-                      "200 OK\r\n");
+        return Verify("CWD ANONYMOUS_ROOT:[dir]\r\n", data,
+                      use_epsv() ? PRE_LIST_EPSV : PRE_LIST_PASV, "200 OK\r\n");
       case PRE_LIST:
         return Verify("LIST *.*;0\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderVMSDirectoryListing);
 };
 
 class FtpSocketDataProviderVMSDirectoryListingRootDirectory
     : public FtpSocketDataProvider {
  public:
   FtpSocketDataProviderVMSDirectoryListingRootDirectory() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SYST:
         return Verify("SYST\r\n", data, PRE_PWD, "215 VMS\r\n");
       case PRE_PWD:
         return Verify("PWD\r\n", data, PRE_TYPE,
                       "257 \"ANONYMOUS_ROOT:[000000]\"\r\n");
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_PASV,
+      case PRE_LIST_EPSV:
+        return Verify("EPSV\r\n", data, PRE_LIST_PASV,
                       "500 EPSV command unknown\r\n");
       case PRE_SIZE:
-        return Verify("SIZE ANONYMOUS_ROOT\r\n", data, PRE_CWD_PASV,
+        return Verify("SIZE ANONYMOUS_ROOT\r\n", data, PRE_CWD,
                       "550 I can only retrieve regular files\r\n");
       case PRE_CWD:
-        return Verify("CWD ANONYMOUS_ROOT:[000000]\r\n", data, PRE_LIST,
-                      "200 OK\r\n");
+        return Verify("CWD ANONYMOUS_ROOT:[000000]\r\n", data,
+                      use_epsv() ? PRE_LIST_EPSV : PRE_LIST_PASV, "200 OK\r\n");
       case PRE_LIST:
         return Verify("LIST *.*;0\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(
       FtpSocketDataProviderVMSDirectoryListingRootDirectory);
 };
 
 class FtpSocketDataProviderFileDownloadWithFileTypecode
     : public FtpSocketDataProvider {
  public:
   FtpSocketDataProviderFileDownloadWithFileTypecode() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SIZE:
-        return Verify("SIZE /file\r\n", data, PRE_RETR,
-                      "213 18\r\n");
+        return Verify("SIZE /file\r\n", data,
+                      use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV, "213 18\r\n");
       case PRE_RETR:
         return Verify("RETR /file\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownloadWithFileTypecode);
 };
 
 class FtpSocketDataProviderFileDownload : public FtpSocketDataProvider {
  public:
   FtpSocketDataProviderFileDownload() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SIZE:
-        return Verify("SIZE /file\r\n", data, PRE_CWD,
-                      "213 18\r\n");
+        return Verify("SIZE /file\r\n", data, PRE_CWD, "213 18\r\n");
       case PRE_CWD:
         return Verify("CWD /file\r\n", data,
                       use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
                       "550 Not a directory\r\n");
       case PRE_RETR:
         return Verify("RETR /file\r\n", data, PRE_QUIT, "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownload);
 };
 
 class FtpSocketDataProviderFileNotFound : public FtpSocketDataProvider {
  public:
   FtpSocketDataProviderFileNotFound() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SIZE:
-        return Verify("SIZE /file\r\n", data,
-                      use_epsv() ? PRE_CWD_EPSV : PRE_CWD_PASV,
+        return Verify("SIZE /file\r\n", data, PRE_CWD,
                       "550 File Not Found\r\n");
       case PRE_CWD:
         return Verify("CWD /file\r\n", data,
                       use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
                       "550 File Not Found\r\n");
       case PRE_RETR:
         return Verify("RETR /file\r\n", data, PRE_QUIT,
                       "550 File Not Found\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileNotFound);
 };
 
 class FtpSocketDataProviderFileDownloadWithPasvFallback
     : public FtpSocketDataProviderFileDownload {
  public:
   FtpSocketDataProviderFileDownloadWithPasvFallback() {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_PASV,
-                      "500 No can do\r\n");
+      case PRE_RETR_EPSV:
+        return Verify("EPSV\r\n", data, PRE_RETR_PASV, "500 No can do\r\n");
       case PRE_CWD:
-        return Verify("CWD /file\r\n", data, PRE_RETR_PASV,
+        return Verify("CWD /file\r\n", data,
+                      use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
                       "550 Not a directory\r\n");
       default:
         return FtpSocketDataProviderFileDownload::OnWrite(data);
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderFileDownloadWithPasvFallback);
 };
 
@@ skipping 53 matching lines @@
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
       case PRE_SYST:
         return Verify("SYST\r\n", data, PRE_PWD, "215 VMS\r\n");
       case PRE_PWD:
         return Verify("PWD\r\n", data, PRE_TYPE,
                       "257 \"ANONYMOUS_ROOT:[000000]\"\r\n");
-      case PRE_EPSV:
-        return Verify("EPSV\r\n", data, PRE_PASV,
+      case PRE_LIST_EPSV:
+        return Verify("EPSV\r\n", data, PRE_LIST_PASV,
                       "500 EPSV command unknown\r\n");
       case PRE_SIZE:
         return Verify("SIZE ANONYMOUS_ROOT:[000000]file\r\n", data, PRE_CWD,
                       "213 18\r\n");
       case PRE_CWD:
-        return Verify("CWD ANONYMOUS_ROOT:[file]\r\n", data, PRE_RETR_PASV,
+        return Verify("CWD ANONYMOUS_ROOT:[file]\r\n", data,
+                      use_epsv() ? PRE_RETR_EPSV : PRE_RETR_PASV,
                       "550 Not a directory\r\n");
       case PRE_RETR:
         return Verify("RETR ANONYMOUS_ROOT:[000000]file\r\n", data, PRE_QUIT,
                       "200 OK\r\n");
       default:
         return FtpSocketDataProvider::OnWrite(data);
     }
   }
 
  private:
@@ skipping 114 matching lines @@
                                size_t epsv_response_length,
                                State expected_state)
       : epsv_response_(epsv_response),
         epsv_response_length_(epsv_response_length),
         expected_state_(expected_state) {}
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
-      case PRE_EPSV:
+      case PRE_RETR_EPSV:
         return Verify("EPSV\r\n", data, expected_state_,
                       epsv_response_, epsv_response_length_);
       default:
         return FtpSocketDataProviderFileDownload::OnWrite(data);
     }
   }
 
  private:
   const char* epsv_response_;
   const size_t epsv_response_length_;
   const State expected_state_;
 
   DISALLOW_COPY_AND_ASSIGN(FtpSocketDataProviderEvilEpsv);
 };
 
 class FtpSocketDataProviderEvilPasv
     : public FtpSocketDataProviderFileDownloadWithPasvFallback {
  public:
   FtpSocketDataProviderEvilPasv(const char* pasv_response, State expected_state)
       : pasv_response_(pasv_response),
         expected_state_(expected_state) {
   }
 
   MockWriteResult OnWrite(const std::string& data) override {
     if (InjectFault())
       return MockWriteResult(ASYNC, data.length());
     switch (state()) {
-      case PRE_PASV:
+      case PRE_RETR_PASV:
         return Verify("PASV\r\n", data, expected_state_, pasv_response_);
       default:
         return FtpSocketDataProviderFileDownloadWithPasvFallback::OnWrite(data);
     }
   }
 
  private:
   const char* pasv_response_;
   const State expected_state_;
 
@@ skipping 170 matching lines @@
     EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
   }
 
   void TransactionFailHelper(FtpSocketDataProvider* ctrl_socket,
                              const char* request,
                              FtpSocketDataProvider::State state,
                              FtpSocketDataProvider::State next_state,
                              const char* response,
                              int expected_result) {
     ctrl_socket->InjectFailure(state, next_state, response);
-    ExecuteTransaction(ctrl_socket, request, 1, expected_result);
+    ExecuteTransaction(ctrl_socket, request, 0, expected_result);
   }
 
   scoped_ptr<MockHostResolver> host_resolver_;
   scoped_refptr<FtpNetworkSession> session_;
   MockClientSocketFactory mock_socket_factory_;
   FtpNetworkTransaction transaction_;
   TestCompletionCallback callback_;
 };
 
 TEST_P(FtpNetworkTransactionTest, FailedLookup) {
@@ skipping 25 matching lines @@
   // ERR_NAME_NOT_RESOLVED, since that would mean the decorated hostname
   // was used.
   FtpSocketDataProviderEvilSize ctrl_socket(
       "213 99999999999999999999999999999999\r\n",
       FtpSocketDataProvider::PRE_QUIT);
   ExecuteTransaction(&ctrl_socket, "ftp://[::1]/file", 1, ERR_INVALID_RESPONSE);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransaction) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 
   EXPECT_TRUE(transaction_.GetResponseInfo()->is_directory_listing);
   EXPECT_EQ(-1, transaction_.GetResponseInfo()->expected_content_size);
   EXPECT_EQ((GetFamily() == AF_INET) ? "127.0.0.1" : "::1",
             transaction_.GetResponseInfo()->socket_address.host());
   EXPECT_EQ(21, transaction_.GetResponseInfo()->socket_address.port());
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionWithPasvFallback) {
   FtpSocketDataProviderDirectoryListingWithPasvFallback ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 
   EXPECT_TRUE(transaction_.GetResponseInfo()->is_directory_listing);
   EXPECT_EQ(-1, transaction_.GetResponseInfo()->expected_content_size);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionWithTypecode) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host;type=d", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host;type=d", 0, OK);
 
   EXPECT_TRUE(transaction_.GetResponseInfo()->is_directory_listing);
   EXPECT_EQ(-1, transaction_.GetResponseInfo()->expected_content_size);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionMultilineWelcome) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   ctrl_socket.set_multiline_welcome(true);
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionShortReads2) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   ctrl_socket.set_short_read_limit(2);
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionShortReads5) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   ctrl_socket.set_short_read_limit(5);
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionMultilineWelcomeShort) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   // The client will not consume all three 230 lines. That's good, we want to
   // test that scenario.
   ctrl_socket.allow_unconsumed_reads(true);
   ctrl_socket.set_multiline_welcome(true);
   ctrl_socket.set_short_read_limit(5);
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 // Regression test for http://crbug.com/60555.
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionZeroSize) {
   FtpSocketDataProviderDirectoryListingZeroSize ctrl_socket;
   ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionVMS) {
   FtpSocketDataProviderVMSDirectoryListing ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/dir", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/dir", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionVMSRootDirectory) {
   FtpSocketDataProviderVMSDirectoryListingRootDirectory ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionTransferStarting) {
   FtpSocketDataProviderDirectoryListingTransferStarting ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransaction) {
   FtpSocketDataProviderFileDownload ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 
   // We pass an artificial value of 18 as a response to the SIZE command.
   EXPECT_EQ(18, transaction_.GetResponseInfo()->expected_content_size);
   EXPECT_EQ((GetFamily() == AF_INET) ? "127.0.0.1" : "::1",
             transaction_.GetResponseInfo()->socket_address.host());
   EXPECT_EQ(21, transaction_.GetResponseInfo()->socket_address.port());
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionWithPasvFallback) {
   FtpSocketDataProviderFileDownloadWithPasvFallback ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 
   // We pass an artificial value of 18 as a response to the SIZE command.
   EXPECT_EQ(18, transaction_.GetResponseInfo()->expected_content_size);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionWithTypecodeA) {
   FtpSocketDataProviderFileDownloadWithFileTypecode ctrl_socket;
   ctrl_socket.set_data_type('A');
   ExecuteTransaction(&ctrl_socket, "ftp://host/file;type=a", 0, OK);
 
   // We pass an artificial value of 18 as a response to the SIZE command.
   EXPECT_EQ(18, transaction_.GetResponseInfo()->expected_content_size);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionWithTypecodeI) {
   FtpSocketDataProviderFileDownloadWithFileTypecode ctrl_socket;
   ExecuteTransaction(&ctrl_socket, "ftp://host/file;type=i", 0, OK);
 
   // We pass an artificial value of 18 as a response to the SIZE command.
   EXPECT_EQ(18, transaction_.GetResponseInfo()->expected_content_size);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionMultilineWelcome) {
   FtpSocketDataProviderFileDownload ctrl_socket;
   ctrl_socket.set_multiline_welcome(true);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionShortReads2) {
   FtpSocketDataProviderFileDownload ctrl_socket;
   ctrl_socket.set_short_read_limit(2);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionShortReads5) {
   FtpSocketDataProviderFileDownload ctrl_socket;
   ctrl_socket.set_short_read_limit(5);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionZeroSize) {
   FtpSocketDataProviderFileDownloadZeroSize ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionCWD451) {
   FtpSocketDataProviderFileDownloadCWD451 ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionVMS) {
   FtpSocketDataProviderVMSFileDownload ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionTransferStarting) {
   FtpSocketDataProviderFileDownloadTransferStarting ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionInvalidResponse) {
   FtpSocketDataProviderFileDownloadInvalidResponse ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_INVALID_RESPONSE);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvReallyBadFormat) {
   FtpSocketDataProviderEvilPasv ctrl_socket("227 Portscan (127,0,0,\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_INVALID_RESPONSE);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort1) {
   FtpSocketDataProviderEvilPasv ctrl_socket("227 Portscan (127,0,0,1,0,22)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort2) {
   // Still unsafe. 1 * 256 + 2 = 258, which is < 1024.
   FtpSocketDataProviderEvilPasv ctrl_socket("227 Portscan (127,0,0,1,1,2)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort3) {
   // Still unsafe. 3 * 256 + 4 = 772, which is < 1024.
   FtpSocketDataProviderEvilPasv ctrl_socket("227 Portscan (127,0,0,1,3,4)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort4) {
   // Unsafe. 8 * 256 + 1 = 2049, which is used by nfs.
   FtpSocketDataProviderEvilPasv ctrl_socket("227 Portscan (127,0,0,1,8,1)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafeHost) {
   FtpSocketDataProviderEvilPasv ctrl_socket(
-      "227 Portscan (10,1,2,3,123,456)\r\n", FtpSocketDataProvider::PRE_SIZE);
+      "227 Portscan (10,1,2,3,123,456)\r\n", FtpSocketDataProvider::PRE_RETR);
   ctrl_socket.set_use_epsv(GetFamily() != AF_INET);
   std::string mock_data("mock-data");
   MockRead data_reads[] = {
     MockRead(mock_data.c_str()),
   };
   StaticSocketDataProvider data_socket1;
   StaticSocketDataProvider data_socket2(data_reads, arraysize(data_reads),
                                         NULL, 0);
   mock_socket_factory_.AddSocketDataProvider(&ctrl_socket);
   mock_socket_factory_.AddSocketDataProvider(&data_socket1);
@@ skipping 113 matching lines @@
                                             FtpSocketDataProvider::PRE_QUIT);
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilEpsvWeirdSep) {
   // This test makes no sense for IPv4 connections (we don't use EPSV there).
   if (GetFamily() == AF_INET)
     return;
 
   FtpSocketDataProviderEvilEpsv ctrl_socket("227 Portscan ($$$31744$)\r\n",
-                                            FtpSocketDataProvider::PRE_SIZE);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+                                            FtpSocketDataProvider::PRE_RETR);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest,
        DownloadTransactionEvilEpsvWeirdSepUnsafePort) {
   // This test makes no sense for IPv4 connections (we don't use EPSV there).
   if (GetFamily() == AF_INET)
     return;
 
   FtpSocketDataProviderEvilEpsv ctrl_socket("227 Portscan ($$$317$)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_UNSAFE_PORT);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilEpsvIllegalHost) {
   // This test makes no sense for IPv4 connections (we don't use EPSV there).
   if (GetFamily() == AF_INET)
     return;
 
   FtpSocketDataProviderEvilEpsv ctrl_socket("227 Portscan (|2|::1|31744|)\r\n",
                                             FtpSocketDataProvider::PRE_QUIT);
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilLoginBadUsername) {
   FtpSocketDataProviderEvilLogin ctrl_socket("hello%0Aworld", "test");
-  ExecuteTransaction(&ctrl_socket, "ftp://hello%0Aworld:test@host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://hello%0Aworld:test@host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilLoginBadPassword) {
   FtpSocketDataProviderEvilLogin ctrl_socket("test", "hello%0Dworld");
-  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%0Dworld@host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%0Dworld@host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionSpaceInLogin) {
   FtpSocketDataProviderEvilLogin ctrl_socket("hello world", "test");
-  ExecuteTransaction(&ctrl_socket, "ftp://hello%20world:test@host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://hello%20world:test@host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionSpaceInPassword) {
   FtpSocketDataProviderEvilLogin ctrl_socket("test", "hello world");
-  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%20world@host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%20world@host/file", 0, OK);
 }
 
 TEST_P(FtpNetworkTransactionTest, EvilRestartUser) {
   FtpSocketDataProvider ctrl_socket1;
   ctrl_socket1.InjectFailure(FtpSocketDataProvider::PRE_PASSWD,
                              FtpSocketDataProvider::PRE_QUIT,
                              "530 Login authentication failed\r\n");
   mock_socket_factory_.AddSocketDataProvider(&ctrl_socket1);
 
   FtpRequestInfo request_info = GetRequestInfo("ftp://host/file");
@@ skipping 53 matching lines @@
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.RestartWithAuth(
                 AuthCredentials(base::ASCIIToUTF16("innocent"),
                                 base::ASCIIToUTF16("foo\nownz0red")),
                 callback_.callback()));
   EXPECT_EQ(ERR_MALFORMED_IDENTITY, callback_.WaitForResult());
 }
 
 TEST_P(FtpNetworkTransactionTest, Escaping) {
   FtpSocketDataProviderEscaping ctrl_socket;
-  ExecuteTransaction(&ctrl_socket, "ftp://host/%20%21%22%23%24%25%79%80%81",
-                     1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/%20%21%22%23%24%25%79%80%81", 0,
+                     OK);
 }
 
 // Test for http://crbug.com/23794.
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionEvilSize) {
   // Try to overflow int64 in the response.
   FtpSocketDataProviderEvilSize ctrl_socket(
       "213 99999999999999999999999999999999\r\n",
       FtpSocketDataProvider::PRE_QUIT);
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);

[davidben, 2015/01/27 19:59:00]

This (and line 1376) are the only two calls left that don't pass 0 into
ExecuteTransaction. It looks like they don't care because it never gets that
far. You can probably just remove that parameter altogether since we'll (I
think?) never open two data connections anymore.

[xunjieli, 2015/01/27 20:55:28]

Done. Removed. Thanks!

 }
 
 // Test for http://crbug.com/36360.
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionBigSize) {
   // Pass a valid, but large file size. The transaction should not fail.
   FtpSocketDataProviderEvilSize ctrl_socket(
       "213 3204427776\r\n",
       FtpSocketDataProvider::PRE_CWD);
-  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, OK);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", 0, OK);
   EXPECT_EQ(3204427776LL,
             transaction_.GetResponseInfo()->expected_content_size);
 }
 
 // Regression test for http://crbug.com/25023.
 TEST_P(FtpNetworkTransactionTest, CloseConnection) {
   FtpSocketDataProviderCloseConnection ctrl_socket;
   ExecuteTransaction(&ctrl_socket, "ftp://host", 1, ERR_EMPTY_RESPONSE);
 }
 
@@ skipping 67 matching lines @@
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionFailEpsv) {
   // This test makes no sense for IPv4 connections (we don't use EPSV there).
   if (GetFamily() == AF_INET)
     return;
 
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   // Use unallocated 599 FTP error code to make sure it falls into the generic
   // ERR_FTP_FAILED bucket.
-  TransactionFailHelper(&ctrl_socket,
-                        "ftp://host",
-                        FtpSocketDataProvider::PRE_EPSV,
-                        FtpSocketDataProvider::PRE_NOPASV,
-                        "599 fail\r\n",
-                        ERR_FTP_FAILED);
+  TransactionFailHelper(
+      &ctrl_socket, "ftp://host", FtpSocketDataProvider::PRE_LIST_EPSV,
+      FtpSocketDataProvider::PRE_NOPASV, "599 fail\r\n", ERR_FTP_FAILED);
 }
 
 TEST_P(FtpNetworkTransactionTest, DirectoryTransactionFailCwd) {
   FtpSocketDataProviderDirectoryListing ctrl_socket;
   // Use unallocated 599 FTP error code to make sure it falls into the generic
   // ERR_FTP_FAILED bucket.
   TransactionFailHelper(&ctrl_socket,
                         "ftp://host",
                         FtpSocketDataProvider::PRE_CWD,
                         FtpSocketDataProvider::PRE_QUIT,
@@ skipping 72 matching lines @@
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionFailEpsv) {
   // This test makes no sense for IPv4 connections (we don't use EPSV there).
   if (GetFamily() == AF_INET)
     return;
 
   FtpSocketDataProviderFileDownload ctrl_socket;
   // Use unallocated 599 FTP error code to make sure it falls into the generic
   // ERR_FTP_FAILED bucket.
-  TransactionFailHelper(&ctrl_socket,
-                        "ftp://host/file",
-                        FtpSocketDataProvider::PRE_EPSV,
-                        FtpSocketDataProvider::PRE_NOPASV,
-                        "599 fail\r\n",
-                        ERR_FTP_FAILED);
+  TransactionFailHelper(
+      &ctrl_socket, "ftp://host/file", FtpSocketDataProvider::PRE_RETR_EPSV,
+      FtpSocketDataProvider::PRE_NOPASV, "599 fail\r\n", ERR_FTP_FAILED);
 }
 
 TEST_P(FtpNetworkTransactionTest, DownloadTransactionFailRetr) {
   FtpSocketDataProviderFileDownload ctrl_socket;
   // Use unallocated 599 FTP error code to make sure it falls into the generic
   // ERR_FTP_FAILED bucket.
   TransactionFailHelper(&ctrl_socket,
                         "ftp://host/file",
                         FtpSocketDataProvider::PRE_RETR,
                         FtpSocketDataProvider::PRE_QUIT,
@@ skipping 15 matching lines @@
                         FtpSocketDataProvider::PRE_TYPE,
                         "257 \"\"\r\n",
                         OK);
 }
 
 INSTANTIATE_TEST_CASE_P(FTP,
                         FtpNetworkTransactionTest,
                         ::testing::Values(AF_INET, AF_INET6));
 
 }  // namespace net