Fix NavigationControllerBrowserTest.SubframeForwardRedirect

Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess determines that a cross process redirect should happen for the subframe (because of site isolation) then we might need to use a new site instance.

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

Review-Url: https://codereview.chromium.org/2594263004
Cr-Commit-Position: refs/heads/master@{#442053}
Committed: https://chromium.googlesource.com/chromium/src/+/015ba065ac2806f71b4c3b9e05b6d9013f473de1

[jam, 2016-12-22 17:05:16]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect

BUG=
==========

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect

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

[jam, 2016-12-22 17:05:29]

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

[commit-bot: I haz the power, 2016-12-22 17:05:43]

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

[jam, 2016-12-22 17:06:40]

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

[commit-bot: I haz the power, 2016-12-22 17:06:44]

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-12-22 18:34:48]

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

[commit-bot: I haz the power, 2016-12-22 18:34:49]

Dry run: 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_...)

[jam, 2016-12-22 22:05:43]

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

[commit-bot: I haz the power, 2016-12-22 22:06:01]

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

[jam, 2016-12-22 23:01:21]

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

[commit-bot: I haz the power, 2016-12-22 23:02:12]

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-12-22 23:26:35]

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

[commit-bot: I haz the power, 2016-12-22 23:26:36]

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

[jam, 2016-12-22 23:45:32]

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

[commit-bot: I haz the power, 2016-12-22 23:46:04]

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

[jam, 2016-12-22 23:46:30]

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

[commit-bot: I haz the power, 2016-12-22 23:46:54]

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

[jam, 2016-12-22 23:49:59]

Patchset #3 (id:40001) has been deleted

[jam, 2016-12-22 23:50:12]

Patchset #3 (id:60001) has been deleted

[jam, 2016-12-22 23:50:47]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-22 23:51:35]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jam, 2016-12-22 23:51:53]

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

[commit-bot: I haz the power, 2016-12-22 23:52:19]

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

[jam, 2016-12-22 23:53:01]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect

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

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect

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

[jam, 2016-12-23 00:08:15]

Patchset #1 (id:1) has been deleted

[jam, 2016-12-23 00:08:20]

Patchset #1 (id:20001) has been deleted

[jam, 2016-12-23 00:08:52]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect

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

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

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

[jam, 2016-12-23 00:18:53]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

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

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the
constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess
determines that a cross process redirect should happen for a subframe (because
of site isolation) then we might need to use a new site instance.

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

[jam, 2016-12-23 01:09:15]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the
constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess
determines that a cross process redirect should happen for a subframe (because
of site isolation) then we might need to use a new site instance.

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

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the
constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess
determines that a cross process redirect should happen for the subframe (because
of site isolation) then we might need to use a new site instance.

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

[jam, 2016-12-23 01:09:16]

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

[commit-bot: I haz the power, 2016-12-23 01:11:00]

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

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

Dry run: This issue passed the CQ dry run.

[jam, 2016-12-27 16:52:36]

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

[commit-bot: I haz the power, 2016-12-27 16:52:46]

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-12-27 18:46:02]

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

[commit-bot: I haz the power, 2016-12-27 18:46:03]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-03 16:33:40]

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

[commit-bot: I haz the power, 2017-01-03 16:33:52]

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, 2017-01-03 18:10:07]

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

[commit-bot: I haz the power, 2017-01-03 18:10:08]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-01-04 18:39:51]

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

[nasko, 2017-01-04 18:39:52]

Looks good, but adding Charlie for an extra pair of eyes. It should be
PlzNavigate only change and not affect the current navigation code.

[Charlie Reis, 2017-01-04 20:28:56]

On 2017/01/04 18:39:52, nasko wrote:
> Looks good, but adding Charlie for an extra pair of eyes. It should be
> PlzNavigate only change and not affect the current navigation code.

I'm looking, but this is a bit complicated to reason about.  (I'm trying to
understand if it's safe to replace the SiteInstance we created earlier given
that it has side effects, as well as why only some parameters are passed to
GetSiteInstanceForNavigation the second time.)  I'm seeing if I can come up with
a better recommendation.

[jam, 2017-01-04 23:46:54]

On 2017/01/04 20:28:56, Charlie Reis wrote:
> On 2017/01/04 18:39:52, nasko wrote:
> > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > PlzNavigate only change and not affect the current navigation code.
> 
> I'm looking, but this is a bit complicated to reason about.  (I'm trying to
> understand if it's safe to replace the SiteInstance we created earlier given
> that it has side effects, as well as why only some parameters are passed to
> GetSiteInstanceForNavigation the second time.) 

Thanks. The reason request.dest_site_instance() isn't passed in is because it'll
be used, although in the case that we need a new process we don't want to use
the existing process' siteinstance.

