SPDY - Re-land greedy read support for SpdySession

Re-land greedy read support for SpdySession

Added Unit test to close the socket during spdy_session's 
read and verifying that getting new URLs from the same 
network session don't result in a failure.

Modified mock socket code to close the connection if 
mock_read closes the connection (because of ERR_CONNECTION_CLOSED error).

Implement greedy approach to read all the data and process 
it from the ClientSocket until we block.

Implemented the greedy approach to improves the network
stack performance on the mobile.

spdy_session_test_util.* files have the common code between
SpdySessionSpd2Test and SpdySessionSpd3Test.  Created this 
file to reduce errors and to avoid duplicating of code.

Review URL: https://chromiumcodereview.appspot.com/11644088

The change adds a scoped_refptr to SpdySession in DoLoop
to clean up the state and to finish pending reads, in case
SpdyFramer receives GOAway and closes all streams.

DoReadComplete was holding the last reference before and
DoLoop was accessing the object after DoReadComplete deleted
the object at the end of the function.

Review URL: https://codereview.chromium.org/12212102
 
Added unit tests for use after free of SpdySession
and unit test to test yielding + async during Read.

A test that crashes when scoped_refptr to SpdySession 
deleted from DoLoop.

Added the following test per rch@ to test interactions of
yielding + async during Read.

Do the following MockReads and verify all the data is 
consumed and SpdySession has yielded during Read.

SYNCHRONOUS 8K
SYNCHRONOUS 8K
SYNCHRONOUS 8K
SYNCHRONOUS 2K
ASYNC 8K
SYNCHRONOUS 8K
SYNCHRONOUS 8K
SYNCHRONOUS 8K
SYNCHRONOUS 2K

Review URL: https://chromiumcodereview.appspot.com/12207122

Reverted URL: https://chromiumcodereview.appspot.com/13608003/

 
BUG=175574, 166958, 224701, 175069
R=rch@chromium.org, jar@chromium.org
TEST=network unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194540

[Ryan Hamilton, 2013-04-12 02:50:31]

I like the test in patch set 1.  I think you could possibly commit that now.

Obviously, we can't land patch set 2 since it's buggy :>

https://codereview.chromium.org/13834009/diff/1/net/http/http_network_transac...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://codereview.chromium.org/13834009/diff/1/net/http/http_network_transac...
net/http/http_network_transaction_spdy2_unittest.cc:11302: // session. Verify
that new url's from the same HttpNetworkSession do work.
nit: from the same HttpNetworkSession (and a new SpdySession) do work.

