Add suggestion to use "no-cors" with Fetch fails CORS check.

Add suggestion to use "no-cors" with Fetch fails CORS check.

See https://code.google.com/p/chromium/issues/detail?id=452745#c17 for details.

BUG=452745
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201949

[jeremyarcher, 2015-08-19 08:08:41]

jeremyarcher@google.com changed reviewers:
+ falken@chromium.org, kenjibaheux@chromium.org, mattgaunt@chromium.org

[jeremyarcher, 2015-08-19 08:08:42]

I believe this fixes #452745. I changed "fix" to "change," 
because it sounded a bit more neutral, but would happily
change it back.

[falken, 2015-08-19 08:20:55]

falken@chromium.org changed reviewers:
+ hiroshige@chromium.org

[falken, 2015-08-19 08:20:55]

this looks good to me, a fetch OWNER is needed though

[falken, 2015-08-19 08:21:55]

On 2015/08/19 08:20:55, falken wrote:
> this looks good to me, a fetch OWNER is needed though

(Added hiroshige@ as he had some opinions on the bug.)

[hiroshige, 2015-08-19 11:38:18]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org

[hiroshige, 2015-08-19 11:38:18]

> a fetch OWNER is needed though
a core/fetch OWNER is needed (I'm a modules/fetch OWNER).
Added +japhet@ as a core/fetch OWNER.

In addition to Fetch API, this change also affects other network accesses that
requires CORS, including XHRs, Web Fonts, etc.
So we got a message like:
"Font from origin 'http://...' has been blocked from loading by Cross-Origin
Resource Sharing policy: No 'Access-Control-Allow-Origin' header is present on
the requested resource. Origin 'http://localhost:8001' is therefore not allowed
access. Alternatively, you can use the 'no-cors' mode with fetch to get an
opaque response."
when cross-origin WebFont loading fails.

So how about changing the additional messages to for example:
"Add/Change the header, or if you want an opaque response use Fetch API with the
'no-cors' mode."
so that users not interested in opaque responses can ignore the latter half.

(Alternatively, we might able to add the message only for Fetch API.
Possibly adding the message in
FetchManager::Loader::didFailAccessControlCheck()? But this also adds the
message when even the no-cors mode cannot get opaque response...)

Also, please update -expected.txt affected by this change.
For example, LayoutTests/http/tests/fetch/chromium/error-messages-expected.txt
tests (the existence of) the error messsage for CORS failure in Fetch API, and
thus should be updated.

[jeremyarcher, 2015-08-20 02:23:41]

On 2015/08/19 11:38:18, hiroshige (ooo zombie) wrote:
> > a fetch OWNER is needed though
> a core/fetch OWNER is needed (I'm a modules/fetch OWNER).
> Added +japhet@ as a core/fetch OWNER.
> 
> In addition to Fetch API, this change also affects other network accesses that
> requires CORS, including XHRs, Web Fonts, etc.
> So we got a message like:
> "Font from origin 'http://...' has been blocked from loading by Cross-Origin
> Resource Sharing policy: No 'Access-Control-Allow-Origin' header is present on
> the requested resource. Origin 'http://localhost:8001' is therefore not
allowed
> access. Alternatively, you can use the 'no-cors' mode with fetch to get an
> opaque response."
> when cross-origin WebFont loading fails.
> 
> So how about changing the additional messages to for example:
> "Add/Change the header, or if you want an opaque response use Fetch API with
the
> 'no-cors' mode."
> so that users not interested in opaque responses can ignore the latter half.

I've made this change in the patch.

> (Alternatively, we might able to add the message only for Fetch API.
> Possibly adding the message in
> FetchManager::Loader::didFailAccessControlCheck()? But this also adds the
> message when even the no-cors mode cannot get opaque response...)

Happy to do either! I've kept it as-is for the moment, but will investigate that
path further.

> Also, please update -expected.txt affected by this change.
> For example, LayoutTests/http/tests/fetch/chromium/error-messages-expected.txt
> tests (the existence of) the error messsage for CORS failure in Fetch API, and
> thus should be updated.

Dang, this is what I get for using ^R to run tests (ended up only running the
Service Worker tests, not the one for XHR and Fetch). I've uploaded a patch
containing
all 112 [!] changed expectations, and have manually read over the diff to make
sure
it's not doing anything crazy.

For reference, I used the following to generated the modified expectations file.

% Tools/Scripts/run-webkit-tests
% (cd ~/blink/src/webkit/Release/layout-test-results &&
     find http/ -type f | grep 'actual.txt') |
  sed 's:\(.*\)-actual.txt:cp
~/blink/src/webkit/Release/layout-test-results/\1-actual.txt
LayoutTests/\1-expected.txt:g' |
  bash -ex
% git diff # human sanity check
% git commit -a

I believe this fixes most of the expectations; at least, run-webkit-tests now
returns only
the same unexpected passes that were previously present before the refactor.

[jeremyarcher, 2015-08-28 06:39:20]

jeremyarcher@google.com changed reviewers:
- kenjibaheux@chromium.org, mattgaunt@chromium.org

[jeremyarcher, 2015-08-28 06:39:21]

Ping! (-kenji -matt)

[hiroshige, 2015-08-28 08:24:22]

hiroshige@chromium.org changed reviewers:
+ tyoshino@chromium.org

[hiroshige, 2015-08-28 08:24:23]

I briefly discussed this with tyoshino@ and yhirano@, and changed my mind (sorry
for inconvenience). Limiting the additional message only in Fetch API would be
better.

How about the followings?
- Add a WebURLRequest::RequestContext parameter to passesAccessControlCheck(),
- Add the messages only if RequestContext is Fetch API, and
- Supply the RequestContext from |DocumentThreadableLoader::m_requestContext|?
(RequestContext can be obtained from ResourceRequest, but ResourceRequest is not
available in some call sites of passesAccessControlCheck()).

If this works correctly, I expect we can revert the most of the expectations
(other than http/tests/fetch).

(FYI ./third_party/WebKit/Tools/Scripts/run-webkit-tests has --reset-results
option that updates the expectation files. This also creates new expectation
files and I have to git clean though...)

https://codereview.chromium.org/1300083002/diff/20001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/1300083002/diff/20001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:161: errorDescription.append("
Alternatively, if you want an opaque response, switch to The Fetch API in
'no-cors' mode.");
nit: s/The Fetch API/the Fetch API/

[tyoshino (SeeGerritForStatus), 2015-08-31 05:52:15]

I wrote my thoughts on the bug:
https://code.google.com/p/chromium/issues/detail?id=452745#c20

As hiroshige@ already commented, showing this message for APIs or elements
without 'no-cors' option is confusing. This advice should show up for use cases
which can be switched to use fetch. I.e. basically only to users of XHR, Fetch.
I said hiroshige@ that showing this to XHR is also bad, but I changed my mind.
It's possible that an opaque response serves some of XHR users, and they can be
educated by this message.

[jeremyarcher, 2015-09-01 03:46:01]

On 2015/08/31 05:52:15, tyoshino wrote:
> I wrote my thoughts on the bug:
> https://code.google.com/p/chromium/issues/detail?id=452745#c20
> 
> As hiroshige@ already commented, showing this message for APIs or elements
> without 'no-cors' option is confusing. This advice should show up for use
cases
> which can be switched to use fetch. I.e. basically only to users of XHR,
Fetch.
> I said hiroshige@ that showing this to XHR is also bad, but I changed my mind.
> It's possible that an opaque response serves some of XHR users, and they can
be
> educated by this message.

I've changed the code so that it only displays the message to users of Fetch.
I wasn't sure- should this be applied to XHR as well?

For reference, the error is currently:

  Fetch API cannot load //blah. No 'Access-Control-Allow-Origin' header is
present
  on the requested resource. Origin '//somethingnotblah' is therefore not
allowed
  access. If an opaque response serves your needs, set the request's mode to
'no-cors'
  to fetch the resource with CORS disabled.

Other thoughts?

[hiroshige, 2015-09-01 15:44:17]

On 2015/09/01 03:46:01, jeremyarcher wrote:
> I've changed the code so that it only displays the message to users of Fetch.
> I wasn't sure- should this be applied to XHR as well?

tyoshino@ and I are neutral about adding the message to XHR, i.e. either
omitting the message (like Patch Set 3) or adding the message for XHRs is OK.

non-owner lgtm.

tyoshino@ and other reviewers,
How about message texts?
Currently two different texts are used for two cases (L162 and L179 of
CrossOriginAccessControl.cpp):
> If an opaque response serves your needs, set the request's mode to 'no-cors'
to fetch the resource with CORS disabled.
> Either change the header or use the 'no-cors' mode with fetch to get an opaque
response.
I slightly prefer to use somehow similar texts in these lines, but it's
optional.

[jeremyarcher, 2015-09-02 03:47:20]

+japhet for OWNERS

[jeremyarcher, 2015-09-07 04:01:56]

Oops-

+japhet, for real this time.

[tyoshino (SeeGerritForStatus), 2015-09-07 08:41:07]

Thanks! lgtm

https://codereview.chromium.org/1300083002/diff/40001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/1300083002/diff/40001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:179: errorDescription.append("
Either change the header or use the 'no-cors' mode with fetch to get an opaque
response.");
Please update the text after "or" here as well. For example,

" Have the server send the header with a valid value, or if an opaque response
serves your needs, set the request's mode to 'no-cors' to fetch the resource
with CORS disabled."

The suggestions made here are for two different things (CORS enabled to get a
non-opaque, CORS disabled to get an opaque). Simply connecting them with "either
... or ..." doesn't sound so good.

[jeremyarcher, 2015-09-07 08:47:28]

https://codereview.chromium.org/1300083002/diff/40001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/1300083002/diff/40001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:179: errorDescription.append("
Either change the header or use the 'no-cors' mode with fetch to get an opaque
response.");
On 2015/09/07 08:41:07, tyoshino wrote:
> Please update the text after "or" here as well. For example,
> 
> " Have the server send the header with a valid value, or if an opaque response
> serves your needs, set the request's mode to 'no-cors' to fetch the resource
> with CORS disabled."
> 
> The suggestions made here are for two different things (CORS enabled to get a
> non-opaque, CORS disabled to get an opaque). Simply connecting them with
"either
> ... or ..." doesn't sound so good.

Good eyes! I've made the patch.

[tyoshino (SeeGerritForStatus), 2015-09-08 06:26:20]

lgtm!

[Nate Chapin, 2015-09-08 18:07:44]

lgtm

[jeremyarcher, 2015-09-08 23:56:57]

The CQ bit was checked by jeremyarcher@google.com

[jeremyarcher, 2015-09-08 23:56:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/1300083002/#ps60001
(title: "Update case where header is present but not for the correct origin.")

[commit-bot: I haz the power, 2015-09-08 23:57:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1300083002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1300083002/60001

[commit-bot: I haz the power, 2015-09-09 04:20:19]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201949

[sof, 2015-09-09 05:22:03]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-09-09 05:22:04]

https://codereview.chromium.org/1300083002/diff/60001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/1300083002/diff/60001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.h:62: bool
passesAccessControlCheck(const ResourceResponse&, StoredCredentials,
SecurityOrigin*, String& errorDescription, WebURLRequest::RequestContext
requestType);
Can we try to follow
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...
and move the added argument as the 2nd?

[jeremyarcher, 2015-09-09 05:25:39]

On 2015/09/09 05:22:04, sof wrote:
>
https://codereview.chromium.org/1300083002/diff/60001/Source/core/fetch/Cross...
> File Source/core/fetch/CrossOriginAccessControl.h (right):
> 
>
https://codereview.chromium.org/1300083002/diff/60001/Source/core/fetch/Cross...
> Source/core/fetch/CrossOriginAccessControl.h:62: bool
> passesAccessControlCheck(const ResourceResponse&, StoredCredentials,
> SecurityOrigin*, String& errorDescription, WebURLRequest::RequestContext
> requestType);
> Can we try to follow
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Para...
> and move the added argument as the 2nd?

Absolutely! I'll open it as a new CL.