Additional fixes for prerender/instant + browsing history.

A number of issues weren't addressed with the earlier patch for prerender + browsing history, particularly for instant pages.

  - The "remove first entry" option used by instant was not being handled correctly when there was only one committed entry in the NavigationController.

  - Renderer-issued navigations which were committed in the browser but not yet known by the browser/NavigationController were being incorrectly pruned. This did not happen regularly in the prerender case, but does in the instant case, particularly when changing what's typed in the omnibox.

  - Some additional sanity testing to make sure that the message is sent to the correct render process.

  - Additional unit tests are added.

BUG=89798
TEST=NavigationControllerTest.CopyStateFromAndPrune*, RenderViewTest.SetHistoryLengthAndPrune.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96724

[cbentzel, 2011-08-11 19:54:17]

This is incomplete but I wanted you to see the changes I'm making. It's been a
very meeting-heavy day...

[Charlie Reis, 2011-08-11 20:49:24]

Some initial thoughts.

http://codereview.chromium.org/7618016/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:606: // one that needs to be
pruned.
Well, we're hoping it isn't possible if called correctly, but we want to be
safe.  Let's rephrase to something like "Ensure that we update the history of
the intended RenderView."

http://codereview.chromium.org/7618016/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7618016/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:748: // |length| entries, and moves the current
page_id to the last
Nit: |page_id|

http://codereview.chromium.org/7618016/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7618016/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1031: if (page_id_ != page_id) {
Dropping this message doesn't seem like the right thing to do if we know this
case is possible-- that means we know we'll end up with the broken back button
again, right?  It'll happen if the prerendered page starts navigating a second
time just before it's swapped in.  (Not common, but possible to write a test
for.)

Given this inherent race, are we really gaining something from verifying the
browser and renderer page_ids match?  Or should we just make sure the browser
handles it properly if it gets a new page_id from a renderer it has just swapped
in?  (Maybe it already does handle this case properly?)

http://codereview.chromium.org/7618016/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1039: DCHECK(page_id == -1);
I'm wondering if this whole block would be clearer by starting with
DCHECK(page_id >= -1) on line 1025 and putting this comment before the "if". 
Also, drop "done, and" to make it the comment bit easier to follow.

