Set network_accessed earlier, when network transaction creates stream.

Set network_accessed earlier, when network transaction creates stream.

HttpResponseInfo contains a "network_accessed" flag that is currently
set when HttpNetworkTransaction finishes sending the request. This is
not entirely accurate; it ends up false for requests that don't send a
request but do touch the network in other ways (e.g. DNS resolution
fails, or TCP connection is terminated before we finish sending the
request).

This flag is used only by Domain Reliability for checking whether a
request touched the network, and by ServiceWorker for histograms.
horo says ServiceWorker won't be affected, and I would prefer these
semantics for Domain Reliability's use, so I'm changing it.

Note that this also changes several instances of GetResponseInfo
to stop returning NULL.

See also for context:

https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/Wtn65jGyot0

BUG=480565
Committed: https://crrev.com/c0c828496a25acd34b85985b412799933a36f1ff
Cr-Commit-Position: refs/heads/master@{#330011}

[Deprecated (see juliatuttle), 2015-04-24 02:48:23]

CCing horo; haven't chosen a reviewer yet.

[Deprecated (see juliatuttle), 2015-04-24 02:56:15]

Patchset #2 (id:20001) has been deleted

[davidben, 2015-04-24 16:36:40]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2015-04-24 16:36:41]

How hard would it be to make a test for this?

[Deprecated (see juliatuttle), 2015-04-24 17:34:02]

On 2015/04/24 16:36:41, David Benjamin (OOO sick) wrote:
> How hard would it be to make a test for this?

It looked easy, but then I realized this CL might not actually do what I want,
since the response info is only passed up in certain circumstances. I'm playing
with it right now to see what options I have.

[davidben, 2015-04-24 17:35:57]

On 2015/04/24 17:34:02, ttuttle wrote:
> On 2015/04/24 16:36:41, David Benjamin (OOO sick) wrote:
> > How hard would it be to make a test for this?
> 
> It looked easy, but then I realized this CL might not actually do what I want,
> since the response info is only passed up in certain circumstances. I'm
playing
> with it right now to see what options I have.

Is this the cache and transient properties thing again? I think we're in need of
someone to drive that, and it sounds pretty relevant to you. ;-)

[Deprecated (see juliatuttle), 2015-04-24 17:49:11]

On 2015/04/24 17:34:02, ttuttle wrote:
> On 2015/04/24 16:36:41, David Benjamin (OOO sick) wrote:
> > How hard would it be to make a test for this?
> 
> It looked easy, but then I realized this CL might not actually do what I want,
> since the response info is only passed up in certain circumstances. I'm
playing
> with it right now to see what options I have.

Yeah, this doesn't work.

I'm thinking about adding an OnNetworkAccessedCallback to HttpTransaction, so
the network transaction can call back the cache transaction which can call back
the URLRequestHttpJob which can set network_accessed in the transient
HttpResponseInfo in the URLRequest.

How does that sound?

[Deprecated (see juliatuttle), 2015-04-24 17:50:33]

On 2015/04/24 17:35:57, David Benjamin (OOO sick) wrote:
> On 2015/04/24 17:34:02, ttuttle wrote:
> > On 2015/04/24 16:36:41, David Benjamin (OOO sick) wrote:
> > > How hard would it be to make a test for this?
> > 
> > It looked easy, but then I realized this CL might not actually do what I
want,
> > since the response info is only passed up in certain circumstances. I'm
> playing
> > with it right now to see what options I have.
> 
> Is this the cache and transient properties thing again? I think we're in need
of
> someone to drive that, and it sounds pretty relevant to you. ;-)

Something like that.

I feel like maybe we should split HttpResponseInfo into a transient and a
persistent class. network_accessed would be transient.

If you'd like to file a bug, you could assign it to me. I'd happily create it
and start moving over the small things.

[Deprecated (see juliatuttle), 2015-04-27 18:42:20]

...okay, it looks like there is a very simple way to set the right bit at the
right time without completely refactoring how HttpResponseInfo is handled.

PTAL, davidben.

[davidben, 2015-05-04 17:57:19]

Unit test?

https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
net/url_request/url_request.cc:771: response_info_.network_accessed = true;
Why does this get set in two places and at two completely different layers? The
job overrides response_info_ at various points. Having this NotifyFoo function
go and overwrite whatever happens to be here right now seems clearly wrong.

If the problem is revalidations, the logic should be in HttpCache::Transaction.

[Deprecated (see juliatuttle), 2015-05-08 18:27:25]

On 2015/05/04 17:57:19, David Benjamin wrote:
> Unit test?
> 
>
https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
> File net/url_request/url_request.cc (right):
> 
>
https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
> net/url_request/url_request.cc:771: response_info_.network_accessed = true;
> Why does this get set in two places and at two completely different layers?
The
> job overrides response_info_ at various points. Having this NotifyFoo function
> go and overwrite whatever happens to be here right now seems clearly wrong.
> 
> If the problem is revalidations, the logic should be in
HttpCache::Transaction.

It's not just revalidations -- it's set the first time to cover the time before
a response is actually received, since the erroneous-lack-of-a-response still
accessed the network, and the second time so that when the
actual-response-or-part-thereof is "committed" by copying the response info
over, it stays true.

[Deprecated (see juliatuttle), 2015-05-08 22:03:23]

PTAL, davidben.

[davidben, 2015-05-11 19:01:56]

