Bug 171231

Summary: Resource Timing doesn't require Timing-Allow-Origin header for cross origin requests
Product: WebKit Reporter: John Schulz <JFSIII>
Component: Page LoadingAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: achristensen, ap, beidson, joepeck, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: macOS 10.12   
See Also: https://bugs.webkit.org/show_bug.cgi?id=171394
Attachments:
Description Flags
[IMAGE] Web Inspector - bbc.com Resource Timing Entries
none
List of resources
none
CSS file 1 - no header - non-zero timing
none
CSS file 2 - no header - non-zero timing
none
JS file 1 - no header - zeroed timing
none
JS file 2 - no header - zeroed timing none

Description John Schulz 2017-04-24 10:26:18 PDT
According to https://w3c.github.io/resource-timing/#cross-origin-resources the following properties should be set to 0 for cross-origin requests:

redirectStart, redirectEnd, domainLookupStart, domainLookupEnd, connectStart, connectEnd, requestStart, responseStart, secureConnectionStart, transferSize, encodedBodySize, decodedBodySize

However, as shown in https://twitter.com/JFSIII/status/855505436971155456 & https://twitter.com/JFSIII/status/855531120800129024, this is not always the case.
Comment 1 Radar WebKit Bug Importer 2017-04-24 16:43:35 PDT
<rdar://problem/31800404>
Comment 2 Joseph Pecoraro 2017-04-27 14:32:51 PDT
I handled cache responses in bug 171394. I'll use this one to investigate why non-cached responses for cross-origin resources were somehow allowing these properties.
Comment 3 Joseph Pecoraro 2017-04-27 14:55:08 PDT
Created attachment 308453 [details]
[IMAGE] Web Inspector - bbc.com Resource Timing Entries

Hmm, I'm not able to reproduce this with trunk (and with my changes on bug 171394).

See attached screenshot of:

> console.table(performance.getEntriesByType("resource"), ["name", "fetchStart", "connectStart", "domainLookupStart"])

Performed on bbc.com. This was load without using the cache (Web Inspector shows all resources did not use the memory cache). In this case only a few resources had connect/dns information and each of those domains explicitly had Timing-Allow-Origin: "*". Each of the others are zero'd indicating they are cross-origin without TAO.
Comment 4 Joseph Pecoraro 2017-04-27 14:59:33 PDT
Ahh. So here is my theory. The same resource often shows up a bunch of times, so maybe when you ran:

js> Array.from(performance.getEntriesByType("resource")).filter(r => r.connectEnd).map(r => r.name);

You were seeing the cache responses in the resulting list (which will at the time had their connectStart/domainLookupStart falling back to fetchStart like a normal request). Those cache responses are being corrected, so now these false positives don't show up.

Let me try to verify that.
Comment 5 Joseph Pecoraro 2017-04-27 15:08:57 PDT
Hmm, that doesn't seem to fully explain things. On STP 28 I was able to reproduce the issue with a single cross-origin resource that claimed to not be from a cache yet had non-zero'd data. However, I'm unable to reproduce that on my trunk builds, so it seems this will be fixed by the change on 171394.
Comment 6 John Schulz 2017-04-27 15:41:17 PDT
Created attachment 308462 [details]
List of resources
Comment 7 John Schulz 2017-04-27 15:41:40 PDT
I ran the same test on cnn.com which I've never visited in STP 28.

Many uncached resources had connectStart values and did not have a TAO header. I'm attaching some screenshots showing what I saw.
Comment 8 John Schulz 2017-04-27 15:43:40 PDT
Created attachment 308463 [details]
CSS file 1 - no header - non-zero timing
Comment 9 John Schulz 2017-04-27 15:43:54 PDT
Created attachment 308464 [details]
CSS file 2 - no header - non-zero timing
Comment 10 John Schulz 2017-04-27 15:44:16 PDT
Created attachment 308465 [details]
JS file 1 - no header - zeroed timing
Comment 11 John Schulz 2017-04-27 15:44:31 PDT
Created attachment 308466 [details]
JS file 2 - no header - zeroed timing
Comment 12 Joseph Pecoraro 2017-04-27 16:53:17 PDT
Yep, as I'm discovering in bug 171394 that code path might have been possible for ongoing requests. So the more I look into it, I think that should fix this issue.
Comment 13 Joseph Pecoraro 2017-05-01 15:11:30 PDT
Yes, I'm going to mark this as a duplicate of 171394. Sorry for unnecessarily creating an extra bug, I had just originally thought there were two distinct issues!

I'm unable to reproduce after that change:
<http://trac.webkit.org/changeset/215981>

That change fixed two issues.

  (1) Loads from the cache for an earlier completed request.
      - The typical cache case. This will appear in Web Inspector as cached (from Memory or Disk cache)

  (2) Loads that would use the cache result for an "ongoing request".
      - This can happen internally within WebCore making multiple requests for a resource
      - This could eventually appear in Web Inspector as a Network request when the load actually completes

These examples are of case (2). In cases where there was an ongoing request we were incorrectly creating a PerformanceResourceTiming entry before the request completed (and therefore before we knew if there was a TAO or not). So, the information for these requests, with and without TAO should be more accurate.

--

Thanks for reporting this issue! We were reporting inaccurate / misleading in these cases.

You should be able to verify in a WebKit Nightly, or a Safari Technology Preview in a few weeks.

*** This bug has been marked as a duplicate of bug 171394 ***
Comment 14 John Schulz 2017-05-01 20:32:36 PDT
Checked out in WebKit Nightly and things look good to me.