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
Created attachment 308447 [details] [PATCH] Proposed Fix
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
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 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
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 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 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
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 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
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
> > 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.
Created attachment 308496 [details] [PATCH] Proposed Fix
> > 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 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
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 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
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 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
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 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
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 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 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.
> 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.
Created attachment 308657 [details] [PATCH] Proposed Fix
> >> 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?
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 on attachment 308657 [details] [PATCH] Proposed Fix Clearing flags on attachment: 308657 Committed r215981: <http://trac.webkit.org/changeset/215981>
*** Bug 171231 has been marked as a duplicate of this bug. ***