Preserving Content-Type header from http request in OpenURL path.

Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

Committed: https://crrev.com/f097ee58fe6bb4c329999f03253bc4849cc50302
Cr-Commit-Position: refs/heads/master@{#424588}

[Łukasz Anforowicz, 2016-09-20 18:42:44]

Description was changed from

==========
Preserving Content-Type header from http request in OpenURL path - tests.

BUG=648648
==========

to

==========
Preserving Content-Type header from http request in OpenURL path - tests.

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

[Łukasz Anforowicz, 2016-09-20 18:42:54]

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

[commit-bot: I haz the power, 2016-09-20 18:43:17]

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-09-20 18:44:58]

Description was changed from

==========
Preserving Content-Type header from http request in OpenURL path - tests.

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

to

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

[Łukasz Anforowicz, 2016-09-20 19:18:43]

Description was changed from

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

to

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

[commit-bot: I haz the power, 2016-09-20 19:27:48]

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

[commit-bot: I haz the power, 2016-09-20 19:27:49]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Łukasz Anforowicz, 2016-09-22 14:44:06]

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

[commit-bot: I haz the power, 2016-09-22 14:44:36]

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-09-22 14:47:17]

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

[commit-bot: I haz the power, 2016-09-22 14:47:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-09-22 16:18:54]

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

[commit-bot: I haz the power, 2016-09-22 16:19:10]

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-09-22 17:45:18]

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

[commit-bot: I haz the power, 2016-09-22 17:45:19]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Łukasz Anforowicz, 2016-09-22 21:23:15]

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

[Łukasz Anforowicz, 2016-09-22 21:23:17]

Charlie, can you please take a look?

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:493: std::string() /*
extra_headers */);
We are preserving the post body, because we know the renderer might want to
inspect it (i.e. in XSSAuditor).  We probably need to also preserve the extra
headers, because without Content-Type, the renderer might not be able to
properly inspect the post body.

I hope this can be done in another CL, because 1) current CL is sufficient for
fixing https://crbug.com/646261 (once we take care of CURRENT_TAB window
disposition) and 2) preserving extra headers requires more changes [i.e. we
preserve the post body via transfer_navigation_handle, as seen above]).

Hmmm... if the above is reasonable, then I probably should add a TODO here.
How does that sound?

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
content/child/web_url_request_util.cc:220:
flattener.FilterOutHeader("upgrade-insecure-requests");
The filtering is needed, so that
NavigationController::LoadURLParams::extra_headers is round-tripped into
OpenURLParams::extra_headers - we need to filter out headers that were added by
Blink.

Singling out the "referer" and "update-insecure-requests" headers seems rather
ad-hoc and fragile, but I don't see any way to deal with all possible headers in
a comprehensive way.  If there is indeed no comprehensive way / if we indeed
have to find and fix one header at a time, then I think the filtering approach
is as good as any other specific-headers-focused-approach.  In particular, I
don't think filtering is any worse than moving where the header is added (i.e.
whether the header is added by Blink before or after the construction of
FrameHostMsg_OpenURL_Params), especially since the timing of when the header is
added is sensitive to other requirements (e.g. coming from PlzNavigate, as
pointed out by carlosk@).

https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5910:
GetWebURLRequestHeadersForResourceRequest(info.urlRequest),
The change above preserves the old behavior for PlzNavigate, so I assume this is
the right to do (as opposed to filtering yet another set of http request
headers).

[Łukasz Anforowicz, 2016-09-27 18:58:36]

+tsepez@ as FYI for the XSSAuditor-related comment below.

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:493: std::string() /*
extra_headers */);
On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> We are preserving the post body, because we know the renderer might want to
> inspect it (i.e. in XSSAuditor).  We probably need to also preserve the extra
> headers, because without Content-Type, the renderer might not be able to
> properly inspect the post body.
> 
> I hope this can be done in another CL, because 1) current CL is sufficient for
> fixing https://crbug.com/646261 (once we take care of CURRENT_TAB window
> disposition) and 2) preserving extra headers requires more changes [i.e. we
> preserve the post body via transfer_navigation_handle, as seen above]).
> 
> Hmmm... if the above is reasonable, then I probably should add a TODO here.
> How does that sound?

It turns out that XSSAuditor works fine, even if we don't send the Content-Type
header here - XSSAuditor essentially ignores the Content-Type header. 
XSSAuditor can do this, because XSSAuditor looks for substrings in the whole
request body (i.e. without splitting it out into individual fields) - this will
work even if the body contains extra test coming from "multipart/form-data"
encoding.

