Fix regression with back-button not working on prerendered and instant pages.

A fairly recent change introduced a history of page_ids for each RenderView, to validate that Navigation's go to legitimate pages.

In the prerender and instant cases, the history of page_id's was not accurate, because it was not offset to reflect the point in time when the page was swapped in. For example, if the history for the tab looks like

about:blank
http://www.launchprerender.com/
http://www.prerendered_page.com/first_page.html
http://www.prerendered_page.com/second_page.html

The history of page_id's in the prerender-page RenderView should look like

[-1, -1, 13, 14] - with the first two entries being -1 since they are not captured in this render view.

Before this fix, it would look like

[13, 14] - and when the back navigation was attempted, the length was not as long as expected.

BUG=89798
TEST=Go to prerender_test.appspot.com, do a prerender on dev.chromium.org, click on a link within dev.chromium.org, press back and see that it works instead of spinning forever. Also, browser_tests --gtest_filter=*BackToPrerenderedPage, which fails without the change.

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

[gavinp, 2011-08-08 16:39:47]

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

http://codereview.chromium.org/7491096/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1806: // make sure we get the right
one.
NIT: Use a question mark on questions.

[cbentzel, 2011-08-09 13:50:33]

tburkard, mmenke: Please look at the overall change. I have both of you because
this needs to go in as early as possible.

creis: Please review most of the code as it impacts the area you wrote.

brettw: I have you on the review list for expediency in case creis is not able
to get to it today. You also reviewed his change so may have the context for it.

