HttpStreamParser: Don't reuse sockets which receive unparsed data.

HttpStreamParser:  Don't reuse sockets which receive unparsed data.

If HttpStreamParser has extra data left over after consuming a valid
HTTP/1.x response on a socket, it would still allow the socket to be
reused if it was connected and idle.  Since we have no idea what the
data actually was, this just seems like a bad idea.  This CL changes
that behavior, so such sockets are no longer considered reuseable.

This does have the downside of papering over server bugs, but it
still seems like the right way to go.

BUG=603140
Committed: https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406
Cr-Commit-Position: refs/heads/master@{#397504}

[mmenke, 2016-04-14 14:27:09]

mmenke@chromium.org changed reviewers:
+ asanka@chromium.org

[mmenke, 2016-04-14 14:27:10]

asanka:  Sending this to you because I know you have some familiarity with
HttpStreamParser::CanReuseConnection.  This is a really low priority fix, so
review at your leisure - aimed at much as reducing the cases where only Chrome
has issues with buggy servers as fixing the bug.

Recommended review order is to look at http_network_transaction_unittest.cc to
see the cases where we were doing weird stuff, then review http_stream_parser*,
http_network_transaction*, and http_response_body_drainer*.  The rest are just
fallout for the http_response_body_drainer change.

[mmenke, 2016-05-11 19:58:33]

asanka:  Ping!

[asanka, 2016-05-13 15:19:52]

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_basic_str...
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_basic_str...
net/http/http_basic_stream.cc:123: new HttpResponseBodyDrainer(this,
std::move(response_info));
The ownership semantics of the Drain() method are a bit surprising. Would it be
possible to raise HttpResonseBodyDrainer up so that HttpNetworkTransaction
itself creates it? That way we can explicitly pass down the HttpStream and
HttpResponseInfo via unique_ptrs and make it explicit that both of those objects
are now owned by the HttpResponseBodyDrainer. Thus the latter would be
responsible for asserting that |response_info| outlives |session|.

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_stream.h
File net/http/http_stream.h (right):

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_stream.h#...
net/http/http_stream.h:175: scoped_ptr<HttpResponseInfo> response_info) = 0;
Should document here that |response_info| *must* be the same as |response| in
SendRequest(). I'd enforce this via DCHECKs

[mmenke, 2016-05-19 20:54:08]

Patchset #6 (id:100001) has been deleted

[mmenke, 2016-05-20 21:46:42]

Patchset #7 (id:140001) has been deleted

[mmenke, 2016-05-20 21:50:02]

Sorry for the slow followup.

I've gone with a much simpler approach.  For review, I suggest you compare the
non-test files against the original files, before this CL.  If you looked at the
new unittests before, you can just compare them to the unittest files in the
previous CL you looked at, since the tests are pretty much the same, just the
approach requires a lot fewer changes in production code.

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_basic_str...
File net/http/http_basic_stream.cc (right):

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_basic_str...
net/http/http_basic_stream.cc:123: new HttpResponseBodyDrainer(this,
std::move(response_info));
On 2016/05/13 15:19:52, asanka wrote:
> The ownership semantics of the Drain() method are a bit surprising. Would it
be
> possible to raise HttpResonseBodyDrainer up so that HttpNetworkTransaction
> itself creates it? That way we can explicitly pass down the HttpStream and
> HttpResponseInfo via unique_ptrs and make it explicit that both of those
objects
> are now owned by the HttpResponseBodyDrainer. Thus the latter would be
> responsible for asserting that |response_info| outlives |session|.

On one hand, you're right about these ownership semantics...On the other hand,
I'd really like to make it so HttpNetworkTransaction doesn't even know about
draining responses (Draining is HTTP/1.x-specific silliness, and makes no sense
for QUIC/SPDY, so doesn't seem like it should be in the common code shared by
all paths).

Anyhow, I've gone with a different solution, so I'm leaving this as it was
before this CL.

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_stream.h
File net/http/http_stream.h (right):

https://codereview.chromium.org/1884943003/diff/60001/net/http/http_stream.h#...
net/http/http_stream.h:175: scoped_ptr<HttpResponseInfo> response_info) = 0;
On 2016/05/13 15:19:52, asanka wrote:
> Should document here that |response_info| *must* be the same as |response| in
> SendRequest(). I'd enforce this via DCHECKs

