Do not send PASV until RETR or LIST

Do not send PASV until RETR or LIST

Some servers get confused when we send PASV twice. This CL delays sending
PASV/EPSV until RETR or LIST.

BUG=450687
Committed: https://crrev.com/6c29e3876102f96c2fae1ad07e2167d87cc81c77
Cr-Commit-Position: refs/heads/master@{#313381}

[xunjieli, 2015-01-23 19:03:37]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org

[xunjieli, 2015-01-23 19:03:38]

David, here's a one line fix that will load
ftp://ftp.isi.edu/in-notes/rfc1958.txt correctly. 

However, this is still a bug in fetching the directory, eg
ftp://ftp.isi.edu/in-notes/. The server does not expect the PASV sent when we
encounter an error in SIZE (since in this case directory does not have size). I
tried removing the other two ResetDataConnectionAfterError() in
ProcessResponseSIZE(). Wireshark shows everything's fine and the data is
transferred, but Chrome hangs forever, which is very weird. Not sure what is
wrong.

What do you suggest as the next step? The link in the old bug that the two PASVs
fixed no longer works, so I am not sure if removing the PASV will regress.

[xunjieli, 2015-01-23 19:19:34]

On 2015/01/23 19:03:38, xunjieli wrote:
> David, here's a one line fix that will load
> ftp://ftp.isi.edu/in-notes/rfc1958.txt correctly. 
> 
> However, this is still a bug in fetching the directory, eg
> ftp://ftp.isi.edu/in-notes/. The server does not expect the PASV sent when we
> encounter an error in SIZE (since in this case directory does not have size).
I
> tried removing the other two ResetDataConnectionAfterError() in
> ProcessResponseSIZE(). Wireshark shows everything's fine and the data is
> transferred, but Chrome hangs forever, which is very weird. Not sure what is
> wrong.
> 
> What do you suggest as the next step? The link in the old bug that the two
PASVs
> fixed no longer works, so I am not sure if removing the PASV will regress.

Never mind, loading the directory works, it is just very slow. Try a smaller
directory ftp://ftp.isi.edu/in-notes/fyi on the same server gives results back
faster. So basically doing the reverse of
https://chromiumcodereview.appspot.com/11364224/ will fix the bug. Should we do
that? WDYT?

[xunjieli, 2015-01-23 23:15:29]

xunjieli@chromium.org changed reviewers:
- davidben@chromium.org

[xunjieli2, 2015-01-24 02:38:27]

Patchset #3 (id:40001) has been deleted

[xunjieli2, 2015-01-24 02:38:33]

Patchset #3 (id:60001) has been deleted

[xunjieli2, 2015-01-24 22:14:11]

Patchset #1 (id:1) has been deleted

[xunjieli2, 2015-01-24 22:14:16]

Patchset #2 (id:80001) has been deleted

[xunjieli2, 2015-01-24 22:14:22]

Patchset #2 (id:100001) has been deleted

[xunjieli2, 2015-01-24 22:14:29]

Patchset #1 (id:20001) has been deleted

[xunjieli2, 2015-01-24 22:16:47]

Patchset #1 (id:120001) has been deleted

[xunjieli, 2015-01-26 17:30:34]

Patchset #1 (id:140001) has been deleted

[xunjieli, 2015-01-26 17:35:42]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, eroman@chromium.org, phajdan.jr@chromium.org

[xunjieli, 2015-01-26 17:35:42]

Adopted David's suggestion in crbug.com/450687. PTAL. Thanks!

[eroman, 2015-01-26 23:00:38]

lgtm

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:208: use_epsv() ? PRE_LIST_EPSV :
PRE_LIST_PASV, "200 OK\r\n");
Is there a test which covers the circumstances of 450687 ?

[xunjieli, 2015-01-27 14:42:02]

Thanks for the review! Hmm.. not sure why Rietvield does not pick up my CL title
change..

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:208: use_epsv() ? PRE_LIST_EPSV :
PRE_LIST_PASV, "200 OK\r\n");
On 2015/01/26 23:00:38, eroman wrote:
> Is there a test which covers the circumstances of 450687 ?

Unfortunately there isn't one. I considered adding one, but I don't think we can
emulate the server behavior, which is no longer relevant. In 450687, we receive
a not-directory error, and we throw away the first data connection and establish
a second data connection by sending a second PASV. Then the server got confused
and closed the second data connection. 
But after this change, there's no longer a second data connection. We establish
the first data connection after we know whether it's a file or directory. We
never attempt to send PASV again. So I think the situation in 450687 can no
longer be simulated in the test. With the removal of the second data connection,
the situation in 450687 is just a normal file retrieval covered in
FtpNetworkTransactionTest.DownloadTransaction.

