Forwarding POST body into renderer after a cross-site transfer.

Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211, 613004
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5aa2c3740801f1c148c85db6612c24be0a76b6fe
Cr-Commit-Position: refs/heads/master@{#397779}

[Łukasz Anforowicz, 2016-05-10 00:45:55]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

BUG=582211
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-05-10 00:49:37]

https://codereview.chromium.org/1956383003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigator_impl.cc:819: // Option #2: Add the body to
either FrameNav...Entry or NavigationEntryImpl.
I haven't quite wrapped my head yet around option #1 - this option seems most
compatible with clamy@'s PlzNavigate CL (where post data is being extracted from
PageState), but I don't know how to construct or mutate a PageState in order to
populate its POST body.

https://codereview.chromium.org/1956383003/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1956383003/diff/1/content/browser/loader/reso...
content/browser/loader/resource_request_info_impl.h:16: #include
"content/common/resource_request_body.h"
It turns out that this violates DEPS, because of "-content" from
content/browser/loader's specific_include_rules. :-(  OTOH, this seems
accidental (due to mojo refactoring) and maybe we can get away with tweaking the
DEPS rules.

[Łukasz Anforowicz, 2016-05-12 03:05:40]

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

[Łukasz Anforowicz, 2016-05-12 03:05:41]

Camille, could you please take a quick look at this CL?  This CL is very far
from being landable (right now it compiles, but I haven't done any testing +
there are quite a few TODOs and DO-NOT-SUBMITs left), but:

1. I wanted to check with you if the following high-level parts of the CL look
reasonable (or are at least going in the right direction):

1.1. Putting ResourceRequestBody inside NavigationHandle:
    1.1.1. Populating NavigationHandle::resource_request_body_ in callbacks
inside navigation_resource_throttle.cc
    1.1.2. Plumbing ResourceRequestBody via ResourceRequestInfoImpl::body_

1.2. Changing std::vector<unsigned char> browser_initiated_post_data into
scoped_refptr<ResourceRequestBody> post_data (in StartNavigationParams).

2. I wanted to ask for advise on how to populate
StartNavigationParams::post_data.  I could populate it from
NavigationHandle::resource_request_body(), but it feels wrong - I feel that I
should instead be modifying FrameNavigationEntry.  Would you agree with that? 
In particular - would it make sense to try to create a PageState out of
ResourceRequestBody and put it into the new FrameNavigationEntry conjured by
NavigatorImpl::RequestTransferURL [1]?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[clamy, 2016-05-12 05:31:18]

On 2016/05/12 03:05:41, Łukasz Anforowicz wrote:
> Camille, could you please take a quick look at this CL?  This CL is very far
> from being landable (right now it compiles, but I haven't done any testing +
> there are quite a few TODOs and DO-NOT-SUBMITs left), but:
> 
> 1. I wanted to check with you if the following high-level parts of the CL look
> reasonable (or are at least going in the right direction):
> 
> 1.1. Putting ResourceRequestBody inside NavigationHandle:
>     1.1.1. Populating NavigationHandle::resource_request_body_ in callbacks
> inside navigation_resource_throttle.cc

That part looks good.

>     1.1.2. Plumbing ResourceRequestBody via ResourceRequestInfoImpl::body_

I'm not very familiar with that code unfortunately.
> 
> 1.2. Changing std::vector<unsigned char> browser_initiated_post_data into
> scoped_refptr<ResourceRequestBody> post_data (in StartNavigationParams).
> 
> 2. I wanted to ask for advise on how to populate
> StartNavigationParams::post_data.  I could populate it from
> NavigationHandle::resource_request_body(), but it feels wrong - I feel that I
> should instead be modifying FrameNavigationEntry.  Would you agree with that? 
> In particular - would it make sense to try to create a PageState out of
> ResourceRequestBody and put it into the new FrameNavigationEntry conjured by
> NavigatorImpl::RequestTransferURL [1]?

As I explained in comments, generating a PageState would mess up the way we
identify history navigations. Note: we could change that, but it requires more
work.

Storing the ResourceReqestBody in the FrameNavigationEntry is a bit of a
performance issue in history navigations, since we end up storing the same info
twice.

I don't think that getting it from the NavigationHandle _during transfer
navigations_ is problematic.

> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[clamy, 2016-05-12 05:32:48]

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:382:
*dest_render_frame_host->navigation_handle()),
This is unsafe. In non transfer cases the dest RFH does not have a
NavigationHandle at that point.

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:779: bool is_post =
static_cast<bool>(
Suggestion: add the method as a parameter of RequestTransferURL. The caller,
RFHM::OnCrossSiteResponse has access to the NavigationHandle which stores it.

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:789: // (using
NavigationHandleImpl::resource_request_body() I've introduced)
If you populate the PageState the RenderFrame will interpret it as a history
navigation. We check whether the PageState is valid at the beginning of
RenderFrameImpl::NavigateInternal to determine if this is a history navigation
or not. This is not the behavior we want.

https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
content/common/navigation_params.h:191: scoped_refptr<ResourceRequestBody>
post_data;
Note: following my CL, this may end up in CommonNavigationParams. In any case,
since it will be used both by vanilla navigations and PlzNavigate, it shouldn't
be in the vanilla specific params.

Also, I'm a bit worried about replacing browser_initiated_post_data. If you do
that, you should properly set it up when creating the StartNavigationParams.

[Łukasz Anforowicz, 2016-05-17 00:59:43]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  StartNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-05-17 23:52:56]

Camille, could you please take another look?  I think the remaining potential
issues are:

- If it is ok that I change 
    std::vector<unsigned char> browser_initiated_post_data
  into
    scoped_refptr<ResourceRequestBody> post_data
  in StartNavigationParams struct

- If the conversion routine in render_frame_impl.cc needs more work and/or if we
need to consolidate the conversion code between WebHTTPBody and
ExplodedHttpBodyElement -vs- ResourceRequestBody::Element.

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:382:
*dest_render_frame_host->navigation_handle()),
On 2016/05/12 05:32:48, clamy wrote:
> This is unsafe. In non transfer cases the dest RFH does not have a
> NavigationHandle at that point.

Done.  In subsequent patchsets I've added |const NavigationHandleImpl*
transfer_navigation_handle| parameter to RequestTransferURL method.

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:779: bool is_post =
static_cast<bool>(
On 2016/05/12 05:32:48, clamy wrote:
> Suggestion: add the method as a parameter of RequestTransferURL. The caller,
> RFHM::OnCrossSiteResponse has access to the NavigationHandle which stores it.

I've ended up passing |const NavigationHandleImpl* transfer_navigation_handle|
as a parameter of RequestTransferURL method.  NavigationHandle lets me access
both 1) HTTP method and 2) POST body.

https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:789: // (using
NavigationHandleImpl::resource_request_body() I've introduced)
On 2016/05/12 05:32:48, clamy wrote:
> If you populate the PageState the RenderFrame will interpret it as a history
> navigation. We check whether the PageState is valid at the beginning of
> RenderFrameImpl::NavigateInternal to determine if this is a history navigation
> or not. This is not the behavior we want.

Acknowledged.

https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
content/common/navigation_params.h:191: scoped_refptr<ResourceRequestBody>
post_data;
On 2016/05/12 05:32:48, clamy wrote:
> Note: following my CL, this may end up in CommonNavigationParams. In any case,
> since it will be used both by vanilla navigations and PlzNavigate, it
shouldn't
> be in the vanilla specific params.

In your CL [1] this ends up in PlzNavigate-specific IPC message -
FrameMsg_CommitNavigation.  Please let me know if you'd like me to tweak this CL
and move |post_data| to another location (shared by PlzNavigate?).

[1] https://codereview.chromium.org/1907443006/patch/200001/210010
> 
> Also, I'm a bit worried about replacing browser_initiated_post_data. If you do
> that, you should properly set it up when creating the StartNavigationParams.

