Use sockets with unread data if they've never been used before.

Use sockets with unread data if they've never been used before.

This fixes preconnects to SPDY-capable origins.  StreamSocket's semantics
change slightly. WasEverUsed() for layered sockets is now whether the top-level
socket has read or written user data, rather than whether the underlying
transport socket was used.

Change SSL and SOCKS socket implementations to use the new WasEverUsed()
semantics. This doesn't affect SOCKS much except that preconnect misses now
time out more aggressively. For SSL, it fixes the SPDY issue.

Add tests for SSL socket and socket pool implementations.

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

[davidben, 2014-02-21 23:35:23]

Dunno if we want to rope in more reviewers for this one.

[mmenke, 2014-02-25 17:01:52]

Didn't review the entire CL, see comments.

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:657: return !IsReusable();
Hmm...So the old extra check here was useless, given AssignIdleSocketToRequest's
behavior?  The inconsistency was presumably due to a bug fix, which this is
effectively undoing.

Digging through the history gets us all the way back to
https://codereview.chromium.org/21501 (http://crbug.com/4606).  The bug mentions
an SSL socket receiving data when the server is closing a connection.  The
server will, apparently, send the data and wait for a response.  This may mean
this fix is going in the wrong direction.  Should talk to wtc about this.

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
net/socket/client_socket_pool_base.h:346: // Note that a preconnected socket
(one that has never been used before)
Suggest rephrasing this to be "Note that a socket that has never been used
before (like a preconnected socket) with unread data may be resued."  While
preconnects are the primary source, there may be other cases (We normally cancel
connect jobs when requests are cancelled, but there may be other ways to get
such a socket...Maybe if we have a single requests, the initial connect job
takes too long, we start a backup connect job, and the request is cancelled, so
we kill a connect job, but just one of them?  Not positive about that case, but
I think the other phrasing is more future proof).

[davidben, 2014-02-25 20:23:04]

+wtc.

Wan-Teh, do you think this approach will be a problem for SSL close_notify?

The potential problem case I can think of is if a server times out unused
sockets more aggressively than we do (so in under 10 seconds), sends a
close_notify and does not close the connection until the client sends
close_notify or closes. In that case, we will not notice the connection cannot
be reused, reuse it and immediately get an error. Any problem like this could
always happen racily right after reuse anyway, but this would make us hit it
more reliably on preconnect miss, should any such servers exist.

That said, HttpNetworkTransaction::ShouldResendRequest() might save us?

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...


If this is a problem, I think we'd need to change the way we preconnect and, if
an SSL socket negotiates SPDY, attach a SpdySession immediately (probably with
an aggressive timeout compatible with that of unused sockets) and reuse at that
layer. In that case, this CL may still be handy just to give unused SSL sockets
the more aggressive timeout (so omit the change in
net/socket/client_socket_pool_base.cc, line 452).

[wtc, 2014-02-27 02:11:37]

Hi David,

I didn't read the CL, so my answer to your question may be wrong.

On 2014/02/25 20:23:04, David Benjamin wrote:
> +wtc.
> 
> Wan-Teh, do you think this approach will be a problem for SSL close_notify?
> 
> The potential problem case I can think of is if a server times out unused
> sockets more aggressively than we do (so in under 10 seconds), sends a
> close_notify and does not close the connection until the client sends
> close_notify or closes.

I think no server is likely to wait for the client to echo a close_notify.
A server is likely to close the connection immediately after sending a
close_notify.

> In that case, we will not notice the connection cannot
> be reused, reuse it and immediately get an error.

In the current code, the close_notify from the server causes Chrome to
consider the connection as "not idle". I believe Chrome will close the
connection in that case.

If Chrome is to reuse a condition in this state, it will be able to send
the request successfully, but the reading of the response headers will
return 0 (because of the SSL close_notify). I believe Chrome will resend
the request in this case.

If you find my answer unsatisfactory, I'll need to read the CL. Sorry
about that.

[davidben, 2014-02-27 16:33:22]

Thanks! Given this, I believe this approach will be fine? (And the retry should
still save us in case of problems.) Matt, do you agree or have I missed
something?