In particular:

- Note how XSSAuditor calls httpBody->flattenToString() [1] which returns the
whole body as a string (omitting strings, only returning verbatim bytes included
in the body [2])

- Note how XSSAuditor searches [3] for substrings in the whole body.

[1]
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...

[2]
https://chromium.googlesource.com/chromium/src/+/009459a350906e2c49d20f4f7ee1...

[3]
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...

[Tom Sepez, 2016-09-27 19:59:09]

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

[Tom Sepez, 2016-09-27 19:59:10]

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:493: std::string() /*
extra_headers */);
That's correct.

[Łukasz Anforowicz, 2016-09-30 00:17:28]

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

[commit-bot: I haz the power, 2016-09-30 00:18:10]

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-09-30 01:40:11]

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

[commit-bot: I haz the power, 2016-09-30 01:40:12]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-30 21:31:55]

Thanks, and sorry for the delay.  I think the main question is around the
filtering and whether there's a more maintainable way to get the headers we
need.

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:493: std::string() /*
extra_headers */);
On 2016/09/27 18:58:36, Łukasz Anforowicz wrote:
> On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> > We are preserving the post body, because we know the renderer might want to
> > inspect it (i.e. in XSSAuditor).  We probably need to also preserve the
extra
> > headers, because without Content-Type, the renderer might not be able to
> > properly inspect the post body.
> > 
> > I hope this can be done in another CL, because 1) current CL is sufficient
for
> > fixing https://crbug.com/646261 (once we take care of CURRENT_TAB window
> > disposition) and 2) preserving extra headers requires more changes [i.e. we
> > preserve the post body via transfer_navigation_handle, as seen above]).
> > 
> > Hmmm... if the above is reasonable, then I probably should add a TODO here.
> > How does that sound?
> 
> It turns out that XSSAuditor works fine, even if we don't send the
Content-Type
> header here - XSSAuditor essentially ignores the Content-Type header. 
> XSSAuditor can do this, because XSSAuditor looks for substrings in the whole
> request body (i.e. without splitting it out into individual fields) - this
will
> work even if the body contains extra test coming from "multipart/form-data"
> encoding.
> 
> In particular:
> 
> - Note how XSSAuditor calls httpBody->flattenToString() [1] which returns the
> whole body as a string (omitting strings, only returning verbatim bytes
included
> in the body [2])
> 
> - Note how XSSAuditor searches [3] for substrings in the whole body.
> 
> [1]
>
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...
> 
> [2]
>
https://chromium.googlesource.com/chromium/src/+/009459a350906e2c49d20f4f7ee1...
> 
> [3]
>
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...

So, do you still intend to pass headers here, or it is safe not to?  I feel like
we should document it either way.

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
content/child/web_url_request_util.cc:220:
flattener.FilterOutHeader("upgrade-insecure-requests");
On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> The filtering is needed, so that
> NavigationController::LoadURLParams::extra_headers is round-tripped into
> OpenURLParams::extra_headers - we need to filter out headers that were added
by
> Blink.
> 
> Singling out the "referer" and "update-insecure-requests" headers seems rather
> ad-hoc and fragile, but I don't see any way to deal with all possible headers
in
> a comprehensive way.  If there is indeed no comprehensive way / if we indeed
> have to find and fix one header at a time, then I think the filtering approach
> is as good as any other specific-headers-focused-approach.  In particular, I
> don't think filtering is any worse than moving where the header is added (i.e.
> whether the header is added by Blink before or after the construction of
> FrameHostMsg_OpenURL_Params), especially since the timing of when the header
is
> added is sensitive to other requirements (e.g. coming from PlzNavigate, as
> pointed out by carlosk@).

Still, it seems hard to maintain this if Blink adds more headers in the future. 
No one will think to add them to this filter.

