Allow forking for POST method (+ test that this works with prerender).

Allow forking for POST method (+ test that this works with prerender).

BUG=101395
Committed: https://crrev.com/3e3be13746968b4e07c0ffdc707b05a14676e435
Cr-Commit-Position: refs/heads/master@{#407586}

[Łukasz Anforowicz, 2016-07-21 23:38:54]

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

[Łukasz Anforowicz, 2016-07-21 23:38:55]

Charlie, can you take a look please?

Removing the early exit means that we will not (more often (*)) hit POST+OpenURL
path for 1) prerender and 2) extensions.  I've tried to cover prerender with a
new test (from experiments with an earlier patchset it seems that POST+prerender
was not covered by existing tests).  I think we don't need extra test coverage
for extensions (because we already have tests that POST to chrome-extension://
URI, just not via OpenURL path).

(*) "more often", because it was already possible with a form that targets a
remote frame.  OTOH, after this CL, POST+OpenURL+prerender/extensions will be
possible in the default Chrome (i.e. without remote frames).

[Łukasz Anforowicz, 2016-07-21 23:55:11]

On 2016/07/21 23:38:55, Łukasz Anforowicz wrote:
> Charlie, can you take a look please?
> 
> Removing the early exit means that we will not (more often (*)) hit
POST+OpenURL
> path for 1) prerender and 2) extensions.  I've tried to cover prerender with a
> new test (from experiments with an earlier patchset it seems that
POST+prerender
> was not covered by existing tests).  I think we don't need extra test coverage
> for extensions (because we already have tests that POST to chrome-extension://
> URI, just not via OpenURL path).
> 
> (*) "more often", because it was already possible with a form that targets a
> remote frame.  OTOH, after this CL, POST+OpenURL+prerender/extensions will be
> possible in the default Chrome (i.e. without remote frames).

aaand there is a silly typo in my earlier message that totally reverses what I
meant... :-/

I wanted to say "we will NOW (more often (*)) hit POST+OpenURL path".

[Charlie Reis, 2016-07-22 20:19:46]

[CC'ing pasko and davidben as FYI for prerender]

LGTM.  Thanks for adding the prerender test-- it does a nice job pointing out
that prerendered pages can't be used when submitting a form, since the content
of the page depends on the POST data.

Are we reasonably confident the extension case works?  I wouldn't expect it to
have problems.

[Charlie Reis, 2016-07-22 20:21:05]

One other reply:

On 2016/07/21 23:55:11, Łukasz Anforowicz wrote:
> On 2016/07/21 23:38:55, Łukasz Anforowicz wrote:
> > Charlie, can you take a look please?
> > 
> > Removing the early exit means that we will not (more often (*)) hit
> POST+OpenURL
> > path for 1) prerender and 2) extensions.  I've tried to cover prerender with
a
> > new test (from experiments with an earlier patchset it seems that
> POST+prerender
> > was not covered by existing tests).  I think we don't need extra test
coverage
> > for extensions (because we already have tests that POST to
chrome-extension://
> > URI, just not via OpenURL path).
> > 
> > (*) "more often", because it was already possible with a form that targets a
> > remote frame.  OTOH, after this CL, POST+OpenURL+prerender/extensions will
be
> > possible in the default Chrome (i.e. without remote frames).

As a side note, it's possible to target remote frames in default Chrome today
(e.g., a top-level window that has been navigated cross-process).

> 
> aaand there is a silly typo in my earlier message that totally reverses what I
> meant... :-/
> 
> I wanted to say "we will NOW (more often (*)) hit POST+OpenURL path".

Thanks for clarifying.  :)

[Łukasz Anforowicz, 2016-07-22 20:31:20]