On 2014/02/27 02:11:37, wtc wrote:
> Hi David,
> 
> I didn't read the CL, so my answer to your question may be wrong.
> 
> On 2014/02/25 20:23:04, David Benjamin wrote:
> > +wtc.
> > 
> > Wan-Teh, do you think this approach will be a problem for SSL close_notify?
> > 
> > The potential problem case I can think of is if a server times out unused
> > sockets more aggressively than we do (so in under 10 seconds), sends a
> > close_notify and does not close the connection until the client sends
> > close_notify or closes.
> 
> I think no server is likely to wait for the client to echo a close_notify.
> A server is likely to close the connection immediately after sending a
> close_notify.
> 
> > In that case, we will not notice the connection cannot
> > be reused, reuse it and immediately get an error.
> 
> In the current code, the close_notify from the server causes Chrome to
> consider the connection as "not idle". I believe Chrome will close the
> connection in that case.

This CL does change that for the first use of a socket; the first use is allowed
to be not idle because the server may speak first in SPDY with a SETTINGS frame.
It still checks for IsConnected(), so if the server closes the connection right
after a close_notify, it'll notice.

> If Chrome is to reuse a condition in this state, it will be able to send
> the request successfully, but the reading of the response headers will
> return 0 (because of the SSL close_notify). I believe Chrome will resend
> the request in this case.
> 
> If you find my answer unsatisfactory, I'll need to read the CL. Sorry
> about that.

[davidben, 2014-02-27 16:36:34]

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/30002/net/socket/client_socket...
net/socket/client_socket_pool_base.h:346: // Note that a preconnected socket
(one that has never been used before)
On 2014/02/25 17:01:53, mmenke wrote:
> Suggest rephrasing this to be "Note that a socket that has never been used
> before (like a preconnected socket) with unread data may be resued."  While
> preconnects are the primary source, there may be other cases (We normally
cancel
> connect jobs when requests are cancelled, but there may be other ways to get
> such a socket...Maybe if we have a single requests, the initial connect job
> takes too long, we start a backup connect job, and the request is cancelled,
so
> we kill a connect job, but just one of them?  Not positive about that case,
but
> I think the other phrasing is more future proof).

Done.

[wtc, 2014-02-27 18:00:32]

On 2014/02/27 16:33:22, David Benjamin wrote:
>
> This CL does change that for the first use of a socket; the first use is
allowed
> to be not idle because the server may speak first in SPDY with a SETTINGS
frame.

Ah, I see. Yes, the IsConnectedAndIdle() function is based on the behavior of
HTTP/1.1. I think the idleness assumption is not valid for SPDY in general.

> It still checks for IsConnected(), so if the server closes the connection
right
> after a close_notify, it'll notice.

The current implementation of IsConnected() (based on recv(MSG_PEEK)) cannot
notice
a TCP FIN after a close_notify, until the close_notify has been consumed by the
SSL
library.

I've been wanting to experiment with an alternative implementation of
IsConnected()
based on getpeername(). One possible error of getpeername() is ENOTCONN. But I
don't know if that error is only returned when the socket has never been
connected.

[davidben, 2014-02-27 18:52:43]

On 2014/02/27 18:00:32, wtc wrote:
> On 2014/02/27 16:33:22, David Benjamin wrote:
> >
> > This CL does change that for the first use of a socket; the first use is
> allowed
> > to be not idle because the server may speak first in SPDY with a SETTINGS
> frame.
> 
> Ah, I see. Yes, the IsConnectedAndIdle() function is based on the behavior of
> HTTP/1.1. I think the idleness assumption is not valid for SPDY in general.
> 
> > It still checks for IsConnected(), so if the server closes the connection
> right
> > after a close_notify, it'll notice.
> 
> The current implementation of IsConnected() (based on recv(MSG_PEEK)) cannot
> notice
> a TCP FIN after a close_notify, until the close_notify has been consumed by
the
> SSL
> library.
> 
> I've been wanting to experiment with an alternative implementation of
> IsConnected()
> based on getpeername(). One possible error of getpeername() is ENOTCONN. But I
> don't know if that error is only returned when the socket has never been
> connected.

Oh, I see. Hrm, that'll be a problem then. There's still the retry, but probably
best not to rely on that outside of races? This'll either need an alternate
IsConnected or SpdySession-level reuse. I'll take a look at what the latter
would entail.

[davidben, 2014-03-14 18:51:39]

+rch since this involves SPDY and such, so you should probably look over this
and make sure the approach is sane. See linked bug for context.