I'm not entirely clear on which headers you need.  If it's just the ones sent by
LoadURLParams (presumably stored in StartNavigationParams?), can we get to them
via NavigationState?  (And does none of this apply for PlzNavigate, which
doesn't use StartNavigationParams or make requests from the renderer?)

https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:5910:
GetWebURLRequestHeadersForResourceRequest(info.urlRequest),
On 2016/09/22 21:23:17, Łukasz Anforowicz wrote:
> The change above preserves the old behavior for PlzNavigate, so I assume this
is
> the right to do (as opposed to filtering yet another set of http request
> headers).

Acknowledged.

https://codereview.chromium.org/2355023002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2355023002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator.h:139: const std::string& extra_headers) {}
Is this only for the RenderFrameProxyHost case?  Maybe we should document that
if so, since we don't appear to be passing anything during a real transfer
(which happens after the request has been made).

(I suppose this depends on the other discussion in
RenderFrameHostManager::OnCrossSiteResponse.)

https://codereview.chromium.org/2355023002/diff/80001/content/child/web_url_r...
File content/child/web_url_request_util.h (right):

https://codereview.chromium.org/2355023002/diff/80001/content/child/web_url_r...
content/child/web_url_request_util.h:29: std::string
GetWebURLRequestHeadersForResourceRequest(
Please document why these are different.

[Łukasz Anforowicz, 2016-09-30 23:12:19]

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

[commit-bot: I haz the power, 2016-09-30 23:12:49]

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-09-30 23:17:00]

Thanks Charlie.  Please take a look at the changes in the latest patchset and my
replies below.


I am also not quite happy with the hardcoded list of headers to filter out
before sending them over IPC to the browser. 

- One  alternative approach would be to filter the headers in prerender code
(this is the place that should know whether it is safe to reuse old content if
the only difference is headers X, Y, Z).  WDYT about this option?

- I am not sure what other options are there (beside the renderer-side filtering
from the CL).  Let's chat on Monday to figure this out?

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:493: std::string() /*
extra_headers */);
On 2016/09/30 21:31:55, Charlie Reis wrote:
> On 2016/09/27 18:58:36, Łukasz Anforowicz wrote:
> > On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> > > We are preserving the post body, because we know the renderer might want
to
> > > inspect it (i.e. in XSSAuditor).  We probably need to also preserve the
> extra
> > > headers, because without Content-Type, the renderer might not be able to
> > > properly inspect the post body.
> > > 
> > > I hope this can be done in another CL, because 1) current CL is sufficient
> for
> > > fixing https://crbug.com/646261 (once we take care of CURRENT_TAB window
> > > disposition) and 2) preserving extra headers requires more changes [i.e.
we
> > > preserve the post body via transfer_navigation_handle, as seen above]).
> > > 
> > > Hmmm... if the above is reasonable, then I probably should add a TODO
here.
> > > How does that sound?
> > 
> > It turns out that XSSAuditor works fine, even if we don't send the
> Content-Type
> > header here - XSSAuditor essentially ignores the Content-Type header. 
> > XSSAuditor can do this, because XSSAuditor looks for substrings in the whole
> > request body (i.e. without splitting it out into individual fields) - this
> will
> > work even if the body contains extra test coming from "multipart/form-data"
> > encoding.
> > 
> > In particular:
> > 
> > - Note how XSSAuditor calls httpBody->flattenToString() [1] which returns
the
> > whole body as a string (omitting strings, only returning verbatim bytes
> included
> > in the body [2])
> > 
> > - Note how XSSAuditor searches [3] for substrings in the whole body.
> > 
> > [1]
> >
>
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...
> > 
> > [2]
> >
>
https://chromium.googlesource.com/chromium/src/+/009459a350906e2c49d20f4f7ee1...
> > 
> > [3]
> >
>
https://chromium.googlesource.com/chromium/src/+/239fdac4d03219287e6f8aef8e7f...
> 
> So, do you still intend to pass headers here, or it is safe not to?  I feel
like
> we should document it either way.

I think it is safe to not pass the headers (this has been confirmed for
XSSAuditor by tsepez@;  I am not aware of other renderer-side code that might
need to inspect headers of the original request *after* a transfer has been
made).

I've added a comment here.

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
content/child/web_url_request_util.cc:220:
flattener.FilterOutHeader("upgrade-insecure-requests");
On 2016/09/30 21:31:55, Charlie Reis wrote:
> On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> > The filtering is needed, so that
> > NavigationController::LoadURLParams::extra_headers is round-tripped into
> > OpenURLParams::extra_headers - we need to filter out headers that were added
> by
> > Blink.
> > 
> > Singling out the "referer" and "update-insecure-requests" headers seems
rather
> > ad-hoc and fragile, but I don't see any way to deal with all possible
headers
> in
> > a comprehensive way.  If there is indeed no comprehensive way / if we indeed
> > have to find and fix one header at a time, then I think the filtering
approach
> > is as good as any other specific-headers-focused-approach.  In particular, I
> > don't think filtering is any worse than moving where the header is added
(i.e.
> > whether the header is added by Blink before or after the construction of
> > FrameHostMsg_OpenURL_Params), especially since the timing of when the header
> is
> > added is sensitive to other requirements (e.g. coming from PlzNavigate, as
> > pointed out by carlosk@).
> 
> Still, it seems hard to maintain this if Blink adds more headers in the
future. 
> No one will think to add them to this filter.

