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

content/renderer/render_view_browsertest.cc

 // Copyright (c) 2012 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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/memory/shared_memory.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 738 matching lines @@
   ASSERT_TRUE(msg);
   ViewHostMsg_UpdateState::Read(msg, &param);
   int page_id = param.a;
   PageState state = param.b;
   EXPECT_EQ(page_id_C, page_id);
   EXPECT_NE(state_A, state);
   EXPECT_NE(state_B, state);
   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 that stale back/forward navigations arriving from the browser are
+// ignored.  See http://crbug.com/86758.
 TEST_F(RenderViewImplTest, StaleNavigationsIgnored) {

[Charlie Reis, 2014/12/03 23:48:07]

Did you end up getting these tests to fail when your new check is commented out?

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

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

   // 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.
   ProcessPendingMessages();
   const IPC::Message* msg_A = render_thread_->sink().GetUniqueMessageMatching(
       ViewHostMsg_UpdateState::ID);
   ASSERT_TRUE(msg_A);
   ViewHostMsg_UpdateState::Param param;
   ViewHostMsg_UpdateState::Read(msg_A, &param);
   int page_id_A = param.a;
   PageState state_A = param.b;
@@ skipping 11 matching lines @@
   params_A.commit_params.page_state = state_A;
   params_A.commit_params.browser_navigation_start =
       base::TimeTicks::FromInternalValue(1);
   frame()->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]);
+  EXPECT_EQ(3, view()->page_id_); // page C is now page id 3
 
   // The browser then sends a stale navigation to B, which should be ignored.
   FrameMsg_Navigate_Params params_B;
   params_B.common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
   params_B.common_params.transition = ui::PAGE_TRANSITION_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.commit_params.page_state =
       state_A;  // Doesn't matter, just has to be present.
   params_B.commit_params.browser_navigation_start =
       base::TimeTicks::FromInternalValue(1);
   frame()->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 we do not ignore navigations after the entry limit is reached,
-// in which case the browser starts dropping entries from the front.  In this
-// case, we'll see a page_id mismatch but the RenderView's id will be older,
-// not newer, than params.page_id.  Use this as a cue that we should update the
-// state and not treat it like a navigation to a cropped forward history item.
-// See http://crbug.com/89798.
-TEST_F(RenderViewImplTest, DontIgnoreBackAfterNavEntryLimit) {

[Charlie Reis, 2014/12/03 23:48:07]

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.

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

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.

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

Acknowledged.

-  // 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.
-  ProcessPendingMessages();
-  const IPC::Message* msg_A = render_thread_->sink().GetUniqueMessageMatching(
-      ViewHostMsg_UpdateState::ID);
-  ASSERT_TRUE(msg_A);
-  ViewHostMsg_UpdateState::Param param;
-  ViewHostMsg_UpdateState::Read(msg_A, &param);
-  int page_id_A = param.a;
-  PageState state_A = param.b;
-  EXPECT_EQ(1, page_id_A);
-  render_thread_->sink().ClearMessages();
-
-  // Load page C, which will trigger an UpdateState message for page B.
-  LoadHTML("<div>Page C</div>");
-  EXPECT_EQ(3, view()->history_list_length_);
-  EXPECT_EQ(2, view()->history_list_offset_);
-  EXPECT_EQ(3, view()->history_page_ids_[2]);
-
-  // Check for a valid UpdateState message for page B.
-  ProcessPendingMessages();
-  const IPC::Message* msg_B = render_thread_->sink().GetUniqueMessageMatching(
-      ViewHostMsg_UpdateState::ID);
-  ASSERT_TRUE(msg_B);
-  ViewHostMsg_UpdateState::Read(msg_B, &param);
-  int page_id_B = param.a;
-  PageState state_B = param.b;
-  EXPECT_EQ(2, page_id_B);
-  render_thread_->sink().ClearMessages();
-
-  // Suppose the browser has limited the number of NavigationEntries to 2.
-  // It has now dropped the first entry, but the renderer isn't notified.

[Charlie Reis, 2014/12/03 23:48:07]

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.

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

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

-  // Ensure that going back to page B (page_id 2) at offset 0 is successful.
-  FrameMsg_Navigate_Params params_B;
-  params_B.common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
-  params_B.common_params.transition = ui::PAGE_TRANSITION_FORWARD_BACK;
-  params_B.current_history_list_length = 2;
-  params_B.current_history_list_offset = 1;
-  params_B.pending_history_list_offset = 0;
-  params_B.page_id = 2;
-  params_B.commit_params.page_state = state_B;
-  params_B.commit_params.browser_navigation_start =
-      base::TimeTicks::FromInternalValue(1);
-  frame()->OnNavigate(params_B);
-  ProcessPendingMessages();
-
-  EXPECT_EQ(2, view()->history_list_length_);
-  EXPECT_EQ(0, view()->history_list_offset_);
-  EXPECT_EQ(2, view()->history_page_ids_[0]);
+  EXPECT_EQ(3, view()->page_id_); // page C, not page B
 }
 
 // Test that our IME backend sends a notification message when the input focus
 // changes.
 TEST_F(RenderViewImplTest, OnImeTypeChanged) {
   // Enable our IME backend code.
   view()->OnSetInputMethodActive(true);
 
   // Load an HTML page consisting of two input fields.
   view()->set_send_content_state_immediately(true);
@@ skipping 724 matching lines @@
   EXPECT_TRUE(web_frame->isViewSourceModeEnabled());
 }
 
 // Regression test for http://crbug.com/41562
 TEST_F(RenderViewImplTest, UpdateTargetURLWithInvalidURL) {
   const GURL invalid_gurl("http://");
   view()->setMouseOverURL(blink::WebURL(invalid_gurl));
   EXPECT_EQ(invalid_gurl, view()->target_url_);
 }
 
