Fix case where a SPDY stream with an un-verified cert would kill the browser...

Fix case where a SPDY stream with an un-verified cert would kill the browser
if an https:// URL was attempted to be fetched across it.  Also fix a bug
where when two SSL connections are made, but only one is moved into a SPDY
session (because the second is redundant), close the redundant connection.
This had been leaking a un-verified SSL connection into the pool, which
could then get re-used as though it was verified.

BUG=64861
TEST=none


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

[Mike Belshe, 2010-12-01 01:17:55]

Adding wtc since Will is out.

[wtc, 2010-12-01 02:41:39]

LGTM.

Is your http_stream_request.cc change alone enough to fix
this crash?  Just wanted to check my understanding.

http://codereview.chromium.org/5409004/diff/1/net/http/http_stream_request.cc
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/5409004/diff/1/net/http/http_stream_request.cc...
net/http/http_stream_request.cc:765: connection_->Reset();
You can avoid duplicating this code by moving it to the
    if (!spdy_session.get()) {
statement at line 770 as follows:

  if (spdy_session.get()) {
    // We don't need our socket.
    if (connection_->socket())
      connection_->socket()->Disconnect();
    connection_->Reset();
  } else {
    // SPDY can be negotiated using the TLS next protocol negotiation (NPN)
    ...

[willchan no longer on Chromium, 2010-12-02 23:02:45]

It'd be great to add metadata to the ClientSocketHandle tracking whether or not
the SSL authentication error has been handled.  We should DCHECK in
ClientSocketHandle::ResetInternal() to make sure that we don't call
ClientSocketPool::ReleaseSocket() on a socket with an authentication error
associated with it.

http://codereview.chromium.org/5409004/diff/7001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/5409004/diff/7001/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:317: LOG(ERROR) << "Tried to get pushed spdy stream for
secure content over an "
Why is this ERROR instead of DFATAL?  It's a bug if this happens, so it should
still crash in debug mode, right?

CloseSessionOnError() is a good safety mechanism though.