Clear provisional item on all server redirects.

Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.

Committed: https://crrev.com/235cc0c8888421ab2c621bac62bd3a2bc6297d05
Cr-Commit-Position: refs/heads/master@{#406861}

[Charlie Reis, 2016-07-19 03:16:13]

Description was changed from

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
==========

to

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-07-19 03:18:11]

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

[commit-bot: I haz the power, 2016-07-19 03:18:37]

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

[Charlie Reis, 2016-07-19 03:47:34]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-07-20 05:43:00]

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

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

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

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

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

[commit-bot: I haz the power, 2016-07-20 07:36:27]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-07-20 17:27:48]

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

[commit-bot: I haz the power, 2016-07-20 17:28:24]

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

[Charlie Reis, 2016-07-20 17:38:22]

creis@chromium.org changed reviewers:
+ japhet@chromium.org, nasko@chromium.org

[Charlie Reis, 2016-07-20 17:38:23]

Nate, can you review the fix in FrameLoader.cpp?  It affects your fix for
https://crbug.com/500554 (https://codereview.chromium.org/1175113009/), where
I'm not sure why it was limited to cross-origin redirects.

Nasko, can you review the browser test?  It's basically the same as
https://codereview.chromium.org/2136873002/, except same-origin (which catches
this case and matters in the original layout test).

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:362: if (m_provisionalItem
&&
!SecurityOrigin::create(m_provisionalItem->url())->isSameSchemeHostPort(SecurityOrigin::create(newURL).get()))
Nate, why was this restricted to cross-origin redirects?  Even same-origin
redirects wouldn't want to reuse things from the previous HistoryItem, like
scroll position, form state, DSN (affecting whether it's in-page or not), etc.

[Nate Chapin, 2016-07-20 17:42:28]

I don't remember why I limited it to cross-origin, either. If the tests are
happy, then LGTM.

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:362: if
(m_provisionalItem)
Nit: the if() isn't really necessary here.

[Charlie Reis, 2016-07-20 18:03:22]

Thanks!

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2159093002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:362: if
(m_provisionalItem)
On 2016/07/20 17:42:27, Nate Chapin wrote:
> Nit: the if() isn't really necessary here.

Done.

[Charlie Reis, 2016-07-20 18:03:28]

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

[commit-bot: I haz the power, 2016-07-20 18:04:28]

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

[nasko, 2016-07-20 18:16:39]

LGTM with a few nits.

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

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3188: GURL
blank_url(url::kAboutBlankURL);
nit: This isn't needed until 3225. It will be more readable closer to usage or
at the beginning of the test.

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3216: // 4.
Go back. The first iframe should redirect to a cross-site page with a
nit: same-origin page?

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3220:
shell()->web_contents()->GetController().GoBack();
nit: controller.GoBack()?

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3258:
shell()->web_contents()->GetController().GoBack();
nit: controller.GoBack()?

[Charlie Reis, 2016-07-20 19:35:08]

Thanks!

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

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3188: GURL
blank_url(url::kAboutBlankURL);
On 2016/07/20 18:16:39, nasko wrote:
> nit: This isn't needed until 3225. It will be more readable closer to usage or
> at the beginning of the test.

Done.

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3216: // 4.
Go back. The first iframe should redirect to a cross-site page with a
On 2016/07/20 18:16:39, nasko wrote:
> nit: same-origin page?

Done.

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3220:
shell()->web_contents()->GetController().GoBack();
On 2016/07/20 18:16:39, nasko wrote:
> nit: controller.GoBack()?

Done.

https://codereview.chromium.org/2159093002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3258:
shell()->web_contents()->GetController().GoBack();
On 2016/07/20 18:16:39, nasko wrote:
> nit: controller.GoBack()?

Done.

[Charlie Reis, 2016-07-20 19:35:12]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-07-20 19:35:12]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2159093002/#ps80001
(title: "Fix Nasko's comments")

[commit-bot: I haz the power, 2016-07-20 19:35:49]

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-20 20:57:51]

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

[commit-bot: I haz the power, 2016-07-20 20:57:52]

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

[Charlie Reis, 2016-07-20 21:09:08]

Description was changed from

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-07-20 21:09:08]

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

[Charlie Reis, 2016-07-20 21:10:14]

TBR'ing clamy@ for disabling the new test in PlzNavigate, as we did for
https://codereview.chromium.org/2136873002/.  We'll likely need to fix something
else in PlzNavigate to get these tests to pass there.

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

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2016-07-20 21:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2159093002/#ps100001
(title: "Disable test for PlzNavigate.")

[commit-bot: I haz the power, 2016-07-20 21:10:59]

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-20 21:13:36]

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

[commit-bot: I haz the power, 2016-07-20 21:13:38]

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

[Charlie Reis, 2016-07-20 21:29:29]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-07-20 21:29:54]

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

[commit-bot: I haz the power, 2016-07-20 23:16:06]

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

[commit-bot: I haz the power, 2016-07-20 23:16:07]

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

[Charlie Reis, 2016-07-20 23:21:10]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-07-20 23:21:36]

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-21 00:59:54]

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

[commit-bot: I haz the power, 2016-07-21 00:59:56]

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

[Charlie Reis, 2016-07-21 04:56:57]

Description was changed from

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
==========

[Charlie Reis, 2016-07-21 04:57:09]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-07-21 04:57:27]

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-21 06:32:47]

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

[commit-bot: I haz the power, 2016-07-21 06:32:49]

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

[Charlie Reis, 2016-07-21 14:42:39]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:43:03]

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-21 15:44:14]

Description was changed from

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
==========

to

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
==========

[commit-bot: I haz the power, 2016-07-21 15:44:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-21 15:45:53]

Description was changed from

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.
==========

to

==========
Clear provisional item on all server redirects.

Whether a redirect is cross-origin or not, the result will be a new
document and should not reuse the provisional HistoryItem during a
back/forward navigation.

BUG=585194
TBR=clamy
TEST=http/tests/navigation/back-to-redirect-with-frame.php
     in --isolate-sites-for-testing mode.

Committed: https://crrev.com/235cc0c8888421ab2c621bac62bd3a2bc6297d05
Cr-Commit-Position: refs/heads/master@{#406861}
==========

[commit-bot: I haz the power, 2016-07-21 15:45:55]

Patchset 6 (id:??) landed as
https://crrev.com/235cc0c8888421ab2c621bac62bd3a2bc6297d05
Cr-Commit-Position: refs/heads/master@{#406861}