Hang if directory listing size is > 8k

[Replaced by http://codereview.chromium.org/159663 ]

BUG=4965
R=wtc

Description:
Hang if directory listing size is > 8K
Example  = ftp://ftp.mozilla.org/pub/addons/

Problem:
Data was spooling on network stack and after 8K data TCP Window size become small and no more data received after 8K.

[ibrar, 2009-06-26 07:26:42]

Please review,



WTC: After this check-in there is one potential bug of incomplete line. This CL
form (phajdan.jr@chromium.org) will fix the issue 

http://codereview.chromium.org/149043

[wtc, 2009-06-26 18:45:59]

http://codereview.chromium.org/146137/diff/2004/2005
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/146137/diff/2004/2005#newcode98
Line 98: // Always read one byte less than the size, because in my many places
we
Nit: remove "my"

http://codereview.chromium.org/146137/diff/2004/2005#newcode105
Line 105: if (rv == ERR_IO_PENDING)
Nit: add braces around this "if" too.

From the Style Guide:

   if one part of an if-else statement uses curly braces,
   the other part must too

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

http://codereview.chromium.org/146137/diff/2004/2005#newcode110
Line 110: int r = DoLoop(OK);
It's strange to see two DoLoop calls.  There has to be a
better way to do this...  I think this should be done in
the DoXXX state functions that need this.

http://codereview.chromium.org/146137/diff/2004/2005#newcode249
Line 249: DCHECK(rv < 0);
Is this correct?  You should DCHECK(rv == 0) instead.

In any case, I have a CL that adds DCHECK(rv == 0) here:
http://codereview.chromium.org/149013

[ibrar, 2009-07-11 07:58:58]

http://codereview.chromium.org/146137/diff/2004/2005
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/146137/diff/2004/2005#newcode98
Line 98: // Always read one byte less than the size, because in my many places
we
On 2009/06/26 18:45:59, wtc wrote:
> Nit: remove "my"

Done.

http://codereview.chromium.org/146137/diff/2004/2005#newcode105
Line 105: if (rv == ERR_IO_PENDING)
On 2009/06/26 18:45:59, wtc wrote:
> Nit: add braces around this "if" too.
> 
> From the Style Guide:
> 
>    if one part of an if-else statement uses curly braces,
>    the other part must too
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Conditionals

Done.

http://codereview.chromium.org/146137/diff/2004/2005#newcode110
Line 110: int r = DoLoop(OK);
On 2009/06/26 18:45:59, wtc wrote:
> It's strange to see two DoLoop calls.  There has to be a
> better way to do this...  I think this should be done in
> the DoXXX state functions that need this.

Done.

http://codereview.chromium.org/146137/diff/2004/2005#newcode249
Line 249: DCHECK(rv < 0);
On 2009/06/26 18:45:59, wtc wrote:
> Is this correct?  You should DCHECK(rv == 0) instead.
> 
> In any case, I have a CL that adds DCHECK(rv == 0) here:
> http://codereview.chromium.org/149013

Actually DCHECK(rv == 0) is wrong we can get rv > 0 here. For now I have removed
this from this CL and will discuss in relative CL.

[wtc, 2009-07-16 00:10:29]

http://codereview.chromium.org/146137/diff/2007/2008
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/146137/diff/2007/2008#newcode91
Line 91: if (data_socket_ == NULL || !data_socket_->IsConnected()) {
This should be moved into the DoDataRead() function.
See my comment in DoDataRead below.

http://codereview.chromium.org/146137/diff/2007/2008#newcode92
Line 92: Stop(OK);  // No more data so send QUIT Command now and wait for
responce.
Typo: responce => response

Just wanted to make sure you intentionally don't check
the return value of Stop (because Stop(OK) always returns
OK).

http://codereview.chromium.org/146137/diff/2007/2008#newcode96
Line 96: // need to null terminate this string.
Really?  Why do we need to null terminate this string?
I guess it's because you had
  DLOG(INFO) << read_data_buf_->data();
in DoDataReadComplete(().  Since this CL deletes that
DLOG statement, perhaps we don't need to null terminate
this string any more?

The data could be binary data, so it doesn't make sense
to null terminate it...

http://codereview.chromium.org/146137/diff/2007/2008#newcode865
Line 865: DCHECK(read_data_buf_len_ > 0);
If you set read_data_buf_len_ to buf_len - 1 in Read(),
you can only DCHECK that read_data_buf_len_ >= 0 here.

http://codereview.chromium.org/146137/diff/2007/2008#newcode866
Line 866: 
Here you can say

  if (data_socket_ == NULL || !data_socket_->IsConnected())
    return Stop(OK);  // No more data so send QUIT Command now and wait for
response.

[ibrar, 2009-07-27 04:49:33]

http://codereview.chromium.org/146137/diff/2007/2008
File net/ftp/ftp_network_transaction.cc (right):

http://codereview.chromium.org/146137/diff/2007/2008#newcode91
Line 91: if (data_socket_ == NULL || !data_socket_->IsConnected()) {
On 2009/07/16 00:10:29, wtc wrote:
> This should be moved into the DoDataRead() function.
> See my comment in DoDataRead below.

Done.

http://codereview.chromium.org/146137/diff/2007/2008#newcode92
Line 92: Stop(OK);  // No more data so send QUIT Command now and wait for
responce.
On 2009/07/16 00:10:29, wtc wrote:
> Typo: responce => response
> 
> Just wanted to make sure you intentionally don't check
> the return value of Stop (because Stop(OK) always returns
> OK).

Done.

http://codereview.chromium.org/146137/diff/2007/2008#newcode96
Line 96: // need to null terminate this string.
On 2009/07/16 00:10:29, wtc wrote:
> Really?  Why do we need to null terminate this string?
> I guess it's because you had
>   DLOG(INFO) << read_data_buf_->data();
> in DoDataReadComplete(().  Since this CL deletes that
> DLOG statement, perhaps we don't need to null terminate
> this string any more?
> 
> The data could be binary data, so it doesn't make sense
> to null terminate it...

We only need a null terminating string when its a directory listing.

http://codereview.chromium.org/146137/diff/2007/2008#newcode865
Line 865: DCHECK(read_data_buf_len_ > 0);
On 2009/07/16 00:10:29, wtc wrote:
> If you set read_data_buf_len_ to buf_len - 1 in Read(),
> you can only DCHECK that read_data_buf_len_ >= 0 here.

Done.

http://codereview.chromium.org/146137/diff/2007/2008#newcode866
Line 866: 
On 2009/07/16 00:10:29, wtc wrote:
> Here you can say
> 
>   if (data_socket_ == NULL || !data_socket_->IsConnected())
>     return Stop(OK);  // No more data so send QUIT Command now and wait for
> response.

Done.