Clean up FTPNetworkTransaction to match the latest trace

net/ftp/ftp_network_transaction.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 "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
@@ skipping 221 matching lines @@
 
 int FtpNetworkTransaction::Stop(int error) {
   if (command_sent_ == COMMAND_QUIT)
     return error;
 
   next_state_ = STATE_CTRL_WRITE_QUIT;
   last_error_ = error;
   return OK;
 }
 
-int FtpNetworkTransaction::RestartIgnoringLastError(

[xunjieli, 2015/01/30 21:50:44]

This is never used anywhere. Is it okay to remove it?

[davidben, 2015/02/04 18:51:18]

Sounds good. :-)

-    const CompletionCallback& callback) {
-  return ERR_NOT_IMPLEMENTED;
-}
-
 int FtpNetworkTransaction::Start(const FtpRequestInfo* request_info,
                                  const CompletionCallback& callback,
                                  const BoundNetLog& net_log) {
   net_log_ = net_log;
   request_ = request_info;
 
   ctrl_response_buffer_.reset(new FtpCtrlResponseBuffer(net_log_));
 
   if (request_->url.has_username()) {
     base::string16 username;
@@ skipping 769 matching lines @@
 
 // RETR command
 int FtpNetworkTransaction::DoCtrlWriteRETR() {
   std::string command = "RETR " + GetRequestPathForFtpCommand(false);
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_RETR);
 }
 
 int FtpNetworkTransaction::ProcessResponseRETR(
     const FtpCtrlResponse& response) {
+  // Resource type should be either filled in by DetectTypecode() or
+  // detected with CWD. RETR is sent only when the resource is a file.
+  DCHECK_EQ(RESOURCE_TYPE_FILE, resource_type_);
+
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
       // 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.
-      resource_type_ = RESOURCE_TYPE_FILE;
       break;
     case ERROR_CLASS_OK:
-      resource_type_ = RESOURCE_TYPE_FILE;
       next_state_ = STATE_CTRL_WRITE_QUIT;
       break;
     case ERROR_CLASS_INFO_NEEDED:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     case ERROR_CLASS_TRANSIENT_ERROR:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     case ERROR_CLASS_PERMANENT_ERROR:
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
 
-  // We should be sure about our resource type now. Otherwise we risk
-  // an infinite loop (RETR can later send CWD, and CWD can later send RETR).
-  DCHECK_NE(RESOURCE_TYPE_UNKNOWN, resource_type_);
-
   return OK;
 }
 
 // SIZE command
 int FtpNetworkTransaction::DoCtrlWriteSIZE() {
   std::string command = "SIZE " + GetRequestPathForFtpCommand(false);
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_SIZE);
 }
 
@@ skipping 18 matching lines @@
       break;
     case ERROR_CLASS_INFO_NEEDED:
       break;
     case ERROR_CLASS_TRANSIENT_ERROR:
       break;
     case ERROR_CLASS_PERMANENT_ERROR:
       // It's possible that SIZE failed because the path is a directory.
       if (resource_type_ == RESOURCE_TYPE_UNKNOWN &&
           response.status_code != 550) {
         return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
       }

[davidben, 2015/02/04 18:51:18]

Do you know what this code does? I'm confused what it's for. As far as I can
tell, this is the only place where RESOURCE_TYPE_UNKNOWN vs
RESOURCE_TYPE_DIRECTORY even does anything.

(This CL also doesn't change this block any, so if you're not sure either and
can't find the answer quickly, let's just leave it for now.)

[xunjieli, 2015/02/04 20:29:29]