[davidben, 2015-01-27 19:59:00]

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:227:
state_after_data_connect_complete_(STATE_CTRL_WRITE_SIZE) {}
This should probably be initialized to STATE_NONE now.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:348: state_after_data_connect_complete_ =
STATE_CTRL_WRITE_SIZE;
This should be STATE_NONE now.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:356: next_state_ = use_epsv_ ?
STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
Nit: I'd probably just do rather than check + NOTREACHED. (It's pretty easy to
visually verify that EstablishDataConnection only gets called with those two
anyway.)

DCHECK(state_after_connect == STATE_CTRL_WRITE_RETR || state_after_connect ==
STATE_CTRL_WRITE_LIST); at the top.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:1067: next_state_ = use_epsv_ ?
STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
I'm actually confused how this worked before this CL too, but this line looks
off. It doesn't set state_after_data_connect_. Looks like there was a test for
this case, but then it got removed in https://codereview.chromium.org/2813044
when the commands were reordered a bunch.

This line was originally added in https://codereview.chromium.org/293049 at
which point we apparently always transitioned to SIZE or CWD. I actually suspect
that the right thing to do is to remove this blog altogether now and just call
Stop(). (If removing it doesn't break tests, then we've clearly lost the test
cases for it.) I think this is from before 2813044 where our trace was (judging
from the tests in 2813044):

files:
USER
PASS
SYST
PWD
TYPE
PASV
SIZE
MDTM # modification time
RETR
QUIT

directories:
USER
PASS
SYST
PWD
TYPE
PASV
SIZE
MDTM # modification time
RETR # fails
PASV # this line, added by 293
CWD
LIST
QUIT

That is, we sniffed files vs directories based on RETR's failure. Then we
sniffed just based on SIZE. And then with http://codereview.chromium.org/5227004
we switched to using CWD, which is why we always follow SIZE with CWD now.

(In fact, I'm not sure it's even possible to get to ProcessReponseRETR without
resource_type_ being RESOURCE_TYPE_FILE anymore. I think 2813044 failed to fix
up a lot of the code and comments. Fixing all that in this CL is probably too
much for one change, but this RETR -> DIRECTORY state change is probably worth
eliminating.)

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:208: use_epsv() ? PRE_LIST_EPSV :
PRE_LIST_PASV, "200 OK\r\n");
On 2015/01/27 14:42:02, xunjieli wrote:
> On 2015/01/26 23:00:38, eroman wrote:
> > Is there a test which covers the circumstances of 450687 ?
> 
> Unfortunately there isn't one. I considered adding one, but I don't think we
can
> emulate the server behavior, which is no longer relevant. In 450687, we
receive
> a not-directory error, and we throw away the first data connection and
establish
> a second data connection by sending a second PASV. Then the server got
confused
> and closed the second data connection. 
> But after this change, there's no longer a second data connection. We
establish
> the first data connection after we know whether it's a file or directory. We
> never attempt to send PASV again. So I think the situation in 450687 can no
> longer be simulated in the test. With the removal of the second data
connection,
> the situation in 450687 is just a normal file retrieval covered in
> FtpNetworkTransactionTest.DownloadTransaction.

I think we're implicitly testing it by asserting that only one PASV is sent.
(And we were arguably simulating the old situation before by asserting our old
trace. We just didn't account for some server barfing on it.)

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:1359:
ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);
This (and line 1376) are the only two calls left that don't pass 0 into
ExecuteTransaction. It looks like they don't care because it never gets that
far. You can probably just remove that parameter altogether since we'll (I
think?) never open two data connections anymore.

[xunjieli, 2015-01-27 20:55:28]

Thanks for the review! I have uploaded a patch to address the comments.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:227:
state_after_data_connect_complete_(STATE_CTRL_WRITE_SIZE) {}
On 2015/01/27 19:58:59, David Benjamin wrote:
> This should probably be initialized to STATE_NONE now.

Done.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:348: state_after_data_connect_complete_ =
STATE_CTRL_WRITE_SIZE;
On 2015/01/27 19:58:59, David Benjamin wrote:
> This should be STATE_NONE now.

Done.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:356: next_state_ = use_epsv_ ?
STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
On 2015/01/27 19:58:59, David Benjamin wrote:
> Nit: I'd probably just do rather than check + NOTREACHED. (It's pretty easy to
> visually verify that EstablishDataConnection only gets called with those two
> anyway.)
> 
> DCHECK(state_after_connect == STATE_CTRL_WRITE_RETR || state_after_connect ==
> STATE_CTRL_WRITE_LIST); at the top.