> I'm seeing if I can come up with
> a better recommendation.

[Charlie Reis, 2017-01-05 23:15:16]

On 2017/01/04 23:46:54, jam wrote:
> On 2017/01/04 20:28:56, Charlie Reis wrote:
> > On 2017/01/04 18:39:52, nasko wrote:
> > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > PlzNavigate only change and not affect the current navigation code.
> > 
> > I'm looking, but this is a bit complicated to reason about.  (I'm trying to
> > understand if it's safe to replace the SiteInstance we created earlier given
> > that it has side effects, as well as why only some parameters are passed to
> > GetSiteInstanceForNavigation the second time.) 
> 
> Thanks. The reason request.dest_site_instance() isn't passed in is because
it'll
> be used, although in the case that we need a new process we don't want to use
> the existing process' siteinstance.
> 
> > I'm seeing if I can come up with
> > a better recommendation.

On 2017/01/04 23:46:54, jam wrote:
> On 2017/01/04 20:28:56, Charlie Reis wrote:
> > On 2017/01/04 18:39:52, nasko wrote:
> > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > PlzNavigate only change and not affect the current navigation code.
> > 
> > I'm looking, but this is a bit complicated to reason about.  (I'm trying to
> > understand if it's safe to replace the SiteInstance we created earlier given
> > that it has side effects, as well as why only some parameters are passed to
> > GetSiteInstanceForNavigation the second time.) 
> 
> Thanks. The reason request.dest_site_instance() isn't passed in is because
it'll
> be used, although in the case that we need a new process we don't want to use
> the existing process' siteinstance.
> 
> > I'm seeing if I can come up with
> > a better recommendation.

Ok, I've looked more closely and I think I have a simpler fix that gets the test
to pass and works in manual testing as well.

This test is setting up a case where a FrameNavigationEntry redirects cross-site
when we go back/forward to it.  Normally we do want to load a history item in
the SiteInstance it was in before, so it's fine for NavigationRequest to track
the dest_site_instance_ and use it at commit time in PlzNavigate.  That stops
being true for redirects, though, and we already have some similar logic to
clear any child history items if a redirect occurs.  When we redirect, we should
find a SiteInstance for the entry as we would for any other new navigation.

As a result, I think a cleaner fix for this bug would be simply clearing
dest_site_instance_ in NavigationRequest::OnRequestRedirected, since it no
longer applies if a redirect occurs.  That seems like a closer fix to the
underlying problem, and it avoids the need to change anything in
RenderFrameHostManager.

Hope that helps!

[jam, 2017-01-05 23:55:51]

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

[commit-bot: I haz the power, 2017-01-05 23:56:51]

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

[jam, 2017-01-05 23:58:06]

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

[commit-bot: I haz the power, 2017-01-05 23:58:55]

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

[jam, 2017-01-06 00:55:44]

On 2017/01/05 23:15:16, Charlie Reis wrote:
> On 2017/01/04 23:46:54, jam wrote:
> > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > On 2017/01/04 18:39:52, nasko wrote:
> > > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > > PlzNavigate only change and not affect the current navigation code.
> > > 
> > > I'm looking, but this is a bit complicated to reason about.  (I'm trying
to
> > > understand if it's safe to replace the SiteInstance we created earlier
given
> > > that it has side effects, as well as why only some parameters are passed
to
> > > GetSiteInstanceForNavigation the second time.) 
> > 
> > Thanks. The reason request.dest_site_instance() isn't passed in is because
> it'll
> > be used, although in the case that we need a new process we don't want to
use
> > the existing process' siteinstance.
> > 
> > > I'm seeing if I can come up with
> > > a better recommendation.
> 
> On 2017/01/04 23:46:54, jam wrote:
> > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > On 2017/01/04 18:39:52, nasko wrote:
> > > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > > PlzNavigate only change and not affect the current navigation code.
> > > 
> > > I'm looking, but this is a bit complicated to reason about.  (I'm trying
to
> > > understand if it's safe to replace the SiteInstance we created earlier
given
> > > that it has side effects, as well as why only some parameters are passed
to
> > > GetSiteInstanceForNavigation the second time.) 
> > 
> > Thanks. The reason request.dest_site_instance() isn't passed in is because
> it'll
> > be used, although in the case that we need a new process we don't want to
use
> > the existing process' siteinstance.
> > 
> > > I'm seeing if I can come up with
> > > a better recommendation.
> 
> Ok, I've looked more closely and I think I have a simpler fix that gets the
test
> to pass and works in manual testing as well.
> 
> This test is setting up a case where a FrameNavigationEntry redirects
cross-site
> when we go back/forward to it.  Normally we do want to load a history item in
> the SiteInstance it was in before, so it's fine for NavigationRequest to track
> the dest_site_instance_ and use it at commit time in PlzNavigate.  That stops
> being true for redirects, though, and we already have some similar logic to
> clear any child history items if a redirect occurs.  When we redirect, we
should
> find a SiteInstance for the entry as we would for any other new navigation.
> 
> As a result, I think a cleaner fix for this bug would be simply clearing
> dest_site_instance_ in NavigationRequest::OnRequestRedirected, since it no
> longer applies if a redirect occurs.  That seems like a closer fix to the
> underlying problem, and it avoids the need to change anything in
> RenderFrameHostManager.
> 
> Hope that helps!

