Test for navigating back to navigation that posts to a cross-site 307 redirect.

Test for navigating back to navigation that posts to a cross-site 307 redirect.

This test is currently broken in --site-per-process mode.

BUG=613004
Committed: https://crrev.com/2ad8fef46aa29f84a97e5600d3c5c8447f79c3ff
Cr-Commit-Position: refs/heads/master@{#395721}

[Łukasz Anforowicz, 2016-05-18 23:57:53]

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

[Łukasz Anforowicz, 2016-05-18 23:57:53]

Charlie, could you please take a look?

This is probably lower priority than other CRs you might have on your plate
(nothing here is needed before the branch point).

I thought that this new browser test would repro the page-id-check-related-crash
you mentioned - the one in https://crbug.com/568703.  The crash does not repro
(but the test uncovered a separate bug - we are loosing post data in
--site-per-process mode;  note that this is a separate bug from the already
known https://crbug.com/582211).

[Charlie Reis, 2016-05-23 20:51:35]

Thanks!

I'm hoping we can find a nicer way to do the 307 redirect than add a new
/cross-site-307/, but I wouldn't be surprised if we need to do this.  Otherwise
looks good.

https://codereview.chromium.org/1993093002/diff/1/content/browser/session_his...
File content/browser/session_history_browsertest.cc (right):

https://codereview.chromium.org/1993093002/diff/1/content/browser/session_his...
content/browser/session_history_browsertest.cc:507:
IN_PROC_BROWSER_TEST_F(SessionHistoryTest,
Let's put an explanation up here about the difference between 302 and 307 and
why we need to preserve the POST on 307 (and link to the bug).  That makes it
easier to tell at a glance what this test is doing, without reading the whole
thing.

https://codereview.chromium.org/1993093002/diff/1/content/public/test/browser...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1993093002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:299: std::string
prefix_307("/cross-site-307/");
Hmm, this feels like it's growing into a bit of a hack.  I'm curious if we can
avoid it, but maybe it's needed.

Does the ".mock_http_headers" feature give us something we can use here?  I've
seen Alex use it in tests like x-frame-options-deny.html.mock-http-headers. 
Maybe the form could post to a same-site test page that had a 307 redirect to
x.com/echoall in the mock headers?

Maybe that's doomed if it can't handle the test server port?  Or maybe there's
some support for ports?  I see redirect-to-title2.html.mock-http-headers uses
port 4000, though I'm not sure if that's handled properly.

If we have to keep this, it's worth documenting carefully.

https://codereview.chromium.org/1993093002/diff/1/testing/buildbot/filters/si...
File testing/buildbot/filters/site-per-process.content_browsertests.filter
(right):

https://codereview.chromium.org/1993093002/diff/1/testing/buildbot/filters/si...
testing/buildbot/filters/site-per-process.content_browsertests.filter:5: #
crbug.com/613004: POST data is lost when navigating back to a post that does
cross-site redirect
nit: Wrap at 80?

[Łukasz Anforowicz, 2016-05-24 19:08:08]

Charlie, can you take another look please?

https://codereview.chromium.org/1993093002/diff/1/content/browser/session_his...
File content/browser/session_history_browsertest.cc (right):

https://codereview.chromium.org/1993093002/diff/1/content/browser/session_his...
content/browser/session_history_browsertest.cc:507:
IN_PROC_BROWSER_TEST_F(SessionHistoryTest,
On 2016/05/23 20:51:35, Charlie Reis (slow to reply) wrote:
> Let's put an explanation up here about the difference between 302 and 307 and
> why we need to preserve the POST on 307 (and link to the bug).  That makes it
> easier to tell at a glance what this test is doing, without reading the whole
> thing.

Done.

https://codereview.chromium.org/1993093002/diff/1/content/public/test/browser...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1993093002/diff/1/content/public/test/browser...
content/public/test/browser_test_utils.cc:299: std::string
prefix_307("/cross-site-307/");
On 2016/05/23 20:51:35, Charlie Reis (slow to reply) wrote:
> Hmm, this feels like it's growing into a bit of a hack.

I think it would feel much less like a hack if it there was more consistency
between the 2 cases.  OTOH, I can't think of good options to achieve
consistency:

- Changing "/cross-site/" to "/cross-site-302/" seems bad, because 1) it
requires modifying all the existing tests and 2) it unnecessarily forces tests
to explicitly declare a mode of redirection when they might just want *any*
redirect (and not specifically a 302 or 307).

- Extracting redirect mode / http status from query parameters seems overly
complicated (have to add parsing of query parameters + be careful to preserve
other query parameters (and other url aspects like fragment navigations) after
the redirect).

> I'm curious if we can
> avoid it, but maybe it's needed.
> 
> Does the ".mock_http_headers" feature give us something we can use here?  I've
> seen Alex use it in tests like x-frame-options-deny.html.mock-http-headers. 
> Maybe the form could post to a same-site test page that had a 307 redirect to
> x.com/echoall in the mock headers?

mock-http-headers won't work, because they are sent back verbatim, and in this
case we need to emit Location: that matches the currently allocated port of
embedder_test_server.
> 
> Maybe that's doomed if it can't handle the test server port?  Or maybe there's
> some support for ports?  I see redirect-to-title2.html.mock-http-headers uses
> port 4000, though I'm not sure if that's handled properly.
> 
Thanks for the pointer.  It seems that
ResourceDispatcherHostBrowserTest::SetUpOnMainThread doesn't go through the
embedded_test_server, but intercepts http requests via
net::URLRequestMockHTTPJob::AddUrlHandlers.  I am guessing that this is why the
port number can be hardcoded here.

> If we have to keep this, it's worth documenting carefully.

I've added an explanation in the comment above declaration of
content::SetupCrossSiteRedirector.

https://codereview.chromium.org/1993093002/diff/1/testing/buildbot/filters/si...
File testing/buildbot/filters/site-per-process.content_browsertests.filter
(right):

https://codereview.chromium.org/1993093002/diff/1/testing/buildbot/filters/si...
testing/buildbot/filters/site-per-process.content_browsertests.filter:5: #
crbug.com/613004: POST data is lost when navigating back to a post that does
cross-site redirect
On 2016/05/23 20:51:35, Charlie Reis (slow to reply) wrote:
> nit: Wrap at 80?

Done.

[Charlie Reis, 2016-05-24 20:49:53]

Thanks for checking.  Ok, I'm good with the new prefix.  LGTM!

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
File content/browser/session_history_browsertest.cc (right):

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
content/browser/session_history_browsertest.cc:507: // Test that verifies that a
cross-process transfer doesn't loose session
nit: lose

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
content/browser/session_history_browsertest.cc:511: // a HTTP redirect reponse
(to preserve port number).  Therefore the test ends
nit: response

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
content/public/test/browser_test_utils.cc:305: base::CompareCase::SENSITIVE)) {
nit: Wrong indent.

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
content/public/test/browser_test_utils.h:269: // cases (i.e. to preserve HTTP
method and POST body across redirects as
nit: e.g.

[Łukasz Anforowicz, 2016-05-24 21:05:19]

Thanks for reviewing.

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
File content/browser/session_history_browsertest.cc (right):

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
content/browser/session_history_browsertest.cc:507: // Test that verifies that a
cross-process transfer doesn't loose session
On 2016/05/24 20:49:53, Charlie Reis wrote:
> nit: lose

Done.

https://codereview.chromium.org/1993093002/diff/20001/content/browser/session...
content/browser/session_history_browsertest.cc:511: // a HTTP redirect reponse
(to preserve port number).  Therefore the test ends
On 2016/05/24 20:49:53, Charlie Reis wrote:
> nit: response

Done.  I did try to proof-read what I wrote and missed this... :-/

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
File content/public/test/browser_test_utils.cc (right):

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
content/public/test/browser_test_utils.cc:305: base::CompareCase::SENSITIVE)) {
On 2016/05/24 20:49:53, Charlie Reis wrote:
> nit: Wrong indent.

Done (via git cl format).

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
File content/public/test/browser_test_utils.h (right):

https://codereview.chromium.org/1993093002/diff/20001/content/public/test/bro...
content/public/test/browser_test_utils.h:269: // cases (i.e. to preserve HTTP
method and POST body across redirects as
On 2016/05/24 20:49:53, Charlie Reis wrote:
> nit: e.g.

Done.  Thanks for pointing this out.  TIL :-)

[Łukasz Anforowicz, 2016-05-24 21:05:27]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-05-24 21:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1993093002/#ps40001
(title: "Addressed remaining CR feedback from creis@.")

[commit-bot: I haz the power, 2016-05-24 21:06:04]

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

[commit-bot: I haz the power, 2016-05-24 23:00:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-24 23:02:31]

Description was changed from

==========
Test for navigating back to navigation that posts to a cross-site 307 redirect.

This test is currently broken in --site-per-process mode.

BUG=613004
==========

to

==========
Test for navigating back to navigation that posts to a cross-site 307 redirect.

This test is currently broken in --site-per-process mode.

BUG=613004

Committed: https://crrev.com/2ad8fef46aa29f84a97e5600d3c5c8447f79c3ff
Cr-Commit-Position: refs/heads/master@{#395721}
==========

[commit-bot: I haz the power, 2016-05-24 23:02:32]

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