Expose receive header end time for URLRequestRedirectJob

Expose receive header end time for URLRequestRedirectJob

Contributed by pan.deng@intel.com

BUG=252621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208909

[Pan, 2013-06-21 08:49:47]

This patch describes the general idea of expose LoadTimingInfo for HSTS hit
redirect, make sense?
Related bug http://code.google.com/p/chromium/issues/detail?id=252066#c3

thanks!
Pan

[mmenke, 2013-06-21 17:53:36]

On 2013/06/21 08:49:47, Pan wrote:
> This patch describes the general idea of expose LoadTimingInfo for HSTS hit
> redirect, make sense?
> Related bug http://code.google.com/p/chromium/issues/detail?id=252066#c3
> 
> thanks!
> Pan

If this is worth doing, we should have a unit test for this. 
url_request_unittest.cc already has a couple STS/HSTS tests.  My suggestion is
to copy or modify one of those, and then use TestLoadTimingCacheHitNoNetwork
(Which perhaps should be renamed) on the TestNetworkDelegate's cached value of
the load timing for the redirect (Not to be confused with the TestDelegate -
one's a URLRequestDelegate, the other is a NetworkDelegate).

[Pan, 2013-06-24 13:29:31]

On 2013/06/21 17:53:36, mmenke wrote:
> On 2013/06/21 08:49:47, Pan wrote:
> > This patch describes the general idea of expose LoadTimingInfo for HSTS hit
> > redirect, make sense?
> > Related bug http://code.google.com/p/chromium/issues/detail?id=252066#c3
> > 
> > thanks!
> > Pan
> 
> If this is worth doing, we should have a unit test for this. 
> url_request_unittest.cc already has a couple STS/HSTS tests.  My suggestion is
> to copy or modify one of those, and then use TestLoadTimingCacheHitNoNetwork
> (Which perhaps should be renamed) on the TestNetworkDelegate's cached value of
> the load timing for the redirect (Not to be confused with the TestDelegate -
> one's a URLRequestDelegate, the other is a NetworkDelegate).

@mmenke
thanks, unit test is added, please review :)

Pan

[mmenke, 2013-06-24 13:58:19]

Thought I made this comment last time, but looks like I did not.

https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
File net/url_request/url_request_redirect_job.cc (right):

https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
net/url_request/url_request_redirect_job.cc:46:
load_timing_info->receive_headers_end = receive_headers_end_;
Hmm...  Would it be more consistent with the spec to also set send start and
send stop times to be this time as well?  Seems like that would more closely
mirror cache behavior, though perhaps they should technically be when we
started/stopped checking the HSTS list.

[Pan, 2013-06-24 23:36:56]

On 2013/06/24 13:58:19, mmenke wrote:
> Thought I made this comment last time, but looks like I did not.
> 
>
https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
> File net/url_request/url_request_redirect_job.cc (right):
> 
>
https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
> net/url_request/url_request_redirect_job.cc:46:
> load_timing_info->receive_headers_end = receive_headers_end_;
> Hmm...  Would it be more consistent with the spec to also set send start and
> send stop times to be this time as well?  Seems like that would more closely
> mirror cache behavior, though perhaps they should technically be when we
> started/stopped checking the HSTS list.

thanks, actually, it is requestStart attribute who may use send_start.
spec says "requestStart attribute return the time immediately before the user
agent starts requesting the resource from the server, or from relevant
application caches or from local resources." 
In network case, requestStart will be actualy send_start,
In the cache case, it falls back to "request_start",
As HSTS is chrome specific feature, personally, I prefer use the request_start
rather than make requestStart and responseStart same value,(we will handle
load_timing is filled but send_start is blank is blank case in blink although
there is layout case cover hsts)
@simonjam, how do you think of it?

thanks
Pan

[mmenke, 2013-06-24 23:48:05]

On 2013/06/24 23:36:56, Pan wrote:
> On 2013/06/24 13:58:19, mmenke wrote:
> > Thought I made this comment last time, but looks like I did not.
> > 
> >
>
https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
> > File net/url_request/url_request_redirect_job.cc (right):
> > 
> >
>
https://codereview.chromium.org/16901013/diff/9001/net/url_request/url_reques...
> > net/url_request/url_request_redirect_job.cc:46:
> > load_timing_info->receive_headers_end = receive_headers_end_;
> > Hmm...  Would it be more consistent with the spec to also set send start and
> > send stop times to be this time as well?  Seems like that would more closely
> > mirror cache behavior, though perhaps they should technically be when we
> > started/stopped checking the HSTS list.
> 
> thanks, actually, it is requestStart attribute who may use send_start.
> spec says "requestStart attribute return the time immediately before the user
> agent starts requesting the resource from the server, or from relevant
> application caches or from local resources." 
> In network case, requestStart will be actualy send_start,
> In the cache case, it falls back to "request_start",
> As HSTS is chrome specific feature, personally, I prefer use the request_start
> rather than make requestStart and responseStart same value,(we will handle
> load_timing is filled but send_start is blank is blank case in blink although
> there is layout case cover hsts)
> @simonjam, how do you think of it?
> 
> thanks
> Pan

Delegates (Like extensions) can actually block requests after request_start, I
believe this can happen in the HSTs case as well.  Nothing can block in between
when we identify the request as an HSTS request, and when we call
StartAsync....I think.  I could certainly be wrong - it's been a while since I
read through the relevant code.

This isn't exactly the most important issue, but since it will probably never be
revisited for quite a while, worth thinking about it now.

[James Simonsen, 2013-06-25 00:22:23]

On 2013/06/24 23:48:05, mmenke wrote:
> On 2013/06/24 23:36:56, Pan wrote:
> > @simonjam, how do you think of it?

The Navigation Timing processing model illustration has gaps for situations like
this. The browser is allowed to do other things in between, but the blocks
themselves should be exactly as described in the spec. My interpretation is that
HSTS shouldn't be included in request/response time.

[Pan, 2013-06-25 01:58:15]

@mmenke and @simonjam

Blink won't know it is a HSTS redirect or not, just handle it as a normal
redirect,
For Resource Timing:
I expected HSTS redirect time included in redirectStart and redirectEnd,
although they may be blank as the 'redirect' may hit cross origin.
For Navigation timing,
HSTS included in inner redirect time, but they will be forced to blank as not
same origin.
so any way, HSTS will be included in redirectStart/redirectEnd as my understand,
so no cares other internal block. (I assumed URLRequestRedirectedJob won't hurt
the real on wire redirect.)
is that true? :)

