Bug 171394 - Zero out PerformanceResourceTiming properties for cached cross-origin responses without Timing-Allow-Origin
Summary: Zero out PerformanceResourceTiming properties for cached cross-origin respons...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
: 171231 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-04-27 14:29 PDT by Joseph Pecoraro
Modified: 2017-05-01 15:11 PDT (History)
12 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (22.10 KB, patch)
2017-04-27 14:31 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.03 MB, application/zip)
2017-04-27 15:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (909.00 KB, application/zip)
2017-04-27 15:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.84 MB, application/zip)
2017-04-27 15:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.01 MB, application/zip)
2017-04-27 16:03 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (22.26 KB, patch)
2017-04-27 20:27 PDT, Joseph Pecoraro
youennf: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (779.37 KB, application/zip)
2017-04-27 21:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (858.45 KB, application/zip)
2017-04-27 21:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.66 MB, application/zip)
2017-04-27 21:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (23.64 MB, application/zip)
2017-04-27 22:04 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (23.17 KB, patch)
2017-04-28 20:24 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-04-27 14:29:02 PDT
Summary:
Zero out PerformanceResourceTiming properties for cached cross-origin responses without Timing-Allow-Origin

See: https://twitter.com/JFSIII/status/855505436971155456

Memory cache / disk cache loaded responses that were cross-origin and lacked Timing-Allow-Origin permission should have zero'd resource timing properties.

Steps to Reproduce:
1. Inspect http://www.bbc.com
2. Reload the page
3. Find a Cross-Origin Memory Cache resource in Inspector's Network Tab
  => Ensure Response Headers do not include Timing-Allow-Origin headers
4. Find the `performance.getEntries()` PerformanceResourceTiming entry for that resource on the page
  => Most properties should be zero'd, instead they have values (equivalent to fetchStart)

Spec:
https://w3c.github.io/resource-timing/#processing-model

> 7. If the last non-redirected fetch of the resource fails the timing allow check,
> the user agent must set redirectStart, redirectEnd, domainLookupStart, domainLookupEnd,
> connectStart, connectEnd, requestStart, responseStart and secureConnectionStart to
> zero and go to step 16.

Notes:
- We did the zero'ing for non-cache responses, but lets do it for cache responses as well.
- This step seems to be concerned with future steps as well as loops in the steps that jump back before this on redirects
Comment 1 Joseph Pecoraro 2017-04-27 14:31:17 PDT
Created attachment 308447 [details]
[PATCH] Proposed Fix
Comment 2 Build Bot 2017-04-27 15:21:29 PDT
Comment on attachment 308447 [details]
[PATCH] Proposed Fix

Attachment 308447 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3622293

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType-element.html
Comment 3 Build Bot 2017-04-27 15:21:31 PDT
Created attachment 308458 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-04-27 15:28:58 PDT
Comment on attachment 308447 [details]
[PATCH] Proposed Fix

Attachment 308447 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3622342

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType-element.html
Comment 5 Build Bot 2017-04-27 15:28:59 PDT
Created attachment 308461 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 6 Joseph Pecoraro 2017-04-27 15:51:23 PDT
Comment on attachment 308447 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
> +                const SecurityOrigin& origin = request.origin() ? *request.origin() : document()->securityOrigin();
> +                ResourceTiming resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->response(), origin);

The other test is failing because resource->response().url() is null. However the url we have here is correct and we can use that, I'm going to spend some time investigating why the resource's URL is null, which doesn't seem to make sense.
Comment 7 Build Bot 2017-04-27 15:57:34 PDT
Comment on attachment 308447 [details]
[PATCH] Proposed Fix

Attachment 308447 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3622403

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType-element.html
Comment 8 Build Bot 2017-04-27 15:57:35 PDT
Created attachment 308467 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-04-27 16:03:31 PDT
Comment on attachment 308447 [details]
[PATCH] Proposed Fix

Attachment 308447 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3622405

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-initiatorType-element.html
Comment 10 Build Bot 2017-04-27 16:03:33 PDT
Created attachment 308468 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 11 Joseph Pecoraro 2017-04-27 16:29:02 PDT
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
> > +                const SecurityOrigin& origin = request.origin() ? *request.origin() : document()->securityOrigin();
> > +                ResourceTiming resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->response(), origin);
> 
> The other test is failing because resource->response().url() is null.
> However the url we have here is correct and we can use that, I'm going to
> spend some time investigating why the resource's URL is null, which doesn't
> seem to make sense.