On 2016/07/22 20:19:46, Charlie Reis wrote:
> [CC'ing pasko and davidben as FYI for prerender]
> 
> LGTM.  Thanks for adding the prerender test-- it does a nice job pointing out
> that prerendered pages can't be used when submitting a form, since the content
> of the page depends on the POST data.
> 
> Are we reasonably confident the extension case works?  I wouldn't expect it to
> have problems.

I think so.  The POST method + chrome-extension:// URIs are already exercised in
multiple tests, including ExtensionWebRequestApiTest.PostData* browser tests.  I
couldn't figure out how to force OpenURL path, because despite setting
browser_handles_all_top_level_requests for the active tab (before doing anything
else in ExtensionWebRequestApiTest.PostData) the renderer where the test
actually runs would not have this preference.  I think this is fine, because
OpenURL path should just preserve/forward POST data without touching it (forward
to the bits of code that already have test coverage as pointed out above).

[Łukasz Anforowicz, 2016-07-22 20:47:21]

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

[Łukasz Anforowicz, 2016-07-22 20:47:21]

mmenke@, could you please take a look at
chrome/browser/prerender/prerender_browsertest.cc?

[mmenke, 2016-07-22 20:54:22]

On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> mmenke@, could you please take a look at
> chrome/browser/prerender/prerender_browsertest.cc?

I'm confused...  I thought we stopped prerendering on navigation. 
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...

[Charlie Reis, 2016-07-22 21:02:43]

On 2016/07/22 20:54:22, mmenke wrote:
> On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> > mmenke@, could you please take a look at
> > chrome/browser/prerender/prerender_browsertest.cc?
> 
> I'm confused...  I thought we stopped prerendering on navigation. 
>
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...

That logic only cancels if the prerender makes a second commit on its own, which
doesn't happen here.  Here, we've prerendered the destination page but the user
does a POST, requiring a different server response.  (Lukasz, what makes us
cancel the prerender in this case, anyway?)

[Łukasz Anforowicz, 2016-07-22 21:06:04]

On 2016/07/22 21:02:43, Charlie Reis wrote:
> On 2016/07/22 20:54:22, mmenke wrote:
> > On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> > > mmenke@, could you please take a look at
> > > chrome/browser/prerender/prerender_browsertest.cc?
> > 
> > I'm confused...  I thought we stopped prerendering on navigation. 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...
> 
> That logic only cancels if the prerender makes a second commit on its own,
which
> doesn't happen here.  Here, we've prerendered the destination page but the
user
> does a POST, requiring a different server response.  (Lukasz, what makes us
> cancel the prerender in this case, anyway?)

I don't think the prerender is "cancelled" in this case - it just ends up not
being used at all (and goes away when the browser is closed - this is why the
test expects FINAL_STATUS_APP_TERMINATING for the prerender).

[Charlie Reis, 2016-07-22 21:08:01]

On 2016/07/22 21:06:04, Łukasz Anforowicz wrote:
> On 2016/07/22 21:02:43, Charlie Reis wrote:
> > On 2016/07/22 20:54:22, mmenke wrote:
> > > On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> > > > mmenke@, could you please take a look at
> > > > chrome/browser/prerender/prerender_browsertest.cc?
> > > 
> > > I'm confused...  I thought we stopped prerendering on navigation. 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...
> > 
> > That logic only cancels if the prerender makes a second commit on its own,
> which
> > doesn't happen here.  Here, we've prerendered the destination page but the
> user
> > does a POST, requiring a different server response.  (Lukasz, what makes us
> > cancel the prerender in this case, anyway?)
> 
> I don't think the prerender is "cancelled" in this case - it just ends up not
> being used at all (and goes away when the browser is closed - this is why the
> test expects FINAL_STATUS_APP_TERMINATING for the prerender).

My question was more about what made it not be used.  Normally a navigation to a
URL that is prerendered means we'll use the prerendered page.

[Łukasz Anforowicz, 2016-07-22 22:26:15]

On 2016/07/22 21:08:01, Charlie Reis wrote:
> On 2016/07/22 21:06:04, Łukasz Anforowicz wrote:
> > On 2016/07/22 21:02:43, Charlie Reis wrote:
> > > On 2016/07/22 20:54:22, mmenke wrote:
> > > > On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> > > > > mmenke@, could you please take a look at
> > > > > chrome/browser/prerender/prerender_browsertest.cc?
> > > > 
> > > > I'm confused...  I thought we stopped prerendering on navigation. 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...
> > > 
> > > That logic only cancels if the prerender makes a second commit on its own,
> > which
> > > doesn't happen here.  Here, we've prerendered the destination page but the
> > user
> > > does a POST, requiring a different server response.  (Lukasz, what makes
us
> > > cancel the prerender in this case, anyway?)
> > 
> > I don't think the prerender is "cancelled" in this case - it just ends up
not
> > being used at all (and goes away when the browser is closed - this is why
the
> > test expects FINAL_STATUS_APP_TERMINATING for the prerender).
> 
> My question was more about what made it not be used.  Normally a navigation to
a
> URL that is prerendered means we'll use the prerendered page.

The new PrerenderBrowserTest.HttpPost test is hitting the following code in
PrerenderManager::MaybeUsePrerenderedPage:

  // Don't prerender if the navigation involves some special parameters.
  if (params->uses_post || !params->extra_headers.empty())
    return false;

with the following callstack:

#2 0x000004532420 prerender::PrerenderManager::MaybeUsePrerenderedPage()
#3 0x000006b417c9 chrome::Navigate()
#4 0x000006b17c13 Browser::OpenURLFromTab()
#5 0x7f0cea9e1203 content::WebContentsImpl::RequestOpenURL()
#6 0x7f0cea06ae69 content::NavigatorImpl::RequestOpenURL()
#7 0x7f0cea095f9a content::RenderFrameHostImpl::OpenURL()
#8 0x7f0cea07eb32 content::RenderFrameHostImpl::OnOpenURL()
...

[Charlie Reis, 2016-07-22 22:29:34]

On 2016/07/22 22:26:15, Łukasz Anforowicz wrote:
> On 2016/07/22 21:08:01, Charlie Reis wrote:
> > On 2016/07/22 21:06:04, Łukasz Anforowicz wrote:
> > > On 2016/07/22 21:02:43, Charlie Reis wrote:
> > > > On 2016/07/22 20:54:22, mmenke wrote:
> > > > > On 2016/07/22 20:47:21, Łukasz Anforowicz wrote:
> > > > > > mmenke@, could you please take a look at
> > > > > > chrome/browser/prerender/prerender_browsertest.cc?
> > > > > 
> > > > > I'm confused...  I thought we stopped prerendering on navigation. 
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/prerender/prerender_conte...
> > > > 
> > > > That logic only cancels if the prerender makes a second commit on its
own,
> > > which
> > > > doesn't happen here.  Here, we've prerendered the destination page but
the
> > > user
> > > > does a POST, requiring a different server response.  (Lukasz, what makes
> us
> > > > cancel the prerender in this case, anyway?)
> > > 
> > > I don't think the prerender is "cancelled" in this case - it just ends up
> not
> > > being used at all (and goes away when the browser is closed - this is why
> the
> > > test expects FINAL_STATUS_APP_TERMINATING for the prerender).
> > 
> > My question was more about what made it not be used.  Normally a navigation
to
> a
> > URL that is prerendered means we'll use the prerendered page.
> 
> The new PrerenderBrowserTest.HttpPost test is hitting the following code in
> PrerenderManager::MaybeUsePrerenderedPage:
> 
>   // Don't prerender if the navigation involves some special parameters.
>   if (params->uses_post || !params->extra_headers.empty())
>     return false;
> 
> with the following callstack:
> 
> #2 0x000004532420 prerender::PrerenderManager::MaybeUsePrerenderedPage()
> #3 0x000006b417c9 chrome::Navigate()
> #4 0x000006b17c13 Browser::OpenURLFromTab()
> #5 0x7f0cea9e1203 content::WebContentsImpl::RequestOpenURL()
> #6 0x7f0cea06ae69 content::NavigatorImpl::RequestOpenURL()
> #7 0x7f0cea095f9a content::RenderFrameHostImpl::OpenURL()
> #8 0x7f0cea07eb32 content::RenderFrameHostImpl::OnOpenURL()
> ...

Great, that looks like the right reason.  Still LGTM.

mmenke@, does that make sense?

[mmenke, 2016-07-25 15:00:46]

LGTM, sorry I didn't get to this Friday.

[Łukasz Anforowicz, 2016-07-25 17:15:15]

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

[Łukasz Anforowicz, 2016-07-25 17:15:15]

Thanks.  sky@, could you take a look at
chrome/renderer/chrome_content_renderer_client.cc?  Note that the changes (i.e.
allowing POST to go through OpenURL code path) have already been signed-off by
the //content owner (creis@).

[sky, 2016-07-25 20:13:14]

LGTM

[Łukasz Anforowicz, 2016-07-25 20:14:03]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-07-25 20:14:35]

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

[commit-bot: I haz the power, 2016-07-25 21:33:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-25 21:35:06]

Description was changed from

==========
Allow forking for POST method (+ test that this works with prerender).

BUG=101395
==========

to

==========
Allow forking for POST method (+ test that this works with prerender).

BUG=101395

Committed: https://crrev.com/3e3be13746968b4e07c0ffdc707b05a14676e435
Cr-Commit-Position: refs/heads/master@{#407586}
==========

[commit-bot: I haz the power, 2016-07-25 21:35:08]

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