Prevent renderer kills for in-page navigations on subframes.

Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/225a74338e488576c405e18610298c39244b85f5
Cr-Commit-Position: refs/heads/master@{#397832}

[Charlie Reis, 2016-05-31 17:49:23]

Description was changed from

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.

These particular repro steps did not affect FrameNavigationEntry
modes, so I expect there are more ways to see the in-page navigation
renderer kills.

BUG=612713
TEST=At least some of the NC_IN_PAGE_NAVIGATION kills are gone.
==========

to

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.

These particular repro steps did not affect FrameNavigationEntry
modes, so I expect there are more ways to see the in-page navigation
renderer kills.

BUG=612713
TEST=At least some of the NC_IN_PAGE_NAVIGATION kills are gone.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2016-06-03 19:20:34]

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

[Charlie Reis, 2016-06-03 19:20:36]

Avi, can you take a look?  I posted an explanation to https://crbug.com/612713
(comments 12-14), and I left some breadcrumbs below.  I'll follow up with a fix
for the OOPIF-modes later, allowing this one to be merged.  Thanks!

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1381:
last_committed_url = rfh->GetLastCommittedURL();
This is wrong after a process swap, when the newly created RFH might not have a
last committed URL even though the FrameTreeNode does.  I noticed it when we
unexpectedly weren't doing the NC_IN_PAGE_NAVIGATION_KILL in --site-per-process
in one of the tests.

(This is somewhat tangential to the rest of the CL, but definitely fixes broken
behavior in this method.)

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1858: // rather than
assuming the NavigationEntry has up to date info on subframes.
I decided to punt this for another CL, since it will take a bit more work to fix
and we won't hit this on M52 beta.

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:138: const WebHistoryItem& old_item =
render_frame->current_history_item();
This fixes BackTwiceToIframeWithContent in default Chrome, since we can't trust
current_entry_ to be accurate for subframes.

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_entry.cc:70:
current_frame->current_history_item().documentSequenceNumber());
This fixes BackToAboutBlankIframe and BackToIframeWithContent in default Chrome,
since we can't trust the item_ of the last committed HistoryEntry to be
accurate.  (I'm actually quite surprised we need this block at all, and we don't
have it in FNE-enabled modes.  I'll have to dig into source history to see why
it was added, but for now this is a fix that makes it work.)

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3280: current_history_item_ = item;
We used to only keep track of this in FNE-enabled modes, but we can keep track
of it in default Chrome as well to fix the bugs in HistoryController and
HistoryEntry.

[Charlie Reis, 2016-06-03 20:17:03]

Description was changed from

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.

These particular repro steps did not affect FrameNavigationEntry
modes, so I expect there are more ways to see the in-page navigation
renderer kills.

BUG=612713
TEST=At least some of the NC_IN_PAGE_NAVIGATION kills are gone.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2016-06-03 20:26:57]

I agree with these fixes, but have some questions about comments.

The old code seems correct at first, and it's not until you hit ugly edge-cases
that it falls apart. Do we want to comment in the code why we're doing things
this slightly more awkward way? I don't trust myself to remember in three years
when I return.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1381:
last_committed_url = rfh->GetLastCommittedURL();
On 2016/06/03 19:20:35, Charlie Reis wrote:
> This is wrong after a process swap, when the newly created RFH might not have
a
> last committed URL even though the FrameTreeNode does.  I noticed it when we
> unexpectedly weren't doing the NC_IN_PAGE_NAVIGATION_KILL in
--site-per-process
> in one of the tests.
> 
> (This is somewhat tangential to the rest of the CL, but definitely fixes
broken
> behavior in this method.)

Interesting; worthy of a comment why we don't do it that way?

Also, would this be a problem in general? Would we want to reimplement
RFH::GetLastCommittedURL() through FrameTreeNode?

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

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3791: // is
elided from the navigation history of a subframe when it is loaded, and
Part 1 of the test, the elision of about:blank, was kept.