It seemed that ResourceRequestBody is a generalization of
browser_initiated_post_data.  In most recent patchsets it should be properly
initialized.

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

https://codereview.chromium.org/1956383003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:549: const
scoped_refptr<ResourceRequestBody>& body) {
This function is more-or-less copy&pasted from
https://codereview.chromium.org/1907443006/patch/200001/210015 CL by clamy@.

I am not sure if it matters which CL lands first.  I am also not sure if this
routine needs more work (FWIW, my CR comments have been sufficiently addressed
in the other CL in https://codereview.chromium.org/1907443006/#msg17).

BTW: I've put together a quick (and for now very dirty) CL that gets rid of
ExplodedHttpBodyElement as a separate type (i.e. makes it a type alias for
ResourceRequestBody::Element).  This might allow us to avoid having to have
separate conversion code.  I am trying to run try jobs at
https://codereview.chromium.org/1987053002.

[clamy, 2016-05-18 15:46:26]

On 2016/05/17 23:52:56, Łukasz Anforowicz wrote:
> Camille, could you please take another look?  I think the remaining potential
> issues are:

I didn't have time to do a full review today, will look at it tomorrow. For the
high-level questions, please find my answer below.

> 
> - If it is ok that I change 
>     std::vector<unsigned char> browser_initiated_post_data
>   into
>     scoped_refptr<ResourceRequestBody> post_data
>   in StartNavigationParams struct

I would imagine so. Right now, in PlzNavigate we already convert the
browser_initiated_post_data into a ResourceRequestBody, so if you go that way we
should probably unify the conversion somewhere (it happens inside the
NavigationRequest in PlzNavigate).

> 
> - If the conversion routine in render_frame_impl.cc needs more work and/or if
we
> need to consolidate the conversion code between WebHTTPBody and
> ExplodedHttpBodyElement -vs- ResourceRequestBody::Element.

I haven't looked deeply at the code yet, but I imagine it's the same problem as
my CL. I don't feel super happy about the conversion situation, but I'm not
quite sure what to do there. At least, we should add big warnings in the
comments, so that someone changing one will change the others. My other idea
would be to create one file with helper methods for all the conversions, so they
are all stored together and we can ensure that they don't go out of sync. It may
also make things easier for people to understand instead of having it sprinkled
over several different files.

> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/navigator_impl.cc (right):
> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:382:
> *dest_render_frame_host->navigation_handle()),
> On 2016/05/12 05:32:48, clamy wrote:
> > This is unsafe. In non transfer cases the dest RFH does not have a
> > NavigationHandle at that point.
> 
> Done.  In subsequent patchsets I've added |const NavigationHandleImpl*
> transfer_navigation_handle| parameter to RequestTransferURL method.
> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:779: bool is_post =
> static_cast<bool>(
> On 2016/05/12 05:32:48, clamy wrote:
> > Suggestion: add the method as a parameter of RequestTransferURL. The caller,
> > RFHM::OnCrossSiteResponse has access to the NavigationHandle which stores
it.
> 
> I've ended up passing |const NavigationHandleImpl* transfer_navigation_handle|
> as a parameter of RequestTransferURL method.  NavigationHandle lets me access
> both 1) HTTP method and 2) POST body.
> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigator_impl.cc:789: // (using
> NavigationHandleImpl::resource_request_body() I've introduced)
> On 2016/05/12 05:32:48, clamy wrote:
> > If you populate the PageState the RenderFrame will interpret it as a history
> > navigation. We check whether the PageState is valid at the beginning of
> > RenderFrameImpl::NavigateInternal to determine if this is a history
navigation
> > or not. This is not the behavior we want.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
> File content/common/navigation_params.h (right):
> 
>
https://codereview.chromium.org/1956383003/diff/20001/content/common/navigati...
> content/common/navigation_params.h:191: scoped_refptr<ResourceRequestBody>
> post_data;
> On 2016/05/12 05:32:48, clamy wrote:
> > Note: following my CL, this may end up in CommonNavigationParams. In any
case,
> > since it will be used both by vanilla navigations and PlzNavigate, it
> shouldn't
> > be in the vanilla specific params.
> 
> In your CL [1] this ends up in PlzNavigate-specific IPC message -
> FrameMsg_CommitNavigation.  Please let me know if you'd like me to tweak this
CL
> and move |post_data| to another location (shared by PlzNavigate?).
> 
> [1] https://codereview.chromium.org/1907443006/patch/200001/210010
> > 
> > Also, I'm a bit worried about replacing browser_initiated_post_data. If you
do
> > that, you should properly set it up when creating the StartNavigationParams.
> 
> It seemed that ResourceRequestBody is a generalization of
> browser_initiated_post_data.  In most recent patchsets it should be properly
> initialized.
> 
>
https://codereview.chromium.org/1956383003/diff/80001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1956383003/diff/80001/content/renderer/render...
> content/renderer/render_frame_impl.cc:549: const
> scoped_refptr<ResourceRequestBody>& body) {
> This function is more-or-less copy&pasted from
> https://codereview.chromium.org/1907443006/patch/200001/210015 CL by clamy@.
> 
> I am not sure if it matters which CL lands first.  I am also not sure if this
> routine needs more work (FWIW, my CR comments have been sufficiently addressed
> in the other CL in https://codereview.chromium.org/1907443006/#msg17).
> 
> BTW: I've put together a quick (and for now very dirty) CL that gets rid of
> ExplodedHttpBodyElement as a separate type (i.e. makes it a type alias for
> ResourceRequestBody::Element).  This might allow us to avoid having to have
> separate conversion code.  I am trying to run try jobs at
> https://codereview.chromium.org/1987053002.

[Łukasz Anforowicz, 2016-05-18 22:50:16]

On 2016/05/18 15:46:26, clamy wrote:
> On 2016/05/17 23:52:56, Łukasz Anforowicz wrote:
> > Camille, could you please take another look?  I think the remaining
potential
> > issues are:
> 
> I didn't have time to do a full review today, will look at it tomorrow. For
the
> high-level questions, please find my answer below.
> 
> > 
> > - If it is ok that I change 
> >     std::vector<unsigned char> browser_initiated_post_data
> >   into
> >     scoped_refptr<ResourceRequestBody> post_data
> >   in StartNavigationParams struct
> 
> I would imagine so. Right now, in PlzNavigate we already convert the
> browser_initiated_post_data into a ResourceRequestBody, so if you go that way
we
> should probably unify the conversion somewhere (it happens inside the
> NavigationRequest in PlzNavigate).

Thanks - I've extracted a NavigationEntryImpl::ConstructResourceRequestBody in
the latest patchset (and I am reusing it from both code locations).
> 
> > 
> > - If the conversion routine in render_frame_impl.cc needs more work and/or
if
> we
> > need to consolidate the conversion code between WebHTTPBody and
> > ExplodedHttpBodyElement -vs- ResourceRequestBody::Element.
> 
> I haven't looked deeply at the code yet, but I imagine it's the same problem
as
> my CL. I don't feel super happy about the conversion situation, but I'm not
> quite sure what to do there. At least, we should add big warnings in the
> comments, so that someone changing one will change the others. My other idea
> would be to create one file with helper methods for all the conversions, so
they
> are all stored together and we can ensure that they don't go out of sync. It
may
> also make things easier for people to understand instead of having it
sprinkled
> over several different files.

I am deduplicating in a *follow-up* CL:
https://codereview.chromium.org/1987053002/.  We could in theory land the
deduplication CL first, but I am not sure if this is necessary (plus landing it
afterwards highlights why the deduplication CL is desirable, without this it
looks like just shuffling code around with no purpose).

[clamy, 2016-05-19 16:08:01]

Thanks! A few more comments. On the deduplication side, I don't think it matters
much if the deduplication CL lands after this one. However we should probably
coordinate on whether your CL lands first or mine does (since that would change
things on the review).

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:163:
scoped_refptr<ResourceRequestBody> ConstructResourceRequestBody() const;
Could we make it clearer that this is creating the body based on browser
initiated post data, and not based on data from the PageState? The naming is not
super clear there.

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:91: request_body ? "POST" :
"GET", dest_url, dest_referrer,
This should be frame_entry.method().

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:379:
entry.ConstructCommonNavigationParams(frame_entry.method(), dest_url,
In the current code, the method is always GET for subframes. Using
frame_entry.method() outside of UseSubframeNavigationEntries mode may lead to
differences in behavior.

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:719: // is null, when we are called
from RenderFrameProxyHost::OnOpenURL).
I think it would be better to pass the post data and the method instead of the
handle itself. When we call RenderFrameProxyHost::OnOpenURL the handle should
always be null. However, we could pass method and the post data in
FrameHostMsg_OpenURL_Params, and then to this function.

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:255: // (and a non-null
NavigationHandle?).
See my comment in RequestTransferURL in favor of not passing the
NavigationHandle but the post data and method directly. We should not
instantiate a NavigationHandle here (there are some timing issues with doing
it).

[Łukasz Anforowicz, 2016-05-19 18:06:59]

Thanks for reviewing, can you take another look?

On 2016/05/19 16:08:01, clamy wrote:
> Thanks! A few more comments. On the deduplication side, I don't think it
matters
> much if the deduplication CL lands after this one. However we should probably
> coordinate on whether your CL lands first or mine does (since that would
change
> things on the review).

I think the easiest way would be to land in the following order:

1. Your CL (https://codereview.chromium.org/1907443006/) - I see you got an LGTM
with just few remaining nits 

2. This CL (rebased on top of your CL - particularily render_frame_impl.cc and
navigation_entry_impl.cc)

3. Deduplication CL (https://codereview.chromium.org/1987053002/).  If this
waits for CL1, then I can also remove
ResourceRequestBody::AppendExplodedHTTPBodyElement (i.e. after unifying the
types there will be no need for the conversion here - therefore this method can
probably go away).

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:163:
scoped_refptr<ResourceRequestBody> ConstructResourceRequestBody() const;
On 2016/05/19 16:08:00, clamy wrote:
> Could we make it clearer that this is creating the body based on browser
> initiated post data, and not based on data from the PageState? The naming is
not
> super clear there.

Thanks for the feedback.  I also wondered about this but had trouble coming up
with a good name.

ConstructBrowserInitiatedPostBody seems wrong - we are not constructing
BrowserInitiatedPostBody, we are converting BrowserInitiatedPostBody into
ResourceRequestBody.

ConstructResourceRequestBodyForBrowserInitiatedPost and
ConvertBrowserInitiatedPostDataIntoResourceRequestBody seem too long to be
readable.

Maybe ConstructBodyFromBrowserInitiatedPostData?

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:91: request_body ? "POST" :
"GET", dest_url, dest_referrer,
On 2016/05/19 16:08:00, clamy wrote:
> This should be frame_entry.method().

Arrgh.  Thanks for catching this.  Done.

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:379:
entry.ConstructCommonNavigationParams(frame_entry.method(), dest_url,
On 2016/05/19 16:08:00, clamy wrote:
> In the current code, the method is always GET for subframes. Using
> frame_entry.method() outside of UseSubframeNavigationEntries mode may lead to
> differences in behavior.

Oh, I see - this is the concern that you're addressing in your CL inside
https://codereview.chromium.org/1907443006/patch/260001/270003.

I think I'll just copy your approach and rebase on top of your CL (and wait with
landing this until your CL can land).

Done?

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:719: // is null, when we are called
from RenderFrameProxyHost::OnOpenURL).
On 2016/05/19 16:08:00, clamy wrote:
> I think it would be better to pass the post data and the method instead of the
> handle itself. When we call RenderFrameProxyHost::OnOpenURL the handle should
> always be null. However, we could pass method and the post data in
> FrameHostMsg_OpenURL_Params, and then to this function.

Done.  And thanks a lot for the suggestion - it really helps :-) (I think I
might try to handle the open url case next week).

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:255: // (and a non-null
NavigationHandle?).
On 2016/05/19 16:08:00, clamy wrote:
> See my comment in RequestTransferURL in favor of not passing the
> NavigationHandle but the post data and method directly. We should not
> instantiate a NavigationHandle here (there are some timing issues with doing
> it).