[mmenke, 2011-08-09 14:54:03]

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1662:
back_nav_observer.Wait();
Maybe run a script here or something, to make sure the correct page has really
loaded?

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_loader.html (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_loader.html:7: var eventObject =
document.createEvent("MouseEvents");
nit:  Use single quotes in Javascript (double only in HTML).

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_page_with_link.html (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_page_with_link.html:16: function
DidDisplayPass() {
This doesn't seem to get called, so I recommend removing it.

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_page_with_link.html:21: var eventObject =
document.createEvent("MouseEvents");
nit:  Single quotes.

[cbentzel, 2011-08-09 15:42:39]

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1662:
back_nav_observer.Wait();
On 2011/08/09 14:54:03, Matt Menke wrote:
> Maybe run a script here or something, to make sure the correct page has really
> loaded?

Perhaps. I tried using a title watcher but that hung. May just call javascript
here.

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_loader.html (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_loader.html:7: var eventObject =
document.createEvent("MouseEvents");
On 2011/08/09 14:54:03, Matt Menke wrote:
> nit:  Use single quotes in Javascript (double only in HTML).

Done.

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_page_with_link.html (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_page_with_link.html:16: function
DidDisplayPass() {
On 2011/08/09 14:54:03, Matt Menke wrote:
> This doesn't seem to get called, so I recommend removing it.

Done.

http://codereview.chromium.org/7491096/diff/8002/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_page_with_link.html:21: var eventObject =
document.createEvent("MouseEvents");
On 2011/08/09 14:54:03, Matt Menke wrote:
> nit:  Single quotes.

Done.

[mmenke, 2011-08-09 15:48:37]

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1662:
back_nav_observer.Wait();
On 2011/08/09 15:42:39, cbentzel wrote:
> On 2011/08/09 14:54:03, Matt Menke wrote:
> > Maybe run a script here or something, to make sure the correct page has
really
> > loaded?
> 
> Perhaps. I tried using a title watcher but that hung. May just call javascript
> here.

I was thinking of calling Javascript, but the title watcher strikes me as a
better idea.  Both "prerender_page_with_link.html" and "prerender_page.html"
have the same title.  Maybe that's why it hung?

[mmenke, 2011-08-09 15:49:35]

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1662:
back_nav_observer.Wait();
On 2011/08/09 15:48:37, Matt Menke wrote:
> On 2011/08/09 15:42:39, cbentzel wrote:
> > On 2011/08/09 14:54:03, Matt Menke wrote:
> > > Maybe run a script here or something, to make sure the correct page has
> really
> > > loaded?
> > 
> > Perhaps. I tried using a title watcher but that hung. May just call
javascript
> > here.
> 
> I was thinking of calling Javascript, but the title watcher strikes me as a
> better idea.  Both "prerender_page_with_link.html" and "prerender_page.html"
> have the same title.  Maybe that's why it hung?

Oops...No they don't.  That is odd.

[cbentzel, 2011-08-09 15:57:36]

On 2011/08/09 15:49:35, Matt Menke wrote:
>
http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_browsertest.cc (right):
> 
>
http://codereview.chromium.org/7491096/diff/8002/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_browsertest.cc:1662:
> back_nav_observer.Wait();
> On 2011/08/09 15:48:37, Matt Menke wrote:
> > On 2011/08/09 15:42:39, cbentzel wrote:
> > > On 2011/08/09 14:54:03, Matt Menke wrote:
> > > > Maybe run a script here or something, to make sure the correct page has
> > really
> > > > loaded?
> > > 
> > > Perhaps. I tried using a title watcher but that hung. May just call
> javascript
> > > here.
> > 
> > I was thinking of calling Javascript, but the title watcher strikes me as a
> > better idea.  Both "prerender_page_with_link.html" and "prerender_page.html"
> > have the same title.  Maybe that's why it hung?
> 
> Oops...No they don't.  That is odd.

I changed this to call javascript in the page, which fails if the function is
not present. I'll look at the title watcher separately.

[mmenke, 2011-08-09 15:59:23]

The BrowserTest LGTM.  Rest of the code looks fine, too, but I'm not familiar
enough with how navigation history currently works to review for correctness, of
course.

[brettw, 2011-08-09 16:29:46]

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

http://codereview.chromium.org/7491096/diff/5019/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1808: rvh->Send(new
ViewMsg_OffsetAndPruneHistory(rvh->routing_id(), offset));
Don't we need to do something similar to this for the swapping RVH case for
normal cross-site transitions? How does that work? I'm surprised we needed new
machinery for doing this.

[cbentzel, 2011-08-09 16:47:22]

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

http://codereview.chromium.org/7491096/diff/5019/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1808: rvh->Send(new
ViewMsg_OffsetAndPruneHistory(rvh->routing_id(), offset));
On 2011/08/09 16:29:47, brettw wrote:
> Don't we need to do something similar to this for the swapping RVH case for
> normal cross-site transitions? How does that work? I'm surprised we needed new
> machinery for doing this.

cross-site transitions work correctly with the existing code - this was
something I looked at early on when debugging why the prerender case doesn't
work.

from the bug:

I was concerned about the general cross-domain navigation case as well when
debugging this last night.

The reason it works is the initial OnNavigate call has non-zero
current_history_list_offset and current_history_list_length.

Let's say the navigation controller has a sequence like the following

chrome://newtab
www.a.com
www.b.com
www.b.com/foo

We'll create a new RenderView when transitioning from www.a.com to www.b.com.
However, the ViewMsg_Navigate_Params has a current_history_list_offset of 2 and
length of 3 [I think, will need to see the log statements again]. The first two
entries are filled in with -1, since they are controlled by other render views.

When you do the Go Back at this point, a Navigate is sent with
pending_history_list_offset to 2, and that's treated like a valid page
correctly.

[tburkard, 2011-08-09 17:36:03]

LGTM

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

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1023: int current_page_id = -1;
I'd add a comment explaining what this does (either here or in the headerfile)

[brettw, 2011-08-09 18:14:40]

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

http://codereview.chromium.org/7491096/diff/5019/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1808: rvh->Send(new
ViewMsg_OffsetAndPruneHistory(rvh->routing_id(), offset));
Okay, I think that answers my question: in the cross-site case the offsets are
sent with OnNavigate. Since we don't necessarily know the right ones at the time
of preloading, they may be wrong.

This brings up an interesting point that history.length will be wrong in a
prerendered page. I think this probably isn't worth fixing right now, but it's
worth keeping in mind (maybe we should file a bug or put a note somewhere in the
code about this?)

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

http://codereview.chromium.org/7491096/diff/5019/content/common/view_messages...
content/common/view_messages.h:747: // Used to fix page_id offsets when pruning
history.
Can you expand on this? It doesn't actually tell what it means. I'm actually a
bit confused. I was trying to write a comment for my code sample below and I was
having trouble describing what this message is telling us to do.

With a name like offset, I would expect things to be shifted. Is my
understanding of what this is supposed to do correct? : It sets sets the current
page as the most recent one in history with the given number of previous history
items. And we assume all previous history items are in other processes since
this is a preloaded page with no history.

If that's true, "OffsetAndPrune" seems maybe misleading or insufficient. Maybe
this is complicated enough that we just need to name this as prerendering
related? :
  ViewMsg_SetHistoryStateForPreloadedPage, history_length)

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

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1024: if (history_list_offset_ >= 0) {
No {} for single-line conditionals.

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1030: if (history_list_length_ >= 0)
What case will the history_list_length_ be negative? That seems suspicious.

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1033: for (int i = 0; i < history_list_offset_;
++i)
I think this version is a bit easier to follow:

// <clear comment about what type of list we're trying to generate>.
std::vector<int32> new_page_ids_;
new_page_ids.resize(history_list_length_, -1);
new_page_ids[history_list_offset_] = current_page_id;
history_page_ids.swap(new_page_ids);

[cbentzel, 2011-08-09 18:21:14]

I will add comments describing what this is doing.

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

http://codereview.chromium.org/7491096/diff/5019/content/common/view_messages...
content/common/view_messages.h:747: // Used to fix page_id offsets when pruning
history.
On 2011/08/09 18:14:40, brettw wrote:
> Can you expand on this? It doesn't actually tell what it means. I'm actually a
> bit confused. I was trying to write a comment for my code sample below and I
was
> having trouble describing what this message is telling us to do.
> 
> With a name like offset, I would expect things to be shifted. Is my
> understanding of what this is supposed to do correct? : It sets sets the
current
> page as the most recent one in history with the given number of previous
history
> items. And we assume all previous history items are in other processes since
> this is a preloaded page with no history.
> 
> If that's true, "OffsetAndPrune" seems maybe misleading or insufficient. Maybe
> this is complicated enough that we just need to name this as prerendering
> related? :
>   ViewMsg_SetHistoryStateForPreloadedPage, history_length)

This impacts Instant as well [and yes, names are confusing].

Your assumption is correct. Basically, when the prerendered page is created it
will typically have a history_page_ids_ of

[4]

If there were 5 previous history items at the time OffsetAndPruneHistory is
called, it will look like

[-1, -1, -1, -1, -1, 4]

Perhaps I should call this "ShiftHistory"?

[cbentzel, 2011-08-09 19:12:14]

Thanks for the reviews so far.

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

http://codereview.chromium.org/7491096/diff/5019/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1808: rvh->Send(new
ViewMsg_OffsetAndPruneHistory(rvh->routing_id(), offset));
On 2011/08/09 18:14:40, brettw wrote:
> Okay, I think that answers my question: in the cross-site case the offsets are
> sent with OnNavigate. Since we don't necessarily know the right ones at the
time
> of preloading, they may be wrong.
> 
> This brings up an interesting point that history.length will be wrong in a
> prerendered page. I think this probably isn't worth fixing right now, but it's
> worth keeping in mind (maybe we should file a bug or put a note somewhere in
the
> code about this?)

Thanks for thinking of that case. I entered a bug:
http://code.google.com/p/chromium/issues/detail?id=92202

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

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1023: int current_page_id = -1;
On 2011/08/09 17:36:03, tburkard wrote:
> I'd add a comment explaining what this does (either here or in the headerfile)

I did this at the ViewMsg declaration, which looked more consistent.

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1024: if (history_list_offset_ >= 0) {
On 2011/08/09 18:14:40, brettw wrote:
> No {} for single-line conditionals.

Done.

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1030: if (history_list_length_ >= 0)
On 2011/08/09 18:14:40, brettw wrote:
> What case will the history_list_length_ be negative? That seems suspicious.

That would be suspicious. Removed during the migration to the new version. I do
still check for negative history_list_offset_ because this may be swapped in
after a Navigate message but before a didCommitProvisionalLoad.

http://codereview.chromium.org/7491096/diff/5019/content/renderer/render_view...
content/renderer/render_view.cc:1033: for (int i = 0; i < history_list_offset_;
++i)
On 2011/08/09 18:14:40, brettw wrote:
> I think this version is a bit easier to follow:
> 
> // <clear comment about what type of list we're trying to generate>.
> std::vector<int32> new_page_ids_;
> new_page_ids.resize(history_list_length_, -1);
> new_page_ids[history_list_offset_] = current_page_id;
> history_page_ids.swap(new_page_ids);

Yes, that's cleaner. Moved to that.

[brettw, 2011-08-09 19:38:33]

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

http://codereview.chromium.org/7491096/diff/5019/content/common/view_messages...
content/common/view_messages.h:747: // Used to fix page_id offsets when pruning
history.
This would be a great example to put in the comment here, and maybe the handler
in render_view.cc should reference this location.

Maybe something like: SetHistoryLengthAndClear
and call the parameter "length"?

http://codereview.chromium.org/7491096/diff/12003/content/common/view_message...
content/common/view_messages.h:752: // down the pipe.
I still don't feel like this is shifting history. This only "shifts" if the
current entry is the first one. If you have 3 current history items, this
doesn't shift at all. This is what I was trying to communicate with my previous
suggestions.

Also: I really liked your example of [-1, -1, -1, 4] for an example length of 3.
Can you add that here?

[brettw, 2011-08-09 19:40:04]

Sorry for the confusing comments. Apparently I forgot to send mail from my last
pass so there are two comments for the same line.

[cbentzel, 2011-08-09 20:23:10]

I changed the name to SetHistoryLengthAndClear and updated comments. No
functional changes were made with the most recent patchsets.

[brettw, 2011-08-09 20:55:45]

LGTM.

Charlie: it would be great if you could take a look at this when you get a
chance (even after it's checked in).

[cbentzel, 2011-08-09 20:58:05]

Charlie's looking at it now. I'm going to hold off landing until he
gets eyes on it, but I do want to get this in today.

On Tue, Aug 9, 2011 at 4:55 PM,  <brettw@chromium.org> wrote:
> LGTM.
>
> Charlie: it would be great if you could take a look at this when you get a
> chance (even after it's checked in).
>
> http://codereview.chromium.org/7491096/
>

[Charlie Reis, 2011-08-09 20:59:31]

Thanks Chris.  My comments below-- I'm mainly concerned about what would happen
if this were called at the wrong time, so some extra checks, assertions, and
comments might help.

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/navigation_controller.cc:936:
tab_contents_->SetHistoryLengthAndClear(entry_count() - 1);
There's some hidden assumptions here that make me a little uneasy about putting
this call here.  The comments on CopyStateFromAndPrune seem to suggest this is
somewhat general, but this call assumes all previous entries were not rendered
by the current RenderViewHost and can thus be marked -1.

Just wondering if we should update the comment or contract to say this method is
explicitly for preparing a TabContents to be swapped in.

Outside of that, are we guaranteed this message will be sent before the new
TabContents has a chance to do a history.back() or history.forward()?  Otherwise
that could lead to a race.

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

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:600: RenderViewHost* rvh =
render_view_host();
This concerns me because a TabContents can have multiple RenderViewHosts (e.g.,
a pending one), making a race possible here if this is called at the wrong time.
 We would not want to send this message to an in-use RenderViewHost, since it
could corrupt its history and lead to worse problems.

Are there assertions we can make here to be sure we're actually swapping in a
new TabContents that doesn't have a pending RVH?

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:626: // Sets the history for this
tab_contents to |history_length| entries, and
Sounds odd to me to "set the history for a TabContents."  History is maintained
in the NavigationController and not here, but NavigationController is the one
telling us to do this.

Perhaps we should say that it updates the history length in the current
RenderView.

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

http://codereview.chromium.org/7491096/diff/15013/content/common/view_message...
content/common/view_messages.h:747: // Set's the history length of page_id's for
a RenderView by
Nit: No apostrophes in either case, and s/by/to/.  Maybe "Sets the length of
history_page_ids_"?

http://codereview.chromium.org/7491096/diff/15013/content/common/view_message...
content/common/view_messages.h:755: // prerendered page), so it looks like [-1
-1 -1 7].
Please mention the invariant about when this is safe to send.  It must be sent
before the new renderer has a chance to navigate back or forward, correct?

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

http://codereview.chromium.org/7491096/diff/15013/content/renderer/render_vie...
content/renderer/render_view.cc:1024: DCHECK(history_list_length_ == 0 ||
history_list_length_ == 1);
Should we return early if this isn't the case?  Release builds will proceed
without hitting this DCHECK, and it doesn't seem safe to continue if we're not
in this initial state.

http://codereview.chromium.org/7491096/diff/15013/content/renderer/render_vie...
content/renderer/render_view.cc:1025: int current_page_id = -1;
I think the page_id_ field should be the same thing, right?  Maybe you can use
that and DCHECK that it's correct.

http://codereview.chromium.org/7491096/diff/15013/content/renderer/render_vie...
content/renderer/render_view.cc:1028: int new_history_list_length =
history_length + history_list_length_;
Why are we adding the two?  It sounds like we're supposed to be setting it to
the given history_length, but this shifts it by the given amount.

More specifically, are there actually separate cases when the
history_list_length_ will be 0 and 1?  I would have guessed it to be 1, and just
set the history_list_length_ to the argument plus 1.

[cbentzel, 2011-08-09 21:21:12]

Still looking at the other issues you brought up.

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
File content/browser/tab_contents/navigation_controller.cc (right):

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/navigation_controller.cc:936:
tab_contents_->SetHistoryLengthAndClear(entry_count() - 1);
On 2011/08/09 20:59:31, creis wrote:
> There's some hidden assumptions here that make me a little uneasy about
putting
> this call here.  The comments on CopyStateFromAndPrune seem to suggest this is
> somewhat general, but this call assumes all previous entries were not rendered
> by the current RenderViewHost and can thus be marked -1.
> 
> Just wondering if we should update the comment or contract to say this method
is
> explicitly for preparing a TabContents to be swapped in.
> 
> Outside of that, are we guaranteed this message will be sent before the new
> TabContents has a chance to do a history.back() or history.forward()? 
Otherwise
> that could lead to a race.

We chatted about history.back() and history.forward() for the prerendered page.
In general these should be no-ops, but I don't think we handle the case where
history.pushState or location setting takes place with this well.

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

http://codereview.chromium.org/7491096/diff/15013/content/renderer/render_vie...
content/renderer/render_view.cc:1024: DCHECK(history_list_length_ == 0 ||
history_list_length_ == 1);
On 2011/08/09 20:59:31, creis wrote:
> Should we return early if this isn't the case?  Release builds will proceed
> without hitting this DCHECK, and it doesn't seem safe to continue if we're not
> in this initial state.

Yes. Changed to returning early.

http://codereview.chromium.org/7491096/diff/15013/content/renderer/render_vie...
content/renderer/render_view.cc:1028: int new_history_list_length =
history_length + history_list_length_;
On 2011/08/09 20:59:31, creis wrote:
> Why are we adding the two?  It sounds like we're supposed to be setting it to
> the given history_length, but this shifts it by the given amount.
> 
> More specifically, are there actually separate cases when the
> history_list_length_ will be 0 and 1?  I would have guessed it to be 1, and
just
> set the history_list_length_ to the argument plus 1.

The prerendered page could be swapped in between the initial navigate and the
commit of the provisional load, when history_list_length_ is 0. I added a
comment for this.

[cbentzel, 2011-08-09 22:22:14]

My most recent patch set does a better job handling the case that the
prerendered page may have multiple page_id's at the time it's swapped in.
Looking at the NavigationController's CopyStateFromAndPrune, this case is
handled but we weren't handling it as well in the RenderView case. 

One other option is to pass down the existing index that we want to preserve in
the IPC as well as the length to be adjusted.

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

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:600: RenderViewHost* rvh =
render_view_host();
On 2011/08/09 20:59:31, creis wrote:
> This concerns me because a TabContents can have multiple RenderViewHosts
(e.g.,
> a pending one), making a race possible here if this is called at the wrong
time.
>  We would not want to send this message to an in-use RenderViewHost, since it
> could corrupt its history and lead to worse problems.
> 
> Are there assertions we can make here to be sure we're actually swapping in a
> new TabContents that doesn't have a pending RVH?

Is there a way to retrieve whether a NavigationEntry would be handled by a
particular RenderViewHost? I could skip sending this message down if the current
render_view_host_ does not work for the NavigationEntry we are trying to retain
when pruning.

[Charlie Reis, 2011-08-09 22:51:42]

Thanks-- comments inline.

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

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:600: RenderViewHost* rvh =
render_view_host();
On 2011/08/09 22:22:15, cbentzel wrote:
> On 2011/08/09 20:59:31, creis wrote:
> > This concerns me because a TabContents can have multiple RenderViewHosts
> (e.g.,
> > a pending one), making a race possible here if this is called at the wrong
> time.
> >  We would not want to send this message to an in-use RenderViewHost, since
it
> > could corrupt its history and lead to worse problems.
> > 
> > Are there assertions we can make here to be sure we're actually swapping in
a
> > new TabContents that doesn't have a pending RVH?
> 
> Is there a way to retrieve whether a NavigationEntry would be handled by a
> particular RenderViewHost? I could skip sending this message down if the
current
> render_view_host_ does not work for the NavigationEntry we are trying to
retain
> when pruning. 

Yes-- you can compare the SiteInstance of the NavigationEntry (if it has one)
with the SiteInstance of the RVH.  If they don't match, abort.

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

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1027: // only add one entry, regardless of how
long the current history is.
Please add a TODO about investigating what happens if a prerendered page
navigates to several entries before it is swapped in.  Seems like it could cause
issues if we simply cropped those entries from the history, since the page might
assume it could go back to earlier states.

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1029: if (history_list_length_ > 0) {
Nit: no braces on a one-liner.

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1034: int current_page_id = -1;
Is this needed, or is page_id_ sufficient?

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1039: std::vector<int32>
new_history_page_ids_(new_history_list_length, -1);
Nit: local variable shouldn't have an underscore, should it?

[cbentzel, 2011-08-09 23:14:23]

Thanks again for the review. Last patchset is what I plan to land with
additional work done later.

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

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1027: // only add one entry, regardless of how
long the current history is.
On 2011/08/09 22:51:42, creis wrote:
> Please add a TODO about investigating what happens if a prerendered page
> navigates to several entries before it is swapped in.  Seems like it could
cause
> issues if we simply cropped those entries from the history, since the page
might
> assume it could go back to earlier states.

This does match what the NavigationController does in CopyStateFromAndPrune - it
leaves only the "active entry" - but this may be incorrect behavior. Another
approach is to cancel prerenders if location's or pushState's are attempted. 

Regardless, I added a TODO and will investigate.

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1029: if (history_list_length_ > 0) {
On 2011/08/09 22:51:42, creis wrote:
> Nit: no braces on a one-liner.

I always mismatch the style guide here [perhaps due to latent disagreement with
the rule].

Done.

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1034: int current_page_id = -1;
On 2011/08/09 22:51:42, creis wrote:
> Is this needed, or is page_id_ sufficient?

page_id_ does look sufficient. I'm wondering if that should be removed longer
term to prevent redundant data which may drift.

http://codereview.chromium.org/7491096/diff/13013/content/renderer/render_vie...
content/renderer/render_view.cc:1039: std::vector<int32>
new_history_page_ids_(new_history_list_length, -1);
On 2011/08/09 22:51:42, creis wrote:
> Nit: local variable shouldn't have an underscore, should it?

Done.

[Charlie Reis, 2011-08-09 23:25:16]

OK, LGTM for tonight.  Let's follow up for round 2 tomorrow.

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

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:600: RenderViewHost* rvh =
render_view_host();
On 2011/08/09 22:51:42, creis wrote:
> On 2011/08/09 22:22:15, cbentzel wrote:
> > On 2011/08/09 20:59:31, creis wrote:
> > > This concerns me because a TabContents can have multiple RenderViewHosts
> > (e.g.,
> > > a pending one), making a race possible here if this is called at the wrong
> > time.
> > >  We would not want to send this message to an in-use RenderViewHost, since
> it
> > > could corrupt its history and lead to worse problems.
> > > 
> > > Are there assertions we can make here to be sure we're actually swapping
in
> a
> > > new TabContents that doesn't have a pending RVH?
> > 
> > Is there a way to retrieve whether a NavigationEntry would be handled by a
> > particular RenderViewHost? I could skip sending this message down if the
> current
> > render_view_host_ does not work for the NavigationEntry we are trying to
> retain
> > when pruning. 
> 
> Yes-- you can compare the SiteInstance of the NavigationEntry (if it has one)
> with the SiteInstance of the RVH.  If they don't match, abort.

Please remember this for the followup CL.  (Maybe with a TODO?)

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:626: // Sets the history for this
tab_contents to |history_length| entries, and
On 2011/08/09 20:59:31, creis wrote:
> Sounds odd to me to "set the history for a TabContents."  History is
maintained
> in the NavigationController and not here, but NavigationController is the one
> telling us to do this.
> 
> Perhaps we should say that it updates the history length in the current
> RenderView.

Can you squeeze in this fix as well?

[cbentzel, 2011-08-09 23:29:37]

I kicked off tryjobs and don't want to introduce anything else at this
point. I already created a new branch that has these changes in it so
I won't forget.

On Tue, Aug 9, 2011 at 7:25 PM,  <creis@chromium.org> wrote:
> OK, LGTM for tonight.  Let's follow up for round 2 tomorrow.
>
>
>
http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
> File content/browser/tab_contents/tab_contents.cc (right):
>
>
http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
> content/browser/tab_contents/tab_contents.cc:600: RenderViewHost* rvh =
> render_view_host();
> On 2011/08/09 22:51:42, creis wrote:
>>
>> On 2011/08/09 22:22:15, cbentzel wrote:
>> > On 2011/08/09 20:59:31, creis wrote:
>> > > This concerns me because a TabContents can have multiple
>
> RenderViewHosts
>>
>> > (e.g.,
>> > > a pending one), making a race possible here if this is called at
>
> the wrong
>>
>> > time.
>> > >  We would not want to send this message to an in-use
>
> RenderViewHost, since
>>
>> it
>> > > could corrupt its history and lead to worse problems.
>> > >
>> > > Are there assertions we can make here to be sure we're actually
>
> swapping in
>>
>> a
>> > > new TabContents that doesn't have a pending RVH?
>> >
>> > Is there a way to retrieve whether a NavigationEntry would be
>
> handled by a
>>
>> > particular RenderViewHost? I could skip sending this message down if
>
> the
>>
>> current
>> > render_view_host_ does not work for the NavigationEntry we are
>
> trying to
>>
>> retain
>> > when pruning.
>
>> Yes-- you can compare the SiteInstance of the NavigationEntry (if it
>
> has one)
>>
>> with the SiteInstance of the RVH.  If they don't match, abort.
>
> Please remember this for the followup CL.  (Maybe with a TODO?)
>
>
http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
> File content/browser/tab_contents/tab_contents.h (right):
>
>
http://codereview.chromium.org/7491096/diff/15013/content/browser/tab_content...
> content/browser/tab_contents/tab_contents.h:626: // Sets the history for
> this tab_contents to |history_length| entries, and
> On 2011/08/09 20:59:31, creis wrote:
>>
>> Sounds odd to me to "set the history for a TabContents."  History is
>
> maintained
>>
>> in the NavigationController and not here, but NavigationController is
>
> the one
>>
>> telling us to do this.
>
>> Perhaps we should say that it updates the history length in the
>
> current
>>
>> RenderView.
>
> Can you squeeze in this fix as well?
>
> http://codereview.chromium.org/7491096/
>