Bug 171231 - Resource Timing doesn't require Timing-Allow-Origin header for cross origin requests
Summary: Resource Timing doesn't require Timing-Allow-Origin header for cross origin r...
Status: RESOLVED DUPLICATE of bug 171394
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-24 10:26 PDT by John Schulz
Modified: 2017-05-01 20:32 PDT (History)
6 users (show)

See Also:


Attachments
[IMAGE] Web Inspector - bbc.com Resource Timing Entries (1017.19 KB, image/png)
2017-04-27 14:55 PDT, Joseph Pecoraro
no flags Details
List of resources (302.02 KB, image/png)
2017-04-27 15:41 PDT, John Schulz
no flags Details
CSS file 1 - no header - non-zero timing (159.34 KB, image/png)
2017-04-27 15:43 PDT, John Schulz
no flags Details
CSS file 2 - no header - non-zero timing (158.07 KB, image/png)
2017-04-27 15:43 PDT, John Schulz
no flags Details
JS file 1 - no header - zeroed timing (169.86 KB, image/png)
2017-04-27 15:44 PDT, John Schulz
no flags Details
JS file 2 - no header - zeroed timing (165.07 KB, image/png)
2017-04-27 15:44 PDT, John Schulz
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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.