Ack.
> 
> I'm not entirely clear on which headers you need.

There are 2 conflicting requirements:

Req#1: We don't want OpenURL IPC to loose any headers that have been added by
Blink (e.g. Content-Type header).  From that perspective, we want to send *all*
the headers via OpenURL IPC.

Req#2: We don't want headers which Blink *always* adds, because

1) this is wasteful (after OpenURL roundtrips via //chrome back to //content and
Blink the headers will be readded anyway)

2) it confuses prerender - it A) passes extra_headers=X (empty today) to
NavigationController::LoadURLWithParams and B) wants to compare them with
OpenURLParams.extra_headers from another navigation (to see if it can reuse web
contents created in (A)).

I don't know how to reconcile those 2 requirements without hardcoding affected
headers *somewhere* (if not above, then in prerender?  or in overall Blink code
- by tweaking *when* Blink adds headers?  bleh...).

> If it's just the ones sent by
> LoadURLParams (presumably stored in StartNavigationParams?), can we get to
them
> via NavigationState?

I don't understand what you mean by "NavigationState" :-( 
content::NavigationState doesn't look very interesting (i.e. doesn't seem to
contain http request headers).  There is content::NavigationStateImpl, but it
seems to only be used for RenderFrameImpl (and not for RemoteFrameProxy which is
the object that before this CL would send an incomplete OpenURL IPC to the
browser).

RE: presumably stored in StartNavigationParams

Right - when a renderer submits a form that targets a remote frame, we want to
send to the browser (in OpenURL IPC) the extra headers that this renderer has
put into WebURLRequest.  The browser will send those extra headers to the other
renderer (where the remote frame lives) via
StartNavigationParams::extra_headers.