Hey Charlie, thanks for the suggestion. I tried that approach, however it seems
to have broken two few tests that you wrote with plznavigate and site per
process:

NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry

(see patchset 3 which is with plznavigate enabled compared to
https://codereview.chromium.org/2385413004/#ps260001 which is plznavigate
enabled on trunk)

[Charlie Reis, 2017-01-06 01:06:32]

On 2017/01/06 00:55:44, jam wrote:
> On 2017/01/05 23:15:16, Charlie Reis wrote:
> > On 2017/01/04 23:46:54, jam wrote:
> > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > > > PlzNavigate only change and not affect the current navigation code.
> > > > 
> > > > I'm looking, but this is a bit complicated to reason about.  (I'm trying
> to
> > > > understand if it's safe to replace the SiteInstance we created earlier
> given
> > > > that it has side effects, as well as why only some parameters are passed
> to
> > > > GetSiteInstanceForNavigation the second time.) 
> > > 
> > > Thanks. The reason request.dest_site_instance() isn't passed in is because
> > it'll
> > > be used, although in the case that we need a new process we don't want to
> use
> > > the existing process' siteinstance.
> > > 
> > > > I'm seeing if I can come up with
> > > > a better recommendation.
> > 
> > On 2017/01/04 23:46:54, jam wrote:
> > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > Looks good, but adding Charlie for an extra pair of eyes. It should be
> > > > > PlzNavigate only change and not affect the current navigation code.
> > > > 
> > > > I'm looking, but this is a bit complicated to reason about.  (I'm trying
> to
> > > > understand if it's safe to replace the SiteInstance we created earlier
> given
> > > > that it has side effects, as well as why only some parameters are passed
> to
> > > > GetSiteInstanceForNavigation the second time.) 
> > > 
> > > Thanks. The reason request.dest_site_instance() isn't passed in is because
> > it'll
> > > be used, although in the case that we need a new process we don't want to
> use
> > > the existing process' siteinstance.
> > > 
> > > > I'm seeing if I can come up with
> > > > a better recommendation.
> > 
> > Ok, I've looked more closely and I think I have a simpler fix that gets the
> test
> > to pass and works in manual testing as well.
> > 
> > This test is setting up a case where a FrameNavigationEntry redirects
> cross-site
> > when we go back/forward to it.  Normally we do want to load a history item
in
> > the SiteInstance it was in before, so it's fine for NavigationRequest to
track
> > the dest_site_instance_ and use it at commit time in PlzNavigate.  That
stops
> > being true for redirects, though, and we already have some similar logic to
> > clear any child history items if a redirect occurs.  When we redirect, we
> should
> > find a SiteInstance for the entry as we would for any other new navigation.
> > 
> > As a result, I think a cleaner fix for this bug would be simply clearing
> > dest_site_instance_ in NavigationRequest::OnRequestRedirected, since it no
> > longer applies if a redirect occurs.  That seems like a closer fix to the
> > underlying problem, and it avoids the need to change anything in
> > RenderFrameHostManager.
> > 
> > Hope that helps!
> 
> Hey Charlie, thanks for the suggestion. I tried that approach, however it
seems
> to have broken two few tests that you wrote with plznavigate and site per
> process:
> 
> NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> 
> (see patchset 3 which is with plznavigate enabled compared to
> https://codereview.chromium.org/2385413004/#ps260001 which is plznavigate
> enabled on trunk)

Ok, I'll take a look at those in a debugger tomorrow to see what's going wrong.

[commit-bot: I haz the power, 2017-01-06 01:43:13]

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

[commit-bot: I haz the power, 2017-01-06 01:43:14]

Dry run: 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_...)

[jam, 2017-01-06 18:58:13]

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

[commit-bot: I haz the power, 2017-01-06 18:58:46]

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

[jam, 2017-01-06 18:58:47]