Done.

[clamy, 2016-05-20 15:49:14]

Thanks! A few comments on the rebase (I haven't had time to do a full review
though).

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:163:
scoped_refptr<ResourceRequestBody> ConstructResourceRequestBody() const;
On 2016/05/19 18:06:58, Łukasz Anforowicz wrote:
> On 2016/05/19 16:08:00, clamy wrote:
> > Could we make it clearer that this is creating the body based on browser
> > initiated post data, and not based on data from the PageState? The naming is
> not
> > super clear there.
> 
> Thanks for the feedback.  I also wondered about this but had trouble coming up
> with a good name.
> 
> ConstructBrowserInitiatedPostBody seems wrong - we are not constructing
> BrowserInitiatedPostBody, we are converting BrowserInitiatedPostBody into
> ResourceRequestBody.
> 
> ConstructResourceRequestBodyForBrowserInitiatedPost and
> ConvertBrowserInitiatedPostDataIntoResourceRequestBody seem too long to be
> readable.
> 
> Maybe ConstructBodyFromBrowserInitiatedPostData?
> 
ConstructBodyFromBrowserInitiatedPostData sounds the best to me.

https://codereview.chromium.org/1956383003/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1956383003/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:86:
entry.ConstructBodyFromBrowserInitiatedPostData();
It's very important to check frame_entry.GetPostData(), as otherwise history
POST navigations won't work with PlzNavigate enabled.
the code should be:
scoped_refptr<ResourceRequestBody> request_body;
if (frame_entry.method() == "POST") {
  request_body = frame_entry.GetPostData();
  if (!request_body && entry.GetBrowserInitiatedPostData()) {
    request_body = entry.ConstructBodyFromBrowserInitiatedPostData();
  }
}

