Make new FtpNetworkTransaction handle short reads correctly....

Make new FtpNetworkTransaction handle short reads correctly.

The problem was that some functions were parsing response lines, but without checking that they have the entire line available.

This change will also make it easier to handle multi-line greeting (230 welcome messages). I plan to do that afterwards.

TEST=Covered by net_unittests.

http://crbug.com/15259

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=19954

[wtc, 2009-06-26 01:08:06]

I will review this tomorrow.  Just a quick question on
ftp_network_transaction.h.

http://codereview.chromium.org/149043/diff/1/4
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/149043/diff/1/4#newcode107
Line 107: int ProcessResponseUSER(ResponseLine response);
Should these be const ResponseLine& ?

[wtc, 2009-06-26 21:22:05]

LGTM.  Excellent work!

Please fix the problems below before you check this in.
I go on vacation tonight, so I won't be able to review your
new Patch Set.  If you want, you can ask Ibrar to have a
look.

The most serious problem is that I believe the parsing
for loop in DoCtrlRead() doesn't work if response_message_buf_
contains more data after the first response line.

http://codereview.chromium.org/149043/diff/1/3
File net/ftp/ftp_network_transaction.cc (left):

http://codereview.chromium.org/149043/diff/1/3#oldcode386
Line 386: memcpy(response_message_buf_->data() + response_message_buf_len_,
It's nice that you can get rid of this unnecessary copying.

http://codereview.chromium.org/149043/diff/1/3
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/149043/diff/1/3#newcode429
Line 429: int FtpNetworkTransaction::DoCtrlReadComplete(int result) {
You may want to create a separate function for the response
line parsing code, so that the logic of this function can
be seen from a glance.

http://codereview.chromium.org/149043/diff/1/3#newcode439
Line 439: STATUS,
Nit: perhaps name this state "CODE"?

A short comment explaining that these three states correspond
to the three components of a response line, and that we
proceed linearly through these states, would be nice.

Your parsing code below is very easy to follow though, so
the comment isn't essential, but it would be helpful.

http://codereview.chromium.org/149043/diff/1/3#newcode450
Line 450: if (data[i] == ' ') {
Question: do we need to allow '\t'?

http://codereview.chromium.org/149043/diff/1/3#newcode461
Line 461: if (data[i] == '\r') {
Question: In FTP, do we need to tolerate an endline of just
\n?

http://codereview.chromium.org/149043/diff/1/3#newcode471
Line 471: cut_pos = i + 1;
We actually want to break out of the for loop at this point,
right?

Otherwise, if there is more data after '\n', we will return
Stop(ERR_FAILED).  Is that what we want?

So I'm not sure if this for loop works if there is more
data after '\n', in particular, if there are multiple
response lines.

http://codereview.chromium.org/149043/diff/1/3#newcode481
Line 481: if (cut_pos > 0) {
Add a comment
  // Parsed a response line.

http://codereview.chromium.org/149043/diff/1/3#newcode489
Line 489: next_state_ = STATE_CTRL_READ;
Add a comment
  // Incomplete response line.  Read more.

If we get here, we have parsed the status code and status
text in vain.  It seems wasteful...  But don't worry about
this if this rarely happens.

http://codereview.chromium.org/149043/diff/1/3#newcode492
Line 492: read_ctrl_buf_->SetOffset(response_message_buf_len_);
You are no longer using read_ctrl_buf_size_.

We should update read_ctrl_buf_size_ here so that in
DoCtrlRead() we can just pass read_ctrl_buf_size_  (or
read_ctrl_buf_size_ - 1?) as the second argument to
ctrl_socket_->Read() on line 425.

If you do this, I believe response_message_buf_ can be just
a plain IOBuffer because we won't need its size() method.

http://codereview.chromium.org/149043/diff/1/4
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/149043/diff/1/4#newcode76
Line 76: typedef std::pair<int, std::string> ResponseLine;
Should we define a struct, which allows us to have more
readable member names than 'first' and 'second'?

http://codereview.chromium.org/149043/diff/1/4#newcode81
Line 81: int ProcessControlSocketResponses();
Perhaps shorten this to ProcessCtrlResponses to be consistent
with the rest of the file, where we use "Ctrl" or "CTRL" to
refer to the control socket?

http://codereview.chromium.org/149043/diff/1/4#newcode154
Line 154: scoped_refptr<IOBufferWithSize> response_message_buf_;
Add a comment to describe the relation between
response_message_buf_ and read_ctrl_buf_.  It's not obvious
until I read the .cc file, especially the constructor.

Nit: Should we list the response_message_buf* and
read_ctrl_buf* members consecutively to hint that they're
closely related?

http://codereview.chromium.org/149043/diff/1/4#newcode158
Line 158: std::queue<ResponseLine> ctrl_responses_;
It seems that we can use use a std::vector.

[wtc, 2009-06-27 01:48:42]

http://codereview.chromium.org/149043/diff/1/4
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/149043/diff/1/4#newcode107
Line 107: int ProcessResponseUSER(ResponseLine response);
Another option is to pass the two components of ResponseLine
as parameters to these functions, for example,
  int ProcessResponseUSER(int status_code,
                          const std::string& status_text);

This is also more readable than 'first' and 'second'.

[Paweł Hajdan Jr., 2009-07-01 22:44:00]

eroman, could you look at the changes?

I think this update fixes all the problems.

[eroman, 2009-07-02 02:34:19]

lgtm, but please read my comments.

http://codereview.chromium.org/149043/diff/2010/3004
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/149043/diff/2010/3004#newcode141
Line 141: return Stop(ERR_FAILED);
Can we give a more specific error code?

How about ERR_INVALID_RESPONSE.
Or something even more specific.

http://codereview.chromium.org/149043/diff/2010/3004#newcode190
Line 190: std::string str(response_message_buf_->data(), 3);
This looks dangerous.

Can you add:

  DCHECK_GE(3, response_message_buf_->size());

While it seems ParseCtrlResponse() will error if there are fewer than 3 numbers,
I wander if callers of URLRequest may still end up calling GetResponseCode() on
a failed request, meaning we can hit this....

http://codereview.chromium.org/149043/diff/2010/3004#newcode219
Line 219: if (ctrl_responses_.size() != 1)
I wander if this repeated boilerplate can't be extracted.

What do you think about something like:

if (command_sent_ == COMMAND_NONE) {
 ...
 return;
}

// Every other command expects a single ctrl_response_ ...

if (ctrl_responses.size() != 1)
  return Stop(ERR_INVALID_RESPONSE);

ResponseLine line = ctrl_responses_.front();
ctrl_responses_.pop();

switch (command_sent_) {
  case COMMAND_USER:
    return ProcessResponseUSER(line);
  case COMMAND_PASS:
    return ProcessResponsePASS(line);
  ...
}

http://codereview.chromium.org/149043/diff/2010/3004#newcode504
Line 504: next_state_ = STATE_CTRL_READ;
What if cut_pos = 0 because there was a parse error, and Stop() was called? 
(Seems that Stop() returns 0 and sets last_error_).

Now we will be overwriting  the "next_state_" which it set.
I am not really familiar with how Stop() is used, I will try to look at this
more later.

http://codereview.chromium.org/149043/diff/2010/3004#newcode762
Line 762: if (!StringToInt(response.text, &file_data_len_))
How does this work if the response code overflowed?
(the resulting int could be negative, and this if would still pass).

http://codereview.chromium.org/149043/diff/2010/3005
File net/ftp/ftp_network_transaction.h (right):

http://codereview.chromium.org/149043/diff/2010/3005#newcode87
Line 87: // Executes correct ProcessResponse + |command_name| function based on
last
nit: in the subsequent new comment "response_message_buf_" is not decorated by
|bars|. I suggest keeping a consistent style throughout the file.

http://codereview.chromium.org/149043/diff/2010/3006
File net/ftp/ftp_network_transaction_unittest.cc (right):

http://codereview.chromium.org/149043/diff/2010/3006#newcode260
Line 260: EXPECT_EQ(static_cast<int>(mock_data.length()),
callback_.WaitForResult());
>80c

[Paweł Hajdan Jr., 2009-07-02 19:34:21]

Thanks for catching these issues. I think this new changeset fixes them.

It turns out I could just remove GetResponseCode function. It was private, and
just dead code.

Could you just take another look?

[eroman, 2009-07-02 22:56:48]

LGTM

http://codereview.chromium.org/149043/diff/9/11
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/149043/diff/9/11#newcode461
Line 461: return Stop(rv);
I am not excited about the use of Stop() in this file, but fine for now.

(Stop() strikes me as fragile, since it returns OK -- depending where this
happens, state machine could advance to other unexpected states; seems the
better thing to do is propagate the failure all the way out).

http://codereview.chromium.org/149043/diff/9/11#newcode467
Line 467: response_message_buf_len_ - cut_pos);
Can you add:

DCHECK_GE(response_message_buf_len_, cut_pos); 

(I presume calling memmove with 0 is a no-op and safe).