On 2015/05/08 18:27:25, ttuttle wrote:
> On 2015/05/04 17:57:19, David Benjamin wrote:
> > Unit test?
> > 
> >
>
https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
> > File net/url_request/url_request.cc (right):
> > 
> >
>
https://codereview.chromium.org/1072423005/diff/60001/net/url_request/url_req...
> > net/url_request/url_request.cc:771: response_info_.network_accessed = true;
> > Why does this get set in two places and at two completely different layers?
> The
> > job overrides response_info_ at various points. Having this NotifyFoo
function
> > go and overwrite whatever happens to be here right now seems clearly wrong.
> > 
> > If the problem is revalidations, the logic should be in
> HttpCache::Transaction.
> 
> It's not just revalidations -- it's set the first time to cover the time
before
> a response is actually received, since the erroneous-lack-of-a-response still
> accessed the network, and the second time so that when the
> actual-response-or-part-thereof is "committed" by copying the response info
> over, it stays true.

This still seems a layering violation. The network request comes from the
HttpNetworkTransaction. That is, in turn, owned by the HttpCache::Transaction,
which is, in turn, owned by the URLRequestJob, owned by the URLRequest.
Information should bubble up through those layers and not bypass things.

The copy of the response_ in the URLRequest gets overwritten by the one in the
transaction. In fact, if you look at URLRequestJob::NotifyStartError, we still
pull the response info from the job (and thus the transaction) if Start fails.
That's where your network_accessed bit should come from.

https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

The problem is that the URLRequestHttpJob has a null response_info_ at this
point:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

That too is not for lack of trying by URLRequestHttpJob. It queries the
transaction on error:
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

The root problem is that HttpCache::Transaction doesn't bubble the information
up correctly, which makes sense because its the one that's multiplexing between
0, 1, or N network transactions. (Also makes sense because, per that transient
properties document, the root of all our problems is that HttpCache::Transaction
isn't filling out the response info sensibly.)

It seems to only pull from its response and just remembers to fill in the
ssl_info on SSL errors, which is how that information gets plumbed up. You
should probably be bubbling network_accessed up through there similarly.
network_acccessed is a transient property and thus needs separate consideration.
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...

I don't believe your tests capture the revalidation case, do they? But if we
have a cacheable max-age=0 resource that we fetch, and successfully revalidate,
then BOTH was_cached and network_accessed should be true. That too is transient
property shuffling that should happen in HttpCache::Transaction.

[Deprecated (see juliatuttle), 2015-05-14 00:07:34]

PTAL, davidben.

[davidben, 2015-05-14 22:28:14]

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1083: if (response)
This can't ever be NULL, can it? That one testing transaction aside (see
http_network_transaction.cc)

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1085: 
Let's move this after line 1094. In the non-error case, we're going to handle
network_accessed already anyway and possibly need more complex logic. Also I
think this codepath kicks in for range requests and the like which might do
weirder things.

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_network_...
net/http/http_network_transaction.cc:398: return &response_;
With this, except for FailingHttpTransaction, GetResponseInfo never returns
NULL. How about fixing that one and changing the documentation of
HttpTransaction to not return NULL? Also document that it may only be called
after Start() completes.

Also mention in the commit message that we're changing the meaning of
GetResponseInfo slightly.

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9197: 
Revalidate case? Perhaps do that test at the HttpCache::Transaction level where
you can more easily mock out the server's response.

[Deprecated (see juliatuttle), 2015-05-14 23:52:10]

PTAL.

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1083: if (response)
On 2015/05/14 22:28:14, David Benjamin wrote:
> This can't ever be NULL, can it? That one testing transaction aside (see
> http_network_transaction.cc)

Not anymore, no.

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:1085: 
On 2015/05/14 22:28:14, David Benjamin wrote:
> Let's move this after line 1094. In the non-error case, we're going to handle
> network_accessed already anyway and possibly need more complex logic. Also I
> think this codepath kicks in for range requests and the like which might do
> weirder things.

Done.

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/http/http_network_...
net/http/http_network_transaction.cc:398: return &response_;
On 2015/05/14 22:28:14, David Benjamin wrote:
> With this, except for FailingHttpTransaction, GetResponseInfo never returns
> NULL. How about fixing that one and changing the documentation of
> HttpTransaction to not return NULL? Also document that it may only be called
> after Start() completes.
> 
> Also mention in the commit message that we're changing the meaning of
> GetResponseInfo slightly.

Done.

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9197: 
On 2015/05/14 22:28:14, David Benjamin wrote:
> Revalidate case? Perhaps do that test at the HttpCache::Transaction level
where
> you can more easily mock out the server's response.

Done.

[davidben, 2015-05-14 23:55:04]

lgtm

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1072423005/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:9197: 
On 2015/05/14 23:52:10, ttuttle wrote:
> On 2015/05/14 22:28:14, David Benjamin wrote:
> > Revalidate case? Perhaps do that test at the HttpCache::Transaction level
> where
> > you can more easily mock out the server's response.
> 
> Done.

[For posterity, we noticed out-of-band that http_cache_unittest.cc already
tested this.]

[Deprecated (see juliatuttle), 2015-05-15 00:34:12]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-05-15 00:34:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1072423005/#ps180001
(title: "Don't break build on iOS")

[commit-bot: I haz the power, 2015-05-15 00:35:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1072423005/180001

[commit-bot: I haz the power, 2015-05-15 01:26:10]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2015-05-15 01:27:19]

Patchset 9 (id:??) landed as
https://crrev.com/c0c828496a25acd34b85985b412799933a36f1ff
Cr-Commit-Position: refs/heads/master@{#330011}