Prevent recycling TCPClient socket in the case of https, so as not to crash....

Prevent recycling TCPClientSocket in the case of https, so as not to crash.

The situation this protects against is better explained in comment 13:

http://code.google.com/p/chromium/issues/detail?id=3772#c13


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

[wtc, 2008-11-04 04:53:25]

I suggest that you back out the instrumentation code
and then add the bug fix in two CLs.  This will make
it easier to see what the bug fix is.

Alternatively, you can leave the instrumentation code
in for now, because I'd like to play with it to collect
some more data (e.g., the first 128 bytes of the request
to ensure that it's a CONNECT method).  I'll remove
the instrumentation code for you.  How does that sound?

http://codereview.chromium.org/9088/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/9088/diff/1/2#newcode431
Line 431: // the CONNECT method, since the tunnel is already established.
This block comment isn't necessary.  At least, it should
not be specific to using_ssl_ and using_tunnel_.

The point is that if reused_socket_ is true, socket_ is
already connected, so we can skip several states and
proceed to write the request headers.

http://codereview.chromium.org/9088/diff/1/2#newcode755
Line 755: (content_length_ != -1 && content_read_ > content_length_))
I would add "|| establishing_tunnel_" here.

[eroman, 2008-11-04 05:03:22]

> I suggest that you back out the instrumentation code
> and then add the bug fix in two CLs.  This will make
> it easier to see what the bug fix is.

Will mail instrumentation back-out cl first then.

[eroman, 2008-11-04 05:19:40]

http://codereview.chromium.org/9088/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/9088/diff/1/2#newcode431
Line 431: // the CONNECT method, since the tunnel is already established.
On 2008/11/04 04:53:25, wtc wrote:
> This block comment isn't necessary.  At least, it should
> not be specific to using_ssl_ and using_tunnel_.
> 
> The point is that if reused_socket_ is true, socket_ is
> already connected, so we can skip several states and
> proceed to write the request headers.

Removed comment.

http://codereview.chromium.org/9088/diff/1/2#newcode755
Line 755: (content_length_ != -1 && content_read_ > content_length_))
On 2008/11/04 04:53:25, wtc wrote:
> I would add "|| establishing_tunnel_" here.

Done -- moved the check from if (done) block into  this one.

[wtc, 2008-11-04 15:08:03]

LGTM.  Please give me a day to play with the instrumentation
hacks, before checking this in.

http://codereview.chromium.org/9088/diff/14/15
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/9088/diff/14/15#newcode760
Line 760: // content length, or if the tunnel was not established.
Add "successfully" after "established" to make it clear
that we tried to establish the tunnel but failed.

Nit: the ordering of the conditions in the comment should
match the ordering of the conditions in the code, to help
code comprehension.

[eroman, 2008-11-04 22:25:47]

Will add a unit-test for this shortly.

(ended up committing in a hurry to fix a build redness).