> (And does none of this apply for PlzNavigate, which
> doesn't use StartNavigationParams or make requests from the renderer?)

Hmmm... it does apply to PlzNavigate - it has BeginNavigationParams::headers.  I
see that it gets populated in RenderFrameImpl::BeginNavigation right before it
sends a FrameHostMsg_BeginNavigation message to the browser.  FWIW, it uses
GetWebURLRequestHeadersForResourceRequest - it probably should use
GetWebURLRequestHeadersForOpenURLParams instead (so my CR comment is wrong in
https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...).

https://codereview.chromium.org/2355023002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/2355023002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator.h:139: const std::string& extra_headers) {}
On 2016/09/30 21:31:55, Charlie Reis wrote:
> Is this only for the RenderFrameProxyHost case?  Maybe we should document that
> if so, since we don't appear to be passing anything during a real transfer
> (which happens after the request has been made).
> 
> (I suppose this depends on the other discussion in
> RenderFrameHostManager::OnCrossSiteResponse.)

Good point.  I haven't considered this angle.  FWIW, I've added a TODO comment
in RenderFrameProxyHost::OnOpenURL, next to the RequestTransferURL call that we
want to replace with RequestOpenURL call in the long-term.

https://codereview.chromium.org/2355023002/diff/80001/content/child/web_url_r...
File content/child/web_url_request_util.h (right):

https://codereview.chromium.org/2355023002/diff/80001/content/child/web_url_r...
content/child/web_url_request_util.h:29: std::string
GetWebURLRequestHeadersForResourceRequest(
On 2016/09/30 21:31:55, Charlie Reis wrote:
> Please document why these are different.

Done.  I should have done that before.  Doing this also helped me articulate the
difference - maybe the comment I've added above helps explain (and justify) the
filtering done here (and the fact that there are 2 separate filter lists).

[Charlie Reis, 2016-09-30 23:44:44]

On 2016/09/30 23:17:00, Łukasz Anforowicz wrote:
> Thanks Charlie.  Please take a look at the changes in the latest patchset and
my
> replies below.
> 
> 
> I am also not quite happy with the hardcoded list of headers to filter out
> before sending them over IPC to the browser. 
> 
> - One  alternative approach would be to filter the headers in prerender code
> (this is the place that should know whether it is safe to reuse old content if
> the only difference is headers X, Y, Z).  WDYT about this option?

After hearing more of your explanation, I'm thinking this might possibly be
preferable.  It sounds like most of the places in the code want to see all the
headers (except Referer), and it's just prerendering that gets confused if
Blink's other standard headers are present.  Putting the logic to deal with that
near prerender seems to make sense, on the one hand.

On the other hand, that would be even harder to maintain-- no one will think to
go update prerendering code when adding a new header, and I'm inclined to think
there's already more headers that would trip up prerendering today in certain
modes (e.g., LoFi headers when that's enabled?).  We could hope that prerender
will go away, but maybe there's a way to make it more robust to this problem?

> 
> - I am not sure what other options are there (beside the renderer-side
filtering
> from the CL).  Let's chat on Monday to figure this out?

Sure.  Talking it out might help.

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/2355023002/diff/60001/content/child/web_url_r...
content/child/web_url_request_util.cc:220:
flattener.FilterOutHeader("upgrade-insecure-requests");
On 2016/09/30 23:17:00, Łukasz Anforowicz wrote:
> On 2016/09/30 21:31:55, Charlie Reis wrote:
> > On 2016/09/22 21:23:16, Łukasz Anforowicz wrote:
> > > The filtering is needed, so that
> > > NavigationController::LoadURLParams::extra_headers is round-tripped into
> > > OpenURLParams::extra_headers - we need to filter out headers that were
added
> > by
> > > Blink.
> > > 
> > > Singling out the "referer" and "update-insecure-requests" headers seems
> rather
> > > ad-hoc and fragile, but I don't see any way to deal with all possible
> headers
> > in
> > > a comprehensive way.  If there is indeed no comprehensive way / if we
indeed
> > > have to find and fix one header at a time, then I think the filtering
> approach
> > > is as good as any other specific-headers-focused-approach.  In particular,
I
> > > don't think filtering is any worse than moving where the header is added
> (i.e.
> > > whether the header is added by Blink before or after the construction of
> > > FrameHostMsg_OpenURL_Params), especially since the timing of when the
header
> > is
> > > added is sensitive to other requirements (e.g. coming from PlzNavigate, as
> > > pointed out by carlosk@).
> > 
> > Still, it seems hard to maintain this if Blink adds more headers in the
> future. 
> > No one will think to add them to this filter.
> 
> Ack.
> > 
> > I'm not entirely clear on which headers you need.
> 
> There are 2 conflicting requirements:
> 
> Req#1: We don't want OpenURL IPC to loose any headers that have been added by
> Blink (e.g. Content-Type header).  From that perspective, we want to send
*all*
> the headers via OpenURL IPC.
> 
> Req#2: We don't want headers which Blink *always* adds, because
> 
> 1) this is wasteful (after OpenURL roundtrips via //chrome back to //content
and
> Blink the headers will be readded anyway)
> 
> 2) it confuses prerender - it A) passes extra_headers=X (empty today) to
> NavigationController::LoadURLWithParams and B) wants to compare them with
> OpenURLParams.extra_headers from another navigation (to see if it can reuse
web
> contents created in (A)).
> 
> I don't know how to reconcile those 2 requirements without hardcoding affected
> headers *somewhere* (if not above, then in prerender?  or in overall Blink
code
> - by tweaking *when* Blink adds headers?  bleh...).

Yeah, I see how it's a bit awkward regardless of the approach we choose.

> 
> > If it's just the ones sent by
> > LoadURLParams (presumably stored in StartNavigationParams?), can we get to
> them
> > via NavigationState?
> 
> I don't understand what you mean by "NavigationState" :-( 
> content::NavigationState doesn't look very interesting (i.e. doesn't seem to
> contain http request headers).  There is content::NavigationStateImpl, but it
> seems to only be used for RenderFrameImpl (and not for RemoteFrameProxy which
is
> the object that before this CL would send an incomplete OpenURL IPC to the
> browser).

Yes, I meant NavigationStateImpl on RenderFrameImpl, but it's only present for
navigations that come down from the browser process.  I was thinking about your
LoadURLParams comment the wrong way-- that's after the OpenURL IPC is sent up to
the browser, and we care about grabbing the headers before.  Nevermind.