https://codereview.chromium.org/1956383003/diff/160001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1956383003/diff/160001/content/common/navigat...
content/common/navigation_params.h:191: scoped_refptr<ResourceRequestBody>
post_data;
Actually, I think this should be part of CommonNavigationParams. Right now, we
send it in the BeginNavigation IPC, the CommitNavigation IPC, and your CL adds
it to be used through the StartNavigationParams in the Navigate IPC. Let's have
it be a member of CommonNavigationParams (which is sent in all those IPCs), and
remove the extra member of FramehostMsg_BeginNavigation and
FrameMsg_CommitNavigation.

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5273: scoped_refptr<ResourceRequestBody>
post_data) {
Note: moving post_data to the CommonNavigationParams IPC means we can get rid of
that parameter.

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5429: if (common_params.method == "POST"
&& !browser_side_navigation &&
This means that the post data will only be added to the request if the
navigation is a regular navigation: history and reloads are treated above. Is
that ok for site-per-process (we add it for all navigations with PlzNavigate).

[Łukasz Anforowicz, 2016-05-20 22:04:55]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  StartNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-05-20 22:18:48]

Camille, can you take another look please?

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1956383003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_entry_impl.h:163:
scoped_refptr<ResourceRequestBody> ConstructResourceRequestBody() const;
On 2016/05/20 15:49:14, clamy wrote:
> On 2016/05/19 18:06:58, Łukasz Anforowicz wrote:
> > On 2016/05/19 16:08:00, clamy wrote:
> > > Could we make it clearer that this is creating the body based on browser
> > > initiated post data, and not based on data from the PageState? The naming
is
> > not
> > > super clear there.
> > 
> > Thanks for the feedback.  I also wondered about this but had trouble coming
up
> > with a good name.
> > 
> > ConstructBrowserInitiatedPostBody seems wrong - we are not constructing
> > BrowserInitiatedPostBody, we are converting BrowserInitiatedPostBody into
> > ResourceRequestBody.
> > 
> > ConstructResourceRequestBodyForBrowserInitiatedPost and
> > ConvertBrowserInitiatedPostDataIntoResourceRequestBody seem too long to be
> > readable.
> > 
> > Maybe ConstructBodyFromBrowserInitiatedPostData?
> > 
> ConstructBodyFromBrowserInitiatedPostData sounds the best to me.

Acknowledged.  (the rename already happened in a previous patchset)

https://codereview.chromium.org/1956383003/diff/160001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1956383003/diff/160001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:86:
entry.ConstructBodyFromBrowserInitiatedPostData();
On 2016/05/20 15:49:14, clamy wrote:
> It's very important to check frame_entry.GetPostData(), as otherwise history
> POST navigations won't work with PlzNavigate enabled.
> the code should be:
> scoped_refptr<ResourceRequestBody> request_body;
> if (frame_entry.method() == "POST") {
>   request_body = frame_entry.GetPostData();
>   if (!request_body && entry.GetBrowserInitiatedPostData()) {
>     request_body = entry.ConstructBodyFromBrowserInitiatedPostData();
>   }
> }

Done.

https://codereview.chromium.org/1956383003/diff/160001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1956383003/diff/160001/content/common/navigat...
content/common/navigation_params.h:191: scoped_refptr<ResourceRequestBody>
post_data;
On 2016/05/20 15:49:14, clamy wrote:
> Actually, I think this should be part of CommonNavigationParams. Right now, we
> send it in the BeginNavigation IPC, the CommitNavigation IPC, and your CL adds
> it to be used through the StartNavigationParams in the Navigate IPC. Let's
have
> it be a member of CommonNavigationParams (which is sent in all those IPCs),
and
> remove the extra member of FramehostMsg_BeginNavigation and
> FrameMsg_CommitNavigation.

Done.  I've done that initially here, but then decided to move this refactoring
to a separate CL: https://codereview.chromium.org/1999943002/

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5273: scoped_refptr<ResourceRequestBody>
post_data) {
On 2016/05/20 15:49:14, clamy wrote:
> Note: moving post_data to the CommonNavigationParams IPC means we can get rid
of
> that parameter.

Done.

https://codereview.chromium.org/1956383003/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5429: if (common_params.method == "POST"
&& !browser_side_navigation &&
On 2016/05/20 15:49:14, clamy wrote:
> This means that the post data will only be added to the request if the
> navigation is a regular navigation: history and reloads are treated above. Is
> that ok for site-per-process (we add it for all navigations with PlzNavigate).

Done.  You're right - now that this handles not only browser-initiated-post, it
should probably be moved out of the else branch.

[Łukasz Anforowicz, 2016-05-20 23:24:32]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-05-23 15:32:28]

Thanks! It's looking mostly good.

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:167: const std::string&
method() const { return method_; }
Suggestion: should we change the public interface method IsPost into GetMethod?

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:392: frame_entry, post_body,
dest_url, dest_referrer, navigation_type,
Right now this is only used for transfers right? If we plan to use it for
OpenURL as well, then we may need to integrate it with PlzNavigate as well (this
is not required right now, just mentioning it for the record).

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:83: const
scoped_refptr<ResourceRequestBody>& post_body) override;
A const scoped_refptr<...>& looks very weird. Do we have other instances of this
in the codebase?

[Łukasz Anforowicz, 2016-05-23 16:25:11]

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:167: const std::string&
method() const { return method_; }
On 2016/05/23 15:32:28, clamy wrote:
> Suggestion: should we change the public interface method IsPost into
GetMethod?

Yes - this seems like the right thing to do - only considering GET and POST (and
ignoring PUT and other HTTP methods) seems fishy and therefore GetMethod seems
better than IsPost.

BTW: After your suggestion I've also realized that like other similar accessors,
GetMethod should also verify CHECK_NE(INITIAL, state_).

Done.

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:392: frame_entry, post_body,
dest_url, dest_referrer, navigation_type,
On 2016/05/23 15:32:28, clamy wrote:
> Right now this is only used for transfers right?

Yes, I believe so.

> If we plan to use it for
> OpenURL as well, then we may need to integrate it with PlzNavigate as well
(this
> is not required right now, just mentioning it for the record).

I do plan to plumb POST body for OpenURL as well (mostly for
https://crbugs.com/585284).  I'll loop you in on reviews, once I have something
review-ready.

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1956383003/diff/220001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:83: const
scoped_refptr<ResourceRequestBody>& post_body) override;
On 2016/05/23 15:32:28, clamy wrote:
> A const scoped_refptr<...>& looks very weird. Do we have other instances of
this
> in the codebase?

2286 hits in code search :-) [1]

This pattern avoids increasing/decreasing ref-count for every frame on the
callstack (i.e. each function takes a const-ref, and the final called function
copy-assigns or copy-constructs into a scoped_refptr - only the final copy
causes a refcount bump).

I see that the style guide now asks to just declare "scoped_ptr<T>" and the
caller can decide whether to transfer or increase the ref-count.  I am not sure
if I buy that argument (caller might want to keep its ref-count, but
intermediate chain of functions on the callstack might not + even the final
caller might or might not take refcount depending on other things).

BTW: If function doesn't ever take ref-count, then it should take "const T&" as
discussed in [2].  But this is not such situation - we are eventually going to
pass the refcount to the IPC message payload.

Let's leave this as-is, unless //content OWNERs complain?

[1]
https://code.google.com/p/chromium/codesearch#search/&q=%22const%20scoped_ref...

[2]
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/noOt34XStEU

[Łukasz Anforowicz, 2016-05-26 00:52:02]

Camille, can you take another look please?

[clamy, 2016-05-26 15:35:24]

Thanks! A few more comments.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:299: // Update the
navigation parameters.
Add a DCHECK that resource_request_body is null outside of POSTs?

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:161: // Gets post body
associated with this navigation.
nit: s/Gets/Returns the

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:289: // The body of the
request (i.e. body of a post request).
nit: Precise that this will be null outside of POST requests?

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator.h:123: // before the transfer.
nit: precise that |post_body| should be null is method is not POST.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:733:
SiteIsolationPolicy::AreCrossProcessFramesPossible());
Add a DCHECK that post_body is null for non POST navigations.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:27: class ResourceRequestBody;
nit: Do you need to forward declare ResourceRequestBody again in NavigatorImpl
if it was done in Navigator?

https://codereview.chromium.org/1956383003/diff/260001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:232:
scoped_refptr<ResourceRequestBody> body_;
Should this be reset on a non-POST redirect?

[Łukasz Anforowicz, 2016-05-26 17:14:02]

Camille, can you take another look please?

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:299: // Update the
navigation parameters.
On 2016/05/26 15:35:23, clamy wrote:
> Add a DCHECK that resource_request_body is null outside of POSTs?

Done.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:161: // Gets post body
associated with this navigation.
On 2016/05/26 15:35:23, clamy wrote:
> nit: s/Gets/Returns the

Done.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.h:289: // The body of the
request (i.e. body of a post request).
On 2016/05/26 15:35:23, clamy wrote:
> nit: Precise that this will be null outside of POST requests?

Done.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator.h:123: // before the transfer.
On 2016/05/26 15:35:23, clamy wrote:
> nit: precise that |post_body| should be null is method is not POST.

Done.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:733:
SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2016/05/26 15:35:23, clamy wrote:
> Add a DCHECK that post_body is null for non POST navigations.

Done.

I've also added the DCHECK in CommonNavigationParams.

