Fix TCPClientSocket::GetLocalAddress() to work when socket is not connected.

Fix TCPClientSocket::GetLocalAddress() to work when socket is not connected.

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

[wtc, 2012-07-19 18:36:36]

Review comments on patch set 1:

Thank you for writing the CL.  I suggest some changes below.
You can ignore the suggestion for a new test if it's nontrivial
to do.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.

In the CL's description, the BUG=119312 field should be changed
to BUG=118312.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_u...
File net/socket/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_u...
net/socket/tcp_client_socket_unittest.cc:51: EXPECT_EQ(OK,
connect_callback.WaitForResult());

Ideally, we should close/disconnect accepted_socket, and
verify that socket.IsConnected() becomes false, but
socket.GetLocalAddress() still works.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
net/socket/tcp_client_socket_win.cc:653: } else {

Nit: omit "else" after a return statement.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
net/socket/tcp_client_socket_win.cc:656: }

If socket_ is INVALID_SOCKET, I think we should simply
return an error.  Returning ERR_SOCKET_NOT_CONNECTED in
this case is fine.  We probably should add a new error
code ERR_SOCKET_NOT_BOUND.

  if (socket_ == INVALID_SOCKET)
    return ERR_SOCKET_NOT_CONNECTED;

The !IsConnected() test is inappropriate because it can
become true by the peer's action alone (if the peer closes
the TCP connection).  On the other hand, socket_ can only
be INVALID_SOCKET by our local action (if we have never
called Connect, or have called Disconnect).

BUT, your unit test showed the semantics you want to support.
I think your semantics is reasonable, too.  In that case,
I think Disconnect must clear bind_address_.

Finally, the comment for the GetLocalAddress method in
net/socket/stream_socket.h should be updated to describe
the new behavior.

[Sergey Ulanov, 2012-07-19 21:36:20]

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_l...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_l...
net/socket/tcp_client_socket_libevent.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/07/19 18:36:36, wtc wrote:
> 
> In the CL's description, the BUG=119312 field should be changed
> to BUG=118312.

Done.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_u...
File net/socket/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_u...
net/socket/tcp_client_socket_unittest.cc:51: EXPECT_EQ(OK,
connect_callback.WaitForResult());
On 2012/07/19 18:36:36, wtc wrote:
> 
> Ideally, we should close/disconnect accepted_socket, and
> verify that socket.IsConnected() becomes false, but
> socket.GetLocalAddress() still works.

Done.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
net/socket/tcp_client_socket_win.cc:653: } else {
On 2012/07/19 18:36:36, wtc wrote:
> 
> Nit: omit "else" after a return statement.

Done.

http://codereview.chromium.org/10803027/diff/1/net/socket/tcp_client_socket_w...
net/socket/tcp_client_socket_win.cc:656: }
On 2012/07/19 18:36:36, wtc wrote:
> 
> If socket_ is INVALID_SOCKET, I think we should simply
> return an error.  Returning ERR_SOCKET_NOT_CONNECTED in
> this case is fine.  We probably should add a new error
> code ERR_SOCKET_NOT_BOUND.
> 
>   if (socket_ == INVALID_SOCKET)
>     return ERR_SOCKET_NOT_CONNECTED;
> 
> The !IsConnected() test is inappropriate because it can
> become true by the peer's action alone (if the peer closes
> the TCP connection).  On the other hand, socket_ can only
> be INVALID_SOCKET by our local action (if we have never
> called Connect, or have called Disconnect).
> 
> BUT, your unit test showed the semantics you want to support.
> I think your semantics is reasonable, too.  In that case,
> I think Disconnect must clear bind_address_.

Done.

> Finally, the comment for the GetLocalAddress method in
> net/socket/stream_socket.h should be updated to describe
> the new behavior.

Done.

[wtc, 2012-07-19 22:39:26]

Patch set 2 LGTM.  Thanks!

http://codereview.chromium.org/10803027/diff/6002/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/10803027/diff/6002/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_unittest.cc:53: socket.Disconnect();

Thank you for adding this test.

Ideally we should add the following test before this test:

  EXPECT_TRUE(socket.IsConnected());
  accepted_socket->Disconnect();
  EXPECT_FALSE(socket.IsConnected());  // MAY NEED TO REMOVE THIS BECAUSE IT MAY
BE FLAKY
  EXPECT_EQ(OK, socket.GetLocalAddress(&local_address_result));

http://codereview.chromium.org/10803027/diff/6002/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_unittest.cc:54: EXPECT_FALSE(socket.IsConnected());

Here ideally we want to test that the internal socket_ member
is INVALID_SOCKET, but there may not be a public method to test
that.  So checking that socket.IsConnected() is false
may be the best we can do.

[Sergey Ulanov, 2012-07-19 23:48:48]

http://codereview.chromium.org/10803027/diff/6002/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/10803027/diff/6002/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_unittest.cc:53: socket.Disconnect();
On 2012/07/19 22:39:26, wtc wrote:
> 
> Thank you for adding this test.
> 
> Ideally we should add the following test before this test:
> 
>   EXPECT_TRUE(socket.IsConnected());
>   accepted_socket->Disconnect();
>   EXPECT_FALSE(socket.IsConnected());  // MAY NEED TO REMOVE THIS BECAUSE IT
MAY
> BE FLAKY
>   EXPECT_EQ(OK, socket.GetLocalAddress(&local_address_result));

Done.

[commit-bot: I haz the power, 2012-07-19 23:50:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/10803027/3002

[commit-bot: I haz the power, 2012-07-20 01:11:57]

Change committed as 147586