(Followup: I wrote this before the comment above, so maybe it's unnecessary.)

[cbentzel, 2011-08-13 00:09:27]

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/test_tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/test_tab_contents.h:75: void
ExpectSetHistoryLengthAndPrune(const SiteInstance* site_instance,
I'll need a comment for this function.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/test_tab_contents.h:78: virtual void
SetHistoryLengthAndPrune(const SiteInstance* site_instance,
Perhaps here as well.

[Charlie Reis, 2011-08-13 00:29:04]

Just a few quick comments before I have to run.  I'll try to take another look
later tonight.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/navigation_controller.cc:906: ++minimum_page_id;
Can you add a comment explaining why this is necessary?  I'm not following.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:605: DCHECK(!site_instance ||
rvh->site_instance() == site_instance);
I'm wary about not having an early return in release builds, but I'm ok with it
as is if we can understand why it isn't necessary.

Is the intention that we only call this on TabContents that are about to be
swapped in from prerendering or instant, and thus they won't have a pending
entry?  If so, let's add a DCHECK that
render_manager_.pending_render_view_host() is null.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:630: // virtual for testing.
Nit: capitalize virtual here.

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages...
content/common/view_messages.h:750: // in the render view. All page_id's >=
minimum_page_id are appended to
Nit: No apostrophe.

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages...
content/common/view_messages.h:758: int, /* history_length */
merged_history_length

http://codereview.chromium.org/7618016/diff/8010/content/renderer/render_view.cc
File content/renderer/render_view.cc (left):

http://codereview.chromium.org/7618016/diff/8010/content/renderer/render_view...
content/renderer/render_view.cc:434: 
Any reason to remove these lines?  Those blocks don't seem related to me.

[cbentzel, 2011-08-13 02:19:12]

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/navigation_controller.cc:906: ++minimum_page_id;
On 2011/08/13 00:29:04, creis wrote:
> Can you add a comment explaining why this is necessary?  I'm not following.

Done.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:605: DCHECK(!site_instance ||
rvh->site_instance() == site_instance);
On 2011/08/13 00:29:04, creis wrote:
> I'm wary about not having an early return in release builds, but I'm ok with
it
> as is if we can understand why it isn't necessary.
> 
> Is the intention that we only call this on TabContents that are about to be
> swapped in from prerendering or instant, and thus they won't have a pending
> entry?  If so, let's add a DCHECK that
> render_manager_.pending_render_view_host() is null.

Actually, some of them may have a pending entry - this could be the case if a
prerender is swapped in before the first page is committed. However, in that
case render_view_host_ is non-NULL and is the actual host for the RenderView
that the prerender is taking place on.

The case we need to worry about is when there is a pending_render_view_host_ due
to render-issued cross-SiteInstance navigations. In the Instant case this is
restricted enough to not happen, but on the prerender case it may be possible.

I will have to think some more about how to handle that. Arguably we could
cancel the pending entry and only send to render_view_host_.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:630: // virtual for testing.
On 2011/08/13 00:29:04, creis wrote:
> Nit: capitalize virtual here.

Done.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/test_tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/test_tab_contents.h:75: void
ExpectSetHistoryLengthAndPrune(const SiteInstance* site_instance,
On 2011/08/13 00:09:28, cbentzel wrote:
> I'll need a comment for this function.

Done.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/test_tab_contents.h:78: virtual void
SetHistoryLengthAndPrune(const SiteInstance* site_instance,
On 2011/08/13 00:09:28, cbentzel wrote:
> Perhaps here as well.

Done.

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages...
content/common/view_messages.h:750: // in the render view. All page_id's >=
minimum_page_id are appended to
On 2011/08/13 00:29:04, creis wrote:
> Nit: No apostrophe.

Done.

http://codereview.chromium.org/7618016/diff/8010/content/common/view_messages...
content/common/view_messages.h:758: int, /* history_length */
On 2011/08/13 00:29:04, creis wrote:
> merged_history_length

Done.

http://codereview.chromium.org/7618016/diff/8010/content/renderer/render_view.cc
File content/renderer/render_view.cc (left):

http://codereview.chromium.org/7618016/diff/8010/content/renderer/render_view...
content/renderer/render_view.cc:434: 
On 2011/08/13 00:29:04, creis wrote:
> Any reason to remove these lines?  Those blocks don't seem related to me.

Unrelated, sorry for not catching myself.

[Charlie Reis, 2011-08-13 06:26:26]

Looking close-- thanks.

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/8010/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:605: DCHECK(!site_instance ||
rvh->site_instance() == site_instance);
On 2011/08/13 02:19:12, cbentzel wrote:
> On 2011/08/13 00:29:04, creis wrote:
> > I'm wary about not having an early return in release builds, but I'm ok with
> it
> > as is if we can understand why it isn't necessary.
> > 
> > Is the intention that we only call this on TabContents that are about to be
> > swapped in from prerendering or instant, and thus they won't have a pending
> > entry?  If so, let's add a DCHECK that
> > render_manager_.pending_render_view_host() is null.
> 
> Actually, some of them may have a pending entry - this could be the case if a
> prerender is swapped in before the first page is committed. However, in that
> case render_view_host_ is non-NULL and is the actual host for the RenderView
> that the prerender is taking place on.

Oops, I meant to say "pending RenderViewHost" rather than "pending entry."

> 
> The case we need to worry about is when there is a pending_render_view_host_
due
> to render-issued cross-SiteInstance navigations. In the Instant case this is
> restricted enough to not happen, but on the prerender case it may be possible.
> 
> I will have to think some more about how to handle that. Arguably we could
> cancel the pending entry and only send to render_view_host_. 

Yes, I think an attempt to start a cross-site transition from a prerender
TabContents should cancel the prerendering.  You're right that a
renderer-initiated navigation could cause it, such as navigating to a URL that's
installed as an app.

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:605: DCHECK(!site_instance ||
rvh->site_instance() == site_instance);
As discussed, please either make this return early, or DCHECK that there is no
pending RVH (and ensure that's true by canceling prerendering if a cross-site
navigation is attempted).

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
File content/browser/tab_contents/test_tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/test_tab_contents.h:74: 
Remove one of the blank lines.

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/test_tab_contents.h:94: const SiteInstance*
expect_set_history_length_and_prune_site_instance_;
Might not be safe memory-wise to keep a raw pointer to a SiteInstance.  Either
keep a scoped refptr or the SiteInstance's ID.

http://codereview.chromium.org/7618016/diff/10017/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7618016/diff/10017/content/common/view_message...
content/common/view_messages.h:756: // is preserved. The resulting page history
is [-1 -1 -1 7 8].
This is nice.

[cbentzel, 2011-08-13 14:31:13]

+mmenke,tburkard for the prerender changes.

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:605: DCHECK(!site_instance ||
rvh->site_instance() == site_instance);
On 2011/08/13 06:26:27, creis wrote:
> As discussed, please either make this return early, or DCHECK that there is no
> pending RVH (and ensure that's true by canceling prerendering if a cross-site
> navigation is attempted).

Fixed. I early return in a number of unexpected cases and added NOTREACHED
clauses. This patch also adds logic for prerender where a page will be cancelled
if cross-site navigation is pending. If it had committed earlier during a
prerender, we still proceed. I have tested that case by doing a prerender of a
URL I have installed as an app.

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
File content/browser/tab_contents/test_tab_contents.h (right):

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/test_tab_contents.h:74: 
On 2011/08/13 06:26:27, creis wrote:
> Remove one of the blank lines.

Done.

http://codereview.chromium.org/7618016/diff/10017/content/browser/tab_content...
content/browser/tab_contents/test_tab_contents.h:94: const SiteInstance*
expect_set_history_length_and_prune_site_instance_;
On 2011/08/13 06:26:27, creis wrote:
> Might not be safe memory-wise to keep a raw pointer to a SiteInstance.  Either
> keep a scoped refptr or the SiteInstance's ID.

This may be NULL, so kept a scoped_refptr. In this case we are only comparing
pointer values rather than dereferencing, but may as well be safer.

[Charlie Reis, 2011-08-13 16:21:56]

Have you uploaded the latest changes?  I'm still seeing the patch set I looked
at last night (patch set 8).

[cbentzel, 2011-08-13 16:41:49]

On 2011/08/13 16:21:56, creis wrote:
> Have you uploaded the latest changes?  I'm still seeing the patch set I looked
> at last night (patch set 8).

Sorry. Uploaded.

My change only cancels prerenders if there is a
pending_render_view_host_/site_instance_ at the time we are trying to swap it
in. Although this works for history related issues, we may want to cancel
prerenders for site_instance_ traversals for other reasons [I need to make sure
we keep priority low, concerns about opening up an App in a prerender as it may
have additional permissions, etc.]

[mmenke, 2011-08-13 17:04:24]

Prerender changes LGTM.

We should add some cross-site navigation tests to the prerender tests, too, at
some point.  Would be useful for this, for the visibility API, and for plug
delay-loading/HTML media cancellation.

[Charlie Reis, 2011-08-13 18:39:24]

Looks like the Mac compile (Clang?) needs a few changes.

LGTM apart from the questions below.

http://codereview.chromium.org/7618016/diff/11020/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/7618016/diff/11020/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:667: //
render_view_host_ willz not be deleted before the end of this method, so we
Typo

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
content/renderer/render_view.cc:1005: DCHECK(history_page_ids_[i] >= 0);
This isn't guaranteed, is it?  history_page_ids_ can contain -1s for pages
rendered by a different RenderView (e.g., the NTP).

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
content/renderer/render_view.cc:1015: // the end of the list before?
Can we be pointing to a page that we've pruned?  We wouldn't want to be showing
a page that is no longer in the history.  What do you think is the right
behavior?

[cbentzel, 2011-08-13 21:24:28]

Thank you all for the timely and detailed reviews.

http://codereview.chromium.org/7618016/diff/11020/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/7618016/diff/11020/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager.cc:667: //
render_view_host_ willz not be deleted before the end of this method, so we
On 2011/08/13 18:39:24, creis wrote:
> Typo

Done.

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
content/renderer/render_view.cc:1005: DCHECK(history_page_ids_[i] >= 0);
On 2011/08/13 18:39:24, creis wrote:
> This isn't guaranteed, is it?  history_page_ids_ can contain -1s for pages
> rendered by a different RenderView (e.g., the NTP).

That's correct. In that case we will still want to prune them, as they will be
earlier in the history than the minimum_page_id that we want to retain. There
would be an exception if there was a cross-boundary history.back() [or other
back navigation] but this is disallowed in both the prerender and instant case
which uses it, and the NaviigationController also does not expect it in this
case.

http://codereview.chromium.org/7618016/diff/11020/content/renderer/render_vie...
content/renderer/render_view.cc:1015: // the end of the list before?
On 2011/08/13 18:39:24, creis wrote:
> Can we be pointing to a page that we've pruned?  We wouldn't want to be
showing
> a page that is no longer in the history.  What do you think is the right
> behavior?

At this point, we can't. If the page was pruned it won't be in the
history_page_ids_ at this point. The RenderViewTest.SetHistoryLengthAndPrune
test covers some of the corner cases, such as the browser thinking there is only
one page_id which it wants to prune [in the instant case], but a renderer-issued
navigation commits in the interim.

[Charlie Reis, 2011-08-13 22:05:41]

LGTM

[darin (slow to review), 2011-08-14 03:41:37]

OK, LGTM

[commit-bot: I haz the power, 2011-08-14 16:49:22]

Change committed as 96724

[jam, 2011-08-15 15:59:09]

http://codereview.chromium.org/7618016/diff/12008/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/7618016/diff/12008/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:603: // cleanly. Since it's only
used when swapping in instant and prerendered
ditto

http://codereview.chromium.org/7618016/diff/12008/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7618016/diff/12008/content/common/view_message...
content/common/view_messages.h:747: // Sent to the RenderView when a prerendered
or instant page is committed
content layer doesn't know about prerendering/instant, so the comments shouldn't
mention them.