Keep track of the history's page IDs to avoid navigating to a stale entry.

This ensures that a race between an asynchronous back/forward and a navigation
in the renderer doesn't confuse NavigationController.

BUG=86758
TEST=RenderViewTest.IgnoreStaleNavigations
TEST=NavigationControllerTest.InPage_Replace

Review URL: http://codereview.chromium.org/7327014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92748 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 149 insertions, 7 deletions

diff --git a/content/renderer/render_view.cc b/content/renderer/render_view.cc
index 06d6a88..d1d3a82 100644
--- a/content/renderer/render_view.cc
+++ b/content/renderer/render_view.cc
@@ -409,6 +409,8 @@ RenderView::RenderView(RenderThreadBase* render_thread,
 }
 
 RenderView::~RenderView() {
+  history_page_ids_.clear();
+
   if (decrement_shared_popup_at_destruction_)
     shared_popup_counter_->data--;
 
@@ -687,19 +689,29 @@ void RenderView::OnNavigate(const ViewMsg_Navigate_Params& params) {
   if (!webview())
     return;
 
+  bool is_reload =
+      params.navigation_type == ViewMsg_Navigate_Type::RELOAD ||
+      params.navigation_type == ViewMsg_Navigate_Type::RELOAD_IGNORING_CACHE;
+
+  // If this is a stale back/forward (due to a recent navigation the browser
+  // didn't know about), ignore it.
+  if (IsBackForwardToStaleEntry(params, is_reload))
+    return;
+
   // Swap this renderer back in if necessary.
   if (is_swapped_out_)
     SetSwappedOut(false);
 
   history_list_offset_ = params.current_history_list_offset;
   history_list_length_ = params.current_history_list_length;
+  if (history_list_length_ >= 0)
+    history_page_ids_.resize(history_list_length_, -1);
+  if (params.pending_history_list_offset >= 0 &&
+      params.pending_history_list_offset < history_list_length_)
+    history_page_ids_[params.pending_history_list_offset] = params.page_id;
 
   content::GetContentClient()->SetActiveURL(params.url);
 
-  bool is_reload =
-      params.navigation_type == ViewMsg_Navigate_Type::RELOAD ||
-      params.navigation_type == ViewMsg_Navigate_Type::RELOAD_IGNORING_CACHE;
-
   WebFrame* main_frame = webview()->mainFrame();
   if (is_reload && main_frame->currentHistoryItem().isNull()) {
     // We cannot reload if we do not have any history state.  This happens, for
@@ -792,6 +804,35 @@ void RenderView::OnNavigate(const ViewMsg_Navigate_Params& params) {
   pending_navigation_state_.reset();
 }
 
+bool RenderView::IsBackForwardToStaleEntry(
+    const ViewMsg_Navigate_Params& params,
+    bool is_reload) {
+  // Make sure this isn't a back/forward to an entry we have already cropped
+  // or replaced from our history, before the browser knew about it.  If so,
+  // a new navigation has committed in the mean time, and we can ignore this.
+  bool is_back_forward = !is_reload && !params.state.empty();
+
+  // Note: if the history_list_length_ is 0 for a back/forward, we must be
+  // restoring from a previous session.  We'll update our state in OnNavigate.
+  if (!is_back_forward || history_list_length_ <= 0)
+    return false;
+
+  DCHECK_EQ(static_cast<int>(history_page_ids_.size()), history_list_length_);
+
+  // Check for whether the forward history has been cropped due to a recent
+  // navigation the browser didn't know about.
+  if (params.pending_history_list_offset >= history_list_length_)
+    return true;
+
+  // Check for whether this entry has been replaced with a new one.
+  int expected_page_id =
+      history_page_ids_[params.pending_history_list_offset];
+  if (expected_page_id > 0 && params.page_id != expected_page_id)
+    return true;
+
+  return false;
+}
+
 // Stop loading the current page
 void RenderView::OnStop() {
   if (webview()) {
@@ -2378,6 +2419,8 @@ void RenderView::didCommitProvisionalLoad(WebFrame* frame,
     if (history_list_offset_ >= content::kMaxSessionHistoryEntries)
       history_list_offset_ = content::kMaxSessionHistoryEntries - 1;
     history_list_length_ = history_list_offset_ + 1;
+    history_page_ids_.resize(history_list_length_, -1);
+    history_page_ids_[history_list_offset_] = page_id_;
   } else {
     // Inspect the navigation_state on this frame to see if the navigation
     // corresponds to a session history navigation...  Note: |frame| may or
@@ -2397,6 +2440,12 @@ void RenderView::didCommitProvisionalLoad(WebFrame* frame,
       page_id_ = navigation_state->pending_page_id();
 
       history_list_offset_ = navigation_state->pending_history_list_offset();
+
+      // If the history list is valid, our list of page IDs should be correct.
+      DCHECK(history_list_length_ <= 0 ||
+             history_list_offset_ < 0 ||
+             history_list_offset_ >= history_list_length_ ||
+             history_page_ids_[history_list_offset_] == page_id_);
     }
   }
 
diff --git a/content/renderer/render_view.h b/content/renderer/render_view.h
index 2578eb1..34e2fa1 100644
--- a/content/renderer/render_view.h
+++ b/content/renderer/render_view.h
@@ -207,6 +207,7 @@ class RenderView : public RenderWidget,
   void OnViewContextSwapBuffersAborted();
 
   int page_id() const { return page_id_; }
+  int history_list_offset() const { return history_list_offset_; }
   PepperPluginDelegateImpl* pepper_delegate() { return &pepper_delegate_; }
 
   const WebPreferences& webkit_preferences() const {
@@ -633,6 +634,7 @@ class RenderView : public RenderWidget,
   FRIEND_TEST_ALL_PREFIXES(RenderViewTest, OnImeStateChanged);
   FRIEND_TEST_ALL_PREFIXES(RenderViewTest, OnNavStateChanged);
   FRIEND_TEST_ALL_PREFIXES(RenderViewTest, OnSetTextDirection);
+  FRIEND_TEST_ALL_PREFIXES(RenderViewTest, StaleNavigationsIgnored);
   FRIEND_TEST_ALL_PREFIXES(RenderViewTest, UpdateTargetURLWithInvalidURL);
 #if defined(OS_MACOSX)
   FRIEND_TEST_ALL_PREFIXES(RenderViewTest, MacTestCmdUp);
@@ -861,6 +863,11 @@ class RenderView : public RenderWidget,
   // Should only be called if this object wraps a PluginDocument.
   WebKit::WebPlugin* GetWebPluginFromPluginDocument();
 
+  // Returns true if the |params| navigation is to an entry that has been
+  // cropped due to a recent navigation the browser did not know about.
+  bool IsBackForwardToStaleEntry(const ViewMsg_Navigate_Params& params,
+                                 bool is_reload);
+
   // Returns false unless this is a top-level navigation that crosses origins.
   bool IsNonLocalTopLevelNavigation(const GURL& url,
                                     WebKit::WebFrame* frame,
@@ -980,14 +987,27 @@ class RenderView : public RenderWidget,
   // globally unique in the renderer.
   static int32 next_page_id_;
 
+  // The offset of the current item in the history list.
+  int history_list_offset_;
+
+  // The RenderView's current impression of the history length.  This includes
+  // any items that have committed in this process, but because of cross-process
+  // navigations, the history may have some entries that were committed in other
+  // processes.  We won't know about them until the next navigation in this
+  // process.
+  int history_list_length_;
+
+  // The list of page IDs for each history item this RenderView knows about.
+  // Some entries may be -1 if they were rendered by other processes or were
+  // restored from a previous session.  This lets us detect attempts to
+  // navigate to stale entries that have been cropped from our history.
+  std::vector<int32> history_page_ids_;
+
   // Page info -----------------------------------------------------------------
 
   // The last gotten main frame's encoding.
   std::string last_encoding_name_;
 
-  int history_list_offset_;
-  int history_list_length_;
-
   // UI state ------------------------------------------------------------------
 
   // The state of our target_url transmissions. When we receive a request to
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 6e0d5b0..21c6ef2 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -95,7 +95,11 @@ TEST_F(RenderViewTest, LastCommittedUpdateState) {
 
   // Go back to C and commit, preparing for our real test.
   ViewMsg_Navigate_Params params_C;
+  params_C.navigation_type = ViewMsg_Navigate_Type::NORMAL;
   params_C.transition = PageTransition::FORWARD_BACK;
+  params_C.current_history_list_length = 4;
+  params_C.current_history_list_offset = 3;
+  params_C.pending_history_list_offset = 2;
   params_C.page_id = 3;
   params_C.state = state_C;
   view_->OnNavigate(params_C);
@@ -108,14 +112,22 @@ TEST_F(RenderViewTest, LastCommittedUpdateState) {
 
   // Back to page B (page_id 2), without committing.
   ViewMsg_Navigate_Params params_B;
+  params_B.navigation_type = ViewMsg_Navigate_Type::NORMAL;
   params_B.transition = PageTransition::FORWARD_BACK;
+  params_B.current_history_list_length = 4;
+  params_B.current_history_list_offset = 2;
+  params_B.pending_history_list_offset = 1;
   params_B.page_id = 2;
   params_B.state = state_B;
   view_->OnNavigate(params_B);
 
   // Back to page A (page_id 1) and commit.
   ViewMsg_Navigate_Params params;
+  params.navigation_type = ViewMsg_Navigate_Type::NORMAL;
   params.transition = PageTransition::FORWARD_BACK;
+  params_B.current_history_list_length = 4;
+  params_B.current_history_list_offset = 2;
+  params_B.pending_history_list_offset = 0;
   params.page_id = 1;
   params.state = state_A;
   view_->OnNavigate(params);
@@ -135,6 +147,67 @@ TEST_F(RenderViewTest, LastCommittedUpdateState) {
   EXPECT_EQ(state_C, state);
 }
 
+// Test that the history_page_ids_ list can reveal when a stale back/forward
+// navigation arrives from the browser and can be ignored.  See
+// http://crbug.com/86758.
+TEST_F(RenderViewTest, StaleNavigationsIgnored) {
+  // Load page A.
+  LoadHTML("<div>Page A</div>");
+  EXPECT_EQ(1, view_->history_list_length_);
+  EXPECT_EQ(0, view_->history_list_offset_);
+  EXPECT_EQ(1, view_->history_page_ids_[0]);
+
+  // Load page B, which will trigger an UpdateState message for page A.
+  LoadHTML("<div>Page B</div>");
+  EXPECT_EQ(2, view_->history_list_length_);
+  EXPECT_EQ(1, view_->history_list_offset_);
+  EXPECT_EQ(2, view_->history_page_ids_[1]);
+
+  // Check for a valid UpdateState message for page A.
+  const IPC::Message* msg_A = render_thread_.sink().GetUniqueMessageMatching(
+      ViewHostMsg_UpdateState::ID);
+  ASSERT_TRUE(msg_A);
+  int page_id_A;
+  std::string state_A;
+  ViewHostMsg_UpdateState::Read(msg_A, &page_id_A, &state_A);
+  EXPECT_EQ(1, page_id_A);
+  render_thread_.sink().ClearMessages();
+
+  // Back to page A (page_id 1) and commit.
+  ViewMsg_Navigate_Params params_A;
+  params_A.navigation_type = ViewMsg_Navigate_Type::NORMAL;
+  params_A.transition = PageTransition::FORWARD_BACK;
+  params_A.current_history_list_length = 2;
+  params_A.current_history_list_offset = 1;
+  params_A.pending_history_list_offset = 0;
+  params_A.page_id = 1;
+  params_A.state = state_A;
+  view_->OnNavigate(params_A);
+  ProcessPendingMessages();
+
+  // A new navigation commits, clearing the forward history.
+  LoadHTML("<div>Page C</div>");
+  EXPECT_EQ(2, view_->history_list_length_);
+  EXPECT_EQ(1, view_->history_list_offset_);
+  EXPECT_EQ(3, view_->history_page_ids_[1]);
+
+  // The browser then sends a stale navigation to B, which should be ignored.
+  ViewMsg_Navigate_Params params_B;
+  params_B.navigation_type = ViewMsg_Navigate_Type::NORMAL;
+  params_B.transition = PageTransition::FORWARD_BACK;
+  params_B.current_history_list_length = 2;
+  params_B.current_history_list_offset = 0;
+  params_B.pending_history_list_offset = 1;
+  params_B.page_id = 2;
+  params_B.state = state_A;  // Doesn't matter, just has to be present.
+  view_->OnNavigate(params_B);
+
+  // State should be unchanged.
+  EXPECT_EQ(2, view_->history_list_length_);
+  EXPECT_EQ(1, view_->history_list_offset_);
+  EXPECT_EQ(3, view_->history_page_ids_[1]);
+}
+
 // Test that our IME backend sends a notification message when the input focus
 // changes.
 TEST_F(RenderViewTest, OnImeStateChanged) {