LoadURLParams::extra_headers should not apply to subresource requests.

LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4246472c00afcf4f7e7c55317d6988d158e83e11
Cr-Commit-Position: refs/heads/master@{#425972}

[Łukasz Anforowicz, 2016-10-17 19:22:36]

Description was changed from

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
==========

to

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-10-17 19:22:38]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:23:20]

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

[Łukasz Anforowicz, 2016-10-17 19:24:11]

Description was changed from

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-10-17 19:24:12]

lukasza@chromium.org changed reviewers:
+ creis@chromium.org

[Łukasz Anforowicz, 2016-10-17 21:14:11]

creis@, could you PTAL?

[Charlie Reis, 2016-10-17 22:46:18]

Hmm, I'm not actually clear on whether this is the right thing to do, but that's
because I don't know enough about what extra_headers is intended for.  I can
think of lots of examples where we *would* want headers to apply to
subresources:
 - Script files that get UpgradeInsecureRequests
 - Image files with LoFi headers

Maybe those don't use this extra_headers parameter?  Which ones do (and are
problematic to include on subresources)?

https://codereview.chromium.org/2429623002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2429623002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4260: request.getFrameType() !=
WebURLRequest::FrameTypeNone) {
nit: Please add a comment here, similar to the TODO you added above.

[Łukasz Anforowicz, 2016-10-17 23:47:56]

On 2016/10/17 22:46:18, Charlie Reis wrote:
> Hmm, I'm not actually clear on whether this is the right thing to do, but
that's
> because I don't know enough about what extra_headers is intended for.  I can
> think of lots of examples where we *would* want headers to apply to
> subresources:
>  - Script files that get UpgradeInsecureRequests
>  - Image files with LoFi headers

The two examples you gave are controlled by Blink - it applies these headers to
requests (navigational or subrequest) as needed (most importantly - without
paying attention to what came from the content layer in StartNavigationParams):

- UIR: FrameLoader::modifyRequestForCSP
- LoFi: header is set on the browser-side, based on what Blink puts into
WebURLRequest::getLoFiState()


> Maybe those don't use this extra_headers parameter?  Which ones do (and are
> problematic to include on subresources)?

OpenURL -> chrome::Navigate -> NavigationController::LoadWithParams is
problematic - OpenURLParams::extra_headers are meant only for the navigation
request (right?) and in case of a POST request can include Content-Type header
(which should only be used with POST requests and not with GET requests for
subresources).  Reattaching extra_headers to subresource requests breaks
flipkart (hmmm crbug is down... FWIW, I link to the flipkart bug from comment #0
of https://crbug.com/656179).


There is a risk, that some user of //content API was assuming the current
behavior (of applying extra_headers to all requests), but I am not sure how to
quantify this risk + the bots are green... I think that fixing the known issue
around OpenURL in general (all headers meant only for navigation) and
Content-Type in particular (meant only for POST navigation) is more important
than the unknown risk :-/.

https://codereview.chromium.org/2429623002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2429623002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4260: request.getFrameType() !=
WebURLRequest::FrameTypeNone) {
On 2016/10/17 22:46:18, Charlie Reis wrote:
> nit: Please add a comment here, similar to the TODO you added above.

Done.

[Łukasz Anforowicz, 2016-10-17 23:47:56]

On 2016/10/17 22:46:18, Charlie Reis wrote:
> Hmm, I'm not actually clear on whether this is the right thing to do, but
that's
> because I don't know enough about what extra_headers is intended for.  I can
> think of lots of examples where we *would* want headers to apply to
> subresources:
>  - Script files that get UpgradeInsecureRequests
>  - Image files with LoFi headers

The two examples you gave are controlled by Blink - it applies these headers to
requests (navigational or subrequest) as needed (most importantly - without
paying attention to what came from the content layer in StartNavigationParams):

- UIR: FrameLoader::modifyRequestForCSP
- LoFi: header is set on the browser-side, based on what Blink puts into
WebURLRequest::getLoFiState()


> Maybe those don't use this extra_headers parameter?  Which ones do (and are
> problematic to include on subresources)?