thanks
Pan

[mmenke, 2013-06-25 14:11:54]

On 2013/06/25 01:58:15, Pan wrote:
> @mmenke and @simonjam
> 
> Blink won't know it is a HSTS redirect or not, just handle it as a normal
> redirect,
> For Resource Timing:
> I expected HSTS redirect time included in redirectStart and redirectEnd,
> although they may be blank as the 'redirect' may hit cross origin.
> For Navigation timing,
> HSTS included in inner redirect time, but they will be forced to blank as not
> same origin.
> so any way, HSTS will be included in redirectStart/redirectEnd as my
understand,
> so no cares other internal block. (I assumed URLRequestRedirectedJob won't
hurt
> the real on wire redirect.)
> is that true? :)
> 
> thanks
> Pan

The other times do appear in HAR files, I believe.

[James Simonsen, 2013-06-25 22:35:06]

On 2013/06/25 01:58:15, Pan wrote:
> @mmenke and @simonjam
> 
> Blink won't know it is a HSTS redirect or not, just handle it as a normal
> redirect,
> For Resource Timing:
> I expected HSTS redirect time included in redirectStart and redirectEnd,
> although they may be blank as the 'redirect' may hit cross origin.
> For Navigation timing,
> HSTS included in inner redirect time, but they will be forced to blank as not
> same origin.
> so any way, HSTS will be included in redirectStart/redirectEnd as my
understand,
> so no cares other internal block. (I assumed URLRequestRedirectedJob won't
hurt
> the real on wire redirect.)
> is that true? :)
> 
> thanks
> Pan

