OOPIF: Fix subframe back/forward after recreating FTNs.

OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/273cbf61991e5339108de04c8a51fdb00a0916d5
Cr-Commit-Position: refs/heads/master@{#386756}

[Charlie Reis, 2016-03-19 05:55:02]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

BUG=586234
TEST=See bug for repro steps.
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

BUG=586234
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-03-30 17:39:58]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

BUG=586234
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

BUG=586324
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-04-03 17:28:16]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

BUG=586324
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

Also needed to relax the restriction that a FrameNavigationEntry's item
and document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-04-04 04:38:28]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  Fall back to the frame's unique
name in this case.

Also needed to relax the restriction that a FrameNavigationEntry's item
and document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-04-04 16:21:19]

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

[Charlie Reis, 2016-04-04 16:21:20]

Alex, can you take a look?  This re-enables one the last few disabled tests,
plus it fixes Istiaque's bug (568768).  (We weren't doing a good job with
FrameNavigationEntries with stale FTN IDs before.)

[alexmos, 2016-04-06 17:14:23]

Nice, LGTM with a couple of nits and questions below.

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2728: //
AddOrUpdateEntry, we'll end up using the old entry (with a PageState) during
nit: s/AddOrUpdateEntry/AddOrUpdateFrameEntry/ ?

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
In 568768, the second problem was "restoring the slashdot.org history item
multiple times before load stop" - is that covered somehow by this fix/test, or
is it a separate issue?

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
I'm not sure I fully understand this bit: "we'll end up using the old entry
(with a PageState) during the transfer, causing the renderer to crash" - out of
curiosity, where would the renderer crash?  Is there an explicit way to check
that the subframe's FNE was found correctly at the end of the test?

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2739:
NavigateToURL(shell(), url);
nit: EXPECT_TRUE

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:44: bool
MatchesFrame(FrameTreeNode* frame_tree_node);
This now has the side effect of sometimes updating FTN ID, but the function name
doesn't reflect that, so you don't realize this looking at the call sites.  I
couldn't come up with anything better to suggest though, so I think this is
fine. :)

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:440: unique_name);
Would we want to strengthen this to a CHECK at some point, and eventually remove
frame_unique_name from  FrameHostMsg_OpenURL_Params, which seems redundant now
that we have it in replication state?

[alexmos, 2016-04-06 17:19:43]

On 2016/04/04 16:21:20, Charlie Reis (slow til 4-8) wrote:
> Alex, can you take a look?  This re-enables one the last few disabled tests,
> plus it fixes Istiaque's bug (568768).  (We weren't doing a good job with
> FrameNavigationEntries with stale FTN IDs before.)

Also, would this fix SessionHistoryTest.FrameBackForward for PlzNavigate?  If
so, maybe also update browser-side-navigation.linux.content_browsertests.filter?

[Charlie Reis, 2016-04-12 16:25:50]

Sorry for the long delay!  New patch up.  Can you give it a sanity check for me?

On 2016/04/06 17:19:43, alexmos wrote:
> On 2016/04/04 16:21:20, Charlie Reis (slow til 4-8) wrote:
> > Alex, can you take a look?  This re-enables one the last few disabled tests,
> > plus it fixes Istiaque's bug (568768).  (We weren't doing a good job with
> > FrameNavigationEntries with stale FTN IDs before.)
> 
> Also, would this fix SessionHistoryTest.FrameBackForward for PlzNavigate?  If
> so, maybe also update
browser-side-navigation.linux.content_browsertests.filter?

Good point.  Yes, so I've updated that filter.

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2728: //
AddOrUpdateEntry, we'll end up using the old entry (with a PageState) during
On 2016/04/06 17:14:22, alexmos wrote:
> nit: s/AddOrUpdateEntry/AddOrUpdateFrameEntry/ ?

Done.

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
On 2016/04/06 17:14:22, alexmos wrote:
> In 568768, the second problem was "restoring the http://slashdot.org history
item
> multiple times before load stop" - is that covered somehow by this fix/test,
or
> is it a separate issue?

I'm not able to repro that part of the bug anymore without my fix.  I think it
might have been fixed by some other change in the interim.

My guess is that we were treating further button clicks as part of the
back/forward navigation (since load stop hadn't happened yet), and now we don't.

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
On 2016/04/06 17:14:22, alexmos wrote:
> I'm not sure I fully understand this bit: "we'll end up using the old entry
> (with a PageState) during the transfer, causing the renderer to crash" - out
of
> curiosity, where would the renderer crash?  Is there an explicit way to check
> that the subframe's FNE was found correctly at the end of the test?

Ah, yes, that comment could have been clearer.  I've updated it to make it more
explicit about what was going wrong.  The failures were in patch set 3 in the
following tests:
PasswordManagerBrowserTestBase.CrossSiteIframeNotFillTest
BrowserViewTest.AvoidUnnecessaryVisibilityChanges
ServiceWorkerTest.WebAccessibleResourcesIframeSrc

Unfortunately, I couldn't find an easy way to verify the specific problem of
duplicating the FNE in the test.  It would almost be possible to clone the last
committed entry and call AddOrUpdateFrameEntry on it, but that requires the
FrameTreeNode to be created first, which leads to the transfer.  Instead, I'm
just letting the test crash during the transfer if it fails.

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2739:
NavigateToURL(shell(), url);
On 2016/04/06 17:14:22, alexmos wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:44: bool
MatchesFrame(FrameTreeNode* frame_tree_node);
On 2016/04/06 17:14:22, alexmos wrote:
> This now has the side effect of sometimes updating FTN ID, but the function
name
> doesn't reflect that, so you don't realize this looking at the call sites.  I
> couldn't come up with anything better to suggest though, so I think this is
> fine. :)