On 2017/01/06 01:06:32, Charlie Reis wrote:
> On 2017/01/06 00:55:44, jam wrote:
> > On 2017/01/05 23:15:16, Charlie Reis wrote:
> > > On 2017/01/04 23:46:54, jam wrote:
> > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > Looks good, but adding Charlie for an extra pair of eyes. It should
be
> > > > > > PlzNavigate only change and not affect the current navigation code.
> > > > > 
> > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
trying
> > to
> > > > > understand if it's safe to replace the SiteInstance we created earlier
> > given
> > > > > that it has side effects, as well as why only some parameters are
passed
> > to
> > > > > GetSiteInstanceForNavigation the second time.) 
> > > > 
> > > > Thanks. The reason request.dest_site_instance() isn't passed in is
because
> > > it'll
> > > > be used, although in the case that we need a new process we don't want
to
> > use
> > > > the existing process' siteinstance.
> > > > 
> > > > > I'm seeing if I can come up with
> > > > > a better recommendation.
> > > 
> > > On 2017/01/04 23:46:54, jam wrote:
> > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > Looks good, but adding Charlie for an extra pair of eyes. It should
be
> > > > > > PlzNavigate only change and not affect the current navigation code.
> > > > > 
> > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
trying
> > to
> > > > > understand if it's safe to replace the SiteInstance we created earlier
> > given
> > > > > that it has side effects, as well as why only some parameters are
passed
> > to
> > > > > GetSiteInstanceForNavigation the second time.) 
> > > > 
> > > > Thanks. The reason request.dest_site_instance() isn't passed in is
because
> > > it'll
> > > > be used, although in the case that we need a new process we don't want
to
> > use
> > > > the existing process' siteinstance.
> > > > 
> > > > > I'm seeing if I can come up with
> > > > > a better recommendation.
> > > 
> > > Ok, I've looked more closely and I think I have a simpler fix that gets
the
> > test
> > > to pass and works in manual testing as well.
> > > 
> > > This test is setting up a case where a FrameNavigationEntry redirects
> > cross-site
> > > when we go back/forward to it.  Normally we do want to load a history item
> in
> > > the SiteInstance it was in before, so it's fine for NavigationRequest to
> track
> > > the dest_site_instance_ and use it at commit time in PlzNavigate.  That
> stops
> > > being true for redirects, though, and we already have some similar logic
to
> > > clear any child history items if a redirect occurs.  When we redirect, we
> > should
> > > find a SiteInstance for the entry as we would for any other new
navigation.
> > > 
> > > As a result, I think a cleaner fix for this bug would be simply clearing
> > > dest_site_instance_ in NavigationRequest::OnRequestRedirected, since it no
> > > longer applies if a redirect occurs.  That seems like a closer fix to the
> > > underlying problem, and it avoids the need to change anything in
> > > RenderFrameHostManager.
> > > 
> > > Hope that helps!
> > 
> > Hey Charlie, thanks for the suggestion. I tried that approach, however it
> seems
> > to have broken two few tests that you wrote with plznavigate and site per
> > process:
> > 
> > NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> > NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> > 
> > (see patchset 3 which is with plznavigate enabled compared to
> > https://codereview.chromium.org/2385413004/#ps260001 which is plznavigate
> > enabled on trunk)
> 
> Ok, I'll take a look at those in a debugger tomorrow to see what's going
wrong.

btw I had a chance to debug them, and it looks like the dcheck that's failing
needs to be updated after your suggestion to NavigationRequest. ptal

[Charlie Reis, 2017-01-06 20:13:34]

On 2017/01/06 18:58:47, jam wrote:
> On 2017/01/06 01:06:32, Charlie Reis wrote:
> > On 2017/01/06 00:55:44, jam wrote:
> > > On 2017/01/05 23:15:16, Charlie Reis wrote:
> > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
should
> be
> > > > > > > PlzNavigate only change and not affect the current navigation
code.
> > > > > > 
> > > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
> trying
> > > to
> > > > > > understand if it's safe to replace the SiteInstance we created
earlier
> > > given
> > > > > > that it has side effects, as well as why only some parameters are
> passed
> > > to
> > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > 
> > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> because
> > > > it'll
> > > > > be used, although in the case that we need a new process we don't want
> to
> > > use
> > > > > the existing process' siteinstance.
> > > > > 
> > > > > > I'm seeing if I can come up with
> > > > > > a better recommendation.
> > > > 
> > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
should
> be
> > > > > > > PlzNavigate only change and not affect the current navigation
code.
> > > > > > 
> > > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
> trying
> > > to
> > > > > > understand if it's safe to replace the SiteInstance we created
earlier
> > > given
> > > > > > that it has side effects, as well as why only some parameters are
> passed
> > > to
> > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > 
> > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> because
> > > > it'll
> > > > > be used, although in the case that we need a new process we don't want
> to
> > > use
> > > > > the existing process' siteinstance.
> > > > > 
> > > > > > I'm seeing if I can come up with
> > > > > > a better recommendation.
> > > > 
> > > > Ok, I've looked more closely and I think I have a simpler fix that gets
> the
> > > test
> > > > to pass and works in manual testing as well.
> > > > 
> > > > This test is setting up a case where a FrameNavigationEntry redirects
> > > cross-site
> > > > when we go back/forward to it.  Normally we do want to load a history
item
> > in
> > > > the SiteInstance it was in before, so it's fine for NavigationRequest to
> > track
> > > > the dest_site_instance_ and use it at commit time in PlzNavigate.  That
> > stops
> > > > being true for redirects, though, and we already have some similar logic
> to
> > > > clear any child history items if a redirect occurs.  When we redirect,
we
> > > should
> > > > find a SiteInstance for the entry as we would for any other new
> navigation.
> > > > 
> > > > As a result, I think a cleaner fix for this bug would be simply clearing
> > > > dest_site_instance_ in NavigationRequest::OnRequestRedirected, since it
no
> > > > longer applies if a redirect occurs.  That seems like a closer fix to
the
> > > > underlying problem, and it avoids the need to change anything in
> > > > RenderFrameHostManager.
> > > > 
> > > > Hope that helps!
> > > 
> > > Hey Charlie, thanks for the suggestion. I tried that approach, however it
> > seems
> > > to have broken two few tests that you wrote with plznavigate and site per
> > > process:
> > > 
> > > NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> > > NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> > > 
> > > (see patchset 3 which is with plznavigate enabled compared to
> > > https://codereview.chromium.org/2385413004/#ps260001 which is plznavigate
> > > enabled on trunk)
> > 
> > Ok, I'll take a look at those in a debugger tomorrow to see what's going
> wrong.
> 
> btw I had a chance to debug them, and it looks like the dcheck that's failing
> needs to be updated after your suggestion to NavigationRequest. ptal