This comment is weird. It talks about the history of the test rather than the
current state, which doesn't seem right. What does this test do? It's tests the
elision of about:blank, plus it does a regression test for 612713. We should
have the comment say that.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3860: //
NavigationControllerImpl::FindFramesToNavigate for more information.
All the bug links from above (bug 542299, 598043, the WHATWG link) should move
here, as they're relevant here, and that leaves the test description clear to
describe the state of the test now.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3873:
EXPECT_EQ(frame_url, frame->current_url());
The comment at 3857 belongs on this line. The debate is whether this line should
be EXPECT_EQ(frame_url... or EXPECT_EQ(blank_url... .

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3919: //
iframe to test proper restoration in that case.
Is this comment correct? It's roughly like the first test, but there are a few
new differences that you added.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3990: //
NavigationControllerImpl::FindFramesToNavigate for more information.
Same comments, same move of this comment to the EXPECT_EQ on line 4003.

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:138: const WebHistoryItem& old_item =
render_frame->current_history_item();
On 2016/06/03 19:20:35, Charlie Reis wrote:
> This fixes BackTwiceToIframeWithContent in default Chrome, since we can't
trust
> current_entry_ to be accurate for subframes.

Agreed that this is a fix; is a comment appropriate?

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_entry.cc:70:
current_frame->current_history_item().documentSequenceNumber());
On 2016/06/03 19:20:36, Charlie Reis wrote:
> This fixes BackToAboutBlankIframe and BackToIframeWithContent in default
Chrome,
> since we can't trust the item_ of the last committed HistoryEntry to be
> accurate.  (I'm actually quite surprised we need this block at all, and we
don't
> have it in FNE-enabled modes.  I'll have to dig into source history to see why
> it was added, but for now this is a fix that makes it work.)

Pull out |current_frame->current_history_item()| with a comment?

[Charlie Reis, 2016-06-03 20:53:28]

Thanks for requesting comments-- that's definitely worthwhile.  (I was in a bit
of a rush to get this up for initial try jobs, given ligimole's request.)

PTAL!

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1381:
last_committed_url = rfh->GetLastCommittedURL();
On 2016/06/03 20:26:57, Avi wrote:
> On 2016/06/03 19:20:35, Charlie Reis wrote:
> > This is wrong after a process swap, when the newly created RFH might not
have
> a
> > last committed URL even though the FrameTreeNode does.  I noticed it when we
> > unexpectedly weren't doing the NC_IN_PAGE_NAVIGATION_KILL in
> --site-per-process
> > in one of the tests.
> > 
> > (This is somewhat tangential to the rest of the CL, but definitely fixes
> broken
> > behavior in this method.)
> 
> Interesting; worthy of a comment why we don't do it that way?

Good idea.  Done.

> Also, would this be a problem in general? Would we want to reimplement
> RFH::GetLastCommittedURL() through FrameTreeNode?

Quite the opposite-- that's how it used to work (before
https://crbug.com/590034), which led to problems when you called
RFH::GetLastCommittedURL() and got a different frame's URL.  That matters for
things like security checks relevant to the RFH itself.

In this case, we care more about what the last page in the FrameTreeNode was,
regardless of which RFH it committed in.

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

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3791: // is
elided from the navigation history of a subframe when it is loaded, and
On 2016/06/03 20:26:57, Avi wrote:
> Part 1 of the test, the elision of about:blank, was kept.
> 
> This comment is weird. It talks about the history of the test rather than the
> current state, which doesn't seem right. What does this test do? It's tests
the
> elision of about:blank, plus it does a regression test for 612713. We should
> have the comment say that.

Done.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3860: //
NavigationControllerImpl::FindFramesToNavigate for more information.
On 2016/06/03 20:26:57, Avi wrote:
> All the bug links from above (bug 542299, 598043, the WHATWG link) should move
> here, as they're relevant here, and that leaves the test description clear to
> describe the state of the test now.

Done.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3873:
EXPECT_EQ(frame_url, frame->current_url());
On 2016/06/03 20:26:57, Avi wrote:
> The comment at 3857 belongs on this line. The debate is whether this line
should
> be EXPECT_EQ(frame_url... or EXPECT_EQ(blank_url... .

Done.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3919: //
iframe to test proper restoration in that case.
On 2016/06/03 20:26:57, Avi wrote:
> Is this comment correct? It's roughly like the first test, but there are a few
> new differences that you added.

Yes, I think it's still accurate.  The behavior is basically the same, except
that we test the case that the earlier frame URL is not replaced.

https://codereview.chromium.org/2023013002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:3990: //
NavigationControllerImpl::FindFramesToNavigate for more information.
On 2016/06/03 20:26:57, Avi wrote:
> Same comments, same move of this comment to the EXPECT_EQ on line 4003.

Done.

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:138: const WebHistoryItem& old_item =
render_frame->current_history_item();
On 2016/06/03 20:26:57, Avi wrote:
> On 2016/06/03 19:20:35, Charlie Reis wrote:
> > This fixes BackTwiceToIframeWithContent in default Chrome, since we can't
> trust
> > current_entry_ to be accurate for subframes.
> 
> Agreed that this is a fix; is a comment appropriate?

Done.

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/2023013002/diff/20001/content/renderer/histor...
content/renderer/history_entry.cc:70:
current_frame->current_history_item().documentSequenceNumber());
On 2016/06/03 20:26:57, Avi wrote:
> On 2016/06/03 19:20:36, Charlie Reis wrote:
> > This fixes BackToAboutBlankIframe and BackToIframeWithContent in default
> Chrome,
> > since we can't trust the item_ of the last committed HistoryEntry to be
> > accurate.  (I'm actually quite surprised we need this block at all, and we
> don't
> > have it in FNE-enabled modes.  I'll have to dig into source history to see
why
> > it was added, but for now this is a fix that makes it work.)
> 
> Pull out |current_frame->current_history_item()| with a comment?

Done.

[Charlie Reis, 2016-06-03 20:53:37]

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

[commit-bot: I haz the power, 2016-06-03 20:54:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023013002/40001

[commit-bot: I haz the power, 2016-06-03 20:56:55]

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

[commit-bot: I haz the power, 2016-06-03 20:56:56]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Reis, 2016-06-03 21:01:48]

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

[commit-bot: I haz the power, 2016-06-03 21:03:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023013002/60001

[Avi (use Gerrit), 2016-06-03 21:04:35]

lgtm

👍

[Charlie Reis, 2016-06-03 21:05:07]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-06-03 21:05:11]

The CQ bit was checked by creis@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023013002/60001

[commit-bot: I haz the power, 2016-06-03 22:56:38]

Description was changed from

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-03 22:56:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-03 22:57:43]

Description was changed from

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Prevent renderer kills for in-page navigations on subframes.

This switches HistoryController and HistoryEntry to look at the last
committed WebHistoryItem for a frame rather than assuming that the
whole HistoryEntry accurately reflects the state of the page.  The
previous logic led to NC_IN_PAGE_NAVIGATION renderer kills when going
back cross-site in ways that looked in-page to the HistoryEntry.

This CL does not yet fix the kills that happen in FrameNavigationEntry
modes, which are covered by the BackTwiceToIframeWithContent test.
Those will be fixed in a follow-up CL.

BUG=612713
TEST=Fewer NC_IN_PAGE_NAVIGATION kills.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/225a74338e488576c405e18610298c39244b85f5
Cr-Commit-Position: refs/heads/master@{#397832}
==========

[commit-bot: I haz the power, 2016-06-03 22:57:45]

Patchset 4 (id:??) landed as
https://crrev.com/225a74338e488576c405e18610298c39244b85f5
Cr-Commit-Position: refs/heads/master@{#397832}