CHECK to make sure connection_group is never empty.

CHECK to make sure connection_group is never empty.
Testing to see if this is the reason we hit the CHECK(!group_name_.empty()) in ClientSocketHandle::GetLoadState().
BUG=http://crbug.com/15374
TEST=none

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

[willchan no longer on Chromium, 2009-07-07 17:48:36]

From what I can tell, either the connection_group is sometimes empty, or the
ResourceDispatcherHost has references to old URLRequests which were in
STATE_INIT_CONNECTION_COMPLETE when the HttpNetworkTransaction got deleted.  I'm
a bit skeptical of the second theory, so here's a changelist to test the first
one out.

[eroman, 2009-07-07 19:19:18]

lgtm

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

http://codereview.chromium.org/155150/diff/1/2#newcode595
Line 595: CHECK(!connection_group.empty()) << "URL: " <<
request_->url.GetOrigin()
This seems unlikely to have been the case for the original bug report. Since the
only way I can see for |connection_group| to be empty is for |proxy_mode_| to be
|kDirectConnection|, and for |request_->url.getOrigin().spec()| to have returned
"".

However, for GURL::spec() to have returned "", the user (who I presume had
DCHECKs enabled), would have already hit a DCHECK in GURL::spec() for calling
spec() on an invalid GURL.

No harm in checking though.

Can you link to the bug number, and add a TODO since I presume this is a
temporary instrumentation?

http://codereview.chromium.org/155150/diff/1/3
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/155150/diff/1/3#newcode59
Line 59: group_name_.clear();
I wander if there is a code-path where GetLoadState() can be called *after*
ResetInternal() has been called.

[willchan no longer on Chromium, 2009-07-07 20:21:11]

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

http://codereview.chromium.org/155150/diff/1/2#newcode595
Line 595: CHECK(!connection_group.empty()) << "URL: " <<
request_->url.GetOrigin()
On 2009/07/07 19:19:18, eroman wrote:
> This seems unlikely to have been the case for the original bug report. Since
the
> only way I can see for |connection_group| to be empty is for |proxy_mode_| to
be
> |kDirectConnection|, and for |request_->url.getOrigin().spec()| to have
returned
> "".

Correct

> 
> However, for GURL::spec() to have returned "", the user (who I presume had
> DCHECKs enabled), would have already hit a DCHECK in GURL::spec() for calling
> spec() on an invalid GURL.

Why do you presume DCHECKs were enabled?  The users are hitting the
CHECK(!group_name_.empty()) in ClientSocketHandle::GetLoadState().  It's not a
DCHECK.

> 
> No harm in checking though.
> 
> Can you link to the bug number, and add a TODO since I presume this is a
> temporary instrumentation?
>

http://codereview.chromium.org/155150/diff/1/3
File net/socket/client_socket_handle.cc (right):

http://codereview.chromium.org/155150/diff/1/3#newcode59
Line 59: group_name_.clear();
On 2009/07/07 19:19:18, eroman wrote:
> I wander if there is a code-path where GetLoadState() can be called *after*
> ResetInternal() has been called.

Yes, I tried to think about it.  The only caller of
ClientSocketHandle::GetLoadState() is HttpNetworkTransaction::GetLoadState(),
which only calls it if it's in STATE_INIT_CONNECTION_COMPLETE, which is only set
in DoInitConnection().  To me, it seems like either we're somehow possibly
accessing a deleted HttpNetworkTransaction that has already reset the
ClientSocketHandle but is still in STATE_INIT_CONNECTION_COMPLETE, or the
ClientSocketHandle got initialized with an empty connection group.  Thoughts?

[eroman, 2009-07-07 20:43:51]

On Tue, Jul 7, 2009 at 1:21 PM, <willchan@chromium.org> wrote:
>
> http://codereview.chromium.org/155150/diff/1/2
> File net/http/http_network_transaction.cc (right):
>
> http://codereview.chromium.org/155150/diff/1/2#newcode595
> Line 595: CHECK(!connection_group.empty()) << "URL: " <<
> request_->url.GetOrigin()
> On 2009/07/07 19:19:18, eroman wrote:
>>
>> This seems unlikely to have been the case for the original bug report.
>
> Since the
>>
>> only way I can see for |connection_group| to be empty is for
>
> |proxy_mode_| to be
>>
>> |kDirectConnection|, and for |request_->url.getOrigin().spec()| to
>
> have returned
>>
>> "".
>
> Correct
>
>
>> However, for GURL::spec() to have returned "", the user (who I presume
>
> had
>>
>> DCHECKs enabled), would have already hit a DCHECK in GURL::spec() for
>
> calling
>>
>> spec() on an invalid GURL.
>
> Why do you presume DCHECKs were enabled? =A0The users are hitting the
> CHECK(!group_name_.empty()) in ClientSocketHandle::GetLoadState(). =A0It'=
s
> not a DCHECK.

Oh, ok. I had assumed the empty check was done using a DCHECK, but I
see now it is actually a CHECK.

>
>
>> No harm in checking though.
>
>> Can you link to the bug number, and add a TODO since I presume this is
>
> a
>>
>> temporary instrumentation?
>
>
> http://codereview.chromium.org/155150/diff/1/3
> File net/socket/client_socket_handle.cc (right):
>
> http://codereview.chromium.org/155150/diff/1/3#newcode59
> Line 59: group_name_.clear();
> On 2009/07/07 19:19:18, eroman wrote:
>>
>> I wander if there is a code-path where GetLoadState() can be called
>
> *after*
>>
>> ResetInternal() has been called.
>
> Yes, I tried to think about it. =A0The only caller of
> ClientSocketHandle::GetLoadState() is
> HttpNetworkTransaction::GetLoadState(), which only calls it if it's in
> STATE_INIT_CONNECTION_COMPLETE, which is only set in DoInitConnection().
> =A0To me, it seems like either we're somehow possibly accessing a deleted
> HttpNetworkTransaction that has already reset the ClientSocketHandle but
> is still in STATE_INIT_CONNECTION_COMPLETE, or the ClientSocketHandle
> got initialized with an empty connection group. =A0Thoughts?
>
> http://codereview.chromium.org/155150
>

[wtc, 2009-07-15 23:09:11]

LGTM.