Cool.  I think I'm ok with that change-- let me confirm what happens in the
non-PlzNavigate case, since I'm surprised we don't have a problem there.  I'll
confirm unless this introduces a problem.

[Charlie Reis, 2017-01-06 20:14:10]

On 2017/01/06 20:13:34, Charlie Reis wrote:
> confirm unless this introduces a problem.

s/confirm/approve/

[Charlie Reis, 2017-01-06 20:29:26]

On 2017/01/06 20:13:34, Charlie Reis wrote:
> On 2017/01/06 18:58:47, jam wrote:
> > On 2017/01/06 01:06:32, Charlie Reis wrote:
> > > On 2017/01/06 00:55:44, jam wrote:
> > > > On 2017/01/05 23:15:16, Charlie Reis wrote:
> > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> should
> > be
> > > > > > > > PlzNavigate only change and not affect the current navigation
> code.
> > > > > > > 
> > > > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
> > trying
> > > > to
> > > > > > > understand if it's safe to replace the SiteInstance we created
> earlier
> > > > given
> > > > > > > that it has side effects, as well as why only some parameters are
> > passed
> > > > to
> > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > 
> > > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> > because
> > > > > it'll
> > > > > > be used, although in the case that we need a new process we don't
want
> > to
> > > > use
> > > > > > the existing process' siteinstance.
> > > > > > 
> > > > > > > I'm seeing if I can come up with
> > > > > > > a better recommendation.
> > > > > 
> > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> should
> > be
> > > > > > > > PlzNavigate only change and not affect the current navigation
> code.
> > > > > > > 
> > > > > > > I'm looking, but this is a bit complicated to reason about.  (I'm
> > trying
> > > > to
> > > > > > > understand if it's safe to replace the SiteInstance we created
> earlier
> > > > given
> > > > > > > that it has side effects, as well as why only some parameters are
> > passed
> > > > to
> > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > 
> > > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> > because
> > > > > it'll
> > > > > > be used, although in the case that we need a new process we don't
want
> > to
> > > > use
> > > > > > the existing process' siteinstance.
> > > > > > 
> > > > > > > I'm seeing if I can come up with
> > > > > > > a better recommendation.
> > > > > 
> > > > > Ok, I've looked more closely and I think I have a simpler fix that
gets
> > the
> > > > test
> > > > > to pass and works in manual testing as well.
> > > > > 
> > > > > This test is setting up a case where a FrameNavigationEntry redirects
> > > > cross-site
> > > > > when we go back/forward to it.  Normally we do want to load a history
> item
> > > in
> > > > > the SiteInstance it was in before, so it's fine for NavigationRequest
to
> > > track
> > > > > the dest_site_instance_ and use it at commit time in PlzNavigate. 
That
> > > stops
> > > > > being true for redirects, though, and we already have some similar
logic
> > to
> > > > > clear any child history items if a redirect occurs.  When we redirect,
> we
> > > > should
> > > > > find a SiteInstance for the entry as we would for any other new
> > navigation.
> > > > > 
> > > > > As a result, I think a cleaner fix for this bug would be simply
clearing
> > > > > dest_site_instance_ in NavigationRequest::OnRequestRedirected, since
it
> no
> > > > > longer applies if a redirect occurs.  That seems like a closer fix to
> the
> > > > > underlying problem, and it avoids the need to change anything in
> > > > > RenderFrameHostManager.
> > > > > 
> > > > > Hope that helps!
> > > > 
> > > > Hey Charlie, thanks for the suggestion. I tried that approach, however
it
> > > seems
> > > > to have broken two few tests that you wrote with plznavigate and site
per
> > > > process:
> > > > 
> > > > NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> > > > NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> > > > 
> > > > (see patchset 3 which is with plznavigate enabled compared to
> > > > https://codereview.chromium.org/2385413004/#ps260001 which is
plznavigate
> > > > enabled on trunk)
> > > 
> > > Ok, I'll take a look at those in a debugger tomorrow to see what's going
> > wrong.
> > 
> > btw I had a chance to debug them, and it looks like the dcheck that's
failing
> > needs to be updated after your suggestion to NavigationRequest. ptal
> 
> Cool.  I think I'm ok with that change-- let me confirm what happens in the
> non-PlzNavigate case, since I'm surprised we don't have a problem there.  I'll
> approve unless this introduces a problem.