Yeah, this discussion is probably moot for Navigation and Resource Timing,
because they're just bundled into an opaque redirect block.

[Pan, 2013-06-25 22:43:10]

thanks,
then, let me look into the HAR. :)

Pan

[Pan, 2013-06-26 08:59:03]

On 2013/06/25 14:11:54, mmenke wrote:
> On 2013/06/25 01:58:15, Pan wrote:
> > @mmenke and @simonjam
> > 
> > Blink won't know it is a HSTS redirect or not, just handle it as a normal
> > redirect,
> > For Resource Timing:
> > I expected HSTS redirect time included in redirectStart and redirectEnd,
> > although they may be blank as the 'redirect' may hit cross origin.
> > For Navigation timing,
> > HSTS included in inner redirect time, but they will be forced to blank as
not
> > same origin.
> > so any way, HSTS will be included in redirectStart/redirectEnd as my
> understand,
> > so no cares other internal block. (I assumed URLRequestRedirectedJob won't
> hurt
> > the real on wire redirect.)
> > is that true? :)
> > 
> > thanks
> > Pan
> 
> The other times do appear in HAR files, I believe.

Right, although Navigation timing and Resource Timing only need the
receive_header_end, it may affect HAR and DevTools Network Panel.
please review. :)

thanks!
Pan

[mmenke, 2013-06-26 19:55:40]

https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:193:
EXPECT_FALSE(load_timing_info.receive_headers_end.is_null());
This is basically equivalent to TestLoadTimingCacheHitNoNetwork, except the
EXPECT_EQ instead of EXPECT_LE for send/receive headers times.

Suggest just calling into TestLoadTimingCacheHitNoNetwork, with a comment along
the lines that HSTS redirects are similar to cache hits, so populate the same
times.

If you disagree, should at least match the order that we test the values in the
two functions.  And throw in an:

EXPECT_LE(load_timing_info.request_start, load_timing_info.send_start);

[Pan, 2013-06-27 01:18:09]

On 2013/06/26 19:55:40, mmenke wrote:
>
https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
> File net/url_request/url_request_unittest.cc (right):
> 
>
https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
> net/url_request/url_request_unittest.cc:193:
> EXPECT_FALSE(load_timing_info.receive_headers_end.is_null());
> This is basically equivalent to TestLoadTimingCacheHitNoNetwork, except the
> EXPECT_EQ instead of EXPECT_LE for send/receive headers times.
> 
> Suggest just calling into TestLoadTimingCacheHitNoNetwork, with a comment
along
> the lines that HSTS redirects are similar to cache hits, so populate the same
> times.
> 
> If you disagree, should at least match the order that we test the values in
the
> two functions.  And throw in an:
> 
> EXPECT_LE(load_timing_info.request_start, load_timing_info.send_start);

agree, of course :)
thanks!

Pan

[mmenke, 2013-06-27 01:24:24]

On 2013/06/27 01:18:09, Pan wrote:
> On 2013/06/26 19:55:40, mmenke wrote:
> >
>
https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
> > File net/url_request/url_request_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/16901013/diff/22001/net/url_request/url_reque...
> > net/url_request/url_request_unittest.cc:193:
> > EXPECT_FALSE(load_timing_info.receive_headers_end.is_null());
> > This is basically equivalent to TestLoadTimingCacheHitNoNetwork, except the
> > EXPECT_EQ instead of EXPECT_LE for send/receive headers times.
> > 
> > Suggest just calling into TestLoadTimingCacheHitNoNetwork, with a comment
> along
> > the lines that HSTS redirects are similar to cache hits, so populate the
same
> > times.
> > 
> > If you disagree, should at least match the order that we test the values in
> the
> > two functions.  And throw in an:
> > 
> > EXPECT_LE(load_timing_info.request_start, load_timing_info.send_start);
> 
> agree, of course :)
> thanks!
> 
> Pan

LGTM, thanks!

[commit-bot: I haz the power, 2013-06-27 01:25:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/16901013/32001

[commit-bot: I haz the power, 2013-06-27 14:16:41]

Change committed as 208909