https://codereview.chromium.org/13834009/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/13834009/diff/1/net/socket/socket_test_util.c...
net/socket/socket_test_util.cc:1063: if (read_data_.result ==
ERR_CONNECTION_CLOSED) {
This should probably be the same error code used in the MockTcpClientSocket,
just to be consistent.

https://codereview.chromium.org/13834009/diff/11001/net/http/http_network_tra...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://codereview.chromium.org/13834009/diff/11001/net/http/http_network_tra...
net/http/http_network_transaction_spdy2_unittest.cc:11304:
TEST_F(HttpNetworkTransactionSpdy2Test, DISABLED_ErrorSocketNotConnected) {
Why is this DISABLED_?

[ramant (doing other things), 2013-04-12 03:13:24]

This patch fixes the regression. It acts on the error (including reading of no
bytes) right away and closes the SpdySession instead of Posting a task.

Enabled the disabled tests. All network unit tests pass with this patch.

https://codereview.chromium.org/13834009/diff/11001/net/http/http_network_tra...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://codereview.chromium.org/13834009/diff/11001/net/http/http_network_tra...
net/http/http_network_transaction_spdy2_unittest.cc:11304:
TEST_F(HttpNetworkTransactionSpdy2Test, DISABLED_ErrorSocketNotConnected) {
On 2013/04/12 02:50:31, Ryan Hamilton wrote:
> Why is this DISABLED_?

Enabled them in patch # 3.

Done.

[ramant (doing other things), 2013-04-12 03:23:09]

https://codereview.chromium.org/13834009/diff/1/net/http/http_network_transac...
File net/http/http_network_transaction_spdy2_unittest.cc (right):

https://codereview.chromium.org/13834009/diff/1/net/http/http_network_transac...
net/http/http_network_transaction_spdy2_unittest.cc:11302: // session. Verify
that new url's from the same HttpNetworkSession do work.
On 2013/04/12 02:50:31, Ryan Hamilton wrote:
> nit: from the same HttpNetworkSession (and a new SpdySession) do work.

Done.

https://codereview.chromium.org/13834009/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/13834009/diff/1/net/socket/socket_test_util.c...
net/socket/socket_test_util.cc:1063: if (read_data_.result ==
ERR_CONNECTION_CLOSED) {
On 2013/04/12 02:50:31, Ryan Hamilton wrote:
> This should probably be the same error code used in the MockTcpClientSocket,
> just to be consistent.

Done.

[ramant (doing other things), 2013-04-12 05:48:17]

Hi Ryan and Jim,
  Was talking with Jim to make the code behave as before. Would like to talk
with Ryan about posting a task when we are going to close the session. 

thanks
raman

[ramant (doing other things), 2013-04-13 21:41:21]

Hi Ryan and Jim,
  Please take another look. Fixed the comments from Ryan.

thanks
raman

[Ryan Hamilton, 2013-04-14 02:21:46]

To facilitate later readers of the CL, can you clean up the description?
* Make sure that the first line accurately describes the change.  Something like
"Re-land greedy read support for SpdySession". 
* Remove the extra "> > " prefixes from the description
* Remove the extra R=, TEST= and BUG= from the descriptions of each of the
original CLs.  Possibly update the BUG= line at the bottom.

Otherwise, looking great.  Thanks

https://codereview.chromium.org/13834009/diff/52003/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/13834009/diff/52003/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:480:
UMA_HISTOGRAM_COUNTS("Net.SpdySession.InitAndError.BytesRead", bytes_read_);
We had also talked about adding a histogram of the number of bytes when the
session was closed.  That would be somewhat redundant with this, I guess?

[ramant (doing other things), 2013-04-16 19:12:23]

PTAL. thanks.

https://codereview.chromium.org/13834009/diff/52003/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/13834009/diff/52003/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:480:
UMA_HISTOGRAM_COUNTS("Net.SpdySession.InitAndError.BytesRead", bytes_read_);
On 2013/04/14 02:21:46, Ryan Hamilton wrote:
> We had also talked about adding a histogram of the number of bytes when the
> session was closed.  That would be somewhat redundant with this, I guess?

Added new histograms per our discussion.
Done.

[Ryan Hamilton, 2013-04-16 19:54:47]

lgtm

https://codereview.chromium.org/13834009/diff/63001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/13834009/diff/63001/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:1188:
UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SpdySession.ClosedOnError.Error", -err);
jar: how slowly is slowly?

https://codereview.chromium.org/13834009/diff/63001/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:1188:
UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SpdySession.ClosedOnError.Error", -err);
nit: I think I would drop the trailing .Error since it seems redundant with
"ClosedOnError"

[ramant (doing other things), 2013-04-16 21:45:15]

https://codereview.chromium.org/13834009/diff/63001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/13834009/diff/63001/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:1188:
UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SpdySession.ClosedOnError.Error", -err);
On 2013/04/16 19:54:47, Ryan Hamilton wrote:
> nit: I think I would drop the trailing .Error since it seems redundant with
> "ClosedOnError"

Done.

[jar (doing other things), 2013-04-17 00:16:37]

lgtm

[ramant (doing other things), 2013-04-17 06:12:34]

Committed patchset #8 manually as r194540 (presubmit successful).