Fix crash bug in SpdyProxyClientSocket.GetPeerAddress where

Fix crash bug in SpdyProxyClientSocket.GetPeerAddress where
spdy_stream_ was used when NULL.  We now copy the peer address
and local address from spdy_stream_ when we connect.

BUG=106073
TEST=SpdyProxyClientSocket.GetPeerAddressReturnsCorrectValue

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

[Ryan Hamilton, 2011-12-01 19:56:12]

Hi Wan-Teh,

Here's small CL that fixes a crash bug I was just informed of in
SpdyProxyClientSocket.  Can you take a look?

Thanks,

Ryan

[wtc, 2011-12-01 22:22:15]

LGTM.  But I have some questions for you below.

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:127: return next_state_ == STATE_OPEN ||
next_state_ == STATE_CLOSED;

It seems that IsConnected() should return false if
next_state_ is STATE_CLOSED or spdy_stream_ is NULL.
I don't understand this "closed but still connected" state.
Could you explain?

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:267: *address = peer_address_;

Is it normal for spdy_stream_ to become NULL when IsConnected()
is true?

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.h:16: #include "net/base/ip_endpoint.h"

Nit: list ip_endpoint.h after host_port_pair.h.

[Ryan Hamilton, 2011-12-02 22:24:13]

Please take another look. I've changed the implementation to match our
discussions.

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:127: return next_state_ == STATE_OPEN ||
next_state_ == STATE_CLOSED;
On 2011/12/01 22:22:15, wtc wrote:
> 
> It seems that IsConnected() should return false if
> next_state_ is STATE_CLOSED or spdy_stream_ is NULL.
> I don't understand this "closed but still connected" state.
> Could you explain?

Done.

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:267: *address = peer_address_;
On 2011/12/01 22:22:15, wtc wrote:
> 
> Is it normal for spdy_stream_ to become NULL when IsConnected()
> is true?

It was, but because of the change you suggested this is no longer true.

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/8771012/diff/1005/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.h:16: #include "net/base/ip_endpoint.h"
On 2011/12/01 22:22:15, wtc wrote:
> 
> Nit: list ip_endpoint.h after host_port_pair.h.

Done.  (But then we decided to do a different implementation, and these headers
are not  included)

[wtc, 2011-12-02 23:16:40]

Patch Set 4 LGTM.

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:170: Disconnect();

This Disconnect() call may not be necessary.

I understand you added the Disconnect() call to replace
the eof_has_been_read_ member, because you want Read()
to return 0 only once.

It is also reasonable for Read() to return 0 repeatedly
once the peer closed the connection.  I don't remember
how a TCP socket handles this case.  It would be a good
idea to do an experiment to find how how a TCP socket
behaves and match its behavior, but it may not be worth
the effort.

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:617:
EXPECT_EQ(ERR_SOCKET_NOT_CONNECTED, sock_->GetPeerAddress(&addr));

Should we also do
  EXPECT_FALSE(sock_->IsConnected());
?

[Ryan Hamilton, 2011-12-02 23:59:17]

Thanks for the LGTM!

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:170: Disconnect();
On 2011/12/02 23:16:40, wtc wrote:
> 
> This Disconnect() call may not be necessary.
> 
> I understand you added the Disconnect() call to replace
> the eof_has_been_read_ member, because you want Read()
> to return 0 only once.
> 
> It is also reasonable for Read() to return 0 repeatedly
> once the peer closed the connection.  I don't remember
> how a TCP socket handles this case.  It would be a good
> idea to do an experiment to find how how a TCP socket
> behaves and match its behavior, but it may not be worth
> the effort.

You're right.  recv will return 0 each time you call it on a closed socket. 
I've removed the call to Disconnect().

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/8771012/diff/10002/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket_unittest.cc:617:
EXPECT_EQ(ERR_SOCKET_NOT_CONNECTED, sock_->GetPeerAddress(&addr));
On 2011/12/02 23:16:40, wtc wrote:
> 
> Should we also do
>   EXPECT_FALSE(sock_->IsConnected());
> ?

Done.

[commit-bot: I haz the power, 2011-12-03 00:01:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/8771012/13004

[commit-bot: I haz the power, 2011-12-03 03:36:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/8771012/13004

[commit-bot: I haz the power, 2011-12-03 03:38:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/8771012/8004

[commit-bot: I haz the power, 2011-12-03 04:54:19]

Change committed as 112871