Make HttpNetworkTransaction no longer own a reference to HttpNetworkSession.

Make HttpNetworkTransaction no longer own a reference to HttpNetworkSession.
It is a bug if anything is holding on to a transaction after the levels of
the network stack above the Session are destroyed, so there's no reason to hold
on to the reference.

There have historically been a number of such bugs, and they currently produce
useless crash stack traces, which show crashes as callbacks try to call into
deleted higher level objects.  This should hopefully make them crash more
consistently, with more useful stack traces when owners of these requests
delete them in the absence of the network stack, assuming nothing else is holding
on to the HttpNetworkSession.

BUG=295344, 294910
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229081

[mmenke, 2013-10-15 17:48:24]

wtc:  Mind reviewing this?  The unit test changes are largely mechanical.  My
only concern is whether there's some reason this change is a bad idea that I'm
not aware of.

[wtc, 2013-10-15 23:08:21]

Patch set 3 LGTM.

High-level commenrs:

1. This seems like a good idea. There could be a lot of HttpNetworkTransaction
objects. In general it can make debugging difficult if the reference count of an
object is huge.

2. I don't understand this statement in the CL's description:

    This should hopefully make them crash more
    consistently when owners of these requests
    delete them in the absence of the network
    stack.

Could you elaborate? Did you mean if the HttpNetworkSession object is deleted
(because reference count will reach 0), we will crash more consistently? Thanks.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:3748: // Configure against proxy
server "myproxy:70".

Nit: this comment could be describing |session| because "myproxy:70" is used to
create |session_deps_| and |&session_deps_| is used to create |session|.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7364:
session_deps_.socket_factory->AddSocketDataProvider(&data);

Nit: it may be safer to move line 7352 after this line, so that we create
|session| after we are done modifying |session_deps_|.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7393:
session_deps_.socket_factory->AddSocketDataProvider(&data);

Nit: same here. Consider creating |session| after this line.

[mmenke, 2013-10-15 23:49:37]

On 2013/10/15 23:08:21, wtc wrote:
> Patch set 3 LGTM.
> 
> High-level commenrs:
> 
> 1. This seems like a good idea. There could be a lot of HttpNetworkTransaction
> objects. In general it can make debugging difficult if the reference count of
an
> object is huge.
> 
> 2. I don't understand this statement in the CL's description:
> 
>     This should hopefully make them crash more
>     consistently when owners of these requests
>     delete them in the absence of the network
>     stack.
> 
> Could you elaborate? Did you mean if the HttpNetworkSession object is deleted
> (because reference count will reach 0), we will crash more consistently?
Thanks.

Yes - if the HttpNetworkSession object is deleted before the requests (Which
should be the case - only the HttpNetworkLayer(s) will now be holding on to a
reference, other than test code.  We may even be able to get rid of refcounting
completely), then there's no socket pool.  So we should crash when we try to
return the socket to the pool, or when we try to add a ResponseDrainer to the
HttpNetworkSession.

Of course, if the request manages to complete before it's destroyed, we'll get a
useless stack trace.

I'll fix the nits tomorrow.

[wtc, 2013-10-16 15:04:32]

More review comments on patch set 3:

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:5509: for (int i = 0; i < 2; i++)
{

I wonder why this test needs to repeat this twice.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7364:
session_deps_.socket_factory->AddSocketDataProvider(&data);

I took another look at this. I think your change is fine. It should be OK to
call session_deps_.socket_factory->AddSocketDataProvider() after we have created
|session|, as long as we call it before trans->Start().

So this becomes a matter of taste, and I saw both styles used in this file.

[mmenke, 2013-10-16 15:58:44]

Thanks for the careful review!

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:3748: // Configure against proxy
server "myproxy:70".
On 2013/10/15 23:08:21, wtc wrote:
> 
> Nit: this comment could be describing |session| because "myproxy:70" is used
to
> create |session_deps_| and |&session_deps_| is used to create |session|.

Done.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:5509: for (int i = 0; i < 2; i++)
{
On 2013/10/16 15:04:32, wtc wrote:
> 
> I wonder why this test needs to repeat this twice.

https://codereview.chromium.org/43115 - sounds like (Among other things) there
was a bug where we'd return the SSL socket to the socket pool in the bad cert
case, and then reuse it for the next request.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7364:
session_deps_.socket_factory->AddSocketDataProvider(&data);
On 2013/10/16 15:04:32, wtc wrote:
> 
> I took another look at this. I think your change is fine. It should be OK to
> call session_deps_.socket_factory->AddSocketDataProvider() after we have
created
> |session|, as long as we call it before trans->Start().
> 
> So this becomes a matter of taste, and I saw both styles used in this file.

Order changed.  I agree with you that the ordering seems like a bad idea, even
if it does work.

https://codereview.chromium.org/24246016/diff/56001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7393:
session_deps_.socket_factory->AddSocketDataProvider(&data);
On 2013/10/15 23:08:21, wtc wrote:
> 
> Nit: same here. Consider creating |session| after this line.

Done.

[commit-bot: I haz the power, 2013-10-16 16:26:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/94001

[wtc, 2013-10-16 18:12:43]

Patch set 4 LGTM. Thanks.

https://codereview.chromium.org/24246016/diff/94001/net/http/http_network_tra...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/24246016/diff/94001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7462:
session_deps_.socket_factory->AddSocketDataProvider(&data2);

Nit: if you want to wait until we are done setting up session_deps_ to create
|session|, we should create |session| after this line.

No need to abort the commit queue job for this.

[mmenke, 2013-10-16 18:17:52]

https://codereview.chromium.org/24246016/diff/94001/net/http/http_network_tra...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/24246016/diff/94001/net/http/http_network_tra...
net/http/http_network_transaction_unittest.cc:7462:
session_deps_.socket_factory->AddSocketDataProvider(&data2);
On 2013/10/16 18:12:44, wtc wrote:
> 
> Nit: if you want to wait until we are done setting up session_deps_ to create
> |session|, we should create |session| after this line.
> 
> No need to abort the commit queue job for this.

Done.  While there's no need to abort the CQ job, there's no real cost to doing
so - I'm not in any hurry to land the CL.  Could bypass the CQ, but I'm
sufficiently paranoid that I prefer not to.

[commit-bot: I haz the power, 2013-10-16 18:21:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/137001

[commit-bot: I haz the power, 2013-10-16 20:37:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/137001

[commit-bot: I haz the power, 2013-10-16 21:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/137001

[commit-bot: I haz the power, 2013-10-17 00:15:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/166001

[commit-bot: I haz the power, 2013-10-17 01:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/166001

[commit-bot: I haz the power, 2013-10-17 04:03:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-10-17 04:39:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/24246016/166001

[commit-bot: I haz the power, 2013-10-17 08:56:08]

Change committed as 229081