> 
> RE: presumably stored in StartNavigationParams
> 
> Right - when a renderer submits a form that targets a remote frame, we want to
> send to the browser (in OpenURL IPC) the extra headers that this renderer has
> put into WebURLRequest.  The browser will send those extra headers to the
other
> renderer (where the remote frame lives) via
> StartNavigationParams::extra_headers.
> 
> > (And does none of this apply for PlzNavigate, which
> > doesn't use StartNavigationParams or make requests from the renderer?)
> 
> Hmmm... it does apply to PlzNavigate - it has BeginNavigationParams::headers. 
I
> see that it gets populated in RenderFrameImpl::BeginNavigation right before it
> sends a FrameHostMsg_BeginNavigation message to the browser.  FWIW, it uses
> GetWebURLRequestHeadersForResourceRequest - it probably should use
> GetWebURLRequestHeadersForOpenURLParams instead (so my CR comment is wrong in
>
https://codereview.chromium.org/2355023002/diff/60001/content/renderer/render...).

[Łukasz Anforowicz, 2016-10-01 00:10:31]

On 2016/09/30 23:44:44, Charlie Reis wrote:
> On 2016/09/30 23:17:00, Łukasz Anforowicz wrote:
> > Thanks Charlie.  Please take a look at the changes in the latest patchset
and
> my
> > replies below.
> > 
> > 
> > I am also not quite happy with the hardcoded list of headers to filter out
> > before sending them over IPC to the browser. 
> > 
> > - One  alternative approach would be to filter the headers in prerender code
> > (this is the place that should know whether it is safe to reuse old content
if
> > the only difference is headers X, Y, Z).  WDYT about this option?
> 
> After hearing more of your explanation, I'm thinking this might possibly be
> preferable.  It sounds like most of the places in the code want to see all the
> headers (except Referer), and it's just prerendering that gets confused if
> Blink's other standard headers are present.  Putting the logic to deal with
that
> near prerender seems to make sense, on the one hand.

The weird thing is that prerender would need to filter NOT because it
doesn't care about Upgrade-Insecure-Requests request header.  Prerender
DOES care about this header (because the server might give a different
reply depending on whether the browser can automatically upgrade mixed
http/https content or not).  Prerender would need to filter, because it
knows^H^H^Hpretends to know that Blink will always add this header (when
hypothetically this or some other header might be added to some, but not
all requests [now or after a future code change]).
 
> On the other hand, that would be even harder to maintain-- no one will think
to
> go update prerendering code when adding a new header, and I'm inclined to
think
> there's already more headers that would trip up prerendering today in certain
> modes (e.g., LoFi headers when that's enabled?).  We could hope that prerender
> will go away, but maybe there's a way to make it more robust to this problem?

Well, maybe this is not that bad:

- Prerender was always broken in this regard (i.e. we never sent request
  headers via OpenURL IPC, so the prerender code check that no extra
  headers are present only ever worked for browser initiated requests).

- The fallback behavior (throwing away prerendered content when [new]
  unrecognized extra headers are present) seems safe (although a bit
  wasteful).

- Finally - I *did* notice that I need to update prerender code, because
  prerender tests started failing.  So maybe we would also notice for
  new headers in the future.  OTOH, in my case I have a header that is
  in practice always present in all navigation requests.  For a header
  that appears rarely, it might be indeed easy to miss updating the
  filters list (regardless of whether they are in prerender or in
  content/renderer's navigation code :->).

[commit-bot: I haz the power, 2016-10-01 00:15:00]

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

[commit-bot: I haz the power, 2016-10-01 00:15:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Łukasz Anforowicz, 2016-10-03 23:27:28]

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

[commit-bot: I haz the power, 2016-10-03 23:28:00]

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-04 01:49:52]

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

[commit-bot: I haz the power, 2016-10-04 01:49:53]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-04 14:09:49]

Charlie, in the latest patchset I moved header filtering to
prerender_manager.cc, to see how this would look like.

I guess before this CL can land we need to:

- Chat if the ad-hoc filtering of headers can be replaced with something else
(something more likely to cover all possible headers that Blink might be
adding).

- If the ad-hoc filtering is inevitable, then whether to do it in predendering
code vs in the renderer (I think doing this in the prerendering code is okay and
might be preferrable because we are trying to eventually remove prerendering
from Chrome;  OTOH it feels wrong that headers do not round-trip between
chrome::NavigateParams and content::OpenURLParams - this feels like something
that might in the future affect something other than prerendering).

- Discuss the risk of this CL to prerender - before this CL prerender didn't see
extra_headers at all (because they would be dropped by the OpenURL IPC).  After
this CL, prerender continues to work even if Upgrade-Insecure-Requests header is
present.  OTOH, there is a risk that there might be other headers that
prerendering doesn't know about, but that after this CL will block reusing of
prerendered contents.  I guess one way I could help here is to try to compile a
list of headers that Blink can add today (with no guarantees that this list
won't change tomorrow :-).

[Charlie Reis, 2016-10-06 17:39:51]

I like this new approach, and I agree with your reasoning that prerendering is a
good place to track which headers should and should not cause a prerender
discard.  (The list in https://crbug.com/648648#c12 is also useful.)  While no
one is really maintaining that and newly added headers might cause prerenders to
fail, I would expect tests or metrics to catch that.  (Plus we do hope that
prerendering is going away.)

LGTM.

Sanity check: I don't see try jobs for the new layout tests.  Do you want to run
some before it lands, so that we don't have to revert if they fail?

[Łukasz Anforowicz, 2016-10-06 18:07:30]

Thanks Charlie.

On 2016/10/06 17:39:51, Charlie Reis wrote:
> I like this new approach, and I agree with your reasoning that prerendering is
a
> good place to track which headers should and should not cause a prerender
> discard.  (The list in https://crbug.com/648648#c12 is also useful.)  While no
> one is really maintaining that and newly added headers might cause prerenders
to
> fail, I would expect tests or metrics to catch that.  (Plus we do hope that
> prerendering is going away.)
> 
> LGTM.
> 
> Sanity check: I don't see try jobs for the new layout tests.  Do you want to
run
> some before it lands, so that we don't have to revert if they fail?

Ok - I've kicked off a few randomly-picked tryjobs (one for each of
win/mac/linux, some rel, some dbg).

[Łukasz Anforowicz, 2016-10-06 18:09:22]

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

[Łukasz Anforowicz, 2016-10-06 18:09:23]

Nasko, can you do an IPC review please? (for content/common/frame_messages.h)

[nasko, 2016-10-06 18:15:01]

https://codereview.chromium.org/2355023002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2355023002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:463: IPC_STRUCT_MEMBER(std::string,
extra_headers)
You've put this member after post body in all areas of the code, why not keep
the ordering here?

https://codereview.chromium.org/2355023002/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2355023002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.h:881: const std::string& extra_headers,
Why not keep this in the same order as the struct members?

[Tom Sepez, 2016-10-06 18:27:26]

Deferring to Nasko.

[Tom Sepez, 2016-10-06 18:27:43]

tsepez@chromium.org changed reviewers:
- tsepez@chromium.org

[Łukasz Anforowicz, 2016-10-06 18:33:10]

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

[Łukasz Anforowicz, 2016-10-06 18:33:54]

Nasko, can you take another look?

https://codereview.chromium.org/2355023002/diff/120001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2355023002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:463: IPC_STRUCT_MEMBER(std::string,
extra_headers)
On 2016/10/06 18:15:01, nasko (slow) wrote:
> You've put this member after post body in all areas of the code, why not keep
> the ordering here?

Good point - I should have kept the order similar to the order of fields inside
content::OpenURLParams.  Done.

https://codereview.chromium.org/2355023002/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2355023002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.h:881: const std::string& extra_headers,
On 2016/10/06 18:15:01, nasko (slow) wrote:
> Why not keep this in the same order as the struct members?

The parameters should probably be in the same order as the order of fields in
content::OpenURLParams (so |extra_headers| follows |resource_request_body| and
|uses_post|).

FWIW, in the latest patchset I've tweaked the order of fields in
FrameHostMsg_OpenURL_Params, so that it matches (matches the order of method
parameters here + the order of fields in content::OpenURLParams struct).

Given above, there is no need to change things here, right?

[commit-bot: I haz the power, 2016-10-06 18:34:00]

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

[nasko, 2016-10-06 18:38:59]

LGTM

[Łukasz Anforowicz, 2016-10-06 18:45:08]

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

[Łukasz Anforowicz, 2016-10-06 18:45:08]

mmenke@, could you PTAL at chrome/browser/prerender/prerender_manager.cc?

Some discussion leaning toward filtering/ignoring some headers in
prerender_manager.cc was captured in https://crbug.com/648648#c12 and
https://crrev.com/2355023002/#msg38 and https://crrev.com/2355023002/#msg37

PS. creis@ - the win7_blink_rel and linux_precise_blink_rel  blink tryjobs
passed in https://codereview.chromium.org/2355023002/#ps120001.  I am still
waiting for mac10.9_blink_dbg to come back, but things are looking okay so far.

[commit-bot: I haz the power, 2016-10-06 20:06:43]

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

[commit-bot: I haz the power, 2016-10-06 20:06:43]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-06 20:52:03]

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:90: // |extra_headers| contains
"Upgrade-Insecure-Requests" header.
Would it make more sense to just add "Upgrade-Insecure-Requests" in
ResourceDispatcherHost instead?  A bit less IPC overhead, less redundant memory
use, etc.  Not expecting big savings, just wondiner if there's a reason for the
current architecture (And not a big fan of having magic here)

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:104: }
Also, should we have a prerender browser test for this?  I don't think the
layout test you added tests this code?

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:332: return false;
nit:  Add braces when if condition takes up more than 1 line.