Ok, this LGTM.

The non-PlzNavigate --site-per-process case actually gets this wrong today.  It
misclassifies the redirect as NAVIGATION_TYPE_NEW_PAGE rather than
NAVIGATION_TYPE_EXISTING_PAGE.  That means we don't crash on the DCHECK, but it
also means we'll incorrectly clear the forward history.

Looks like PlzNavigate will get this right with your CL.  I'll file a bug for
the non-PlzNavigate case, so feel free to proceed.  Thanks!

[jam, 2017-01-06 20:35:12]

On 2017/01/06 20:29:26, Charlie Reis wrote:
> On 2017/01/06 20:13:34, Charlie Reis wrote:
> > On 2017/01/06 18:58:47, jam wrote:
> > > On 2017/01/06 01:06:32, Charlie Reis wrote:
> > > > On 2017/01/06 00:55:44, jam wrote:
> > > > > On 2017/01/05 23:15:16, Charlie Reis wrote:
> > > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> > should
> > > be
> > > > > > > > > PlzNavigate only change and not affect the current navigation
> > code.
> > > > > > > > 
> > > > > > > > I'm looking, but this is a bit complicated to reason about. 
(I'm
> > > trying
> > > > > to
> > > > > > > > understand if it's safe to replace the SiteInstance we created
> > earlier
> > > > > given
> > > > > > > > that it has side effects, as well as why only some parameters
are
> > > passed
> > > > > to
> > > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > > 
> > > > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> > > because
> > > > > > it'll
> > > > > > > be used, although in the case that we need a new process we don't
> want
> > > to
> > > > > use
> > > > > > > the existing process' siteinstance.
> > > > > > > 
> > > > > > > > I'm seeing if I can come up with
> > > > > > > > a better recommendation.
> > > > > > 
> > > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> > should
> > > be
> > > > > > > > > PlzNavigate only change and not affect the current navigation
> > code.
> > > > > > > > 
> > > > > > > > I'm looking, but this is a bit complicated to reason about. 
(I'm
> > > trying
> > > > > to
> > > > > > > > understand if it's safe to replace the SiteInstance we created
> > earlier
> > > > > given
> > > > > > > > that it has side effects, as well as why only some parameters
are
> > > passed
> > > > > to
> > > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > > 
> > > > > > > Thanks. The reason request.dest_site_instance() isn't passed in is
> > > because
> > > > > > it'll
> > > > > > > be used, although in the case that we need a new process we don't
> want
> > > to
> > > > > use
> > > > > > > the existing process' siteinstance.
> > > > > > > 
> > > > > > > > I'm seeing if I can come up with
> > > > > > > > a better recommendation.
> > > > > > 
> > > > > > Ok, I've looked more closely and I think I have a simpler fix that
> gets
> > > the
> > > > > test
> > > > > > to pass and works in manual testing as well.
> > > > > > 
> > > > > > This test is setting up a case where a FrameNavigationEntry
redirects
> > > > > cross-site
> > > > > > when we go back/forward to it.  Normally we do want to load a
history
> > item
> > > > in
> > > > > > the SiteInstance it was in before, so it's fine for
NavigationRequest
> to
> > > > track
> > > > > > the dest_site_instance_ and use it at commit time in PlzNavigate. 
> That
> > > > stops
> > > > > > being true for redirects, though, and we already have some similar
> logic
> > > to
> > > > > > clear any child history items if a redirect occurs.  When we
redirect,
> > we
> > > > > should
> > > > > > find a SiteInstance for the entry as we would for any other new
> > > navigation.
> > > > > > 
> > > > > > As a result, I think a cleaner fix for this bug would be simply
> clearing
> > > > > > dest_site_instance_ in NavigationRequest::OnRequestRedirected, since
> it
> > no
> > > > > > longer applies if a redirect occurs.  That seems like a closer fix
to
> > the
> > > > > > underlying problem, and it avoids the need to change anything in
> > > > > > RenderFrameHostManager.
> > > > > > 
> > > > > > Hope that helps!
> > > > > 
> > > > > Hey Charlie, thanks for the suggestion. I tried that approach, however
> it
> > > > seems
> > > > > to have broken two few tests that you wrote with plznavigate and site
> per
> > > > > process:
> > > > > 
> > > > > NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> > > > > NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> > > > > 
> > > > > (see patchset 3 which is with plznavigate enabled compared to
> > > > > https://codereview.chromium.org/2385413004/#ps260001 which is
> plznavigate
> > > > > enabled on trunk)
> > > > 
> > > > Ok, I'll take a look at those in a debugger tomorrow to see what's going
> > > wrong.
> > > 
> > > btw I had a chance to debug them, and it looks like the dcheck that's
> failing
> > > needs to be updated after your suggestion to NavigationRequest. ptal
> > 
> > Cool.  I think I'm ok with that change-- let me confirm what happens in the
> > non-PlzNavigate case, since I'm surprised we don't have a problem there. 
I'll
> > approve unless this introduces a problem.
> 
> Ok, this LGTM.
> 
> The non-PlzNavigate --site-per-process case actually gets this wrong today. 
It
> misclassifies the redirect as NAVIGATION_TYPE_NEW_PAGE rather than
> NAVIGATION_TYPE_EXISTING_PAGE.  That means we don't crash on the DCHECK, but
it
> also means we'll incorrectly clear the forward history.
> 
> Looks like PlzNavigate will get this right with your CL.  I'll file a bug for
> the non-PlzNavigate case, so feel free to proceed.  Thanks!

cool, thanks for checking and for the simpler fix.

[jam, 2017-01-06 20:35:17]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-01-06 20:35:21]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-06 20:35:46]

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, 2017-01-06 21:17:04]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483734921935980,
"parent_rev": "4f1fd6a03a5d4d6299dfd02cc8485b42720d1e08", "commit_rev":
"015ba065ac2806f71b4c3b9e05b6d9013f473de1"}

