FTP: Open a fresh data connection after a command error.

FTP: Open a fresh data connection after a command error.

Apparently some FTP servers interpret RFC 959 in a way
that makes it possible for them to close the data connection
on a command error. We can't be sure whether that happened
or not, so just always open a new connection after receiving
an error message.

BUG=121335
TEST=Covered by net_unittests.


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

[eroman, 2012-11-13 20:36:21]

lgtm

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
net/ftp/ftp_network_transaction.cc:361: // It is ambiguous whan an irrecoverable
error condition is,
typo: "whan" --> "what" or "when" ?

[Paweł Hajdan Jr., 2012-11-14 17:30:18]

Matt, what do you think? I'd like to see your comments before landing, and I've
added you to get you more familiar with the code - hopefully you don't mind :)
but if you do please let me know.

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
File net/ftp/ftp_network_transaction.cc (right):

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
net/ftp/ftp_network_transaction.cc:361: // It is ambiguous whan an irrecoverable
error condition is,
On 2012/11/13 20:36:21, eroman wrote:
> typo: "whan" --> "what" or "when" ?

Done.

[mmenke, 2012-11-14 17:35:00]

I don't mind.  I'm still working my way through the unit test infrastructure. 
I'll get back to you later today, probably not too long after the all hands, if
it's interesting.

On 2012/11/14 17:30:18, Paweł Hajdan Jr. wrote:
> Matt, what do you think? I'd like to see your comments before landing, and
I've
> added you to get you more familiar with the code - hopefully you don't mind :)
> but if you do please let me know.
> 
>
https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
> File net/ftp/ftp_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
> net/ftp/ftp_network_transaction.cc:361: // It is ambiguous whan an
irrecoverable
> error condition is,
> On 2012/11/13 20:36:21, eroman wrote:
> > typo: "whan" --> "what" or "when" ?
> 
> Done.

[mmenke, 2012-11-14 21:05:39]

LGTM.

Seems like there should be a better way of doing this - looks like we call SIZE
of directories a lot, and establishing a new connection whenever that happens
just doesn't seem terribly efficient.

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
net/ftp/ftp_network_transaction.h:257: State state_after_data_connection_;
nit:  Think state_after_data_connect_ would be clearer, since "connection" is
used elsewhere to mean the connection itself rather than the act of connecting. 
Or maybe even state_after_data_connect_complete_.

[Paweł Hajdan Jr., 2012-11-14 23:30:31]

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
File net/ftp/ftp_network_transaction.h (right):

https://codereview.chromium.org/11364224/diff/1/net/ftp/ftp_network_transacti...
net/ftp/ftp_network_transaction.h:257: State state_after_data_connection_;
On 2012/11/14 21:05:39, Matt Menke wrote:
> nit:  Think state_after_data_connect_ would be clearer, since "connection" is
> used elsewhere to mean the connection itself rather than the act of
connecting. 
> Or maybe even state_after_data_connect_complete_.

Done.

[commit-bot: I haz the power, 2012-11-14 23:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/11364224/8003

[commit-bot: I haz the power, 2012-11-15 02:48:33]

Change committed as 167832

[Ryan Sleevi, 2012-11-15 07:11:25]

This CL is causing a number of issues on the Memory.FYI waterfall

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Heapcheck/bu...

Leak of 4 bytes in 1 objects allocated from:
	@ 41fbfb __gnu_cxx::new_allocator::allocate
	@ 41e815 std::_Vector_base::_M_allocate
	@ 41d333 std::vector::_M_allocate_and_copy
	@ 41c221 std::vector::operator=
	@ 41b816 net::IPEndPoint::operator=
	@ 167ab44 net::MockConnect::MockConnect
	@ 918760 net::SocketDataProvider::SocketDataProvider
	@ 167b1f0 net::StaticSocketDataProvider::StaticSocketDataProvider
	@ 7e69d8 net::FtpNetworkTransactionTest::ExecuteTransaction
	@ 7dd63f
net::FtpNetworkTransactionTest_DownloadTransactionMultilineWelcome_Test::TestBody


Leak of 112 bytes in 1 objects allocated from:
	@ 7e69b3 net::FtpNetworkTransactionTest::ExecuteTransaction
	@ 7e77b4 net::FtpNetworkTransactionTest::TransactionFailHelper
	@ 7e0505
net::FtpNetworkTransactionTest_DownloadTransactionFailUser_Test::TestBody

[dhollowa, 2012-11-15 17:07:33]

On 2012/11/15 07:11:25, Ryan Sleevi wrote:
> This CL is causing a number of issues on the Memory.FYI waterfall
> 
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%2520Heapcheck/...
> 
> Leak of 4 bytes in 1 objects allocated from:
> 	@ 41fbfb __gnu_cxx::new_allocator::allocate
> 	@ 41e815 std::_Vector_base::_M_allocate
> 	@ 41d333 std::vector::_M_allocate_and_copy
> 	@ 41c221 std::vector::operator=
> 	@ 41b816 net::IPEndPoint::operator=
> 	@ 167ab44 net::MockConnect::MockConnect
> 	@ 918760 net::SocketDataProvider::SocketDataProvider
> 	@ 167b1f0 net::StaticSocketDataProvider::StaticSocketDataProvider
> 	@ 7e69d8 net::FtpNetworkTransactionTest::ExecuteTransaction
> 	@ 7dd63f
>
net::FtpNetworkTransactionTest_DownloadTransactionMultilineWelcome_Test::TestBody
> 
> 
> Leak of 112 bytes in 1 objects allocated from:
> 	@ 7e69b3 net::FtpNetworkTransactionTest::ExecuteTransaction
> 	@ 7e77b4 net::FtpNetworkTransactionTest::TransactionFailHelper
> 	@ 7e0505
> net::FtpNetworkTransactionTest_DownloadTransactionFailUser_Test::TestBody

Suppression here:
  https://codereview.chromium.org/11415010/

This was a bad one!  Many many variants.