My only worry is that sometimes POST might have an empty body.  In such cases we
should be using non-null, empty ResourceRequestBody, but I am not 100% sure if
with the current codebase we won't ever treat null and empty interchangably. 
Still - let's try adding the DCHECK - this should put us in a better place in
the long term.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:27: class ResourceRequestBody;
On 2016/05/26 15:35:24, clamy wrote:
> nit: Do you need to forward declare ResourceRequestBody again in NavigatorImpl
> if it was done in Navigator?

I am not sure what is the right pattern here.  My thinking was:

- No need to reinclude or re-fwd-declare things from declarations of methods
that override the base class (i.e. the include for the based class should have
already include or fwd-declare these things)

- OTOH, the rule above doesn't apply to NavigateToEntry method, so I thought
that it would be best to independently make sure that scoped_refptr and
ResourceRequestBody are visible - this is why I've added the fwd declaration.

Based on the above - I think it is best to have the fwd decl here.

https://codereview.chromium.org/1956383003/diff/260001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/loader...
content/browser/loader/resource_request_info_impl.h:232:
scoped_refptr<ResourceRequestBody> body_;
On 2016/05/26 15:35:24, clamy wrote:
> Should this be reset on a non-POST redirect?

Good point.  Done.

[clamy, 2016-05-30 16:40:43]

Thanks!

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:733:
SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2016/05/26 17:14:02, Łukasz Anforowicz wrote:
> On 2016/05/26 15:35:23, clamy wrote:
> > Add a DCHECK that post_body is null for non POST navigations.
> 
> Done.
> 
> I've also added the DCHECK in CommonNavigationParams.
> 
> My only worry is that sometimes POST might have an empty body.  In such cases
we
> should be using non-null, empty ResourceRequestBody, but I am not 100% sure if
> with the current codebase we won't ever treat null and empty interchangably. 
> Still - let's try adding the DCHECK - this should put us in a better place in
> the long term.

Yes this is why I suggested a check that the body is null in non-POST
navigations, and not a check that the body is not null in POST navigations. I'm
not sure that we will always have one in POSTs, and ti complexifies testing
code.

If we want to keep, it should also be written without if else clauses (you
shoudln't be using if/else when writing only dchecks as it's not clear that the
compiler will properly compile out the if/else when compiling for release).

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.h:27: class ResourceRequestBody;
On 2016/05/26 17:14:02, Łukasz Anforowicz wrote:
> On 2016/05/26 15:35:24, clamy wrote:
> > nit: Do you need to forward declare ResourceRequestBody again in
NavigatorImpl
> > if it was done in Navigator?
> 
> I am not sure what is the right pattern here.  My thinking was:
> 
> - No need to reinclude or re-fwd-declare things from declarations of methods
> that override the base class (i.e. the include for the based class should have
> already include or fwd-declare these things)
> 
> - OTOH, the rule above doesn't apply to NavigateToEntry method, so I thought
> that it would be best to independently make sure that scoped_refptr and
> ResourceRequestBody are visible - this is why I've added the fwd declaration.
> 
> Based on the above - I think it is best to have the fwd decl here.

Acknowledged.

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:66: if (method == "POST") {
As mentioned in navigator_impl.cc I'm really not sure we want to add a check for
post data in POST navigation. And there should not be an if/else clause that
contains only DCHECKs.

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:72: DCHECK(!post_data);
This will break in PlzNavigate. We always send the POST data if any (we don't
use the aforementionned branches RenderFrameImpl::NavigateInternal.

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:77: // DCHECK(post_data);
Please don't add commented out code.

[Łukasz Anforowicz, 2016-05-31 16:25:20]

Camille, can you take another look please?  (I've just kicked off the trybots
and haven't yet checked the results, but relaxing the DCHECK condition should
keep everything green).

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/260001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:733:
SiteIsolationPolicy::AreCrossProcessFramesPossible());
On 2016/05/30 16:40:43, clamy wrote:
> On 2016/05/26 17:14:02, Łukasz Anforowicz wrote:
> > On 2016/05/26 15:35:23, clamy wrote:
> > > Add a DCHECK that post_body is null for non POST navigations.
> > 
> > Done.
> > 
> > I've also added the DCHECK in CommonNavigationParams.
> > 
> > My only worry is that sometimes POST might have an empty body.  In such
cases
> we
> > should be using non-null, empty ResourceRequestBody, but I am not 100% sure
if
> > with the current codebase we won't ever treat null and empty interchangably.

> > Still - let's try adding the DCHECK - this should put us in a better place
in
> > the long term.
> 
> Yes this is why I suggested a check that the body is null in non-POST
> navigations, and not a check that the body is not null in POST navigations.
I'm
> not sure that we will always have one in POSTs, and ti complexifies testing
> code.

Done - kept only testing for absence of body if method != POST.

> If we want to keep, it should also be written without if else clauses (you
> shoudln't be using if/else when writing only dchecks as it's not clear that
the
> compiler will properly compile out the if/else when compiling for release).

Thanks for raising this.  I misremembered the DCHECK_IMPLIES thread on
chromium-dev [1] - I thought that it was said that we can safely use "if"
statement, but it is more subtle than that (i.e. the compiler might not always
be able to tell that there were no side effects).  Being lazy (i.e. not wanting
to learn how to get disassembly and learn how to find the relevant pieces of
disassembly) I just got rid of the "if" statement.

Done - |if (a) DCHECK(b)| has been rewritten as |DCHECK(!a || b)|.

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/1Jpudx4lMMA/dj_1O...

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:66: if (method == "POST") {
On 2016/05/30 16:40:43, clamy wrote:
> As mentioned in navigator_impl.cc I'm really not sure we want to add a check
for
> post data in POST navigation. And there should not be an if/else clause that
> contains only DCHECKs.

Done.

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:72: DCHECK(!post_data);
On 2016/05/30 16:40:43, clamy wrote:
> This will break in PlzNavigate. We always send the POST data if any (we don't
> use the aforementionned branches RenderFrameImpl::NavigateInternal.

Done (this branch of "if" statement was removed altogether).

https://codereview.chromium.org/1956383003/diff/340001/content/common/navigat...
content/common/navigation_params.cc:77: // DCHECK(post_data);
On 2016/05/30 16:40:43, clamy wrote:
> Please don't add commented out code.

Done (this branch of "if" statement was removed altogether).

[Łukasz Anforowicz, 2016-05-31 18:15:19]

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

[Łukasz Anforowicz, 2016-05-31 18:15:20]

Charlie, I think this CL is really close to being ready for a //content OWNER
review (Camille and me are at this point only iterating on the right amount of
extra DCHECKs to add).  Can you please take a look?

[Charlie Reis, 2016-05-31 21:08:36]

Nice.  And to clarify, this will be enough to get the XSS Auditor to work but
we'll need a little more (https://crbug.com/613004) to get back/forwards to
support POST resubmissions, right?

And heh, I was going to ask about const scoped_refptr<...>& as well, so I'm glad
it came up.  Sounds like it serves a useful purpose and is widespread, so I'm
not concerned.

A few questions about whether the DCHECKs are dangerous enough to worry about
them in release builds, or if it's ok to ignore them.  Otherwise looks good.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:264: method = "GET";
Maybe we can just initialize |method| to "GET" and leave out the else block? 
Leaving it uninitialized above always makes me nervous when people might add
another branch to the if/else (or make it an else if) and forget to initialize
it.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:314: DCHECK(method ==
"POST" || !resource_request_body);
Maybe we should upgrade this to a CHECK?  Seems like we should be careful not to
accidentally send a POST body to the renderer due to unrelated bugs.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:440: is_same_document_history_load,
true, nullptr);
Sanity check: Will we need to change this when we later add support for
back/forward POST resubmissions on transfers?  Or are those not a problem here
because the POST data is hidden away in the PageState?  (Just wondering if we
should leave a TODO.)

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:732: DCHECK(method == "POST" ||
!post_body);
Should we be concerned if this fails in practice?

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:186: info->ResetBody();
Is this a second line of defense, or did we actually get this wrong before and
leave the POST data in the request after converting it to a GET?

[Łukasz Anforowicz, 2016-05-31 22:25:58]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211, 613004
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-05-31 22:40:55]

On 2016/05/31 21:08:36, Charlie Reis wrote:
> And to clarify, this will be enough to get the XSS Auditor to work but
> we'll need a little more (https://crbug.com/613004) to get back/forwards to
> support POST resubmissions, right?

Thanks for checking - I thought that I've tried the back navigation testcase
after this CL, but apparently I was wrong - I tried the test again and it turns
out that after this CL both 1) the XSS Auditor layout tests and 2) the recently
added GoBackToCrossSitePostWithRedirect browser test are passing.  I've updated
test expectations.