The more I looked at this, the less I liked it.  I've done with a simpler
solution.

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
net/http/http_stream_parser.cc:598: int
HttpStreamParser::DoReadHeadersComplete(int result) {
We could probably null out response_ now, whenever this method returns with an
io_state of STATE_DONE, but seems beyond the scope of this CL.

[mmenke, 2016-05-27 16:42:34]

asanka:  Ping!  Not that there's any rush on this - not blocking anything, and
unlikely to run into merge issues.

[asanka, 2016-06-01 19:43:35]

lgtm % nits.

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:2260:
base::RunLoop().RunUntilIdle();
For consistency, shouldn't this be done for the other tests as well?

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
net/http/http_stream_parser.cc:1109: if (IsResponseBodyComplete() &&
read_buf_->offset() > 0)
Is it possible for (read_buf_->offset() > 0) to be true but IsMoreDataBuffered()
to be false? If not, shall the use the latter? Otherwise it's not obvious that
that's what is being tested here.

[mmenke, 2016-06-02 15:56:23]

Thanks for the feedback!

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:2260:
base::RunLoop().RunUntilIdle();
On 2016/06/01 19:43:34, asanka wrote:
> For consistency, shouldn't this be done for the other tests as well?

I don't think so.  None of the other tests need it, and I'm not a big fan of
spinning the message loop for the sake of spinning the message loop - it has a
tendency to paper over bugs.  In this case, we need it so that if the
ResponseBodyDrainer is going to return the socket to the pool, it will do so. 

Actually, I supposed all reads are sync, and so that should most likely happen
synchronously, anyways, but since there's no concrete every we can wait for, but
I don't want this test to depend on the implementation details of the drainer. 
We could probe the number of active sockets in the socket pool, but since we
don't have a method to get at that currently, I'm reluctant to depend on it.

I've simplified the test by calling GetIdleSocketCountInTransportSocketPool
instead of using another transaction - we have other tests in this file that use
that method to do the exact same thing, and also spin the message loop before
doing so, so this is following an established pattern.

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://codereview.chromium.org/1884943003/diff/160001/net/http/http_stream_p...
net/http/http_stream_parser.cc:1109: if (IsResponseBodyComplete() &&
read_buf_->offset() > 0)
On 2016/06/01 19:43:35, asanka wrote:
> Is it possible for (read_buf_->offset() > 0) to be true but
IsMoreDataBuffered()
> to be false? If not, shall the use the latter? Otherwise it's not obvious that
> that's what is being tested here.

Done.  Good suggestion - it's much clearer.

[mmenke, 2016-06-02 19:09:32]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-06-02 19:09:33]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1884943003/#ps220001
(title: "oops")

[commit-bot: I haz the power, 2016-06-02 19:10:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884943003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884943003/220001

[commit-bot: I haz the power, 2016-06-02 20:54:29]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-06-02 20:56:58]

Description was changed from

==========
HttpStreamParser:  Don't reuse sockets which receive unparsed data.

If HttpStreamParser has extra data left over after consuming a valid
HTTP/1.x response on a socket, it would still allow the socket to be
reused if it was connected and idle.  Since we have no idea what the
data actually was, this just seems like a bad idea.  This CL changes
that behavior, so such sockets are no longer considered reuseable.

This does have the downside of papering over server bugs, but it
still seems like the right way to go.

BUG=603140
==========

to

==========
HttpStreamParser:  Don't reuse sockets which receive unparsed data.

If HttpStreamParser has extra data left over after consuming a valid
HTTP/1.x response on a socket, it would still allow the socket to be
reused if it was connected and idle.  Since we have no idea what the
data actually was, this just seems like a bad idea.  This CL changes
that behavior, so such sockets are no longer considered reuseable.

This does have the downside of papering over server bugs, but it
still seems like the right way to go.

BUG=603140

Committed: https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406
Cr-Commit-Position: refs/heads/master@{#397504}
==========

[commit-bot: I haz the power, 2016-06-02 20:56:59]

Patchset 10 (id:??) landed as
https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406
Cr-Commit-Position: refs/heads/master@{#397504}