Fixes to socket_test_util.cc.

Fixes to socket_test_util.cc.

- Removes ref-counting from DelayedSocketData and OrderedSocketData.
- Calls CompleteRead (which reads from underlying data) only if
  a Socket::Read call is "blocked" (previously returned with ERR_IO_PENDING).
- Removes unsafe pointers to the handed out sockets (the factory does not own them and cannot guarantee their lifetime).

BUG=110626
TEST=./net_unittests --gtest_filter=Spdy*:Http*:Socket*:WebSocket*:Ftp*


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

[szym, 2012-01-19 02:42:06]

On 2012/01/18 20:53:36, szym wrote:

I'm planning to remove other unsafe methods from MockClientSocketFactory:

GetMockTCPClientSocket
GetMockSSLClientSocket
udp_client_sockets
tcp_client_sockets

are all unsafe. The Factory does not own the sockets and cannot guarantee they
will be live. 

Only one test would be affected by this removal:
FtpNetworkTransactionTest.DownloadTransactionEvilPasvUnsafeHost
In this case, I plan to either expose the |data_socket_| in
FtpNetworkTransaction or make the test friend of the class.

[cbentzel, 2012-01-19 16:04:53]

It's been a while since I've worked with the mock sockets and
socket_test_util's. Expect a review late tonight or early tomorrow morning
as I page it back into memory.

On Wed, Jan 18, 2012 at 9:42 PM, <szym@chromium.org> wrote:

> On 2012/01/18 20:53:36, szym wrote:
>
> I'm planning to remove other unsafe methods from MockClientSocketFactory:
>
> GetMockTCPClientSocket
> GetMockSSLClientSocket
> udp_client_sockets
> tcp_client_sockets
>
> are all unsafe. The Factory does not own the sockets and cannot guarantee
> they
> will be live.
>
> Only one test would be affected by this removal:
> FtpNetworkTransactionTest.**DownloadTransactionEvilPasvUns**afeHost
> In this case, I plan to either expose the |data_socket_| in
> FtpNetworkTransaction or make the test friend of the class.
>
>
>
>
http://codereview.chromium.**org/9251019/<http://codereview.chromium.org/9251...
>

[cbentzel, 2012-01-19 16:43:23]

wtc or eroman may also be good reviewers for this CL. I will still look.

[szym, 2012-01-19 17:06:03]

On 2012/01/19 16:43:23, cbentzel wrote:
> wtc or eroman may also be good reviewers for this CL. I will still look.

wtc: would you have time to review this?

[cbentzel, 2012-01-20 16:34:33]

Actually, mmenke would be good given the work he's been doing with the network
stack and socket pools. Plus he sits next to you.

[cbentzel, 2012-01-20 17:54:39]

Were the RefCounted base classes just left over after a conversion to using
WeakPtr's for the DelayedSocketData and OrderedSocketData?

[szym, 2012-01-20 18:07:18]

On 2012/01/20 17:54:39, cbentzel wrote:
> Were the RefCounted base classes just left over after a conversion to using
> WeakPtr's for the DelayedSocketData and OrderedSocketData?

The original implementation of DelayedSocketData in http://crrev.com/46325
(which precedes OrderedSocketData) did have to post itself to the MessageLoop:
http://codereview.chromium.org/1755005/diff/27001/net/socket/socket_test_util.cc
So I believe you are right that WeakPtr made it obsolete.

[szym, 2012-01-20 18:57:24]

On 2012/01/20 18:07:18, szym wrote:
> On 2012/01/20 17:54:39, cbentzel wrote:
> > Were the RefCounted base classes just left over after a conversion to using
> > WeakPtr's for the DelayedSocketData and OrderedSocketData?
> 
> The original implementation of DelayedSocketData in http://crrev.com/46325
> (which precedes OrderedSocketData) did have to post itself to the MessageLoop:
>
http://codereview.chromium.org/1755005/diff/27001/net/socket/socket_test_util.cc
> So I believe you are right that WeakPtr made it obsolete.

Correction, DelayedSocketData was introduced in http://crrev.com/31772
In that version, CompleteRead would be executed 1000ms after the write.

[eroman, 2012-01-21 01:18:30]

I quickly scanned through this change and LGTM.

Was going to suggest you run through the linux_valgrind trybots but looks like
you already did that ;)

http://codereview.chromium.org/9251019/diff/8002/net/dns/dns_transaction_unit...
File net/dns/dns_transaction_unittest.cc (left):

http://codereview.chromium.org/9251019/diff/8002/net/dns/dns_transaction_unit...
net/dns/dns_transaction_unittest.cc:219: EXPECT_EQ(3u,
factory().udp_client_sockets().size());
is this change intentional?

http://codereview.chromium.org/9251019/diff/8002/net/dns/dns_transaction_unit...
net/dns/dns_transaction_unittest.cc:257: EXPECT_EQ(3u,
factory().udp_client_sockets().size());
is this change intentional?

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (right):

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util....
net/socket/socket_test_util.h:312: bool blocked_;
something like "read_pending_" or "read_in_progress_" would be clearer IMO.

[cbentzel, 2012-01-21 02:03:51]

LGTM

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util....
net/socket/socket_test_util.cc:297: 100);
Do you still need this to be 100ms or is 0ms OK at this point?

[szym, 2012-01-21 06:59:16]

Thank you both for reviewing this.

http://codereview.chromium.org/9251019/diff/8002/net/dns/dns_transaction_unit...
File net/dns/dns_transaction_unittest.cc (left):

http://codereview.chromium.org/9251019/diff/8002/net/dns/dns_transaction_unit...
net/dns/dns_transaction_unittest.cc:219: EXPECT_EQ(3u,
factory().udp_client_sockets().size());
On 2012/01/21 01:18:30, eroman wrote:
> is this change intentional?

Yes. This check was redundant. There is only 3 SocketDataProviders and all of
them have to be at_xxx_eof.

Furthermore, the test will be replaced by
http://codereview.chromium.org/9190031/

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util....
net/socket/socket_test_util.cc:297: 100);
On 2012/01/21 02:03:51, cbentzel wrote:
> Do you still need this to be 100ms or is 0ms OK at this point?

I am not sure. See comment below.

http://codereview.chromium.org/9251019/diff/8002/net/socket/socket_test_util....
net/socket/socket_test_util.cc:383: // SpdyStream::ReadResponseHeaders() is
called, we hit a NOTREACHED().
TODO: ask willchan if this is still needed.

[commit-bot: I haz the power, 2012-01-22 01:57:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9251019/17002

[commit-bot: I haz the power, 2012-01-22 03:25:07]

Change committed as 118634