Bug 132664 - [CSS Shapes] Same URL for display and shape-outside can cause CORS problems
Summary: [CSS Shapes] Same URL for display and shape-outside can cause CORS problems
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-07 14:10 PDT by Hans Muller
Modified: 2016-10-06 02:00 PDT (History)
11 users (show)

See Also:


Attachments
Test case (762 bytes, text/html)
2014-05-07 14:10 PDT, Hans Muller
no flags Details
Test case. (1.41 KB, text/html)
2014-05-11 20:37 PDT, Hans Muller
no flags Details
Patch (14.63 KB, patch)
2014-09-04 08:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 2014-05-07 14:10:00 PDT
Created attachment 231022 [details]
Test case

If an ordinary image for some URL is cached, then a subsequent (and valid) cross-origin request for the same image fails, because the cached image wasn't fetched with CORS.  You can see as much in the attached example, by running the testImage() function.

CSS Shapes apps regularly aggravate this problem, because the same image URL is often used for display (without CORS) and for shape-outside (with CORS).
Comment 1 Hans Muller 2014-05-11 20:37:44 PDT
Created attachment 231272 [details]
Test case.

Slightly revised the test case.  There are a few additional comments, etc.
Comment 2 Alexey Proskuryakov 2014-05-12 11:19:44 PDT
I expect that this will be fairly involved to fix cleanly. WebCore loader is not suited for the same resource being used for multiple purposes, or even for resources used for the same purpose in multiple subframes. The cases that work generally do so by avoiding caching altogether.

Would it be possible to have a way to directly express this use case in markup? If it's a common case, a nice syntax for it may be appropriate.

A workaround would be to use a different URL, of course (e.g. by adding a query such as "?for-shape").
Comment 3 Theresa O'Connor 2014-05-13 10:28:07 PDT
(In reply to comment #2)
> Would it be possible to have a way to directly express this use case in markup?

CSS V&U Level 3 provides for this by enhancing the attr() function:

shape-outside: attr(src url)

See also bug 26609.
Comment 4 youenn fablet 2014-06-11 05:20:55 PDT
(In reply to comment #2)
> I expect that this will be fairly involved to fix cleanly. WebCore loader is not suited for the same resource being used for multiple purposes, or even for resources used for the same purpose in multiple subframes. The cases that work generally do so by avoiding caching altogether.

Resources should probably not be reused when the request origin policy options are not the same.
How about mandating the loader to reload whenever the HTTP origin of the new request does not match the origin of the cached one?
Comment 5 youenn fablet 2014-09-04 08:54:56 PDT
Created attachment 237627 [details]
Patch
Comment 6 Alexey Proskuryakov 2014-12-17 11:04:43 PST
Comment on attachment 237627 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        CachedResourceLoader now reloads cached resources for which the Origin between the cached request and the new request is not the same.

This is probably an improvement overall, however it's not too great that shape-outside resources won't be cached, and will instead cause two loads on every page load.

> LayoutTests/ChangeLog:3
> +        [CSS Shapes] Same URL for display and shape-outside can cause CORS problems

There doesn't appear to be a test for shape-outside added with this patch. Can it be tested?
Comment 7 youenn fablet 2014-12-18 09:45:34 PST
(In reply to comment #6)
> Comment on attachment 237627 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=237627&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        CachedResourceLoader now reloads cached resources for which the Origin between the cached request and the new request is not the same.
> 
> This is probably an improvement overall, however it's not too great that
> shape-outside resources won't be cached, and will instead cause two loads on
> every page load.

The changelog is in fact a simplification of what the newly added check does:
it verifies upfront whether the request requires CORS check in which case Reload happens only if cached response is failing CORS check with the request origin.
If a web site adds correct CORS information for the first request, Reload will never happen.

I can update the changelog if that helps.

An alternative/generalization of the current check would be to Reload according the Vary header of the response.


> > LayoutTests/ChangeLog:3
> > +        [CSS Shapes] Same URL for display and shape-outside can cause CORS problems
> 
> There doesn't appear to be a test for shape-outside added with this patch.
> Can it be tested?

Sure, I can try to add one.
Comment 8 Brent Fulgham 2016-08-22 11:25:09 PDT
Hi Youenn. What's the status with this change? It's desperately out of date, so we can't really review it...
Comment 9 youenn fablet 2016-08-22 11:31:28 PDT
(In reply to comment #8)
> Hi Youenn. What's the status with this change? It's desperately out of date,
> so we can't really review it...

I will rebase the patch. I am not yet exactly sure when will be the best time for it. Please let me know if this is somehow blocking
Comment 10 youenn fablet 2016-08-30 09:46:15 PDT
I am not able to reproduce the particular bug for CSS Shapes.
AFAICS, shape-outside URLs are fetched in CORS mode but in anonymous mode, thus without credentials.
img elements load data in No-CORS mode but with credentials.
This difference triggers reloading of the cached resource.

When reloading, all necessary checks are done.

If there is no objection, I'll close that bug as fixed.