diff options
author | phajdan.jr@chromium.org <phajdan.jr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-08-05 18:34:47 +0000 |
---|---|---|
committer | phajdan.jr@chromium.org <phajdan.jr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-08-05 18:34:47 +0000 |
commit | b17a1a7fa91f4416b31fbc6fc622b901e37ae199 (patch) | |
tree | 9543d6475b5470601ac82bdb0aef58a80fd4651f /net | |
parent | a05cfeea397e7be36e621cf60e218b1ac71d2054 (diff) | |
download | chromium_src-b17a1a7fa91f4416b31fbc6fc622b901e37ae199.zip chromium_src-b17a1a7fa91f4416b31fbc6fc622b901e37ae199.tar.gz chromium_src-b17a1a7fa91f4416b31fbc6fc622b901e37ae199.tar.bz2 |
Correctly handle multiple control responses for RETR command.
Re-enable tests which were intermittently failing before this fix and remove
debugging code used to track down the issue.
TEST=Covered by net_unittests.
http://crbug.com/18036
Review URL: http://codereview.chromium.org/160537
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@22500 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'net')
-rw-r--r-- | net/ftp/ftp_ctrl_response_buffer.cc | 15 | ||||
-rw-r--r-- | net/ftp/ftp_network_transaction.cc | 28 | ||||
-rw-r--r-- | net/ftp/ftp_network_transaction_unittest.cc | 30 | ||||
-rw-r--r-- | net/url_request/url_request_unittest.cc | 6 |
4 files changed, 54 insertions, 25 deletions
diff --git a/net/ftp/ftp_ctrl_response_buffer.cc b/net/ftp/ftp_ctrl_response_buffer.cc index 9441f4b..02bbd08 100644 --- a/net/ftp/ftp_ctrl_response_buffer.cc +++ b/net/ftp/ftp_ctrl_response_buffer.cc @@ -8,19 +8,6 @@ #include "base/string_util.h" #include "net/base/net_errors.h" -namespace { - -// TODO(phajdan.jr): Remove when http://crbug.com/18036 is diagnosed. -void LogResponse(const net::FtpCtrlResponse& response) { - DLOG(INFO) << "received response with code " << response.status_code; - for (std::vector<std::string>::const_iterator i = response.lines.begin(); - i != response.lines.end(); ++i) { - DLOG(INFO) << "line [" << *i << "]"; - } -} - -} // namespace - namespace net { // static @@ -43,7 +30,6 @@ int FtpCtrlResponseBuffer::ConsumeData(const char* data, int data_length) { line_buf_ = line.status_text; } else { response_buf_.lines.push_back(line.status_text); - LogResponse(response_buf_); responses_.push(response_buf_); // Prepare to handle following lines. @@ -63,7 +49,6 @@ int FtpCtrlResponseBuffer::ConsumeData(const char* data, int data_length) { if (!line.is_multiline) { response_buf_.lines.push_back(line_buf_); - LogResponse(response_buf_); responses_.push(response_buf_); // Prepare to handle following lines. diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc index aac8b74..e3b2709 100644 --- a/net/ftp/ftp_network_transaction.cc +++ b/net/ftp/ftp_network_transaction.cc @@ -111,6 +111,11 @@ uint64 FtpNetworkTransaction::GetUploadProgress() const { // Used to prepare and send FTP commad. int FtpNetworkTransaction::SendFtpCommand(const std::string& command, Command cmd) { + // If we send a new command when we still have unprocessed responses + // for previous commands, the response receiving code will have no way to know + // which responses are for which command. + DCHECK(!ctrl_response_buffer_.ResponseAvailable()); + DCHECK(!write_command_buf_); DCHECK(!write_buf_); DLOG(INFO) << " >> " << command; @@ -130,12 +135,6 @@ int FtpNetworkTransaction::SendFtpCommand(const std::string& command, int FtpNetworkTransaction::ProcessCtrlResponse() { FtpCtrlResponse response = ctrl_response_buffer_.PopResponse(); - // TODO(phajdan.jr): Remove when http://crbug.com/18036 is diagnosed. - DLOG(INFO) << "Consumed one control response."; - - // We always expect only one response, even if it's multiline. - DCHECK(!ctrl_response_buffer_.ResponseAvailable()); - int rv = OK; switch (command_sent_) { case COMMAND_NONE: @@ -185,6 +184,22 @@ int FtpNetworkTransaction::ProcessCtrlResponse() { DLOG(INFO) << "Missing Command response handling!"; return ERR_FAILED; } + + // We may get multiple responses for some commands, + // see http://crbug.com/18036. + while (ctrl_response_buffer_.ResponseAvailable() && rv == OK) { + response = ctrl_response_buffer_.PopResponse(); + + switch (command_sent_) { + case COMMAND_RETR: + rv = ProcessResponseRETR(response); + break; + default: + // Multiple responses for other commands are invalid. + return ERR_INVALID_RESPONSE; + } + } + return rv; } @@ -723,6 +738,7 @@ int FtpNetworkTransaction::ProcessResponseRETR( const FtpCtrlResponse& response) { switch (GetErrorClass(response.status_code)) { case ERROR_CLASS_INITIATED: + next_state_ = STATE_CTRL_READ; 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 7726416..cc9c1bb 100644 --- a/net/ftp/ftp_network_transaction_unittest.cc +++ b/net/ftp/ftp_network_transaction_unittest.cc @@ -208,6 +208,30 @@ class FtpMockControlSocketFileDownload : public FtpMockControlSocket { DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketFileDownload); }; +class FtpMockControlSocketFileDownloadTransferStarting + : public FtpMockControlSocketFileDownload { + public: + FtpMockControlSocketFileDownloadTransferStarting() { + } + + 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, + "125-Data connection already open.\r\n" + "125 Transfer starting.\r\n" + "226 Transfer complete.\r\n"); + default: + return FtpMockControlSocketFileDownload::OnWrite(data); + } + } + + private: + DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketFileDownloadTransferStarting); +}; + class FtpMockControlSocketFileDownloadRetrFail : public FtpMockControlSocketFileDownload { public: @@ -356,6 +380,12 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionShortReads5) { ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK); } +TEST_F(FtpNetworkTransactionTest, DownloadTransactionTransferStarting) { + FtpMockControlSocketFileDownloadTransferStarting ctrl_socket; + ExecuteTransaction(&ctrl_socket, "ftp://host/file", OK); +} + + TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailUser) { FtpMockControlSocketDirectoryListing ctrl_socket; TransactionFailHelper(&ctrl_socket, diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc index c749462..5a5af4a 100644 --- a/net/url_request/url_request_unittest.cc +++ b/net/url_request/url_request_unittest.cc @@ -1808,8 +1808,7 @@ TEST_F(URLRequestTest, InterceptRespectsCancelInRestart) { EXPECT_EQ(URLRequestStatus::CANCELED, req.status().status()); } -// Needs more work, tracked in http://crbug.com/18036. -TEST_F(URLRequestTest, DISABLED_FTPGetTestAnonymous) { +TEST_F(URLRequestTest, FTPGetTestAnonymous) { scoped_refptr<FTPTestServer> server = FTPTestServer::CreateServer(L""); ASSERT_TRUE(NULL != server.get()); FilePath app_path; @@ -1833,8 +1832,7 @@ TEST_F(URLRequestTest, DISABLED_FTPGetTestAnonymous) { } } -// Needs more work, tracked in http://crbug.com/18036. -TEST_F(URLRequestTest, DISABLED_FTPGetTest) { +TEST_F(URLRequestTest, FTPGetTest) { scoped_refptr<FTPTestServer> server = FTPTestServer::CreateServer(L"", "chrome", "chrome"); ASSERT_TRUE(NULL != server.get()); |