[Łukasz Anforowicz, 2016-10-06 21:23:34]

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:90: // |extra_headers| contains
"Upgrade-Insecure-Requests" header.
On 2016/10/06 20:52:02, mmenke wrote:
> Would it make more sense to just add "Upgrade-Insecure-Requests" in
> ResourceDispatcherHost instead?  A bit less IPC overhead, less redundant
memory
> use, etc.  Not expecting big savings, just wondiner if there's a reason for
the
> current architecture (And not a big fan of having magic here)

Okay - let me try to see how this works out.  I gave a heads-up to mkwst@ +
shared some notes at https://crbug.com/648648#c13

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:104: }
On 2016/10/06 20:52:02, mmenke wrote:
> Also, should we have a prerender browser test for this?  I don't think the
> layout test you added tests this code?

The existing prerender browser tests already exercise this code - they would
have failed without ignoring this header - see
https://crrev.com/2355023002/#ps20001 and the discussion started in
https://crbug.com/648648#c3

https://codereview.chromium.org/2355023002/diff/140001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:332: return false;
On 2016/10/06 20:52:02, mmenke wrote:
> nit:  Add braces when if condition takes up more than 1 line.

Done.

[Łukasz Anforowicz, 2016-10-08 00:05:28]

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

[commit-bot: I haz the power, 2016-10-08 00:05:48]

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-08 01:34:26]

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

