Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 160537: Correctly handle multiple control responses for RETR command. (Closed)

Created:
11 years, 4 months ago by Paweł Hajdan Jr.
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com, ibrar
Visibility:
Public.

Description

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 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=22500

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -25 lines) Patch
M net/ftp/ftp_ctrl_response_buffer.cc View 3 chunks +0 lines, -15 lines 0 comments Download
M net/ftp/ftp_network_transaction.cc View 4 chunks +22 lines, -6 lines 1 comment Download
M net/ftp/ftp_network_transaction_unittest.cc View 2 chunks +30 lines, -0 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 2 chunks +2 lines, -4 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
Paweł Hajdan Jr.
11 years, 4 months ago (2009-08-03 21:03:11 UTC) #1
wtc
LGTM! http://codereview.chromium.org/160537/diff/1/3 File net/ftp/ftp_network_transaction.cc (right): http://codereview.chromium.org/160537/diff/1/3#newcode135 Line 135: FtpCtrlResponse response = ctrl_response_buffer_.PopResponse(); Should this function ...
11 years, 4 months ago (2009-08-04 23:46:30 UTC) #2
Paweł Hajdan Jr.
11 years, 4 months ago (2009-08-05 18:37:07 UTC) #3
On Tue, Aug 4, 2009 at 16:46, <wtc@chromium.org> wrote:

> http://codereview.chromium.org/160537/diff/1/3#newcode135
> Line 135: FtpCtrlResponse response =
> ctrl_response_buffer_.PopResponse();
> Should this function empty ctrl_response_buffer_ before it
> returns an error?  This seems to be required by the new
> DCHECK you added to FtpNetworkTransaction::SendFtpCommand.


This is a valid problem. However, in that case we hit other DCHECK first:

[24925:24925:0805/113345:786735128127:FATAL:/chromium/src/net/ftp/ftp_network_transaction.cc(310)]
Check failed: rv == OK.

I'm going to handle that in a separate issue. The code should call Stop
earlier or something like that I think.

Powered by Google App Engine
This is Rietveld 408576698