> And heh, I was going to ask about const scoped_refptr<...>& as well, so I'm
glad
> it came up.  Sounds like it serves a useful purpose and is widespread, so I'm
> not concerned.

Ack.
> 
> A few questions about whether the DCHECKs are dangerous enough to worry about
> them in release builds, or if it's ok to ignore them.  Otherwise looks good.

I tried to reply inline, below.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:264: method = "GET";
On 2016/05/31 21:08:36, Charlie Reis wrote:
> Maybe we can just initialize |method| to "GET" and leave out the else block? 
> Leaving it uninitialized above always makes me nervous when people might add
> another branch to the if/else (or make it an else if) and forget to initialize
> it.

Done.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:314: DCHECK(method ==
"POST" || !resource_request_body);
On 2016/05/31 21:08:36, Charlie Reis wrote:
> Maybe we should upgrade this to a CHECK?  Seems like we should be careful not
to
> accidentally send a POST body to the renderer due to unrelated bugs.

Maybe we should try this in a follow-up, smaller (= more easily revertable) CL?


I also have a few questions about adding the CHECK:

1. Should we promote the CommonNavigationParams DCHECK instead (this is where we
construct the IPC payload for renderer;  above it just network stack processing
[which also covers subresources (?) or other things where http body won't get
sent to the renderer process]).

2. I agree with crashing the browser if we detect an obviously security problem
(i.e. browser is confused about origin of frames).  Is sending of POST body the
same level of a security issue (i.e. something that we want to crash upon)?


Also - when thinking about this, I've found out that only GET and POST are
supported for page navigations (so - i.e. we don't have to worry about not
CHECKing/crashing for PUT [which would contain a HTTP body]).  So - I wonder if
I should maybe prefer |bool uses_post| over |std::string method| in my other CLs
(i.e. in https://codereview.chromium.org/2022483003/).

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:440: is_same_document_history_load,
true, nullptr);
On 2016/05/31 21:08:36, Charlie Reis wrote:
> Sanity check: Will we need to change this when we later add support for
> back/forward POST resubmissions on transfers?  Or are those not a problem here
> because the POST data is hidden away in the PageState?  (Just wondering if we
> should leave a TODO.)

I don't really know :-(  AFAIK, we don't need to send the POST body to the
renderer to history and reload navigations, because it will be taken from
renderer's in-memory state (i.e. see in RenderFrameImpl::NavigateInternal how
|request| is initialized in |is_reload| and/or |is_history_navigation| cases). 
So - I think PageState should cover this, and we will continue passing
|nullptr|, but I don't really know...

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:732: DCHECK(method == "POST" ||
!post_body);
On 2016/05/31 21:08:36, Charlie Reis wrote:
> Should we be concerned if this fails in practice?

Yes, we should be concerned, but I don't know how much :-)

If this condition fails, it means that we are unnecessarily sending a POST body
to a renderer (and potentially to a renderer that wouldn't have seen this POST
body otherwise).

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:186: info->ResetBody();
On 2016/05/31 21:08:36, Charlie Reis wrote:
> Is this a second line of defense, or did we actually get this wrong before and
> leave the POST data in the request after converting it to a GET?

This is a separate code path.  Network stack operates on net::UploadDataStream
(which is created from content::ResourceRequestBody, but doesn't retain any ties
to the original content::ResourceRequestBody object).

We need to stash ResourceRequestBody inside ResourceRequestInfoImpl so it can be
passed to NavigationHandleImpl::WillStartRequest.  We never use
ResourceRequestInfoImpl::body() afterwards - after the call to
NHI::WillStartRequest, we could in theory ResetBody.  WDYT about doing this?

The current code keeps ResourceRequestInfoImpl::body (even after it is not
really needed after the call to NHI::WillStartRequest), because I felt it would
be weird to have to describe that RRII::original_headers_ are valid forever, but
RRII::body_ is only valid for a little while.

[clamy, 2016-06-01 13:22:38]

Thanks! No more comments from me besides creis's, so lgtm.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:440: is_same_document_history_load,
true, nullptr);
On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> On 2016/05/31 21:08:36, Charlie Reis wrote:
> > Sanity check: Will we need to change this when we later add support for
> > back/forward POST resubmissions on transfers?  Or are those not a problem
here
> > because the POST data is hidden away in the PageState?  (Just wondering if
we
> > should leave a TODO.)
> 
> I don't really know :-(  AFAIK, we don't need to send the POST body to the
> renderer to history and reload navigations, because it will be taken from
> renderer's in-memory state (i.e. see in RenderFrameImpl::NavigateInternal how
> |request| is initialized in |is_reload| and/or |is_history_navigation| cases).

> So - I think PageState should cover this, and we will continue passing
> |nullptr|, but I don't really know...

In the current architecture, the POST body should not be needed for simple
reloads and history since we create the WebURLRequest for them in
RenderFrameImpl using the PageState that is provided by the browser (and I can
confirm we initialize the WebHTTPBody based on the PageState).

A possible issue would be transfers in the middle of a history/reload (if they
are possible). In that case, I imagine the transfer case would take care of
that, but I'm not sure.

[Łukasz Anforowicz, 2016-06-01 16:28:38]

I raised 2 more questions below (man, this CL is sure taking a while to review
and land :-).

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
content/common/navigation_params.cc:67: DCHECK(method == "POST" || !post_data);
I guess we could also repeat the "assertion" from
NavigationControllerImpl::LoadURLWithParams which says something like
(pseudo-code): DCHECK_IMPLIES(method == "POST", url.SchemeIs(http or https)).

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
content/public/browser/navigation_handle.h:94: virtual const std::string&
GetMethod() = 0;
Hmmm... does the IsPost -> GetMethod change still look good in light of the
discussion we had in the other, WIP CL [1]?  I believe that we can only deal
with "GET" and "POST" here, right?

[1] https://codereview.chromium.org/2022483003/

[Charlie Reis, 2016-06-01 23:46:31]

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

[Charlie Reis, 2016-06-01 23:46:33]

[+scottmg for content/browser/loader/DEPS]

Thanks!  LGTM with some thoughts below.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:314: DCHECK(method ==
"POST" || !resource_request_body);
On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> On 2016/05/31 21:08:36, Charlie Reis wrote:
> > Maybe we should upgrade this to a CHECK?  Seems like we should be careful
not
> to
> > accidentally send a POST body to the renderer due to unrelated bugs.
> 
> Maybe we should try this in a follow-up, smaller (= more easily revertable)
CL?

Sure.  Any CHECKs we add can happen in a separate, revertable CL.

> 
> 
> I also have a few questions about adding the CHECK:
> 
> 1. Should we promote the CommonNavigationParams DCHECK instead (this is where
we
> construct the IPC payload for renderer;  above it just network stack
processing
> [which also covers subresources (?) or other things where http body won't get
> sent to the renderer process]).

Yes, that seems like the right place.

> 2. I agree with crashing the browser if we detect an obviously security
problem
> (i.e. browser is confused about origin of frames).  Is sending of POST body
the
> same level of a security issue (i.e. something that we want to crash upon)?

