Fix a crash in net::SSLClientSocketWin::OnIOComplete(int) when...

Fix a crash in net::SSLClientSocketWin::OnIOComplete(int) when
an HttpNetworkTransaction is destroyed while its SSLClientSocket
is verifying a certificate.

Document that ClientSocket::Disconnect aborts any pending IO and
prevents completion callback from running, like a cancel method.

Change the verifier_ member of SSLClientSocket to a scoped_ptr so
that we can destroy the CertVerifier object in the Disconnect method.
(CertVerifier doesn't have a cancel method, so we cancel pending
certificate verification by destroying the CertVerifier object.)

R=willchan
BUG=http://crbug.com/13981
TEST=none.  I can only reproduce this crash by modifying the
source code to create that condition.

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

[wtc, 2009-06-25 19:41:57]

Will: please review this CL.  Please also review
http_network_transaction.cc (outside this CL) to verify
the destructor properly shuts down everything that may call
io_callback_.

Everyone else: please review my changes to client_socket.h.
ClientSocket::Disconnect is now documented to abort any
pending IO and suppress completion callbacks.  It probably
should be moved to Socket, where Read and Write are defined.

[willchan no longer on Chromium, 2009-06-25 20:57:47]

lgtm, although it'd be nice to have tests that verify that Disconnect() does
cancel the verification IO.  It looks like our unittest doesn't really support
those types of tests very well =/

[eroman, 2009-06-25 21:09:38]

lgtm

http://codereview.chromium.org/147159/diff/1012/17
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/147159/diff/1012/17#newcode728
Line 728: verifier.reset(new CertVerifier);
hmm... this strikes me as a compile error, perhaps i am misreading something?

http://codereview.chromium.org/147159/diff/1012/17#newcode736
Line 736: verifier.reset();
how about DCHECKing that its not null prior to reset.

also i don't see |verifier| defined anywhere.

http://codereview.chromium.org/147159/diff/1012/15
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/147159/diff/1012/15#newcode865
Line 865: verifier_.reset();
I recommend additionally adding a comment to cert_verifier.cc describing that
the verifier may get deleted from within the callback.

[wtc, 2009-06-25 22:12:04]

eroman: I made all of your suggested changes.  Thanks.

http://codereview.chromium.org/147159/diff/1012/15
File net/socket/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/147159/diff/1012/15#newcode865
Line 865: verifier_.reset();
On 2009/06/25 21:09:38, eroman wrote:
> I recommend additionally adding a comment to cert_verifier.cc describing that
> the verifier may get deleted from within the callback.

I found that we already have such a comment in
CertVerifier::Request::DoCallback():

    // Drop the verifier's reference to us.  Do this before running the
    // callback since the callback might result in the verifier being
    // destroyed.
    verifier_->request_ = NULL;

    callback_->Run(error_);

[rvargas (doing something else), 2009-06-25 22:32:05]

LGTM. Sorry for taking that long.