[commit-bot: I haz the power, 2017-01-06 21:17:36]

Description was changed from

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the
constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess
determines that a cross process redirect should happen for the subframe (because
of site isolation) then we might need to use a new site instance.

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

to

==========
Fix NavigationControllerBrowserTest.SubframeForwardRedirect with PlzNavigate and
site isolation enabled.

The problem is that NavigationRequest::dest_site_instance_ is set in the
constructor of that class. If RenderFrameHostManager::CanSubframeSwapProcess
determines that a cross process redirect should happen for the subframe (because
of site isolation) then we might need to use a new site instance.

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

Review-Url: https://codereview.chromium.org/2594263004
Cr-Commit-Position: refs/heads/master@{#442053}
Committed:
https://chromium.googlesource.com/chromium/src/+/015ba065ac2806f71b4c3b9e05b6...
==========

[commit-bot: I haz the power, 2017-01-06 21:17:37]

Committed patchset #5 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/015ba065ac2806f71b4c3b9e05b6...

[Charlie Reis, 2017-01-06 21:37:28]

On 2017/01/06 20:35:12, jam wrote:
> On 2017/01/06 20:29:26, Charlie Reis wrote:
> > On 2017/01/06 20:13:34, Charlie Reis wrote:
> > > On 2017/01/06 18:58:47, jam wrote:
> > > > On 2017/01/06 01:06:32, Charlie Reis wrote:
> > > > > On 2017/01/06 00:55:44, jam wrote:
> > > > > > On 2017/01/05 23:15:16, Charlie Reis wrote:
> > > > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> > > should
> > > > be
> > > > > > > > > > PlzNavigate only change and not affect the current
navigation
> > > code.
> > > > > > > > > 
> > > > > > > > > I'm looking, but this is a bit complicated to reason about. 
> (I'm
> > > > trying
> > > > > > to
> > > > > > > > > understand if it's safe to replace the SiteInstance we created
> > > earlier
> > > > > > given
> > > > > > > > > that it has side effects, as well as why only some parameters
> are
> > > > passed
> > > > > > to
> > > > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > > > 
> > > > > > > > Thanks. The reason request.dest_site_instance() isn't passed in
is
> > > > because
> > > > > > > it'll
> > > > > > > > be used, although in the case that we need a new process we
don't
> > want
> > > > to
> > > > > > use
> > > > > > > > the existing process' siteinstance.
> > > > > > > > 
> > > > > > > > > I'm seeing if I can come up with
> > > > > > > > > a better recommendation.
> > > > > > > 
> > > > > > > On 2017/01/04 23:46:54, jam wrote:
> > > > > > > > On 2017/01/04 20:28:56, Charlie Reis wrote:
> > > > > > > > > On 2017/01/04 18:39:52, nasko wrote:
> > > > > > > > > > Looks good, but adding Charlie for an extra pair of eyes. It
> > > should
> > > > be
> > > > > > > > > > PlzNavigate only change and not affect the current
navigation
> > > code.
> > > > > > > > > 
> > > > > > > > > I'm looking, but this is a bit complicated to reason about. 
> (I'm
> > > > trying
> > > > > > to
> > > > > > > > > understand if it's safe to replace the SiteInstance we created
> > > earlier
> > > > > > given
> > > > > > > > > that it has side effects, as well as why only some parameters
> are
> > > > passed
> > > > > > to
> > > > > > > > > GetSiteInstanceForNavigation the second time.) 
> > > > > > > > 
> > > > > > > > Thanks. The reason request.dest_site_instance() isn't passed in
is
> > > > because
> > > > > > > it'll
> > > > > > > > be used, although in the case that we need a new process we
don't
> > want
> > > > to
> > > > > > use
> > > > > > > > the existing process' siteinstance.
> > > > > > > > 
> > > > > > > > > I'm seeing if I can come up with
> > > > > > > > > a better recommendation.
> > > > > > > 
> > > > > > > Ok, I've looked more closely and I think I have a simpler fix that
> > gets
> > > > the
> > > > > > test
> > > > > > > to pass and works in manual testing as well.
> > > > > > > 
> > > > > > > This test is setting up a case where a FrameNavigationEntry
> redirects
> > > > > > cross-site
> > > > > > > when we go back/forward to it.  Normally we do want to load a
> history
> > > item
> > > > > in
> > > > > > > the SiteInstance it was in before, so it's fine for
> NavigationRequest
> > to
> > > > > track
> > > > > > > the dest_site_instance_ and use it at commit time in PlzNavigate. 
> > That
> > > > > stops
> > > > > > > being true for redirects, though, and we already have some similar
> > logic
> > > > to
> > > > > > > clear any child history items if a redirect occurs.  When we
> redirect,
> > > we
> > > > > > should
> > > > > > > find a SiteInstance for the entry as we would for any other new
> > > > navigation.
> > > > > > > 
> > > > > > > As a result, I think a cleaner fix for this bug would be simply
> > clearing
> > > > > > > dest_site_instance_ in NavigationRequest::OnRequestRedirected,
since
> > it
> > > no
> > > > > > > longer applies if a redirect occurs.  That seems like a closer fix
> to
> > > the
> > > > > > > underlying problem, and it avoids the need to change anything in
> > > > > > > RenderFrameHostManager.
> > > > > > > 
> > > > > > > Hope that helps!
> > > > > > 
> > > > > > Hey Charlie, thanks for the suggestion. I tried that approach,
however
> > it
> > > > > seems
> > > > > > to have broken two few tests that you wrote with plznavigate and
site
> > per
> > > > > > process:
> > > > > > 
> > > > > >
NavigationControllerBrowserTest.FrameNavigationEntry_BackWithRedirect
> > > > > > NavigationControllerBrowserTest.ForwardRedirectWithNoCommittedEntry
> > > > > > 
> > > > > > (see patchset 3 which is with plznavigate enabled compared to
> > > > > > https://codereview.chromium.org/2385413004/#ps260001 which is
> > plznavigate
> > > > > > enabled on trunk)
> > > > > 
> > > > > Ok, I'll take a look at those in a debugger tomorrow to see what's
going
> > > > wrong.
> > > > 
> > > > btw I had a chance to debug them, and it looks like the dcheck that's
> > failing
> > > > needs to be updated after your suggestion to NavigationRequest. ptal
> > > 
> > > Cool.  I think I'm ok with that change-- let me confirm what happens in
the
> > > non-PlzNavigate case, since I'm surprised we don't have a problem there. 
> I'll
> > > approve unless this introduces a problem.
> > 
> > Ok, this LGTM.
> > 
> > The non-PlzNavigate --site-per-process case actually gets this wrong today. 
> It
> > misclassifies the redirect as NAVIGATION_TYPE_NEW_PAGE rather than
> > NAVIGATION_TYPE_EXISTING_PAGE.  That means we don't crash on the DCHECK, but
> it
> > also means we'll incorrectly clear the forward history.
> > 
> > Looks like PlzNavigate will get this right with your CL.  I'll file a bug
for
> > the non-PlzNavigate case, so feel free to proceed.  Thanks!
> 
> cool, thanks for checking and for the simpler fix.

np.  Filed https://crbug.com/679033 for the non-PlzNav case.