-TEST_F(RenderViewImplTest, SetHistoryLengthAndPrune) {
-  int expected_page_id = -1;
+TEST_F(RenderViewImplTest, SetHistoryLengthAndOffset) {
+  // No history to merge; one committed page.
+  view()->OnSetHistoryOffsetAndLength(0, 1);
+  EXPECT_EQ(1, view()->history_list_length_);
+  EXPECT_EQ(0, view()->history_list_offset_);
 
-  // No history to merge and no committed pages.
-  view()->OnSetHistoryLengthAndPrune(0, -1);
-  EXPECT_EQ(0, view()->history_list_length_);
-  EXPECT_EQ(-1, view()->history_list_offset_);
-
-  // History to merge and no committed pages.
-  view()->OnSetHistoryLengthAndPrune(2, -1);
+  // History of length 1 to merge; one committed page.
+  view()->OnSetHistoryOffsetAndLength(1, 2);
   EXPECT_EQ(2, view()->history_list_length_);
   EXPECT_EQ(1, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  ClearHistory();
-
-  blink::WebHistoryItem item;
-  item.initialize();
-
-  // No history to merge and a committed page to be kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(0, expected_page_id);
-  EXPECT_EQ(1, view()->history_list_length_);
-  EXPECT_EQ(0, view()->history_list_offset_);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
-  ClearHistory();
-
-  // No history to merge and a committed page to be pruned.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(0, expected_page_id + 1);
-  EXPECT_EQ(0, view()->history_list_length_);
-  EXPECT_EQ(-1, view()->history_list_offset_);
-  ClearHistory();
-
-  // No history to merge and a committed page that the browser was unaware of.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(0, -1);
-  EXPECT_EQ(1, view()->history_list_length_);
-  EXPECT_EQ(0, view()->history_list_offset_);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
-  ClearHistory();
-
-  // History to merge and a committed page to be kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(2, expected_page_id);
-  EXPECT_EQ(3, view()->history_list_length_);
-  EXPECT_EQ(2, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
-  ClearHistory();
-
-  // History to merge and a committed page to be pruned.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(2, expected_page_id + 1);
-  EXPECT_EQ(2, view()->history_list_length_);
-  EXPECT_EQ(1, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  ClearHistory();
-
-  // History to merge and a committed page that the browser was unaware of.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  view()->OnSetHistoryLengthAndPrune(2, -1);
-  EXPECT_EQ(3, view()->history_list_length_);
-  EXPECT_EQ(2, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
-  ClearHistory();
-
-  int expected_page_id_2 = -1;
-
-  // No history to merge and two committed pages, both to be kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(0, expected_page_id);
-  EXPECT_EQ(2, view()->history_list_length_);
-  EXPECT_EQ(1, view()->history_list_offset_);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[1]);
-  ClearHistory();
-
-  // No history to merge and two committed pages, and only the second is kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(0, expected_page_id_2);
-  EXPECT_EQ(1, view()->history_list_length_);
-  EXPECT_EQ(0, view()->history_list_offset_);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[0]);
-  ClearHistory();
-
-  // No history to merge and two committed pages, both of which the browser was
-  // unaware of.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(0, -1);
-  EXPECT_EQ(2, view()->history_list_length_);
-  EXPECT_EQ(1, view()->history_list_offset_);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[0]);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[1]);
-  ClearHistory();
-
-  // History to merge and two committed pages, both to be kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(2, expected_page_id);
-  EXPECT_EQ(4, view()->history_list_length_);
-  EXPECT_EQ(3, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[3]);
-  ClearHistory();
-
-  // History to merge and two committed pages, and only the second is kept.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(2, expected_page_id_2);
-  EXPECT_EQ(3, view()->history_list_length_);
-  EXPECT_EQ(2, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[2]);
-  ClearHistory();
-
-  // History to merge and two committed pages, both of which the browser was
-  // unaware of.
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id = view()->page_id_;
-  frame()->didCommitProvisionalLoad(GetMainFrame(),
-                                    item,
-                                    blink::WebStandardCommit);
-  expected_page_id_2 = view()->page_id_;
-  EXPECT_GT(expected_page_id_2, expected_page_id);
-  view()->OnSetHistoryLengthAndPrune(2, -1);
-  EXPECT_EQ(4, view()->history_list_length_);
-  EXPECT_EQ(3, view()->history_list_offset_);
-  EXPECT_EQ(-1, view()->history_page_ids_[0]);
-  EXPECT_EQ(-1, view()->history_page_ids_[1]);
-  EXPECT_EQ(expected_page_id, view()->history_page_ids_[2]);
-  EXPECT_EQ(expected_page_id_2, view()->history_page_ids_[3]);
 }
 
 TEST_F(RenderViewImplTest, ContextMenu) {
   LoadHTML("<div>Page A</div>");
 
   // Create a right click in the center of the iframe. (I'm hoping this will
   // make this a bit more robust in case of some other formatting or other bug.)
   WebMouseEvent mouse_event;
   mouse_event.type = WebInputEvent::MouseDown;
   mouse_event.button = WebMouseEvent::ButtonRight;
@@ skipping 649 matching lines @@
 
   gfx::Size initial_size_;
 };
 
 TEST_F(RenderViewImplInitialSizeTest, InitialSize) {
   ASSERT_EQ(initial_size_, view_->GetSize());
   ASSERT_EQ(initial_size_, gfx::Size(view_->GetWebView()->size()));
 }
 
 }  // namespace content