Talking with mmenke, we decided to try to fix all the relevant retry cases for
errors on UNUSED_IDLE sockets. Turns out both HTTP and SPDY failed to retry
those cases. This CL should take care of the HTTP case:
https://codereview.chromium.org/197283012/

I haven't split the SPDY part out since, before this CL, preconnect + SPDY
doesn't work anyway. That's fixed in the change to SpdySession::IsReused().
(Test still pending; I'll follow-up in a bit.)

[davidben, 2014-03-14 20:22:12]

On 2014/03/14 18:51:39, David Benjamin wrote:
> +rch since this involves SPDY and such, so you should probably look over this
> and make sure the approach is sane. See linked bug for context.
> 
> Talking with mmenke, we decided to try to fix all the relevant retry cases for
> errors on UNUSED_IDLE sockets. Turns out both HTTP and SPDY failed to retry
> those cases. This CL should take care of the HTTP case:
> https://codereview.chromium.org/197283012/
> 
> I haven't split the SPDY part out since, before this CL, preconnect + SPDY
> doesn't work anyway. That's fixed in the change to SpdySession::IsReused().
> (Test still pending; I'll follow-up in a bit.)

The plot thickens:

HttpStreamFactoryImpl::Job::OnPreconnectsComplete has logic that would be for
attaching a SpdySession to preconnects. Except new_spdy_session_ is only set in
DoCreateStream and preconnects stop at DoInitConnectionComplete.
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

The original form of that block was added in
https://codereview.chromium.org/6591030/

But it looks like that was true then too. As far as I can tell that block never
did anything? Not sure. Though that answers my question of what reuse at the
SPDY level might look like? We'd have to reorder the preconnect logic so that
SpdySessions get attached immediately.

The UNUSED_IDLE error retry CL should probably land anyway since that's a place
where a reuse/close race can still happen. (Though not very easily; we pretty
aggressively time out sockets that have never been used.) If we were to do the
SpdySession option:

- We'd consume the SETTINGS frame earlier. Not sure if that buys us much.
- We'd consume any other stuff the server sends if it decides to push that for
whatever reason.
- The SpdySession would be usable for reuse with other domains early if there's
a wildcard cert or something. Though that logic already doesn't behave well with
preconnect; we may want to persist that like we do with SupportsSpdy on
HttpServerProperties.
- We don't have to rely on the retry to deal with a close_notify received
between preconnect and socket use, given wtc's note about IsConnected in comment
#7 (but we still need the retry for the racy case anyway)

- Unused preconnected sockets have a 10 second timeout right now. If we attach a
SpdySession to them immediately, we'd lose that timeout. Probably would want to
reimplement it since we can now have SpdySessions that never see requests
because they were a preconnect miss.
- This version's basically ready already.

Thoughts?

[davidben, 2014-03-14 21:10:29]

On 2014/03/14 20:22:12, David Benjamin wrote:
> HttpStreamFactoryImpl::Job::OnPreconnectsComplete has logic that would be for
> attaching a SpdySession to preconnects. Except new_spdy_session_ is only set
in
> DoCreateStream and preconnects stop at DoInitConnectionComplete.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_stre...

...okay, nevermind. It looks like OnPreconnectsComplete might be a lie anyway?
InitSocketPoolHelper in client_socket_pool_manager.cc just returns OK after
firing off the preconnects, so DoInitConnection thinks the preconnects succeed
synchronously.

[Ryan Hamilton, 2014-03-14 21:30:47]

net/spdy/ changes look good to me, but as you say, we should really have a test
for this.

[mmenke, 2014-03-18 15:16:54]

nss code should be reviewed by someone who knows nss.  I have no idea how it
handles async IO, and didn't see much benefit in trying to dig through it.

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base.h:349: bool IsReusable() const;
I think "IsUsable" may be more accurate, since it's not clear what it means for
an unused socket to be "reusable"

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:124: void set_idle(bool idle) {
idle_ = idle; }
Think it's worth a comment just what this does (Lets 1 byte be "read")

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:390:
static_cast<MockClientSocket*>(socket())->set_idle(false);
Can we just store the socket as a local and avoid the cast?

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:3742: // http://crbug.com/334467
Suggest a test where a reused socket with unread data can't be used (Assuming we
don't have one here, since we didn't have one for the unused case.  If we do
have one, just say so).

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
File net/socket/socks5_client_socket.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:170: const CompletionCallback& callback) {
While you're here, mind fixing this indent?

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:211: int result) {
nit:  Fix indent

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:213: DCHECK(!callback.is_null());
Seems weird to have this DCHECK here, and not in Read or Write.  So I suggest
also putting it in those two functions (Same for the other SOCKS socket class)

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
File net/socket/socks5_client_socket.h (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.h:147: bool was_used_to_convey_data_;
I think it's a little weird to make "WasEverUsed" return
"was_used_to_convey_data_", since they're intended to mean the same thing.  I'm
fine with renaming either the method or the variable, or both.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2094: const char request_text[] = "GET
/ HTTP/1.0\r\n\r\n";
nit kRequestText?

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2096: new
IOBuffer(arraysize(request_text) - 1));
Suggest you have const with the value of "arraysize(request_text) - 1".  Just
seems weird to repeat it everywhere.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2113: // SSL implementation's internal
buffers. Either call PR_Available and
I'm assuming this means the "Idle" part wouldn't work?  (I'm assuming reading 1
byte works fine).

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

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1683: return buffered_spdy_framer_->frames_received() >
0 ||
Hrm...  If we always received a settings frame first thing, would this always be
true?

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1684: connection_->reuse_type() ==
ClientSocketHandle::UNUSED_IDLE;
can we just use connection->is_reused()?  Yes, it will never be REUSED_IDLE,
anyways, but still seems weird to just be checking for UNUSED_IDLE.

[davidben, 2014-03-18 22:53:54]

SPDY retry fixes split off into https://codereview.chromium.org/200723004/ with
tests and a few other issues writing those tests uncovered.

I'll rebase the remainder of this CL and address mmenke's comments when the
other one's dealt with.

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

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1683: return buffered_spdy_framer_->frames_received() >
0 ||
On 2014/03/18 15:16:55, mmenke wrote:
> Hrm...  If we always received a settings frame first thing, would this always
be
> true?

SETTINGS frames aren't counted in this. It's oddly named and a somewhat odd
check, but I think it works? It counts:

SYN_STREAM
HEADERS
SYN_REPLY
data frames

https://codereview.chromium.org/169643006/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1684: connection_->reuse_type() ==
ClientSocketHandle::UNUSED_IDLE;
On 2014/03/18 15:16:55, mmenke wrote:
> can we just use connection->is_reused()?  Yes, it will never be REUSED_IDLE,
> anyways, but still seems weird to just be checking for UNUSED_IDLE.

is_reused() is actually only REUSED_IDLE, not UNUSED_IDLE, which is sort of
bizarre. I haven't looked at all the callers to see if checking both would be
more correct or anything.

[wtc, 2014-03-19 00:17:46]

On 2014/03/18 15:16:54, mmenke wrote:
> nss code should be reviewed by someone who knows nss.  I have no idea how it
> handles async IO, and didn't see much benefit in trying to dig through it.

I reviewed the nss code in patch set 5. It looks correct.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:844: bool was_used_to_convey_data_;

Nit: document this member. I copied your comment from _openssl.h:

  // Set when Read() or Write() successfully reads or writes data to or from the
  // network.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1213: } else {

We may want to test rv > 0 here because the condition above is not rv <= 0.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:2686: } else {

We may want to test rv > 0 here because the condition above is not rv <= 0.

[davidben, 2014-03-25 19:50:17]

Alright, all the CLs split off have landed. We should hopefully retry correctly
now on UNUSED_IDLE sockets. Rebased it onto trunk and addressed wtc's and
mmenke's comments.

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base.h:349: bool IsReusable() const;
On 2014/03/18 15:16:55, mmenke wrote:
> I think "IsUsable" may be more accurate, since it's not clear what it means
for
> an unused socket to be "reusable"

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:124: void set_idle(bool idle) {
idle_ = idle; }
On 2014/03/18 15:16:55, mmenke wrote:
> Think it's worth a comment just what this does (Lets 1 byte be "read")

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
File net/socket/socks5_client_socket.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:170: const CompletionCallback& callback) {
On 2014/03/18 15:16:55, mmenke wrote:
> While you're here, mind fixing this indent?

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:211: int result) {
On 2014/03/18 15:16:55, mmenke wrote:
> nit:  Fix indent

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.cc:213: DCHECK(!callback.is_null());
On 2014/03/18 15:16:55, mmenke wrote:
> Seems weird to have this DCHECK here, and not in Read or Write.  So I suggest
> also putting it in those two functions (Same for the other SOCKS socket class)

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
File net/socket/socks5_client_socket.h (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/socks5_clien...
net/socket/socks5_client_socket.h:147: bool was_used_to_convey_data_;
On 2014/03/18 15:16:55, mmenke wrote:
> I think it's a little weird to make "WasEverUsed" return
> "was_used_to_convey_data_", since they're intended to mean the same thing. 
I'm
> fine with renaming either the method or the variable, or both.

Done. (was_used_to_convey_data_ is what UseHistory calls it which is how
TCPClientSocket tracks it.)

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:844: bool was_used_to_convey_data_;
On 2014/03/19 00:17:46, wtc wrote:
> 
> Nit: document this member. I copied your comment from _openssl.h:
> 
>   // Set when Read() or Write() successfully reads or writes data to or from
the
>   // network.

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1213: } else {
On 2014/03/19 00:17:46, wtc wrote:
> 
> We may want to test rv > 0 here because the condition above is not rv <= 0.

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:2686: } else {
On 2014/03/19 00:17:46, wtc wrote:
> 
> We may want to test rv > 0 here because the condition above is not rv <= 0.

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2094: const char request_text[] = "GET
/ HTTP/1.0\r\n\r\n";
On 2014/03/18 15:16:55, mmenke wrote:
> nit kRequestText?

Done. (That was from copy-and-paste actually.)

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2096: new
IOBuffer(arraysize(request_text) - 1));
On 2014/03/18 15:16:55, mmenke wrote:
> Suggest you have const with the value of "arraysize(request_text) - 1".  Just
> seems weird to repeat it everywhere.

Done.

https://codereview.chromium.org/169643006/diff/330001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2113: // SSL implementation's internal
buffers. Either call PR_Available and
On 2014/03/18 15:16:55, mmenke wrote:
> I'm assuming this means the "Idle" part wouldn't work?  (I'm assuming reading
1
> byte works fine).

Yeah, IsConnectedAndIdle() will still be true because the remainder of the
unread data will get into NSS and OpenSSL's internal buffers.
IsConnectedAndIdle() fails to check that.

[mmenke, 2014-03-26 15:18:32]

LGTM, just nits

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
net/socket/client_socket_pool_base.h:348: // socket) with unread data may be
reused.  This may be, e.g., a SPDY
"may be reused" -> "may be used"

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:126: void set_idle(bool idle) {
idle_ = idle; }
I don't think "idle" is the right name for this.  Maybe just
"set_has_unread_data" or pending data or somesuch.

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1240: bool
SSLClientSocketNSS::Core::WasEverUsed() {
nit:  const

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1240: bool
SSLClientSocketNSS::Core::WasEverUsed() {
Hrm...The above 3 functions can all be const, too.

[davidben, 2014-03-26 16:58:29]

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
File net/socket/client_socket_pool_base.h (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
net/socket/client_socket_pool_base.h:348: // socket) with unread data may be
reused.  This may be, e.g., a SPDY
On 2014/03/26 15:18:32, mmenke wrote:
> "may be reused" -> "may be used"

Done. Also tweaked the wording slightly.

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/client_socke...
net/socket/client_socket_pool_base_unittest.cc:126: void set_idle(bool idle) {
idle_ = idle; }
On 2014/03/26 15:18:32, mmenke wrote:
> I don't think "idle" is the right name for this.  Maybe just
> "set_has_unread_data" or pending data or somesuch.

Done.

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1240: bool
SSLClientSocketNSS::Core::WasEverUsed() {
On 2014/03/26 15:18:32, mmenke wrote:
> nit:  const

Done.

https://codereview.chromium.org/169643006/diff/400001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:1240: bool
SSLClientSocketNSS::Core::WasEverUsed() {
On 2014/03/26 15:18:32, mmenke wrote:
> Hrm...The above 3 functions can all be const, too.

Done.

[davidben, 2014-03-26 16:59:45]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:00:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/169643006/580001

[commit-bot: I haz the power, 2014-03-26 20:12:33]

Change committed as 259671