Cross origin requests to same origin should match

Cross origin requests to same origin should match

Preloaded CORS mode requests (e.g. preloaded fonts) to same-origin should match regardless of their 'crossorigin' attribute.
This CL makes sure they do.

Spec discussion: https://github.com/w3c/preload/issues/32

BUG=595148

[Yoav Weiss, 2016-03-15 23:17:27]

Description was changed from

==========
Cross origin requests to same origin should match

BUG=
==========

to

==========
Cross origin requests to same origin should match

Preloaded CORS mode requests (e.g. preloaded fonts) to same-origin should match
regardless of their 'crossorigin' attribute.
This CL makes sure they do.

BUG=
==========

[Yoav Weiss, 2016-03-15 23:17:27]

yoav@yoav.ws changed reviewers:
+ japhet@chromium.org

[Yoav Weiss, 2016-03-15 23:21:23]

Description was changed from

==========
Cross origin requests to same origin should match

Preloaded CORS mode requests (e.g. preloaded fonts) to same-origin should match
regardless of their 'crossorigin' attribute.
This CL makes sure they do.

BUG=
==========

to

==========
Cross origin requests to same origin should match

Preloaded CORS mode requests (e.g. preloaded fonts) to same-origin should match
regardless of their 'crossorigin' attribute.
This CL makes sure they do.

Spec discussion: https://github.com/w3c/preload/issues/32

BUG=595148
==========

[Yoav Weiss, 2016-03-15 23:21:57]

Hey Nate! Can you take a look?

[Nate Chapin, 2016-03-17 22:58:18]

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchRequest.cpp (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchRequest.cpp:81: m_options.sameOrigin =
isSameOriginRequest ? IsSameOrigin : NotSameOrigin;
Instead of having this sameOrigin property, why not check
canRequestNoSuborigin() when needed?

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp:8: bool
ResourceLoaderOptions::canReuseRequest(const ResourceLoaderOptions& other) const
Why create a .cpp for this?

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h:98: ,
sameOrigin(NotSameOrigin)
This will make non-CORS loads always be marked NotSameOrigin. That seems
strange?

[Yoav Weiss, 2016-03-18 07:53:14]

I'm no longer 100% sure that this approach is the right one, as the test
failures seem legit and point to a problem with this approach and redirects.

If I understand correctly, when two same origin requests are requested, one with
corsEnabled and one without, we can reuse the requests *unless* the response is
a redirect to a different origin. In order to resolve that, we would need to add
hook that verify cors state upon redirect requests as well. Is Blink aware of
redirects in such a scenario?

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FetchRequest.cpp (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FetchRequest.cpp:81: m_options.sameOrigin =
isSameOriginRequest ? IsSameOrigin : NotSameOrigin;
On 2016/03/17 22:58:18, Nate Chapin wrote:
> Instead of having this sameOrigin property, why not check
> canRequestNoSuborigin() when needed?

Because we don't necessarily have the securityOrigin nor the URL in
canReuseRequest right now. But I guess we could set the origin here and pipe the
URL in from the callers.

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp:8: bool
ResourceLoaderOptions::canReuseRequest(const ResourceLoaderOptions& other) const
On 2016/03/17 22:58:18, Nate Chapin wrote:
> Why create a .cpp for this?

Because a ~2000 file rebuild every time I experimented with this logic was
extremely painful.
Since this is logic rather than just a getter/setter, I thought a cpp is
appropriate.

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h:98: ,
sameOrigin(NotSameOrigin)
On 2016/03/17 22:58:18, Nate Chapin wrote:
> This will make non-CORS loads always be marked NotSameOrigin. That seems
> strange?

True. Maybe a third value is needed

[Nate Chapin, 2016-03-18 20:01:22]

On 2016/03/18 07:53:14, Yoav Weiss wrote:
> I'm no longer 100% sure that this approach is the right one, as the test
> failures seem legit and point to a problem with this approach and redirects.
> 
> If I understand correctly, when two same origin requests are requested, one
with
> corsEnabled and one without, we can reuse the requests *unless* the response
is
> a redirect to a different origin. In order to resolve that, we would need to
add
> hook that verify cors state upon redirect requests as well. Is Blink aware of
> redirects in such a scenario?

ResourceFetcher::willFollowRedirect() has a call to
CrossOriginAccessControl::handleRedirect(). Blink definitely gets notifications
for redirects (except for sync requests), and it looks like we do at least some
CORS handling then.

> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/FetchRequest.cpp (right):
> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/FetchRequest.cpp:81: m_options.sameOrigin
=
> isSameOriginRequest ? IsSameOrigin : NotSameOrigin;
> On 2016/03/17 22:58:18, Nate Chapin wrote:
> > Instead of having this sameOrigin property, why not check
> > canRequestNoSuborigin() when needed?
> 
> Because we don't necessarily have the securityOrigin nor the URL in
> canReuseRequest right now. But I guess we could set the origin here and pipe
the
> URL in from the callers.
> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp (right):
> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.cpp:8: bool
> ResourceLoaderOptions::canReuseRequest(const ResourceLoaderOptions& other)
const
> On 2016/03/17 22:58:18, Nate Chapin wrote:
> > Why create a .cpp for this?
> 
> Because a ~2000 file rebuild every time I experimented with this logic was
> extremely painful.
> Since this is logic rather than just a getter/setter, I thought a cpp is
> appropriate.
> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h (right):
> 
>
https://codereview.chromium.org/1801513003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceLoaderOptions.h:98: ,
> sameOrigin(NotSameOrigin)
> On 2016/03/17 22:58:18, Nate Chapin wrote:
> > This will make non-CORS loads always be marked NotSameOrigin. That seems
> > strange?
> 
> True. Maybe a third value is needed

[Yoav Weiss, 2016-03-21 17:55:11]

On 2016/03/18 20:01:22, Nate Chapin wrote:
> On 2016/03/18 07:53:14, Yoav Weiss wrote:
> > I'm no longer 100% sure that this approach is the right one, as the test
> > failures seem legit and point to a problem with this approach and redirects.
> > 
> > If I understand correctly, when two same origin requests are requested, one
> with
> > corsEnabled and one without, we can reuse the requests *unless* the response
> is
> > a redirect to a different origin. In order to resolve that, we would need to
> add
> > hook that verify cors state upon redirect requests as well. Is Blink aware
of
> > redirects in such a scenario?
> 
> ResourceFetcher::willFollowRedirect() has a call to
> CrossOriginAccessControl::handleRedirect(). Blink definitely gets
notifications
> for redirects (except for sync requests), and it looks like we do at least
some
> CORS handling then.
>

Looked into that, and it seems like willFollowRedirect enables you to return
false, 
which cancels the request.

However for our scenario where we have 2 requests with different corsEnabled,
we'd have to hook up a mechanism here that enables the use of the response in
the non-CORS request, but forces a second fetch for the CORS request in this
scenario.

I'm not sure that the win of removing the "crossorigin" attribute requirement
for same origin are worth the complexity of such a mechanism. WDYT?