Save-Data should not be added to CORS Access-Control-Request-Headers

Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704
Committed: https://crrev.com/dcc9b2874edf8fd00070eb4f14176288535101a2
Cr-Commit-Position: refs/heads/master@{#385637}

[Raj, 2016-04-06 04:23:35]

rajendrant@chromium.org changed reviewers:
+ falken@chromium.org, kinuko@chromium.org, phajdan.jr@chromium.org

[Raj, 2016-04-06 04:23:35]

PTAL

falken: service_worker_browsertest.cc
kinuko: content/test/data/service_worker/* CrossOriginAccessControl.cpp
phajdan: net/test/embedded_test_server/*

Thanks

[falken, 2016-04-06 04:41:37]

This looks good. In the CL description, please link to the github issue about
removing all the headers:

https://github.com/whatwg/fetch/issues/249

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:234: auto it_acrh =
request.headers.find("Access-Control-Request-Headers");
nit: "acrh_iter" is a more common Chromium naming style.

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:1179:
EXPECT_EQ(title1, title_watcher1.WaitAndGetTitle());
Remove the 1 from these names?

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
File content/test/data/service_worker/fetch_cross_origin.html (right):

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_cross_origin.html:14: });
nit: Service worker test code likes to chain promises when possible (and using
arrow operator makes it less verbose):

fetch(queryString.substring(1), {headers: {'X-Custom-Header': 'foo'}})
  .then(data => data.text())
  .then(text => document.title = text)
  .catch(() => document.title = 'FAIL');

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_cross_origin.html:16: document.title =
"FAIL";
Use single-quotes for consistency.

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
File content/test/data/service_worker/fetch_event_pass_through.js (right):

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_event_pass_through.js:6:
event.respondWith(fetch(event.request));
We don't call this pass through. Pass through would be no fetch handler, or a
fetch handler that does not call respondWith().

But there is no good name for this yet. I'd call it:
fetch_event_respond_with_fetch.

[Raj, 2016-04-06 05:08:29]

Description was changed from

==========
Save-Data should not be added to CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers.

As a short-term fix save-data will not be added to ACRH header. Later
all simple headers will not be added to ACRH header.

BUG=599704
==========

to

==========
Save-Data should not be added to CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will not
be added to ACRH header.

Later all simple headers will not be added to ACRH header.
https://github.com/whatwg/fetch/issues/249

BUG=599704
==========

[Raj, 2016-04-06 05:08:42]

Addressed comments.

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:234: auto it_acrh =
request.headers.find("Access-Control-Request-Headers");
On 2016/04/06 04:41:36, falken wrote:
> nit: "acrh_iter" is a more common Chromium naming style.

Done.

https://codereview.chromium.org/1859313002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_browsertest.cc:1179:
EXPECT_EQ(title1, title_watcher1.WaitAndGetTitle());
On 2016/04/06 04:41:36, falken wrote:
> Remove the 1 from these names?

Done.

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
File content/test/data/service_worker/fetch_cross_origin.html (right):

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_cross_origin.html:14: });
On 2016/04/06 04:41:36, falken wrote:
> nit: Service worker test code likes to chain promises when possible (and using
> arrow operator makes it less verbose):
> 
> fetch(queryString.substring(1), {headers: {'X-Custom-Header': 'foo'}})
>   .then(data => data.text())
>   .then(text => document.title = text)
>   .catch(() => document.title = 'FAIL');

Done. Nice.

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_cross_origin.html:16: document.title =
"FAIL";
On 2016/04/06 04:41:36, falken wrote:
> Use single-quotes for consistency.

Done.

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
File content/test/data/service_worker/fetch_event_pass_through.js (right):

https://codereview.chromium.org/1859313002/diff/1/content/test/data/service_w...
content/test/data/service_worker/fetch_event_pass_through.js:6:
event.respondWith(fetch(event.request));
On 2016/04/06 04:41:37, falken wrote:
> We don't call this pass through. Pass through would be no fetch handler, or a
> fetch handler that does not call respondWith().
> 
> But there is no good name for this yet. I'd call it:
> fetch_event_respond_with_fetch.

Done.

[falken, 2016-04-06 05:13:27]

lgtm

[bengr, 2016-04-06 16:50:29]

bengr@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2016-04-06 16:50:30]

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:221:
scoped_ptr<net::test_server::HttpResponse>
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1148:
IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest, CrossOriginFetchWithSaveData) {
Please add a comment to describe what is being tested.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1162:
scoped_refptr<WorkerActivatedObserver> observer =
#include "base/memory/ref_counted.h" -- I don't know why this wasn't already
included.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1177: const
base::string16 title = base::ASCIIToUTF16("PASS");
#include "base/strings/string16.h"

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

https://codereview.chromium.org/1859313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:98: // As a
short-term fix, filter Save-Data out of
Reference a bug so we don't lose track of this.

[Raj, 2016-04-06 17:42:47]

Description was changed from

==========
Save-Data should not be added to CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will not
be added to ACRH header.

Later all simple headers will not be added to ACRH header.
https://github.com/whatwg/fetch/issues/249

BUG=599704
==========

to

==========
Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704
==========

[Raj, 2016-04-06 17:45:20]

Addressed bengr comments.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:221:
scoped_ptr<net::test_server::HttpResponse>
On 2016/04/06 16:50:30, bengr wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1148:
IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest, CrossOriginFetchWithSaveData) {
On 2016/04/06 16:50:30, bengr wrote:
> Please add a comment to describe what is being tested.

Done.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1162:
scoped_refptr<WorkerActivatedObserver> observer =
On 2016/04/06 16:50:30, bengr wrote:
> #include "base/memory/ref_counted.h" -- I don't know why this wasn't already
> included.

Done.

https://codereview.chromium.org/1859313002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1177: const
base::string16 title = base::ASCIIToUTF16("PASS");
On 2016/04/06 16:50:30, bengr wrote:
> #include "base/strings/string16.h"

Done.

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

https://codereview.chromium.org/1859313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:98: // As a
short-term fix, filter Save-Data out of
On 2016/04/06 16:50:30, bengr wrote:
> Reference a bug so we don't lose track of this.

Done.

[bengr, 2016-04-06 17:56:54]

lgtm.

[Paweł Hajdan Jr., 2016-04-06 20:15:46]

LGTM w/comment

https://codereview.chromium.org/1859313002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1184: TitleWatcher
title_watcher(shell()->web_contents(), title);
Shouldn't TitleWatcher be instantiated *before* the test triggers navigation?

My understand is that was the point of separating its construction from
WaitAndGetTitleCall. Otherwise you wouldn't need an object.

And the general idea with such design is to avoid race conditions, i.e. to
register for notifications before they might be triggered.

[Raj, 2016-04-06 22:04:10]

Addressed phajdan comments

https://codereview.chromium.org/1859313002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1859313002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:1184: TitleWatcher
title_watcher(shell()->web_contents(), title);
On 2016/04/06 20:15:46, Paweł Hajdan Jr. wrote:
> Shouldn't TitleWatcher be instantiated *before* the test triggers navigation?
> 
> My understand is that was the point of separating its construction from
> WaitAndGetTitleCall. Otherwise you wouldn't need an object.
> 
> And the general idea with such design is to avoid race conditions, i.e. to
> register for notifications before they might be triggered.

Done.

[kinuko, 2016-04-07 01:12:28]

lgtm

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

https://codereview.chromium.org/1859313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:100: //
TODO(crbug.com/601092): Longer-term all simple headers should
TODO(name): crbug.com/601092 Longer-term all simple headers should...

is the preferred style in blink/chromium.

http://www.chromium.org/blink/coding-style#TOC-Comments

[Raj, 2016-04-07 01:17:47]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-04-07 01:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
bengr@chromium.org, phajdan.jr@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1859313002/#ps100001
(title: "Addressed kinuko comments")

[Raj, 2016-04-07 01:18:14]

Thanks

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

https://codereview.chromium.org/1859313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:100: //
TODO(crbug.com/601092): Longer-term all simple headers should
On 2016/04/07 01:12:28, kinuko wrote:
> TODO(name): crbug.com/601092 Longer-term all simple headers should...
> 
> is the preferred style in blink/chromium.
> 
> http://www.chromium.org/blink/coding-style#TOC-Comments

Done.

[commit-bot: I haz the power, 2016-04-07 01:18:49]

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

[commit-bot: I haz the power, 2016-04-07 03:00:02]

Description was changed from

==========
Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704
==========

to

==========
Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704
==========

[commit-bot: I haz the power, 2016-04-07 03:00:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-07 03:01:36]

Description was changed from

==========
Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704
==========

to

==========
Exclude Save-Data from CORS Access-Control-Request-Headers

When data saver is enabled, and a cross-origin fetch by a webpage is
intercepted by a serviceworker and the serviceworker does a fetch, the
preflight request will have save-data added to
Access-Control-Request-Headers. As a short-term fix save-data will be
excluded from ACRH header.

Later all simple headers will be excluded from ACRH header.
https://github.com/whatwg/fetch/issues/249
https://crbug.com/601092

BUG=599704

Committed: https://crrev.com/dcc9b2874edf8fd00070eb4f14176288535101a2
Cr-Commit-Position: refs/heads/master@{#385637}
==========

[commit-bot: I haz the power, 2016-04-07 03:01:38]

Patchset 6 (id:??) landed as
https://crrev.com/dcc9b2874edf8fd00070eb4f14176288535101a2
Cr-Commit-Position: refs/heads/master@{#385637}