This case was getting reached while the Resource is loading from an earlier request. In that case I think we should bail here and let the load complete.
Comment 12 Joseph Pecoraro 2017-04-27 20:27:52 PDT
Created attachment 308496 [details]
[PATCH] Proposed Fix
Comment 13 Joseph Pecoraro 2017-04-27 20:46:03 PDT
> > The other test is failing because resource->response().url() is null.
> > However the url we have here is correct and we can use that, I'm going to
> > spend some time investigating why the resource's URL is null, which doesn't
> > seem to make sense.
> 
> This case was getting reached while the Resource is loading from an earlier
> request. In that case I think we should bail here and let the load complete.

In the Resource Timing processing model this is:

>   6. If the user agent is to reuse the data from another existing
>      or completed fetch initiated from the current document, abort
>      the remaining steps.

Key words here being existing or completed. Here it would be an existing fetch.
Comment 14 Build Bot 2017-04-27 21:35:10 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

Attachment 308496 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3624048

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
Comment 15 Build Bot 2017-04-27 21:35:11 PDT
Created attachment 308499 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2017-04-27 21:38:30 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

Attachment 308496 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3624062

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
Comment 17 Build Bot 2017-04-27 21:38:31 PDT
Created attachment 308500 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 18 Build Bot 2017-04-27 21:52:03 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

Attachment 308496 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3624072

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
Comment 19 Build Bot 2017-04-27 21:52:04 PDT
Created attachment 308502 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 20 Build Bot 2017-04-27 22:04:33 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

Attachment 308496 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3624084

New failing tests:
imported/w3c/web-platform-tests/resource-timing/rt-resources-per-worker.html
Comment 21 Build Bot 2017-04-27 22:04:36 PDT
Created attachment 308504 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 22 youenn fablet 2017-04-28 08:38:46 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

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

> LayoutTests/imported/w3c/ChangeLog:11
> +        requests include timing data or not.

What is the issue with WPT and the disk cache?
Some fetch API tests are doing disk cache related stuff.
Memory cache is not caching fetch/xhr but we should be able to exercise this on other request types.

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-requests.html:9
> +<script src="resources/rt-utilities.js?pipe=sub"></script>

Nit: rename rt-utilities.js as rt-utilities.sub.js to remove pipe=sub.

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-requests.html:23
> +    const path = "resource-timing/resources/rt-revalidation-response.py";

Is there any reason to use let above and const here?

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-requests.html:115
> +        assertDisallowedTimingData(entry);

Could call assertAlways from assertAllowedTimingData/assertDisallowedTimingData and have this as a oneliner.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
> +                ResourceTiming resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->response(), origin);

The resource has an origin and should know whether it is cross origin or not.
I wonder whether we should not punt on the CachedResource to know whether it should allow resource timing.
That would mean moving passesTimingAllowCheck to CachedResource if it is not used elsewhere.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:783
> +                    ASSERT(request.origin());

Seems fine but what is the exact purpose here?
If we were to catch this, wouldn't we want to check that directly when entering requestResource?
Comment 23 Joseph Pecoraro 2017-04-28 11:02:46 PDT
Comment on attachment 308496 [details]
[PATCH] Proposed Fix

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

The new failing test can be easily remedied by updating the test to filter out an individual resource that may be in the entry list but is unexpected. It doesn't seem like incorrect behavior, so thats all good.

>> LayoutTests/imported/w3c/ChangeLog:11
>> +        requests include timing data or not.
> 
> What is the issue with WPT and the disk cache?
> Some fetch API tests are doing disk cache related stuff.
> Memory cache is not caching fetch/xhr but we should be able to exercise this on other request types.

My problem was that I wasn't able to get a test that reliably hit the memory cache with the wpt server. I'll ask you about it, you may be able to point me to an example I missed!

>> LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-requests.html:9
>> +<script src="resources/rt-utilities.js?pipe=sub"></script>
> 
> Nit: rename rt-utilities.js as rt-utilities.sub.js to remove pipe=sub.

Nice! I'll fix all of these in a small follow-up.

>> LayoutTests/imported/w3c/web-platform-tests/resource-timing/rt-revalidate-requests.html:23
>> +    const path = "resource-timing/resources/rt-revalidation-response.py";
> 
> Is there any reason to use let above and const here?

