CachedResourceLoader is not taking into account fetch options to use or not cached resources

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

SubresourceLoader is doing redirection checks in CORS mode but not in No-Cors mode.
If a resource is fetched in no-cors mode, the cached resource may be reused in cors mode without revalidation each redirection.

Also, reusing a cached resource for another load with different fetch options has impact on the CachedResource.
Its response tainting and its origin clean in particular may not be correct.

[youenn fablet]

If we plan to further align with the specs, we should probably forbid reusing resources with different fetch options.
Or we should have a way to easily clone one reusable resource into a new one with correctly set states.

Note that this does not apply to XHR/fetch API, since resources are not allowed to be reused in that case.

[youenn fablet]

Created attachment 287404 [details]
Patch

[youenn fablet]

This patch is somehow going against https://trac.webkit.org/changeset/201805 since we would reload whenever the origin changes.

I am not sure how important for performances it is.
But it would be much simpler/less error-prone to have simpler rules for the cache.

If performance is an issue and reloading is not a good enough idea, we might want to try separating resource specific data from the other cached resource data (the request, the options, the states), so that we can efficiently create a new CachedResource from one in the cache.

Any insight?

[Build Bot]

Comment on attachment 287404 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html
http/tests/security/cross-origin-cached-resource-parallel.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html
http/tests/loading/cross-origin-XHR-willLoadRequest.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287412 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287404 [details]
Patch

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

New failing tests:
http/tests/security/cross-origin-cached-resource-parallel.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287414 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287404 [details]
Patch

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

New failing tests:
http/tests/security/cross-origin-cached-resource-parallel.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html
http/tests/loading/cross-origin-XHR-willLoadRequest.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287417 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287404 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html
http/tests/security/cross-origin-cached-resource-parallel.html
http/tests/navigation/post-frames-goback1.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287418 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 287518 [details]
Rebasing failing test expectations

[Build Bot]

Comment on attachment 287518 [details]
Rebasing failing test expectations

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html
http/tests/navigation/post-frames-goback1.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.html

[Build Bot]

Created attachment 287520 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 287616 [details]
Cloning CachedImage when needed

[Build Bot]

Comment on attachment 287616 [details]
Cloning CachedImage when needed

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287620 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287616 [details]
Cloning CachedImage when needed

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287621 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287616 [details]
Cloning CachedImage when needed

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287623 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287616 [details]
Cloning CachedImage when needed

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287625 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 287627 [details]
Fixing test

[Build Bot]

Comment on attachment 287627 [details]
Fixing test

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287638 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287627 [details]
Fixing test

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.html

[Build Bot]

Created attachment 287641 [details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[youenn fablet]

Created attachment 287749 [details]
WIP: Adding new expectations for WK2 failing tests

[youenn fablet]

Created attachment 287758 [details]
Adding CachedImage cloning

[youenn fablet]

Created attachment 287759 [details]
Adding CachedImage cloning

[Build Bot]

Comment on attachment 287759 [details]
Adding CachedImage cloning

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

New failing tests:
http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html

[Build Bot]

Created attachment 287810 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 287759 [details]
Adding CachedImage cloning

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

> Source/WebCore/loader/SubresourceLoader.cpp:421
> +    cancel(ResourceError(String(), 0, request().url(), errorMessage, ResourceError::Type::AccessControl));

I think it’s confusing factoring to have this function actually cancel the load of the resource; the name of the function is "check" and it returns a boolean. Is this following some existing pattern in this class?

If the function’s job is to log an error and cancel if there is something wrong, then I think "check" is not a sufficiently clear name for it. A function named check seems like it would do a check and return the results of that check to the caller, not do a check and then, if the check fails, take action causing various side effects.

> Source/WebCore/loader/SubresourceLoader.cpp:425
> +    return false;
> +
> +}

Extra blank line here.

> Source/WebCore/loader/cache/CachedImage.cpp:124
> +    if (m_image && m_image->isSVGImage())

Should be able to write this instead:

    if (is<SVGImage>(m_image))

> Source/WebCore/loader/cache/CachedImage.cpp:125
> +        m_svgImageCache = std::make_unique<SVGImageCache>(static_cast<SVGImage*>(m_image.get()));

