Bug 103867 – [Resource Timing] Record redirectStart & redirectEnd time and expose in resource timing entries

• Component: Platform
• Status: UNCONFIRMED

[pdeng6]

redirectStart & redirectEnd time are fields in resource timing entry,
they are needed to be recorded if there is redirect.

[pdeng6]

Created attachment 177246 [details]
Patch

[Nate Chapin]

Comment on attachment 177246 [details]
Patch

I don't really like leaving these values as member variables on ResourceLoader. I'd feel much better if they were wrapped up as a struct.

Also, how are you planning on accessing this data? Since there are no callers in this patch, it's hard for me to get a complete perspective.

[pdeng6]

Created attachment 180661 [details]
Patch

[pdeng6]

(In reply to comment #2)
> (From update of attachment 177246 [details])
> I don't really like leaving these values as member variables on ResourceLoader. I'd feel much better if they were wrapped up as a struct.
> 
> Also, how are you planning on accessing this data? Since there are no callers in this patch, it's hard for me to get a complete perspective.

I think SubResourceLoader is a good place to catch Redirect timing info, and it loads MainResource since recent refactoring(#49246).
Most resource timing information exposed from CachedResource, is it possible we store redirect in CachedResource?

[Nate Chapin]

Comment on attachment 180661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180661&action=review

> Source/WebCore/loader/cache/CachedResource.h:265
> +#if ENABLE(RESOURCE_TIMING)
> +    double redirectStartTime() const { return m_redirectStartTime; }
> +    double redirectEndTime() const { return m_redirectEndTime; }
> +    void addRedirect(double);
> +#endif
> +

How will this be accessed, and what is the correct behavior if the CachedResource is reused?

This patch may be ok, but I can't be sure without clearly understanding whether it's correct to reuse this data in the case of a cache hit.

[pdeng6]

(In reply to comment #5)
> (From update of attachment 180661 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180661&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.h:265
> > +#if ENABLE(RESOURCE_TIMING)
> > +    double redirectStartTime() const { return m_redirectStartTime; }
> > +    double redirectEndTime() const { return m_redirectEndTime; }
> > +    void addRedirect(double);
> > +#endif
> > +
> 
> How will this be accessed, and what is the correct behavior if the CachedResource is reused?
> 
> This patch may be ok, but I can't be sure without clearly understanding whether it's correct to reuse this data in the case of a cache hit.

Thanks, it will be accessed in CachedResourceLoader::loadDone(CachedResource* resource), same way as resource->resourceRequest(), resource->response() and resource->loadFinishTime(). see https://bugs.webkit.org/show_bug.cgi?id=103777, and landed patch https://bugs.webkit.org/attachment.cgi?id=177328&action=review.

Timing information will be reported only once when resource is loaded, and not reported when hit from memory cache, it is also similar to response() of CachedResource(http://www.w3.org/TR/2012/CR-resource-timing-20120522/#processing-model)

[pdeng6]

Created attachment 182559 [details]
Patch

[pdeng6]

I intent to track changes under loader by this bug at first. :)

Now patch 182559 is about record redirect timing and expose, redirectStart should be the same as initiation time(startTime) when there is redirect in same-origin or timing-allow-origin, otherwise 0. so only last redirect time is recorded in CachedResource.

@Nate, could you please help review the loader part?
@Tony and @James, could you please help review resource timing and test cases? I would like to know your comments before upstream the cases.
thanks.. Pan

[James Simonsen]

Comment on attachment 182559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182559&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:149
> +        m_resource->addRedirect();

We also need to check if this is a cross-origin redirect. If it is, then we must reset everything, including the start value stored in m_initiatorMap. See processing model step 19a.

> Source/WebCore/loader/cache/CachedResource.cpp:878
> +    m_lastRedirectReportTime = monotonicallyIncreasingTime();

This is supposed to be the equivalent to responseEnd, which is when the network stack receives the last byte. You should pull that value from the redirected response instead of using the current time.

> Source/WebCore/loader/cache/CachedResource.h:261
> +    double lastRedirectReportTime() const { return m_lastRedirectReportTime; }

Have you considered storing this in m_initiatorMap instead? It seems nice to keep all of the Resource Timing info together. Perhaps we should rename m_initiatorMap to better reflect that it's storing all of the Resource Timing info.

> Source/WebCore/page/PerformanceResourceTiming.cpp:120
> +    if (m_lastRedirectReportTime && !m_shouldReportDetails)

I'm going to go back on what I said in the last code review. Let's move passesTimingAllowOrigin() back to Performance.cpp and pass in m_shouldReportDetails as a bool. Then, we should just pass in the correct values for startTime and redirectEnd in the constructor. I think it'll be easier to read if all of that logic is in one place instead of sprinkled throughout these functions.

> Source/WebCore/page/PerformanceResourceTiming.cpp:134
> +    return (PerformanceEntry::startTime());

This shouldn't be inside (). Not sure why the old code was that way too.

> Source/WebCore/page/PerformanceResourceTiming.h:52
> +    static PassRefPtr<PerformanceResourceTiming> create(const AtomicString& initiatorType, const ResourceRequest& request, const ResourceResponse& response, double initiationTime, double lastRedirectReportTime, double finishTime, Document* requestingDocument)

It's really confusing that sometimes lastRedirectReportTime comes before finishTime in the args list and sometimes it's after it. Please pick one and be consistent in all the functions.

> LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_cross_origin_redirect_with_timing_allow_origin.html:25
> +                test_noless_than(entry.redirectEnd, entry.redirectStart, "redirectEnd should be no less than redirectStart in timing allowed cross-origin redirect!");

Isn't this the same as test_greater_or_equals? Why do we even have test_noless_than?

> LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_same_origin_redirect.html:15
> +            setup({explicit_done: true});

I don't think you need this if everything is tested during the load event. Same for the other tests.

[pdeng6]

Comment on attachment 182559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182559&action=review

>> Source/WebCore/loader/SubresourceLoader.cpp:149
>> +        m_resource->addRedirect();
> 
> We also need to check if this is a cross-origin redirect. If it is, then we must reset everything, including the start value stored in m_initiatorMap. See processing model step 19a.

thanks for your remind, it is really confused, let's sync understanding first of all.

Assume scenario, doc D req R1 -> R2 -> R3 -> R4. ( "->" means redirect, R4 is the final resource)
case 1: D, R1, R2, R3 and R4 are all from same origin.
case 2: although Ri and its adjacent are not the same origin, Ri allow its prior 'timing', or allowed timing by its posterior.
case 3: others

In case 1 and 2, redirectStart should be the initiate time of R1, redirectEnd should be last byte R3's redirect response. startTime should be initiate time of R1, fetchStart should be R4's request time.
In case 3, redirectStart should be 0, redirectEnd should be 0. startTime should be , startTime should be R4's request time, fetchStart should be R4's request time.
while for other fields, domainLookupStart, domainLookupEnd, connectStart, connectEnd, secureConnectionStart, requestStart and responseStart, should be reported if R4 and D same origin or R4 allow D timing. otherwise 0.

if above right, I would like to store redirect response chain(in CachedResource? I'm not sure how to store in m_initiatorMap of CachedResourceLoader) then feed it to  addResourceTiming(...). in addResourceTiming, pass in the correct values for startTime and redirectEnd in the constructor.

>> LayoutTests/http/tests/w3c/webperf/submission/Intel/resource-timing/test_resource_timing_same_origin_redirect.html:15
>> +            setup({explicit_done: true});
> 
> I don't think you need this if everything is tested during the load event. Same for the other tests.

seems it doesn't work well if I remove this and  the latter done().

[James Simonsen]

(In reply to comment #10)
> case 2: although Ri and its adjacent are not the same origin, Ri allow its prior 'timing', or allowed timing by its posterior.

We don't consider Timing-Allow-Origin during redirects. Its purpose is to allow a specific main document to view timing data for a specific subresource, not to share timing data across the redirect chain.

> if above right, I would like to store redirect response chain(in CachedResource? I'm not sure how to store in m_initiatorMap of CachedResourceLoader) then feed it to  addResourceTiming(...). in addResourceTiming, pass in the correct values for startTime and redirectEnd in the constructor.

I don't think you need all that. Just reset the initiationTime when there's a cross-origin redirect.

> seems it doesn't work well if I remove this and  the latter done().

Not sure then. A bunch of my tests don't use it. Not sure what the difference is.

[pdeng6]

(In reply to comment #11)
> (In reply to comment #10)
> > case 2: although Ri and its adjacent are not the same origin, Ri allow its prior 'timing', or allowed timing by its posterior.
> 
> We don't consider Timing-Allow-Origin during redirects. Its purpose is to allow a specific main document to view timing data for a specific subresource, not to share timing data across the redirect chain.
> 
> > if above right, I would like to store redirect response chain(in CachedResource? I'm not sure how to store in m_initiatorMap of CachedResourceLoader) then feed it to  addResourceTiming(...). in addResourceTiming, pass in the correct values for startTime and redirectEnd in the constructor.
> 
> I don't think you need all that. Just reset the initiationTime when there's a cross-origin redirect.

I don't quite understand this, 

in section4.3, startTime should be initiationTime(redirectStart) when if all the redirects or equivalent are from the same origin as the current document or the Timing-Allow-Origin HTTP response header rules are met.
now, as my understand, my case2 in comment #10 should be: 
case 2: although Ri and Doc D are not the same origin, response of Ri allow-timing by Doc D's origin.
then startTime should be initiationTime(first) or final resource fetch time(last), then why we reset the initiationTime? startTime in processing model never changed after 3.2.

what is more puzzled me is in processing model 3.19a, redirectStart is reset according to "current resource and the resource that is redirected to"

[James Simonsen]

(In reply to comment #12)
> in section4.3, startTime should be initiationTime(redirectStart) when if all the redirects or equivalent are from the same origin as the current document or the Timing-Allow-Origin HTTP response header rules are met.
> now, as my understand, my case2 in comment #10 should be: 
> case 2: although Ri and Doc D are not the same origin, response of Ri allow-timing by Doc D's origin.
> then startTime should be initiationTime(first) or final resource fetch time(last), then why we reset the initiationTime? startTime in processing model never changed after 3.2.
> 
> what is more puzzled me is in processing model 3.19a, redirectStart is reset according to "current resource and the resource that is redirected to"

I believe the processing model is correct. We should probably bring this up on the public-webperf mailing list though.

[Andreas Kling]

Comment on attachment 182559 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.