The Stop case is not tested in the code. I think it is probably safe to get rid
of RESOURCE_TYPE_UNKNOWN here, since according to RFC 3659, it seems that we
should try the next command regardless of the type when we have a 550. WDYT?

       break;
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
   if (resource_type_ == RESOURCE_TYPE_FILE)
     EstablishDataConnection(STATE_CTRL_WRITE_RETR);
   else
     next_state_ = STATE_CTRL_WRITE_CWD;
   return OK;
 }
 
 // CWD command
 int FtpNetworkTransaction::DoCtrlWriteCWD() {
   std::string command = "CWD " + GetRequestPathForFtpCommand(true);
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_CWD);
 }
 
 int FtpNetworkTransaction::ProcessResponseCWD(const FtpCtrlResponse& response) {
-  // We should never issue CWD if we know the target resource is a file.
+  // CWD should be invoked only when the resource is not a file.
   DCHECK_NE(RESOURCE_TYPE_FILE, resource_type_);
 
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
       return Stop(ERR_INVALID_RESPONSE);
     case ERROR_CLASS_OK:
+      resource_type_ = RESOURCE_TYPE_DIRECTORY;
       EstablishDataConnection(STATE_CTRL_WRITE_LIST);
       break;
     case ERROR_CLASS_INFO_NEEDED:
       return Stop(ERR_INVALID_RESPONSE);
     case ERROR_CLASS_TRANSIENT_ERROR:
       // Some FTP servers send response 451 (not a valid CWD response according
       // to RFC 959) instead of 550.
       if (response.status_code == 451)
         return ProcessResponseCWDNotADirectory();
 
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     case ERROR_CLASS_PERMANENT_ERROR:
       if (response.status_code == 550)
         return ProcessResponseCWDNotADirectory();
 
       return Stop(GetNetErrorCodeForFtpResponseCode(response.status_code));
     default:
       NOTREACHED();
       return Stop(ERR_UNEXPECTED);
   }
 
   return OK;
 }
 
 int FtpNetworkTransaction::ProcessResponseCWDNotADirectory() {
   if (resource_type_ == RESOURCE_TYPE_DIRECTORY) {

[davidben, 2015/02/05 19:42:57]

Oof, I'm sorry! I was wrong that RESOURCE_TYPE_UNKNOWN and
RESOURCE_TYPE_DIRECTORY are treated the same outside of that bit up there. This
logic makes it so that if you add type=d and CWD fails, we don't go on to RETR
it, which is probably desirable.

Probably best to put that bit back. (Maybe with a TODO to test this case. :-) )

[xunjieli, 2015/02/05 20:21:57]

Done. Hmm yep, saving a RETR is probably better than none :)

     // We're assuming that the resource is a directory, but the server
     // says it's not true. The most probable interpretation is that it
     // doesn't exist (with FTP we can't be sure).
     return Stop(ERR_FILE_NOT_FOUND);
   }
 
-  // We are here because SIZE failed and we are not sure what the resource
-  // type is. It could still be file, and SIZE could fail because of
-  // an access error (http://crbug.com/56734). Try RETR just to be sure.
+  // If it is not a directory, it is probably a file.
   resource_type_ = RESOURCE_TYPE_FILE;
 
   EstablishDataConnection(STATE_CTRL_WRITE_RETR);
   return OK;
 }
 
 // LIST command
 int FtpNetworkTransaction::DoCtrlWriteLIST() {
   // Use the -l option for mod_ftp configured in LISTIsNLST mode: the option
   // forces LIST output instead of NLST (which would be ambiguous for us
   // to parse).
   std::string command("LIST -l");

[xunjieli, 2015/01/30 21:50:44]

I wonder if we should remove "-l". This is not a part of RFC. This is confusing
some servers. For instance, ftp://ftp.vsm.com.au/ doesn't like "-l", and closes
data connection if we do "LIST -l". Some googling brought up this
http://stackoverflow.com/questions/21065113/ftp-list-why-does-chrome-use-the-...

[davidben, 2015/02/04 18:51:18]

Hrm. Apparently it was added in https://chromiumcodereview.appspot.com/11364204
to deal with OTHER server quirks.

Yaay. :-/ I guess we can leave it alone until someone notices? I don't think
people actually use ftp anymore anyway.

[xunjieli, 2015/02/04 20:29:29]

Sounds good to me.  no easy way around this I guess.

   if (system_type_ == SYSTEM_TYPE_VMS)
     command = "LIST *.*;0";
 
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, command, COMMAND_LIST);
 }
 
 int FtpNetworkTransaction::ProcessResponseLIST(
     const FtpCtrlResponse& response) {
+  // Resource type should be either filled in by DetectTypecode() or
+  // detected with CWD. LIST is sent only when the resource is a directory.
+  DCHECK_EQ(RESOURCE_TYPE_DIRECTORY, resource_type_);
+
   switch (GetErrorClass(response.status_code)) {
     case ERROR_CLASS_INITIATED:
       // 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.
       response_.is_directory_listing = true;
       break;
     case ERROR_CLASS_OK:
       response_.is_directory_listing = true;
@@ skipping 180 matching lines @@
   if (!had_error_type[type]) {
     had_error_type[type] = true;
     UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorHappened",
         type, NUM_OF_NET_ERROR_TYPES);
   }
   UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorCount",
       type, NUM_OF_NET_ERROR_TYPES);
 }
 
 }  // namespace net