Avoid stale navigation requests without excessive page id knowledge in the renderer process.

Avoid stale navigation requests without excessive page id knowledge in the renderer process.

This reverts the page id portions of r92748 and r96724, but keeps some unrelated cleanup as well as some tests.

BUG=431853, 178491
TEST=covered by tests, as well as it shouldn't regress the original bug 86758

Committed: https://crrev.com/2b17759ec12ca2d1c9930a37d1020dff1652bfd6
Cr-Commit-Position: refs/heads/master@{#307614}

[Avi (use Gerrit), 2014-12-03 21:56:48]

[email protected] changed reviewers:
+ [email protected]

[Avi (use Gerrit), 2014-12-03 21:56:49]

Charlie, have at it. Windows trybots haven't yet cycled, but things seem green.

[Charlie Reis, 2014-12-03 23:48:08]

This is great!  There's a lot of complexity going away here, and I think the
only downside is that we'll ignore more back/forward navigation attempts than we
strictly need to.  We might even be able to cut down those false positives.

Lots of questions below, but I don't have any major objections.

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

https://codereview.chromium.org/743803002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1359:
delegate_->GetMaxPageIDForSiteInstance(last_committed->site_instance());
This looks wrong to me.  The max page ID is the largest we've ever seen, while
the last committed could be a back navigation to a smaller page ID.

We're only using it to update the max page ID after the copy, though, so maybe
it's just the wrong name?

https://codereview.chromium.org/743803002/diff/180001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3418:
other_contents->ExpectSetHistoryOffsetAndLength(2, 3);
So much nicer!

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1943: //
SetHistoryOffsetAndLength doesn't work when there are pending cross-site
Does this matter anymore?

It would be great to remove this if not, because it would silently corrupt the
renderer's view of history length/offset in release builds if we got it wrong.

(I also worry it would affect the CreateRenderViewForRenderManager call below,
though perhaps that happens as we're creating the pending RVH and before we've
assigned it.)

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4142:
SetHistoryOffsetAndLengthForView(render_view_host,
Interesting.  We don't have a similar call here today-- is that a bug, or was it
handled somewhere else?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:961: int pending_history_list_offset =
params.pending_history_list_offset;
Do we ever still use pending_history_list_offset?  If not, let's remove it.

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

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:951: // If this is a stale back/forward
(due to a recent navigation the browser
Hmm, I wonder why clamy@ disabled the stale back/forward check for PlzNavigate
in OnCommitNavigation.  It seems to me that they would still want it in that
path.

(For PlzNavigate, "CommitNavigation" means "hey RenderFrame, here's a network
response I'd like you to commit; let me know when you're done."  If the
RenderFrame has committed a synchronous navigation in the meantime and we're
telling it to commit a forward navigation, the stale back/forward logic should
catch it.)

At any rate, your change to pull this code out of PrepareRenderViewForNavigation
is correct based on how it's currently structured.  I'll follow up with clamy@
to see if she wants to move it back in.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:953: // while swapped out are turned into
asynchronous navigations.
This comment is a little hard to follow.  Asynchronous navigations are generally
the reason we need the stale back/forward check.

Is it because swapped out RenderFrames won't ever commit a navigation without
asking the browser first?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:957: params.current_history_list_offset) {
Yes, I think this will be sufficient to replace the list of page IDs.  I'm
trying to think of a way that the renderer could clear the forward history
without a net change to its history_list_offset_, and I haven't come up with
anything.  (Any back/forward navigation necessarily goes through the browser,
and any new navigation that would clear the forward history necessarily
increases the offset.)

One subtle case: we limit history size to 50.  The renderer could keep
committing new navigations and stay at offset 49.  However, if we were at offset
49 we couldn't have a forward history item, so I think we're safe.

I'm wondering if we can reduce false positives here (since in many cases, a
history navigation to something smaller than the current offset should be safe
even after an offset change).  Maybe we should do that in a followup CL, though,
since that's not always true (thanks to the history size limit).  Perhaps a TODO
would suffice?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:833: TEST_F(RenderViewImplTest,
DontIgnoreBackAfterNavEntryLimit) {
Yeah, this is the similar to the test we would need if we tried to allow back
navigations after an offset change.

Does it fail with your change?  I'm wondering if we can keep it.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:876: // It has now dropped the first
entry, but the renderer isn't notified.
Oh, fun... The renderer isn't even notified when we start dropping entries when
the size limit is reached.  I don't think that will be a problem for us, since
the offset will continue to change and we don't use the length in the staleness
check.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:761: TEST_F(RenderViewImplTest,
StaleNavigationsIgnored) {
Did you end up getting these tests to fail when your new check is commented out?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:1400: // Note: if the history_list_length_
is 0 for a back/forward, we must be
Oh, is this the reason you added the SetHistoryOffsetAndLengthForView call to
CreateRenderViewForRenderManager?  That would make sense.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.h:899: // process.
Any reason to delete this?  Sounds like it's still true, and potentially helpful
for reasoning about how fresh the data is.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.h:911: std::vector<int32> history_page_ids_;
Woo hoo!

[Avi (use Gerrit), 2014-12-04 21:15:16]

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

https://codereview.chromium.org/743803002/diff/180001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1359:
delegate_->GetMaxPageIDForSiteInstance(last_committed->site_instance());
Bad name.

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1943: //
SetHistoryOffsetAndLength doesn't work when there are pending cross-site
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Does this matter anymore?

In what way?

Blink needs to know where we are in the history list. If someone does pruning in
the browser process, the renderer needs to know somehow.

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4142:
SetHistoryOffsetAndLengthForView(render_view_host,
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Interesting.  We don't have a similar call here today-- is that a bug, or was
it
> handled somewhere else?

Today, those values are set with the navigation, with values in
FrameMsg_Navigate_Params.

But in the new world, we're deciding if something is a stale nav by comparing
the values in FrameMsg_Navigate_Params to the current values. With a new
RenderView, the first nav that comes in would fail because the values in
FrameMsg_Navigate_Params would be compared to the blank ones.

So either we set a flag in the RenderView that says "we were just initialized
and we know that these history list values are bogus so don't worry about the
check" or we do this, to actually set up the values. I chose the latter but am
not attached to it.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:961: int pending_history_list_offset =
params.pending_history_list_offset;
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Do we ever still use pending_history_list_offset?  If not, let's remove it.

We do, when history navigations commit. See the !is_new_navigation branch of the
if in RenderFrameImpl::didCommitProvisionalLoad.

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

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:951: // If this is a stale back/forward
(due to a recent navigation the browser
Please do. Right now this code was built as a refactor of what's in there now.
If Camille didn't intend that effect, I'd be happy to rework it.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:953: // while swapped out are turned into
asynchronous navigations.
On 2014/12/03 23:48:07, Charlie Reis wrote:
> This comment is a little hard to follow.  Asynchronous navigations are
generally
> the reason we need the stale back/forward check.
> 
> Is it because swapped out RenderFrames won't ever commit a navigation without
> asking the browser first?

That's what I meant, though I'm taking your word for it.

Rephrasing the comment.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:957: params.current_history_list_offset) {
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Yes, I think this will be sufficient to replace the list of page IDs.  I'm
> trying to think of a way that the renderer could clear the forward history
> without a net change to its history_list_offset_, and I haven't come up with
> anything.  (Any back/forward navigation necessarily goes through the browser,
> and any new navigation that would clear the forward history necessarily
> increases the offset.)
> 
> One subtle case: we limit history size to 50.  The renderer could keep
> committing new navigations and stay at offset 49.  However, if we were at
offset
> 49 we couldn't have a forward history item, so I think we're safe.
> 
> I'm wondering if we can reduce false positives here (since in many cases, a
> history navigation to something smaller than the current offset should be safe
> even after an offset change).  Maybe we should do that in a followup CL,
though,
> since that's not always true (thanks to the history size limit).  Perhaps a
TODO
> would suffice?

I'm trying to get things right the first time. I can't think of a concrete case
where this would fire as a false positive, and none of the tests are failing.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:833: TEST_F(RenderViewImplTest,
DontIgnoreBackAfterNavEntryLimit) {
This is one of those tests where it's testing behavior by faking the RVH side of
the conversation. I'm creating a new NavController browsertest,
NavigationControllerBrowserTest.DontIgnoreBackAfterNavEntryLimit.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:876: // It has now dropped the first
entry, but the renderer isn't notified.
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Oh, fun... The renderer isn't even notified when we start dropping entries
when
> the size limit is reached.  I don't think that will be a problem for us, since
> the offset will continue to change and we don't use the length in the
staleness
> check.

That actually caused a bug, if you remember; both the renderer and the browser
dropped entries, but from opposite ends of the history list...
http://crbug.com/426256

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:761: TEST_F(RenderViewImplTest,
StaleNavigationsIgnored) {
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Did you end up getting these tests to fail when your new check is commented
out?

Yes, this test fails in the same way as it did with the old check if the new
check is commented out.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:1400: // Note: if the history_list_length_
is 0 for a back/forward, we must be
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Oh, is this the reason you added the SetHistoryOffsetAndLengthForView call to
> CreateRenderViewForRenderManager?  That would make sense.

No, and I'm not sure I see what you're talking about here. My reasons were
stated as a reply there; what are you thinking here?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.h (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.h:899: // process.
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Any reason to delete this?  Sounds like it's still true, and potentially
helpful
> for reasoning about how fresh the data is.

Done.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.h:911: std::vector<int32> history_page_ids_;
On 2014/12/03 23:48:07, Charlie Reis wrote:
> Woo hoo!

Acknowledged.

[Charlie Reis, 2014-12-04 23:13:58]

Thanks-- replies below.

We should also think about what happens when the stale check kicks in.  Leaving
the tab spinning isn't a great user experience.  Maybe we can send a DidFail or
DidStop IPC, depending on what state we're in at the time of the check?  (Of
course, that happens today as well, if I understand correctly.  Maybe it belongs
in a separate bug and CL.)

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1943: //
SetHistoryOffsetAndLength doesn't work when there are pending cross-site
On 2014/12/04 21:15:16, Avi wrote:
> On 2014/12/03 23:48:07, Charlie Reis wrote:
> > Does this matter anymore?
> 
> In what way?
> 
> Blink needs to know where we are in the history list. If someone does pruning
in
> the browser process, the renderer needs to know somehow.

Sorry, I was ambiguous about "this."  I meant this block (1943-1948), where we
skip the IPC if there's a pending RenderViewHost.  Skipping the IPC in some
cases makes this harder to reason about, and I'm not sure we need to anymore.

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4142:
SetHistoryOffsetAndLengthForView(render_view_host,
On 2014/12/04 21:15:16, Avi wrote:
> On 2014/12/03 23:48:07, Charlie Reis wrote:
> > Interesting.  We don't have a similar call here today-- is that a bug, or
was
> it
> > handled somewhere else?
> 
> Today, those values are set with the navigation, with values in
> FrameMsg_Navigate_Params.
> 
> But in the new world, we're deciding if something is a stale nav by comparing
> the values in FrameMsg_Navigate_Params to the current values. With a new
> RenderView, the first nav that comes in would fail because the values in
> FrameMsg_Navigate_Params would be compared to the blank ones.
> 
> So either we set a flag in the RenderView that says "we were just initialized
> and we know that these history list values are bogus so don't worry about the
> check" or we do this, to actually set up the values. I chose the latter but am
> not attached to it.

I like your approach.  Let's keep it.

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

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:951: // If this is a stale back/forward
(due to a recent navigation the browser
On 2014/12/04 21:15:16, Avi wrote:
> Please do. Right now this code was built as a refactor of what's in there now.
> If Camille didn't intend that effect, I'd be happy to rework it.

I emailed her to find out.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:953: // while swapped out are turned into
asynchronous navigations.
On 2014/12/04 21:15:16, Avi wrote:
> On 2014/12/03 23:48:07, Charlie Reis wrote:
> > This comment is a little hard to follow.  Asynchronous navigations are
> generally
> > the reason we need the stale back/forward check.
> > 
> > Is it because swapped out RenderFrames won't ever commit a navigation
without
> > asking the browser first?
> 
> That's what I meant, though I'm taking your word for it.
> 
> Rephrasing the comment.

Acknowledged.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:957: params.current_history_list_offset) {
On 2014/12/04 21:15:16, Avi wrote:
> On 2014/12/03 23:48:07, Charlie Reis wrote:
> > Yes, I think this will be sufficient to replace the list of page IDs.  I'm
> > trying to think of a way that the renderer could clear the forward history
> > without a net change to its history_list_offset_, and I haven't come up with
> > anything.  (Any back/forward navigation necessarily goes through the
browser,
> > and any new navigation that would clear the forward history necessarily
> > increases the offset.)
> > 
> > One subtle case: we limit history size to 50.  The renderer could keep
> > committing new navigations and stay at offset 49.  However, if we were at
> offset
> > 49 we couldn't have a forward history item, so I think we're safe.
> > 
> > I'm wondering if we can reduce false positives here (since in many cases, a
> > history navigation to something smaller than the current offset should be
safe
> > even after an offset change).  Maybe we should do that in a followup CL,
> though,
> > since that's not always true (thanks to the history size limit).  Perhaps a
> TODO
> > would suffice?
> 
> I'm trying to get things right the first time. I can't think of a concrete
case
> where this would fire as a false positive, and none of the tests are failing.

Oh, here's an easy false positive, unless I'm misunderstanding:

1) Navigate to page A1, then A2.
2) Click Back in the browser process.
3) Before the IPC arrives at the renderer, the RenderFrame commits A2#foo.
4) The IPC arrives at the renderer, telling it to go to A1.

The new stale page ID detection logic would kick in and cause us to ignore it,
because the offset is now 2 but the offset in the IPC was 1.  However, it's
totally safe to load A1.  The browser will still recognize it as a history entry
when it commits.

(The staleness check only matters in cases that the browser won't recognize the
history entry when it commits, like a pruned forward navigation.)

Did you want to try to avoid some of these false positives in this CL?  If so,
we should chat about potential fixes to make sure they work.  (The goal would be
to have fewer cases where the user clicks back/forward and sees nothing happen.)

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_browsertest.cc:833: TEST_F(RenderViewImplTest,
DontIgnoreBackAfterNavEntryLimit) {
On 2014/12/04 21:15:16, Avi wrote:
> This is one of those tests where it's testing behavior by faking the RVH side
of
> the conversation. I'm creating a new NavController browsertest,
> NavigationControllerBrowserTest.DontIgnoreBackAfterNavEntryLimit.

Acknowledged.

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:1400: // Note: if the history_list_length_
is 0 for a back/forward, we must be
On 2014/12/04 21:15:16, Avi wrote:
> On 2014/12/03 23:48:07, Charlie Reis wrote:
> > Oh, is this the reason you added the SetHistoryOffsetAndLengthForView call
to
> > CreateRenderViewForRenderManager?  That would make sense.
> 
> No, and I'm not sure I see what you're talking about here. My reasons were
> stated as a reply there; what are you thinking here?

Your other reply said:
'So either we set a flag in the RenderView that says "we were just initialized
and we know that these history list values are bogus so don't worry about the
check" or we do this, to actually set up the values. I chose the latter but am
not attached to it.'

The former approach you describe is the history_list_length_ <= 0 check below. 
(history_list_length_ == -1 means the values are bogus so don't worry about the
check.)

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

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:51: GURL
url(base::StringPrintf("http://127.0.0.1/%d", url_index));
Where is the test server for this?  (I'm surprised it returns true from
NavigateToURL.)

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:66:
EXPECT_EQ(controller.GetEntryAtIndex(0)->GetURL(),
Throughout this test, let's reverse the arguments.  It should be
EXPECT_EQ(expected, actual), which affects what's printed when it fails.

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:72:
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
Let's verify that we ended up on the expected URL, too.  Hanging is just one way
it could go wrong.

[Avi (use Gerrit), 2014-12-05 22:16:39]

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1943: //
SetHistoryOffsetAndLength doesn't work when there are pending cross-site
On 2014/12/04 23:13:58, Charlie Reis wrote:
> On 2014/12/04 21:15:16, Avi wrote:
> > On 2014/12/03 23:48:07, Charlie Reis wrote:
> > > Does this matter anymore?
> > 
> > In what way?
> > 
> > Blink needs to know where we are in the history list. If someone does
pruning
> in
> > the browser process, the renderer needs to know somehow.
> 
> Sorry, I was ambiguous about "this."  I meant this block (1943-1948), where we
> skip the IPC if there's a pending RenderViewHost.  Skipping the IPC in some
> cases makes this harder to reason about, and I'm not sure we need to anymore.

I'm not sure why this was put in. If it's about the list of page ids, then sure,
let's nuke it. I'm tending to think that way (why would sending two numbers be
affected by cross-site navigations?), but I'm not sure. I'm going to pull it and
see what happens.

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

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:957: params.current_history_list_offset) {
On 2014/12/04 23:13:58, Charlie Reis wrote:
> On 2014/12/04 21:15:16, Avi wrote:
> > On 2014/12/03 23:48:07, Charlie Reis wrote:
> > > Yes, I think this will be sufficient to replace the list of page IDs.  I'm
> > > trying to think of a way that the renderer could clear the forward history
> > > without a net change to its history_list_offset_, and I haven't come up
with
> > > anything.  (Any back/forward navigation necessarily goes through the
> browser,
> > > and any new navigation that would clear the forward history necessarily
> > > increases the offset.)
> > > 
> > > One subtle case: we limit history size to 50.  The renderer could keep
> > > committing new navigations and stay at offset 49.  However, if we were at
> > offset
> > > 49 we couldn't have a forward history item, so I think we're safe.
> > > 
> > > I'm wondering if we can reduce false positives here (since in many cases,
a
> > > history navigation to something smaller than the current offset should be
> safe
> > > even after an offset change).  Maybe we should do that in a followup CL,
> > though,
> > > since that's not always true (thanks to the history size limit).  Perhaps
a
> > TODO
> > > would suffice?
> > 
> > I'm trying to get things right the first time. I can't think of a concrete
> case
> > where this would fire as a false positive, and none of the tests are
failing.
> 
> Oh, here's an easy false positive, unless I'm misunderstanding:
> 
> 1) Navigate to page A1, then A2.
> 2) Click Back in the browser process.
> 3) Before the IPC arrives at the renderer, the RenderFrame commits A2#foo.
> 4) The IPC arrives at the renderer, telling it to go to A1.
> 
> The new stale page ID detection logic would kick in and cause us to ignore it,
> because the offset is now 2 but the offset in the IPC was 1.  However, it's
> totally safe to load A1.  The browser will still recognize it as a history
entry
> when it commits.
> 
> (The staleness check only matters in cases that the browser won't recognize
the
> history entry when it commits, like a pruned forward navigation.)
> 
> Did you want to try to avoid some of these false positives in this CL?  If so,
> we should chat about potential fixes to make sure they work.  (The goal would
be
> to have fewer cases where the user clicks back/forward and sees nothing
happen.)

I'd have to talk to you about that. The pruning issue happens for both forwards
and backwards, in slightly different versions, and at that point we would need
to be clever. And I don't like being clever :( I like failing quickly and
simply. If we figured out a way for the renderer to say "hey I dropped your nav"
so that things wouldn't spin infinitely, would that make you happier about
dropping things on the floor in other weird edge cases where it would have been
safe?

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_view_impl.cc:1400: // Note: if the history_list_length_
is 0 for a back/forward, we must be
On 2014/12/04 23:13:58, Charlie Reis wrote:
> On 2014/12/04 21:15:16, Avi wrote:
> > On 2014/12/03 23:48:07, Charlie Reis wrote:
> > > Oh, is this the reason you added the SetHistoryOffsetAndLengthForView call
> to
> > > CreateRenderViewForRenderManager?  That would make sense.
> > 
> > No, and I'm not sure I see what you're talking about here. My reasons were
> > stated as a reply there; what are you thinking here?
> 
> Your other reply said:
> 'So either we set a flag in the RenderView that says "we were just initialized
> and we know that these history list values are bogus so don't worry about the
> check" or we do this, to actually set up the values. I chose the latter but am
> not attached to it.'
> 
> The former approach you describe is the history_list_length_ <= 0 check below.

> (history_list_length_ == -1 means the values are bogus so don't worry about
the
> check.)

Oh.... right! Yes, got it.

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

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:51: GURL
url(base::StringPrintf("http://127.0.0.1/%d", url_index));
On 2014/12/04 23:13:58, Charlie Reis wrote:
> Where is the test server for this?  (I'm surprised it returns true from
> NavigateToURL.)

Changing to a data URL. (It doesn't return true on Android.)

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:66:
EXPECT_EQ(controller.GetEntryAtIndex(0)->GetURL(),
On 2014/12/04 23:13:58, Charlie Reis wrote:
> Throughout this test, let's reverse the arguments.  It should be
> EXPECT_EQ(expected, actual), which affects what's printed when it fails.

Done.

https://codereview.chromium.org/743803002/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:72:
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
On 2014/12/04 23:13:58, Charlie Reis wrote:
> Let's verify that we ended up on the expected URL, too.  Hanging is just one
way
> it could go wrong.

Done.

[Charlie Reis, 2014-12-05 22:51:15]

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/743803002/diff/180001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1943: //
SetHistoryOffsetAndLength doesn't work when there are pending cross-site
On 2014/12/05 22:16:39, Avi wrote:
> On 2014/12/04 23:13:58, Charlie Reis wrote:
> > On 2014/12/04 21:15:16, Avi wrote:
> > > On 2014/12/03 23:48:07, Charlie Reis wrote:
> > > > Does this matter anymore?
> > > 
> > > In what way?
> > > 
> > > Blink needs to know where we are in the history list. If someone does
> pruning
> > in
> > > the browser process, the renderer needs to know somehow.
> > 
> > Sorry, I was ambiguous about "this."  I meant this block (1943-1948), where
we
> > skip the IPC if there's a pending RenderViewHost.  Skipping the IPC in some
> > cases makes this harder to reason about, and I'm not sure we need to
anymore.
> 
> I'm not sure why this was put in. If it's about the list of page ids, then
sure,
> let's nuke it. I'm tending to think that way (why would sending two numbers be
> affected by cross-site navigations?), but I'm not sure. I'm going to pull it
and
> see what happens.

Acknowledged.

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

https://codereview.chromium.org/743803002/diff/180001/content/renderer/render...
content/renderer/render_frame_impl.cc:957: params.current_history_list_offset) {
On 2014/12/05 22:16:39, Avi wrote:
> On 2014/12/04 23:13:58, Charlie Reis wrote:
> > On 2014/12/04 21:15:16, Avi wrote:
> > > On 2014/12/03 23:48:07, Charlie Reis wrote:
> > > > Yes, I think this will be sufficient to replace the list of page IDs. 
I'm
> > > > trying to think of a way that the renderer could clear the forward
history
> > > > without a net change to its history_list_offset_, and I haven't come up
> with
> > > > anything.  (Any back/forward navigation necessarily goes through the
> > browser,
> > > > and any new navigation that would clear the forward history necessarily
> > > > increases the offset.)
> > > > 
> > > > One subtle case: we limit history size to 50.  The renderer could keep
> > > > committing new navigations and stay at offset 49.  However, if we were
at
> > > offset
> > > > 49 we couldn't have a forward history item, so I think we're safe.
> > > > 
> > > > I'm wondering if we can reduce false positives here (since in many
cases,
> a
> > > > history navigation to something smaller than the current offset should
be
> > safe
> > > > even after an offset change).  Maybe we should do that in a followup CL,
> > > though,
> > > > since that's not always true (thanks to the history size limit). 
Perhaps
> a
> > > TODO
> > > > would suffice?
> > > 
> > > I'm trying to get things right the first time. I can't think of a concrete
> > case
> > > where this would fire as a false positive, and none of the tests are
> failing.
> > 
> > Oh, here's an easy false positive, unless I'm misunderstanding:
> > 
> > 1) Navigate to page A1, then A2.
> > 2) Click Back in the browser process.
> > 3) Before the IPC arrives at the renderer, the RenderFrame commits A2#foo.
> > 4) The IPC arrives at the renderer, telling it to go to A1.
> > 
> > The new stale page ID detection logic would kick in and cause us to ignore
it,
> > because the offset is now 2 but the offset in the IPC was 1.  However, it's
> > totally safe to load A1.  The browser will still recognize it as a history
> entry
> > when it commits.
> > 
> > (The staleness check only matters in cases that the browser won't recognize
> the
> > history entry when it commits, like a pruned forward navigation.)
> > 
> > Did you want to try to avoid some of these false positives in this CL?  If
so,
> > we should chat about potential fixes to make sure they work.  (The goal
would
> be
> > to have fewer cases where the user clicks back/forward and sees nothing
> happen.)
> 
> I'd have to talk to you about that. The pruning issue happens for both
forwards
> and backwards, in slightly different versions, and at that point we would need
> to be clever. And I don't like being clever :( I like failing quickly and
> simply. If we figured out a way for the renderer to say "hey I dropped your
nav"
> so that things wouldn't spin infinitely, would that make you happier about
> dropping things on the floor in other weird edge cases where it would have
been
> safe?