This should use &downcast<SVGImage>(*m_image), and not static_cast.

> Source/WebCore/loader/cache/CachedResource.cpp:244
> +void CachedResource::computeOrigin(CachedResourceLoader& cachedResourceLoader)

Given the context, I think we can just call this "loader". After all, this is a CachedResource member function, and in that context loader would seem to mean a CachedResourceLoader.

> Source/WebCore/loader/cache/CachedResource.cpp:246
> +    if (type() != MainResource) {

Now that this is in a function, I suggest using an early return rather than putting the entire function’s logic nested inside an if.

> Source/WebCore/loader/cache/CachedResource.cpp:250
> +            m_origin = cachedResourceLoader.document()->securityOrigin();

What guarantee do we have that loader.document() is not null? Why does it return a pointer if it can never be null?

> Source/WebCore/loader/cache/CachedResource.cpp:254
> +        if (!(m_resourceRequest.url().protocolIsData() && m_options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set)  && !m_origin->canRequest(m_resourceRequest.url()))
> +            setCrossOrigin();

Code you moved here and it’s not new, but: There’s an extra space here before "&&". And the special case for the data protocol is really ugly. Would be nice to find a better way to factor that.

> Source/WebCore/loader/cache/CachedResource.h:213
> +    void didReceiveResponse(const ResourceResponse&, Document*);

Why a Document* rather than Document&? Can it be null?

> Source/WebCore/loader/cache/CachedResource.h:311
> +    virtual void cloneData(const CachedResource&) { }

What does it mean to "clone" data? I am not sure this is a good function name.

[youenn fablet]

Created attachment 287945 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 287945 [details]
Patch for landing

Clearing flags on attachment: 287945

Committed r205450: <http://trac.webkit.org/changeset/205450>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Thanks for the review.

(In reply to comment #34)
> Comment on attachment 287759 [details]
> Adding CachedImage cloning
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287759&action=review
> 
> > Source/WebCore/loader/SubresourceLoader.cpp:421
> > +    cancel(ResourceError(String(), 0, request().url(), errorMessage, ResourceError::Type::AccessControl));
> 
> I think it’s confusing factoring to have this function actually cancel the
> load of the resource; the name of the function is "check" and it returns a
> boolean. Is this following some existing pattern in this class?

Right, I updated the patch to stick to the current code organisation.
I'll see whether we can make some refactoring in the future.


> > Source/WebCore/loader/SubresourceLoader.cpp:425
> > +    return false;
> > +
> > +}
> 
> Extra blank line here.

OK

> > Source/WebCore/loader/cache/CachedImage.cpp:124
> > +    if (m_image && m_image->isSVGImage())
> 
> Should be able to write this instead:

OK

>     if (is<SVGImage>(m_image))
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:125
> > +        m_svgImageCache = std::make_unique<SVGImageCache>(static_cast<SVGImage*>(m_image.get()));
> 
> This should use &downcast<SVGImage>(*m_image), and not static_cast.

OK

> > Source/WebCore/loader/cache/CachedResource.cpp:244
> > +void CachedResource::computeOrigin(CachedResourceLoader& cachedResourceLoader)
> 
> Given the context, I think we can just call this "loader". After all, this
> is a CachedResource member function, and in that context loader would seem
> to mean a CachedResourceLoader.

OK

> > Source/WebCore/loader/cache/CachedResource.cpp:246
> > +    if (type() != MainResource) {
> 
> Now that this is in a function, I suggest using an early return rather than
> putting the entire function’s logic nested inside an if.

OK

> > Source/WebCore/loader/cache/CachedResource.cpp:250
> > +            m_origin = cachedResourceLoader.document()->securityOrigin();
> 
> What guarantee do we have that loader.document() is not null? Why does it
> return a pointer if it can never be null?

Since the resource is not a main resource, there should be a document.
I added an ASSERT

> > Source/WebCore/loader/cache/CachedResource.cpp:254
> > +        if (!(m_resourceRequest.url().protocolIsData() && m_options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set)  && !m_origin->canRequest(m_resourceRequest.url()))
> > +            setCrossOrigin();
> 
> Code you moved here and it’s not new, but: There’s an extra space here
> before "&&". And the special case for the data protocol is really ugly.
> Would be nice to find a better way to factor that.

OK
As for the refactoring, I agree.
This check should be done early in the CachedResourceLoader loading code.
Further refactoring should help here.


> > Source/WebCore/loader/cache/CachedResource.h:213
> > +    void didReceiveResponse(const ResourceResponse&, Document*);
> 
> Why a Document* rather than Document&? Can it be null?

Removed.

> > Source/WebCore/loader/cache/CachedResource.h:311
> > +    virtual void cloneData(const CachedResource&) { }
> 
> What does it mean to "clone" data? I am not sure this is a good function
> name.

Renamed to setBodyDataFrom.

[Alexey Proskuryakov]

This change made http/tests/security/contentSecurityPolicy/worker-csp-blocks-xhr-redirect-cross-origin.html very flaky, so much that is causes false positives on EWS.

I'm going to roll out because of EWS impact.

[Alexey Proskuryakov]

Diff:

@@ -1,3 +1,4 @@
+CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.
 CONSOLE MESSAGE: XMLHttpRequest cannot load http://127.0.0.1:8000/security/contentSecurityPolicy/resources/redir.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi. Cross-origin redirection denied by Content Security Policy.
 This tests an XHR request made from a worker is blocked if it redirects to a cross-origin resource that is not listed as a connect-src in the CSP of the worker.

imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html is flaky too:

@@ -1,4 +1,3 @@
-CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin.
 CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin.
 CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin.
 CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by Access-Control-Allow-Origin.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 161614

[youenn fablet]

Thanks for spotting these issues.

> @@ -1,3 +1,4 @@
> +CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by
> Access-Control-Allow-Origin.
>  CONSOLE MESSAGE: XMLHttpRequest cannot load
> http://127.0.0.1:8000/security/contentSecurityPolicy/resources/redir.
> php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-
> allow.cgi. Cross-origin redirection denied by Content Security Policy.
>  This tests an XHR request made from a worker is blocked if it redirects to
> a cross-origin resource that is not listed as a connect-src in the CSP of
> the worker.

It seems that DocumentThreadableLoader sets the ResourceRequest to null when being notified of a redirection.
Then SubresourceLoader gets notified of the same redirection response in didReceiveResponse.
This is messing up with CORS checks as SubresourceLoader expects to check the response following the redirectionResponse, not the redirection respponse itself.
I'll mark the test as flaky and fix it in a follow-up patch.

> imported/w3c/web-platform-tests/fetch/api/cors/cors-basic-worker.html is
> flaky too:
> 
> @@ -1,4 +1,3 @@
> -CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by
> Access-Control-Allow-Origin.
>  CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by
> Access-Control-Allow-Origin.
>  CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by
> Access-Control-Allow-Origin.
>  CONSOLE MESSAGE: Origin http://localhost:8800 is not allowed by
> Access-Control-Allow-Origin.

Test should be fixed: one of the sub-test is a promise test but promise is not returned so test is not waiting for the end of the test.
I'll fix it in this landing patch.

[youenn fablet]

Created attachment 288003 [details]
Patch for landing

[youenn fablet]

Comment on attachment 288003 [details]
Patch for landing

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

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.js:23
> +    return fetch(url + RESOURCES_DIR + "top.txt", {"mode": "cors"} ).then(function(resp) {

Here is the fix for WPT flaky test.

[youenn fablet]

Created attachment 288006 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 288006 [details]
Patch for landing

Rejecting attachment 288006 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 288006, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 stage entries for merged file 'LayoutTests/imported/w3c'

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit

You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
fatal: multiple stage entries for merged file 'LayoutTests/imported/w3c'

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 128 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files

Full output: http://webkit-queues.webkit.org/results/2016876

[WebKit Commit Bot]

Comment on attachment 288006 [details]
Patch for landing

Clearing flags on attachment: 288006

Committed r205473: <http://trac.webkit.org/changeset/205473>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.