Additional fixes for prerender/instant + browsing history.

content/renderer/render_view.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_view.h"
 
 #include <algorithm>
 #include <cmath>
 #include <string>
 #include <vector>
@@ skipping 1002 matching lines @@
 void RenderView::OnScrollFocusedEditableNodeIntoView() {
   WebKit::WebNode node = GetFocusedNode();
   if (!node.isNull()) {
     if (IsEditableNode(node))
       // TODO(varunjain): Change webkit API to scroll a particular node into
       // view and use that API here instead.
       webview()->scrollFocusedNodeIntoView();
   }
 }
 
-void RenderView::OnSetHistoryLengthAndClear(int history_length) {
+void RenderView::OnSetHistoryLengthAndClear(int history_length, int32 page_id) {
   DCHECK(history_length >= 0);
 
+  if (page_id >= 0) {
+    // It's possible that the underlying page could have done navigation
+    // at this point, which this isn't prepared for.
+    // TODO(cbentzel): Send a message to the browser? There will be a
+    // mismatch between NavigationController and RenderView here.
+    if (page_id_ != page_id) {

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

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?)

+      return;
+    }
+  } else {
+    // The prerendered page could have been swapped after the initial Navigate
+    // message is sent but prior to the commit of the provisional load being
+    // done, and sent to the browser. In that case, we can't validate so
+    // use the current one.
+    DCHECK(page_id == -1);

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

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.)

+  }
+
   // history_list_length_ may be 0 if this is called between
   // a navigate and a commit of the provisional load. Otherwise,
   // only add one entry, regardless of how long the current history is.
   // TODO(cbentzel): Investigate what happens if a prerendered page
   //   navigates to several entries before it is swapped in. Cropping
   //   those may be a bad idea.
   int new_history_list_length = history_length;
   if (history_list_length_ > 0)
     ++new_history_list_length;
 
@@ skipping 3448 matching lines @@
 }
 #endif
 
 void RenderView::OnContextMenuClosed(
     const webkit_glue::CustomContextMenuContext& custom_context) {
   if (custom_context.is_pepper_menu)
     pepper_delegate_.OnContextMenuClosed(custom_context);
   else
     context_menu_node_.reset();
 }