No reason, just my personal style. I use const like we use `static const ...` in C++.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
>> +                ResourceTiming resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->response(), origin);
> 
> The resource has an origin and should know whether it is cross origin or not.
> I wonder whether we should not punt on the CachedResource to know whether it should allow resource timing.
> That would mean moving passesTimingAllowCheck to CachedResource if it is not used elsewhere.

We can use the `url` and `origin` here to perform the check, but we still don't have enough information for a resource that is still loading. Without the ResourceResponse we don't know if the response headers contained Timing-Allow-Origin or not. Also, the processing algorithm does say if there is an ongoing fetch that we are going to use, to abort. I think this is the exact situation it was thinking of.

>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:783
>> +                    ASSERT(request.origin());
> 
> Seems fine but what is the exact purpose here?
> If we were to catch this, wouldn't we want to check that directly when entering requestResource?

My understanding is that  CachedResourceRequest only has an origin in cases of RawRequests which may be initiated from workers. If it doesn't have an origin then we are inside the CachedResourceLoader for the Document and can use the document origin. So here I'm asserting that we didn't use the document security origin earlier (line 780) when it was a worker initiated load.
Comment 24 Joseph Pecoraro 2017-04-28 20:21:06 PDT
> My problem was that I wasn't able to get a test that reliably hit the memory
> cache with the wpt server. I'll ask you about it, you may be able to point
> me to an example I missed!

With Youenn's help I got a working test with Memory cache resources.

But we don't create multiple PerformanceResourceTiming entries for the same CachedResource, so the memory cache hits don't produce new entries and the test times out. I think that is acceptable behavior based on the spec. We can revisit this decision in another bug.
Comment 25 Joseph Pecoraro 2017-04-28 20:24:35 PDT
Created attachment 308657 [details]
[PATCH] Proposed Fix
Comment 26 youenn fablet 2017-04-29 11:45:30 PDT
> >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:781
> >> +                ResourceTiming resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->response(), origin);
> > 
> > The resource has an origin and should know whether it is cross origin or not.
> > I wonder whether we should not punt on the CachedResource to know whether it should allow resource timing.
> > That would mean moving passesTimingAllowCheck to CachedResource if it is not used elsewhere.
> 
> We can use the `url` and `origin` here to perform the check, but we still
> don't have enough information for a resource that is still loading. Without
> the ResourceResponse we don't know if the response headers contained
> Timing-Allow-Origin or not. Also, the processing algorithm does say if there
> is an ongoing fetch that we are going to use, to abort. I think this is the
> exact situation it was thinking of.

Since the resource is already cached, wouldn't it be better to write something like:
    auto resourceTiming = ResourceTiming::fromCache(url, request.initiatorName(), loadTiming, resource->resourceTimingPolicy());
That would remove the need for the line computing the origin based either on the request or the document.
Until a cross-origin CachedResource has received the response headers, resourceTimingPolicy would return NoTimingInformation.

Note also that the case where shouldUpdateCachedResourceWithCurrentRequest returns true is not handled.
This case happens when:
- the resource is in cache
- but it is requested from a different origin
- we can create the resource with the requested origin from the cached resource with the different origin 
Maybe worth doing that as a follow-up.

> 
> >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:783
> >> +                    ASSERT(request.origin());
> > 
> > Seems fine but what is the exact purpose here?
> > If we were to catch this, wouldn't we want to check that directly when entering requestResource?
> 
> My understanding is that  CachedResourceRequest only has an origin in cases
> of RawRequests which may be initiated from workers. If it doesn't have an
> origin then we are inside the CachedResourceLoader for the Document and can
> use the document origin. So here I'm asserting that we didn't use the
> document security origin earlier (line 780) when it was a worker initiated
> load.

Right, my thinking was that if it is useful here, it is probably useful above that.
So maybe this ASSERT should be done when entering this function and not just in the specific switch case of using a cached resource. Maybe in CachedResourceLoader::prepareFetch?
Comment 27 WebKit Commit Bot 2017-04-29 20:32:28 PDT
The commit-queue encountered the following flaky tests while processing attachment 308657 [details]:

editing/spelling/spellcheck-async.html bug 160571 (authors: g.czajkowski@samsung.com and mark.lam@apple.com)
The commit-queue is continuing to process your patch.
Comment 28 WebKit Commit Bot 2017-04-29 20:33:01 PDT
Comment on attachment 308657 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 308657

Committed r215981: <http://trac.webkit.org/changeset/215981>
Comment 29 Joseph Pecoraro 2017-05-01 15:11:30 PDT
*** Bug 171231 has been marked as a duplicate of this bug. ***