Yeah, it's a bit awkward for the name to reflect that.  I view it as hopefully
temporary, since we should be able to remove FTN ID entirely from here and just
use unique name (maybe once the browser process generates unique name).  That's
what HistoryController does in the old path.

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:440: unique_name);
On 2016/04/06 17:14:22, alexmos wrote:
> Would we want to strengthen this to a CHECK at some point, and eventually
remove
> frame_unique_name from  FrameHostMsg_OpenURL_Params, which seems redundant now
> that we have it in replication state?

We avoid CHECKs on messages received from the renderer, since it could lie just
to take down the browser process.  This is more of a sanity check that we don't
diverge during testing.

And yes, we can probably remove the unique_name from this IPC eventually.  I
added a TODO for it.

[alexmos, 2016-04-12 18:55:40]

LGTM!

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
On 2016/04/12 16:25:51, Charlie Reis wrote:
> On 2016/04/06 17:14:22, alexmos wrote:
> > In 568768, the second problem was "restoring the http://slashdot.org history
> item
> > multiple times before load stop" - is that covered somehow by this fix/test,
> or
> > is it a separate issue?
> 
> I'm not able to repro that part of the bug anymore without my fix.  I think it
> might have been fixed by some other change in the interim.
> 
> My guess is that we were treating further button clicks as part of the
> back/forward navigation (since load stop hadn't happened yet), and now we
don't.

Acknowledged.

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:2729: //
the transfer, causing the renderer to crash.  See https://crbug.com/568768.
On 2016/04/12 16:25:51, Charlie Reis wrote:
> On 2016/04/06 17:14:22, alexmos wrote:
> > I'm not sure I fully understand this bit: "we'll end up using the old entry
> > (with a PageState) during the transfer, causing the renderer to crash" - out
> of
> > curiosity, where would the renderer crash?  Is there an explicit way to
check
> > that the subframe's FNE was found correctly at the end of the test?
> 
> Ah, yes, that comment could have been clearer.  I've updated it to make it
more
> explicit about what was going wrong.  The failures were in patch set 3 in the
> following tests:
> PasswordManagerBrowserTestBase.CrossSiteIframeNotFillTest
> BrowserViewTest.AvoidUnnecessaryVisibilityChanges
> ServiceWorkerTest.WebAccessibleResourcesIframeSrc
> 
> Unfortunately, I couldn't find an easy way to verify the specific problem of
> duplicating the FNE in the test.  It would almost be possible to clone the
last
> committed entry and call AddOrUpdateFrameEntry on it, but that requires the
> FrameTreeNode to be created first, which leads to the transfer.  Instead, I'm
> just letting the test crash during the transfer if it fails.

Acknowledged.  The new comment is great.

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

https://codereview.chromium.org/1816763002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:440: unique_name);
On 2016/04/12 16:25:51, Charlie Reis wrote:
> On 2016/04/06 17:14:22, alexmos wrote:
> > Would we want to strengthen this to a CHECK at some point, and eventually
> remove
> > frame_unique_name from  FrameHostMsg_OpenURL_Params, which seems redundant
now
> > that we have it in replication state?
> 
> We avoid CHECKs on messages received from the renderer, since it could lie
just
> to take down the browser process.  This is more of a sanity check that we
don't
> diverge during testing.
> 
> And yes, we can probably remove the unique_name from this IPC eventually.  I
> added a TODO for it.

Acknowledged.

[Charlie Reis, 2016-04-12 18:59:06]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-04-12 18:59:42]

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

[commit-bot: I haz the power, 2016-04-12 19:29:15]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-04-12 19:29:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-12 19:30:47]

Description was changed from

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
OOPIF: Fix subframe back/forward after recreating FTNs.

In some cases, we would navigate the main frame when going back/forward
in a subframe.  This was because the FTN ID had changed from what was
stored in the FrameNavigationEntry.  We can now use the frame's unique
name to find the FrameNavigationEntry as well.

Also relaxes the restriction that a FrameNavigationEntry's item and
document sequence numbers don't change, at least until
https://crbug.com/596707 is fixed.

BUG=586324, 568768
TEST=See bug for repro steps.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/273cbf61991e5339108de04c8a51fdb00a0916d5
Cr-Commit-Position: refs/heads/master@{#386756}
==========

[commit-bot: I haz the power, 2016-04-12 19:30:48]

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

[Charlie Reis, 2016-04-13 21:20:35]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1889583003/ by creis@chromium.org.

The reason for reverting is: This appears to have caused browser crashes in
https://crbug.com/603245..