Done. Good idea. thanks

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction.cc:1067: next_state_ = use_epsv_ ?
STATE_CTRL_WRITE_EPSV : STATE_CTRL_WRITE_PASV;
On 2015/01/27 19:58:59, David Benjamin wrote:
> I'm actually confused how this worked before this CL too, but this line looks
> off. It doesn't set state_after_data_connect_. Looks like there was a test for
> this case, but then it got removed in https://codereview.chromium.org/2813044
> when the commands were reordered a bunch.
> 
> This line was originally added in https://codereview.chromium.org/293049 at
> which point we apparently always transitioned to SIZE or CWD. I actually
suspect
> that the right thing to do is to remove this blog altogether now and just call
> Stop(). (If removing it doesn't break tests, then we've clearly lost the test
> cases for it.) I think this is from before 2813044 where our trace was
(judging
> from the tests in 2813044):
> 
> files:
> USER
> PASS
> SYST
> PWD
> TYPE
> PASV
> SIZE
> MDTM # modification time
> RETR
> QUIT
> 
> directories:
> USER
> PASS
> SYST
> PWD
> TYPE
> PASV
> SIZE
> MDTM # modification time
> RETR # fails
> PASV # this line, added by 293
> CWD
> LIST
> QUIT
> 
> That is, we sniffed files vs directories based on RETR's failure. Then we
> sniffed just based on SIZE. And then with
http://codereview.chromium.org/5227004
> we switched to using CWD, which is why we always follow SIZE with CWD now.
> 
> (In fact, I'm not sure it's even possible to get to ProcessReponseRETR without
> resource_type_ being RESOURCE_TYPE_FILE anymore. I think 2813044 failed to fix
> up a lot of the code and comments. Fixing all that in this CL is probably too
> much for one change, but this RETR -> DIRECTORY state change is probably worth
> eliminating.)

Done. Right, this part doesn't look correct. Thanks for catching it! The tests
are still passing.

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/861193007/diff/160001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:1359:
ExecuteTransaction(&ctrl_socket, "ftp://host/file", 1, ERR_INVALID_RESPONSE);
On 2015/01/27 19:59:00, David Benjamin wrote:
> This (and line 1376) are the only two calls left that don't pass 0 into
> ExecuteTransaction. It looks like they don't care because it never gets that
> far. You can probably just remove that parameter altogether since we'll (I
> think?) never open two data connections anymore.

Done. Removed. Thanks!

[davidben, 2015-01-27 21:04:09]

lgtm with one comment. Thanks! (The rest of this state machine is probably worth
a cleanup pass, but let's do that separately.)

https://codereview.chromium.org/861193007/diff/180001/net/ftp/ftp_network_tra...
File net/ftp/ftp_network_transaction_unittest.cc (right):

https://codereview.chromium.org/861193007/diff/180001/net/ftp/ftp_network_tra...
net/ftp/ftp_network_transaction_unittest.cc:815: data_reads,
arraysize(data_reads), NULL, 0));
I believe this'll leak the StaticSocketDataProvider (why the old code used
ScopedVector. I think you want

scoped_ptr<StaticSocketDataProvider> data_socket(new ...);
mock_socket_factory_.AddSocketDataProvider(data_socket.get());

[xunjieli, 2015-01-27 21:33:27]

> lgtm with one comment. Thanks! (The rest of this state machine is probably
worth
> a cleanup pass, but let's do that separately.)
> 
Should we file a bug for that, so we don't lose track of it?

>
https://codereview.chromium.org/861193007/diff/180001/net/ftp/ftp_network_tra...
> File net/ftp/ftp_network_transaction_unittest.cc (right):
> 
>
https://codereview.chromium.org/861193007/diff/180001/net/ftp/ftp_network_tra...
> net/ftp/ftp_network_transaction_unittest.cc:815: data_reads,
> arraysize(data_reads), NULL, 0));
> I believe this'll leak the StaticSocketDataProvider (why the old code used
> ScopedVector. I think you want
> 
> scoped_ptr<StaticSocketDataProvider> data_socket(new ...);
> mock_socket_factory_.AddSocketDataProvider(data_socket.get());

Done. thanks!

[davidben, 2015-01-27 21:41:01]

On 2015/01/27 21:33:27, xunjieli wrote:
> > lgtm with one comment. Thanks! (The rest of this state machine is probably
> worth
> > a cleanup pass, but let's do that separately.)
> > 
> Should we file a bug for that, so we don't lose track of it?

Probably, yeah. Filed https://crbug.com/452657.

[xunjieli, 2015-01-27 22:05:06]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2015-01-27 22:05:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861193007/200001

[commit-bot: I haz the power, 2015-01-27 22:39:02]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2015-01-27 22:39:59]

Patchset 3 (id:??) landed as
https://crrev.com/6c29e3876102f96c2fae1ad07e2167d87cc81c77
Cr-Commit-Position: refs/heads/master@{#313381}