Assuming that a renderer can't lie to make this happen (and that it requires a
browser process bug to trigger), then yes, I think we should crash the browser
if it's about to send a POST to a renderer that shouldn't receive it.  That
risks information disclosure (violating our Site Isolation goals) in some cases,
and in all cases indicates a logic bug that might otherwise be concerning.  I
think that's worthwhile to verify.

> Also - when thinking about this, I've found out that only GET and POST are
> supported for page navigations (so - i.e. we don't have to worry about not
> CHECKing/crashing for PUT [which would contain a HTTP body]).  So - I wonder
if
> I should maybe prefer |bool uses_post| over |std::string method| in my other
CLs
> (i.e. in https://codereview.chromium.org/2022483003/).

See my other response.  :)

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:440: is_same_document_history_load,
true, nullptr);
On 2016/06/01 13:22:37, clamy wrote:
> On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> > On 2016/05/31 21:08:36, Charlie Reis wrote:
> > > Sanity check: Will we need to change this when we later add support for
> > > back/forward POST resubmissions on transfers?  Or are those not a problem
> here
> > > because the POST data is hidden away in the PageState?  (Just wondering if
> we
> > > should leave a TODO.)
> > 
> > I don't really know :-(  AFAIK, we don't need to send the POST body to the
> > renderer to history and reload navigations, because it will be taken from
> > renderer's in-memory state (i.e. see in RenderFrameImpl::NavigateInternal
how
> > |request| is initialized in |is_reload| and/or |is_history_navigation|
cases).
> 
> > So - I think PageState should cover this, and we will continue passing
> > |nullptr|, but I don't really know...
> 
> In the current architecture, the POST body should not be needed for simple
> reloads and history since we create the WebURLRequest for them in
> RenderFrameImpl using the PageState that is provided by the browser (and I can
> confirm we initialize the WebHTTPBody based on the PageState).

Yeah, this answers my question.  The PageState should be sufficient in most
cases.

> A possible issue would be transfers in the middle of a history/reload (if they
> are possible). In that case, I imagine the transfer case would take care of
> that, but I'm not sure.

Yeah, transfers during history navigations are possible, and it's kind of a
scary case that we should add some tests for (separately).  I thought it might
explain the NavigateInternal crashes in https://crbug.com/568703#c9, but it
turned out to not be an issue there.

I think you're right that the existing transfer logic for POSTs should kick in
there, so I'm guessing this line is fine.

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:732: DCHECK(method == "POST" ||
!post_body);
On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> On 2016/05/31 21:08:36, Charlie Reis wrote:
> > Should we be concerned if this fails in practice?
> 
> Yes, we should be concerned, but I don't know how much :-)
> 
> If this condition fails, it means that we are unnecessarily sending a POST
body
> to a renderer (and potentially to a renderer that wouldn't have seen this POST
> body otherwise).

Doesn't sound like a crashable offense, but maybe we should put in a
NOTREACHED() block that resets post_body instead of just DCHECKing?  Or is that
tricky?

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:186: info->ResetBody();
On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> On 2016/05/31 21:08:36, Charlie Reis wrote:
> > Is this a second line of defense, or did we actually get this wrong before
and
> > leave the POST data in the request after converting it to a GET?
> 
> This is a separate code path.  Network stack operates on net::UploadDataStream
> (which is created from content::ResourceRequestBody, but doesn't retain any
ties
> to the original content::ResourceRequestBody object).
> 
> We need to stash ResourceRequestBody inside ResourceRequestInfoImpl so it can
be
> passed to NavigationHandleImpl::WillStartRequest.  We never use
> ResourceRequestInfoImpl::body() afterwards - after the call to
> NHI::WillStartRequest, we could in theory ResetBody.  WDYT about doing this?
> 
> The current code keeps ResourceRequestInfoImpl::body (even after it is not
> really needed after the call to NHI::WillStartRequest), because I felt it
would
> be weird to have to describe that RRII::original_headers_ are valid forever,
but
> RRII::body_ is only valid for a little while.

I see.  Sounds like there's a slight risk the two copies could fork and become
different, but I don't think that's likely.  Anyway, seems ok to leave it in
place here.

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
content/browser/loader/DEPS:232: "+content/common/resource_request_body.h",
I'm not 100% sure on this, but hopefully it's not a problem.

