Leave out empty-valued Access-Control-Request-Headers: on preflights.

Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate in a preflight.

R=tyoshino,yhirano
BUG=633729
Review-Url: https://codereview.chromium.org/2633423003
Cr-Commit-Position: refs/heads/master@{#444303}
Committed: https://chromium.googlesource.com/chromium/src/+/cf55a356c36b96ce9ee4bd6aa33235c87412a3aa

[sof, 2017-01-17 20:48:36]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 20:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-01-17 21:55:35]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:55:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-01-17 21:56:24]

Description was changed from

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
header should not be included if the request has no headers to enumerate.

R=
BUG=633729
==========

to

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate.

R=
BUG=633729
==========

[sof, 2017-01-17 21:56:47]

sigbjornf@opera.com changed reviewers:
+ tyoshino@chromium.org, yhirano@chromium.org

[sof, 2017-01-17 21:56:48]

please take a look.

[commit-bot: I haz the power, 2017-01-17 23:46:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 23:46:27]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-18 05:41:55]

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:111: if
(request.httpHeaderFields().size()) {
Do we need this outer branch?

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp:51:
EXPECT_EQ(nullptr,
nullAtom or AtomicString() might be easier to understand.

[tyoshino (SeeGerritForStatus), 2017-01-18 06:00:29]

Thank you for fixing this.

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js:177:
[checkMethod]],
This test is ok to have, but what we should test is a request with a custom but
simple header, I think. Could you please add it (needs change on
getRequestInit() in thorough-util.js)?

[sof, 2017-01-18 06:26:49]

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js:177:
[checkMethod]],
On 2017/01/18 06:00:29, tyoshino wrote:
> This test is ok to have, but what we should test is a request with a custom
but
> simple header, I think. Could you please add it (needs change on
> getRequestInit() in thorough-util.js)?

You mean explicitly furnishing the request with a significant subset of
https://fetch.spec.whatwg.org/#cors-safelisted-request-header ?

[tyoshino (SeeGerritForStatus), 2017-01-18 06:49:02]

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js:177:
[checkMethod]],
On 2017/01/18 06:26:49, sof wrote:
> On 2017/01/18 06:00:29, tyoshino wrote:
> > This test is ok to have, but what we should test is a request with a custom
> but
> > simple header, I think. Could you please add it (needs change on
> > getRequestInit() in thorough-util.js)?
> 
> You mean explicitly furnishing the request with a significant subset of
> https://fetch.spec.whatwg.org/#cors-safelisted-request-header ?

Right. Even before this fix, we skipped the Access-Control-Request-Headers
header generation when there's no header in the ResourceRequest given to the
DocumentThreadableLoader. The problem is when there's any CORS-safelisted header
in the ResourceRequest, the Access-Control-Request-Headers header generation
code is run, and because of my patch, it skips CORS-safelisted headers to
generate an empty header. I think it's worth adding this scenario as a unit
test.

[sof, 2017-01-18 06:57:42]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[sof, 2017-01-18 06:57:48]

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/thorough/cors-preflight2.js:177:
[checkMethod]],
On 2017/01/18 06:49:02, tyoshino wrote:
> On 2017/01/18 06:26:49, sof wrote:
> > On 2017/01/18 06:00:29, tyoshino wrote:
> > > This test is ok to have, but what we should test is a request with a
custom
> > but
> > > simple header, I think. Could you please add it (needs change on
> > > getRequestInit() in thorough-util.js)?
> > 
> > You mean explicitly furnishing the request with a significant subset of
> > https://fetch.spec.whatwg.org/#cors-safelisted-request-header ?
> 
> Right. Even before this fix, we skipped the Access-Control-Request-Headers
> header generation when there's no header in the ResourceRequest given to the
> DocumentThreadableLoader. The problem is when there's any CORS-safelisted
header
> in the ResourceRequest, the Access-Control-Request-Headers header generation
> code is run, and because of my patch, it skips CORS-safelisted headers to
> generate an empty header. I think it's worth adding this scenario as a unit
> test.

ok, that code path was already being exercised by this test - i suspect some
default "CORS safelisted" headers were making it through already, but didn't
investigate which. Might as well be explicit and furnish them.

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:111: if
(request.httpHeaderFields().size()) {
On 2017/01/18 05:41:55, yhirano wrote:
> Do we need this outer branch?

It almost always holds, i reckon & the extra code it executes isn't worth losing
sleep over, so removed.

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp
(right):

https://codereview.chromium.org/2633423003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp:51:
EXPECT_EQ(nullptr,
On 2017/01/18 05:41:55, yhirano wrote:
> nullAtom or AtomicString() might be easier to understand.

Using nullAtom instead + added a comment.

[commit-bot: I haz the power, 2017-01-18 06:58:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tyoshino (SeeGerritForStatus), 2017-01-18 07:20:55]

lgtm

[yhirano, 2017-01-18 07:23:08]

lgtm

[sof, 2017-01-18 07:39:16]

Description was changed from

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate.

R=
BUG=633729
==========

to

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate in a preflight.

R=tyoshino,yhirano
BUG=633729
==========

[commit-bot: I haz the power, 2017-01-18 08:22:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 08:22:19]

Dry run: This issue passed the CQ dry run.

[sof, 2017-01-18 08:26:04]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2017-01-18 08:26:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 08:30:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484727964834740,
"parent_rev": "2a525f9d4ab7f144b179ed013482e3ec98dfd6d0", "commit_rev":
"cf55a356c36b96ce9ee4bd6aa33235c87412a3aa"}

[commit-bot: I haz the power, 2017-01-18 08:30:32]

Description was changed from

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate in a preflight.

R=tyoshino,yhirano
BUG=633729
==========

to

==========
Leave out empty-valued Access-Control-Request-Headers: on preflights.

Following https://github.com/whatwg/fetch/issues/459 , the above
preflight header should not be included if the request following CORS
has no headers to enumerate in a preflight.

R=tyoshino,yhirano
BUG=633729

Review-Url: https://codereview.chromium.org/2633423003
Cr-Commit-Position: refs/heads/master@{#444303}
Committed:
https://chromium.googlesource.com/chromium/src/+/cf55a356c36b96ce9ee4bd6aa332...
==========

[commit-bot: I haz the power, 2017-01-18 08:30:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cf55a356c36b96ce9ee4bd6aa332...