Yes.  I may still be interested in improving the logic later, but that would
make me happy with it in the meantime.  (Seems especially important not to make
the spinner behavior worse given the current concern around
http://crbug.com/429399.)

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

https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
content/renderer/render_frame_impl.cc:4109: // -999 is a special bogus
current_history_list_offset that disables the stale
Can we just leave the old check_history bool instead of using a magic number
here?  Then we can just pass -1 from OnCommitNavigation and not worry about what
it means.  The TODO(clamy) can be to remove |check_history|.

[Avi (use Gerrit), 2014-12-08 22:06:03]

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

https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
content/renderer/render_frame_impl.cc:4109: // -999 is a special bogus
current_history_list_offset that disables the stale
On 2014/12/05 22:51:15, Charlie Reis wrote:
> Can we just leave the old check_history bool instead of using a magic number
> here?  Then we can just pass -1 from OnCommitNavigation and not worry about
what
> it means.  The TODO(clamy) can be to remove |check_history|.

Done.

[Charlie Reis, 2014-12-09 00:58:30]

On 2014/12/08 22:06:03, Avi wrote:
>
https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
> content/renderer/render_frame_impl.cc:4109: // -999 is a special bogus
> current_history_list_offset that disables the stale
> On 2014/12/05 22:51:15, Charlie Reis wrote:
> > Can we just leave the old check_history bool instead of using a magic number
> > here?  Then we can just pass -1 from OnCommitNavigation and not worry about
> what
> > it means.  The TODO(clamy) can be to remove |check_history|.
> 
> Done.

(Do you have a new patch set to upload?  I still see patch set 12.)

[Avi (use Gerrit), 2014-12-09 03:56:38]

On 2014/12/09 00:58:30, Charlie Reis wrote:
> On 2014/12/08 22:06:03, Avi wrote:
> >
>
https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/743803002/diff/220001/content/renderer/render...
> > content/renderer/render_frame_impl.cc:4109: // -999 is a special bogus
> > current_history_list_offset that disables the stale
> > On 2014/12/05 22:51:15, Charlie Reis wrote:
> > > Can we just leave the old check_history bool instead of using a magic
number
> > > here?  Then we can just pass -1 from OnCommitNavigation and not worry
about
> > what
> > > it means.  The TODO(clamy) can be to remove |check_history|.
> > 
> > Done.
> 
> (Do you have a new patch set to upload?  I still see patch set 12.)

Not yet; I sent the message and realized I want to get the patch to not spin
forever if a nav is dropped in first.

[Avi (use Gerrit), 2014-12-09 16:57:33]

So patch set 14 has the update. It's all red because it relies on
https://codereview.chromium.org/790843002. Patch set 13 has that CL rolled in,
and I'm going to be watching its bots.

[Charlie Reis, 2014-12-09 21:35:39]

LGTM!

[Avi (use Gerrit), 2014-12-09 21:52:19]

[email protected] changed reviewers:
+ [email protected]

[Avi (use Gerrit), 2014-12-09 21:52:20]

Nasko, can you take the IPC here?

[nasko, 2014-12-09 23:43:37]

IPC Rubberstamp LGTM

[Avi (use Gerrit), 2014-12-10 00:13:03]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-12-10 00:13:53]

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

[commit-bot: I haz the power, 2014-12-10 02:08:18]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2014-12-10 02:09:03]

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