@scottmg, can you take a look (per https://codereview.chromium.org/1914063003)? 
(jam@ appears to be OOO.)

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
content/common/navigation_params.cc:67: DCHECK(method == "POST" || !post_data);
On 2016/06/01 16:28:38, Łukasz Anforowicz wrote:
> I guess we could also repeat the "assertion" from
> NavigationControllerImpl::LoadURLWithParams which says something like
> (pseudo-code): DCHECK_IMPLIES(method == "POST", url.SchemeIs(http or https)).

Sounds like a good idea.

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
content/public/browser/navigation_handle.h:94: virtual const std::string&
GetMethod() = 0;
On 2016/06/01 16:28:38, Łukasz Anforowicz wrote:
> Hmmm... does the IsPost -> GetMethod change still look good in light of the
> discussion we had in the other, WIP CL [1]?  I believe that we can only deal
> with "GET" and "POST" here, right?
> 
> [1] https://codereview.chromium.org/2022483003/

Heh.  I think we just need to pick an approach and document why, so that we
don't flip flop between them any further.  :)  I'm ok with either approach.

If we can't navigate to something other than a GET or a POST and we want to
enforce that embedders don't try to, then keeping it as a bool seems reasonable.
 Let's document it if so.

[scottmg, 2016-06-02 00:14:32]

scottmg@chromium.org changed reviewers:
+ jam@chromium.org

[scottmg, 2016-06-02 00:14:33]

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
content/browser/loader/DEPS:232: "+content/common/resource_request_body.h",
On 2016/06/01 23:46:32, Charlie Reis wrote:
> I'm not 100% sure on this, but hopefully it's not a problem.
> 
> @scottmg, can you take a look (per https://codereview.chromium.org/1914063003)

> (jam@ appears to be OOO.)

Yes, I think that's probably OK. Can you also move the instance on line 156 up
to the "OK" list?

[Łukasz Anforowicz, 2016-06-02 22:07:05]

scottmg@, PTAL?

creis@ - since your l-g-t-m, I've done:

- some DCHECK tweaks (i.e. NOTREACHED() + reset the body as you suggested)

- went back to |bool IsPost()| rather than |std::string GetMethod()| method in
NavigationHandle

I think this is okay, but you might want to take a quick look at the comment
changes in content/public/browser/navigation_handle.h

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:314: DCHECK(method ==
"POST" || !resource_request_body);
On 2016/06/01 23:46:32, Charlie Reis wrote:
> On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> > On 2016/05/31 21:08:36, Charlie Reis wrote:
> > > Maybe we should upgrade this to a CHECK?  Seems like we should be careful
> not
> > to
> > > accidentally send a POST body to the renderer due to unrelated bugs.
> > 
> > Maybe we should try this in a follow-up, smaller (= more easily revertable)
> CL?
> 
> Sure.  Any CHECKs we add can happen in a separate, revertable CL.

Ack.
> 
> > 
> > 
> > I also have a few questions about adding the CHECK:
> > 
> > 1. Should we promote the CommonNavigationParams DCHECK instead (this is
where
> we
> > construct the IPC payload for renderer;  above it just network stack
> processing
> > [which also covers subresources (?) or other things where http body won't
get
> > sent to the renderer process]).
> 
> Yes, that seems like the right place.
> 
> > 2. I agree with crashing the browser if we detect an obviously security
> problem
> > (i.e. browser is confused about origin of frames).  Is sending of POST body
> the
> > same level of a security issue (i.e. something that we want to crash upon)?
> 
> Assuming that a renderer can't lie to make this happen (and that it requires a
> browser process bug to trigger),

Hmmm... maybe the renderer can manage to send a body for a non-POST method.  I
am not sure if this is possible (feels like something that we should already vet
before starting the request), but I've added some checks against this in the
latest patchset in NavigationHandleImpl::WillStartRequest (symmetrical / similar
to the check already present in NavigationHandleImpl::WillRedirectRequest).

> then yes, I think we should crash the browser
> if it's about to send a POST to a renderer that shouldn't receive it.  That
> risks information disclosure (violating our Site Isolation goals) in some
cases,
> and in all cases indicates a logic bug that might otherwise be concerning.  I
> think that's worthwhile to verify.
> 
> > Also - when thinking about this, I've found out that only GET and POST are
> > supported for page navigations (so - i.e. we don't have to worry about not
> > CHECKing/crashing for PUT [which would contain a HTTP body]).  So - I wonder
> if
> > I should maybe prefer |bool uses_post| over |std::string method| in my other
> CLs
> > (i.e. in https://codereview.chromium.org/2022483003/).
> 
> See my other response.  :)

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/1956383003/diff/360001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:732: DCHECK(method == "POST" ||
!post_body);
On 2016/06/01 23:46:32, Charlie Reis wrote:
> On 2016/05/31 22:40:55, Łukasz Anforowicz wrote:
> > On 2016/05/31 21:08:36, Charlie Reis wrote:
> > > Should we be concerned if this fails in practice?
> > 
> > Yes, we should be concerned, but I don't know how much :-)
> > 
> > If this condition fails, it means that we are unnecessarily sending a POST
> body
> > to a renderer (and potentially to a renderer that wouldn't have seen this
POST
> > body otherwise).
> 
> Doesn't sound like a crashable offense, but maybe we should put in a
> NOTREACHED() block that resets post_body instead of just DCHECKing?  Or is
that
> tricky?

Done.  To reset the parameter we need to make it non-const, but this seems fine
/ is the only trickyness here.

I've also did a similar change in the other places where this CL adds DCHECKs
about post body:
- NavigatorImpl::RequestTransferURL
- CommonNavigationParams constructor

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/1956383003/diff/420001/content/browser/loader...
content/browser/loader/DEPS:232: "+content/common/resource_request_body.h",
On 2016/06/02 00:14:33, scottmg wrote:
> On 2016/06/01 23:46:32, Charlie Reis wrote:
> > I'm not 100% sure on this, but hopefully it's not a problem.
> > 
> > @scottmg, can you take a look (per
https://codereview.chromium.org/1914063003)
> 
> > (jam@ appears to be OOO.)
> 
> Yes, I think that's probably OK. Can you also move the instance on line 156 up
> to the "OK" list?

Done.

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/1956383003/diff/420001/content/common/navigat...
content/common/navigation_params.cc:67: DCHECK(method == "POST" || !post_data);
On 2016/06/01 23:46:32, Charlie Reis wrote:
> On 2016/06/01 16:28:38, Łukasz Anforowicz wrote:
> > I guess we could also repeat the "assertion" from
> > NavigationControllerImpl::LoadURLWithParams which says something like
> > (pseudo-code): DCHECK_IMPLIES(method == "POST", url.SchemeIs(http or
https)).
> 
> Sounds like a good idea.

I tried this in patchset 24, but it turns out that the new DCHECK failed for the
following case:
  url=chrome-extension://...
  method=POST
  body=1
The failure happened in navigator_impl.cc and in navigation_handle_impl.cc (but
I don't have at this point a specific repro for tickling this DCHECK in
navigation_params.cc).  Repro tests (browser_tests):
- InlineLoginUISafeIframeBrowserTest.ConfirmationRequiredForNonsecureSignin
- ExtensionWebRequestApiTest.PostData*

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1956383003/diff/420001/content/public/browser...
content/public/browser/navigation_handle.h:94: virtual const std::string&
GetMethod() = 0;
On 2016/06/01 23:46:32, Charlie Reis wrote:
> On 2016/06/01 16:28:38, Łukasz Anforowicz wrote:
> > Hmmm... does the IsPost -> GetMethod change still look good in light of the
> > discussion we had in the other, WIP CL [1]?  I believe that we can only deal
> > with "GET" and "POST" here, right?
> > 
> > [1] https://codereview.chromium.org/2022483003/
> 
> Heh.  I think we just need to pick an approach and document why, so that we
> don't flip flop between them any further.  :)  I'm ok with either approach.
> 
> If we can't navigate to something other than a GET or a POST and we want to
> enforce that embedders don't try to, then keeping it as a bool seems
reasonable.
>  Let's document it if so.

In this case, let me go back to NavigationHandle::IsPost :-).  I've also added a
comment explaining why we have IsPost rather than GetMethod.

https://codereview.chromium.org/1956383003/diff/480001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1956383003/diff/480001/content/public/browser...
content/public/browser/navigation_handle.h:93: // during the navigation (e.g.
after encountering a server redirect).
It was misleading (IMO) to say "GET", when in reality the alternative to "HTTP
POST" is "whatever default protocol is used for the scheme" (i.e. it won't be
"GET" for file: or data: URIs).

[Łukasz Anforowicz, 2016-06-02 22:09:41]

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

[Łukasz Anforowicz, 2016-06-02 22:09:42]

sky@, could you do an OWNERs review for chrome/test/base/ui_test_utils.cc?

The change makes the test utils populate a POST body when they end up using the
POST method.  Lack of the POST body was trigerring some DCHECKs I've been
experimenting with in this CL.

[scottmg, 2016-06-02 22:12:24]

c/b/l/DEPS lgtm

[Charlie Reis, 2016-06-02 22:44:30]

Changes since patch set 22 LGTM.

[sky, 2016-06-02 23:41:22]

chrome/test/base/ui_test_utils.cc LGTM

[Łukasz Anforowicz, 2016-06-03 15:58:14]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-06-03 15:58:15]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/1956383003/#ps480001
(title: "Removed DCHECK_IMPLIES(method == "POST", url.SchemeIs(http or
https)).")

[commit-bot: I haz the power, 2016-06-03 15:58:49]

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

[commit-bot: I haz the power, 2016-06-03 17:42:03]

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

[commit-bot: I haz the power, 2016-06-03 17:42:05]

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

[Łukasz Anforowicz, 2016-06-03 18:05:32]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-06-03 18:06:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956383003/480001

[commit-bot: I haz the power, 2016-06-03 19:38:25]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-06-03 19:39:29]

Description was changed from

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211, 613004
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forwarding POST body into renderer after a cross-site transfer.

After this CL, ResourceRequestBody from ResourceHostMsg_Request
will get sent after a cross-site transfer in FrameMsg_Navigate:

- ResourceDispatcherHostImpl::BeginRequest stores
  ResourceHostMsg_Request::request_body into
  ResourceRequestInfoImpl::body_

- NavigationResourceThrottle::WillStartRequest forwards
  ResourceRequestInfoImpl::body_ into a call to
  NavigationHandleImpl::WillStartRequest, where the body gets stored in
  NavigationHandleImpl::resource_request_body_

- NavigationHandleImpl::WillRedirectRequest takes care to
  reset the body if a redirect changed the method to a non-POST.
  (see also https://crbug.com/582211#c22).

- RenderFrameHostManager::OnCrossSiteResponse forwards
  NavigationHandleImpl::resource_request_body_ into the call to
  NavigatorImpl::RequestTransferURL.  The body is used to set
  the proper method on FrameNavigationEntry and used to populate
  CommonNavigationParams.

BUG=582211, 613004
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/5aa2c3740801f1c148c85db6612c24be0a76b6fe
Cr-Commit-Position: refs/heads/master@{#397779}
==========

[commit-bot: I haz the power, 2016-06-03 19:39:31]

Patchset 25 (id:??) landed as
https://crrev.com/5aa2c3740801f1c148c85db6612c24be0a76b6fe
Cr-Commit-Position: refs/heads/master@{#397779}