[commit-bot: I haz the power, 2016-10-08 01:34:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Łukasz Anforowicz, 2016-10-11 20:37:46]

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

[commit-bot: I haz the power, 2016-10-11 20:38:31]

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

[mmenke, 2016-10-11 20:46:33]

LGTM

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:100: return false;
add braces.

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:101: }
optional:  Could just do:

extra_headers.RemoveHeader("upgrade-insecure-requests");
return !extra_headers.IsEmpty();

(Doesn't work with multiple "upgrade-insecure-requests" headers, but I don't
think we care?)

[Łukasz Anforowicz, 2016-10-11 21:29:52]

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

[commit-bot: I haz the power, 2016-10-11 21:30:33]

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-11 21:33:43]

Thank you for reviewing.

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:100: return false;
On 2016/10/11 20:46:33, mmenke wrote:
> add braces.

Not applicable after applying your other suggestion.

https://codereview.chromium.org/2355023002/diff/220001/chrome/browser/prerend...
chrome/browser/prerender/prerender_manager.cc:101: }
On 2016/10/11 20:46:33, mmenke wrote:
> optional:  Could just do:
> 
> extra_headers.RemoveHeader("upgrade-insecure-requests");
> return !extra_headers.IsEmpty();
> 
> (Doesn't work with multiple "upgrade-insecure-requests" headers, but I don't
> think we care?)

Good point.  Done.

[commit-bot: I haz the power, 2016-10-11 23:01:16]

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

[commit-bot: I haz the power, 2016-10-11 23:01:17]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-10-11 23:09:18]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-10-11 23:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
nasko@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2355023002/#ps240001
(title: "Simplified AreExtraHeadersCompatibleWithPrerenderContents as suggested
by mmenke@.")

[commit-bot: I haz the power, 2016-10-11 23:10:06]

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-11 23:18:18]

Description was changed from

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

to

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

[commit-bot: I haz the power, 2016-10-11 23:18:19]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-11 23:20:58]

Description was changed from

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

to

==========
Preserving extra http request headers in OpenURL navigation path.

This CL makes sure that extra http request headers (e.g. in case of HTTP
POST, the Content-Type: multipart/form-data; boundary=... header) are
preserved when navigation uses the "OpenURL" code path.

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

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

[commit-bot: I haz the power, 2016-10-11 23:20:59]

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

[Łukasz Anforowicz, 2016-10-14 15:22:38]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2419093002/ by lukasza@chromium.org.

The reason for reverting is: This CL caused a regression -
https://crbug.com/655568..