OpenURL -> chrome::Navigate -> NavigationController::LoadWithParams is
problematic - OpenURLParams::extra_headers are meant only for the navigation
request (right?) and in case of a POST request can include Content-Type header
(which should only be used with POST requests and not with GET requests for
subresources).  Reattaching extra_headers to subresource requests breaks
flipkart (hmmm crbug is down... FWIW, I link to the flipkart bug from comment #0
of https://crbug.com/656179).


There is a risk, that some user of //content API was assuming the current
behavior (of applying extra_headers to all requests), but I am not sure how to
quantify this risk + the bots are green... I think that fixing the known issue
around OpenURL in general (all headers meant only for navigation) and
Content-Type in particular (meant only for POST navigation) is more important
than the unknown risk :-/.

[Charlie Reis, 2016-10-17 23:58:16]

On 2016/10/17 23:47:56, Łukasz Anforowicz wrote:
> On 2016/10/17 22:46:18, Charlie Reis wrote:
> > Hmm, I'm not actually clear on whether this is the right thing to do, but
> that's
> > because I don't know enough about what extra_headers is intended for.  I can
> > think of lots of examples where we *would* want headers to apply to
> > subresources:
> >  - Script files that get UpgradeInsecureRequests
> >  - Image files with LoFi headers
> 
> The two examples you gave are controlled by Blink - it applies these headers
to
> requests (navigational or subrequest) as needed (most importantly - without
> paying attention to what came from the content layer in
StartNavigationParams):
> 
> - UIR: FrameLoader::modifyRequestForCSP
> - LoFi: header is set on the browser-side, based on what Blink puts into
> WebURLRequest::getLoFiState()
> 
> 
> > Maybe those don't use this extra_headers parameter?  Which ones do (and are
> > problematic to include on subresources)?
> 
> OpenURL -> chrome::Navigate -> NavigationController::LoadWithParams is
> problematic - OpenURLParams::extra_headers are meant only for the navigation
> request (right?) and in case of a POST request can include Content-Type header
> (which should only be used with POST requests and not with GET requests for
> subresources).  Reattaching extra_headers to subresource requests breaks
> flipkart (hmmm crbug is down... FWIW, I link to the flipkart bug from comment
#0
> of https://crbug.com/656179).
> 
> 
> There is a risk, that some user of //content API was assuming the current
> behavior (of applying extra_headers to all requests), but I am not sure how to
> quantify this risk + the bots are green... I think that fixing the known issue
> around OpenURL in general (all headers meant only for navigation) and
> Content-Type in particular (meant only for POST navigation) is more important
> than the unknown risk :-/.

I see-- thanks for clarifying on those examples.  I agree that including them on
subresources seems wrong when they're specified on a navigation data structure,
so hopefully it's the right direction.  LGTM.

[Łukasz Anforowicz, 2016-10-18 00:27:56]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 00:28:29]

Dry run: 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, 2016-10-18 01:56:17]

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

[commit-bot: I haz the power, 2016-10-18 01:56:18]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-10-18 14:06:37]

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

[Charlie Harrison, 2016-10-18 14:06:38]

LGTM! It would be great to split this entire method into two: one for navigation
requests and one for subresource requests. I tried to do this locally but it
ended up being rather ugly. I may try again in the near future though.

[Łukasz Anforowicz, 2016-10-18 15:38:19]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-10-18 15:38:54]

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, 2016-10-18 15:44:08]

Description was changed from

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-18 15:44:09]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-18 15:45:52]

Description was changed from

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
LoadURLParams::extra_headers should not apply to subresource requests.

Extra http request headers specified in
NavigationController::LoadURLParams should only apply to the *navigation*
request trigerred by NavigationController::LoadURLWithParams method and
should NOT apply to http requests for *subresources*.

BUG=656179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/4246472c00afcf4f7e7c55317d6988d158e83e11
Cr-Commit-Position: refs/heads/master@{#425972}
==========

[commit-bot: I haz the power, 2016-10-18 15:45:53]

Patchset 2 (id:??) landed as
https://crrev.com/4246472c00afcf4f7e7c55317d6988d158e83e11
Cr-Commit-Position: refs/heads/master@{#425972}