Revert "Don't create pending entries when a navigation is initiated by the page."

TBR=jcivelli

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

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

6 files changed, 12 insertions, 165 deletions

diff --git a/chrome/browser/tab_contents/navigation_controller_unittest.cc b/chrome/browser/tab_contents/navigation_controller_unittest.cc
index 0794a38..8d9f4f6 100644
--- a/chrome/browser/tab_contents/navigation_controller_unittest.cc
+++ b/chrome/browser/tab_contents/navigation_controller_unittest.cc
@@ -1805,59 +1805,6 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune3) {
   EXPECT_EQ(controller().session_id().id(), other_controller.session_id().id());
 }
 
-// Tests that navigations initiated from the page (with the history object)
-// work as expected without navigation entries.
-TEST_F(NavigationControllerTest, HistoryNavigate) {
-  const GURL url1("http://foo1");
-  const GURL url2("http://foo2");
-  const GURL url3("http://foo3");
-
-  NavigateAndCommit(url1);
-  NavigateAndCommit(url2);
-  NavigateAndCommit(url3);
-  controller().GoBack();
-  contents()->CommitPendingNavigation();
-
-  // Casts the TabContents to a RenderViewHostDelegate::BrowserIntegration so we
-  // can call GoToEntryAtOffset which is private.
-  RenderViewHostDelegate::BrowserIntegration* rvh_delegate =
-      static_cast<RenderViewHostDelegate::BrowserIntegration*>(contents());
-
-  // Simulate the page calling history.back(), it should not create a pending
-  // entry.
-  rvh_delegate->GoToEntryAtOffset(-1);
-  EXPECT_EQ(-1, controller().pending_entry_index());
-  // The actual cross-navigation is suspended until the current RVH tells us
-  // it unloaded, simulate that.
-  contents()->ProceedWithCrossSiteNavigation();
-  // Also make sure we told the page to navigate.
-  const IPC::Message* message =
-      process()->sink().GetFirstMessageMatching(ViewMsg_Navigate::ID);
-  ASSERT_TRUE(message != NULL);
-  Tuple1<ViewMsg_Navigate_Params> nav_params;
-  ViewMsg_Navigate::Read(message, &nav_params);
-  EXPECT_EQ(url1, nav_params.a.url);
-  process()->sink().ClearMessages();
-
-  // Now test history.forward()
-  rvh_delegate->GoToEntryAtOffset(1);
-  EXPECT_EQ(-1, controller().pending_entry_index());
-  // The actual cross-navigation is suspended until the current RVH tells us
-  // it unloaded, simulate that.
-  contents()->ProceedWithCrossSiteNavigation();
-  message = process()->sink().GetFirstMessageMatching(ViewMsg_Navigate::ID);
-  ASSERT_TRUE(message != NULL);
-  ViewMsg_Navigate::Read(message, &nav_params);
-  EXPECT_EQ(url3, nav_params.a.url);
-  process()->sink().ClearMessages();
-
-  // Make sure an extravagant history.go() doesn't break.
-  rvh_delegate->GoToEntryAtOffset(120);  // Out of bounds.
-  EXPECT_EQ(-1, controller().pending_entry_index());
-  message = process()->sink().GetFirstMessageMatching(ViewMsg_Navigate::ID);
-  EXPECT_TRUE(message == NULL);
-}
-
 /* TODO(brettw) These test pass on my local machine but fail on the XP buildbot
    (but not Vista) cleaning up the directory after they run.
    This should be fixed.
diff --git a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
index 74d73e6..72dfaca 100644
--- a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -11,7 +11,6 @@
 #include "chrome/browser/tab_contents/render_view_host_manager.h"
 #include "chrome/browser/tab_contents/test_tab_contents.h"
 #include "chrome/common/render_messages.h"
-#include "chrome/common/render_messages_params.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/test_notification_tracker.h"
 #include "chrome/test/testing_profile.h"
@@ -296,49 +295,3 @@ TEST_F(RenderViewHostManagerTest, NonDOMUIChromeURLs) {
 
   EXPECT_TRUE(ShouldSwapProcesses(&manager, &ntp_entry, &about_entry));
 }
-
-// Tests that we don't end up in an inconsistent state if a page does a back and
-// then reload. http://crbug.com/51680
-TEST_F(RenderViewHostManagerTest, PageDoesBackAndReload) {
-  GURL url1("http://www.google.com/");
-  GURL url2("http://www.evil-site.com/");
-
-  // Navigate to a safe site, then an evil site.
-  contents()->NavigateAndCommit(url1);
-  RenderViewHost* host1 = contents()->render_view_host();
-  contents()->NavigateAndCommit(url2);
-  RenderViewHost* host2 = contents()->render_view_host();
-  // We should have got a new RVH for the evil site.
-  EXPECT_NE(host1, host2);
-
-  // Casts the TabContents to a RenderViewHostDelegate::BrowserIntegration so we
-  // can call GoToEntryAtOffset which is private.
-  RenderViewHostDelegate::BrowserIntegration* rvh_delegate =
-      static_cast<RenderViewHostDelegate::BrowserIntegration*>(contents());
-
-  // Now let's simulate the evil page calling history.back().
-  rvh_delegate->GoToEntryAtOffset(-1);
-  // The pending RVH should be the one for the Google.
-  EXPECT_EQ(host1, contents()->render_manager()->pending_render_view_host());
-
-  // Before that RVH has committed, the evil page reloads itself.
-  ViewHostMsg_FrameNavigate_Params params;
-  params.page_id = 1;
-  params.url = url2;
-  params.transition = PageTransition::CLIENT_REDIRECT;
-  params.should_update_history = false;
-  params.gesture = NavigationGestureAuto;
-  params.was_within_same_page = false;
-  params.is_post = false;
-  contents()->TestDidNavigate(host2, params);
-
-  // That should have cancelled the pending RVH, and the evil RVH should be the
-  // current one.
-  EXPECT_TRUE(contents()->render_manager()->pending_render_view_host() == NULL);
-  EXPECT_EQ(host2, contents()->render_manager()->current_host());
-
-  // Also we should not have a pending navigation entry.
-  NavigationEntry* entry = contents()->controller().GetActiveEntry();
-  ASSERT_TRUE(entry != NULL);
-  EXPECT_EQ(url2, entry->url());
-}
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 846c89d..59e18ff 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -224,12 +224,12 @@ ViewMsg_Navigate_Params::NavigationType GetNavigationType(
   return ViewMsg_Navigate_Params::NORMAL;
 }
 
-void MakeNavigateParams(const NavigationEntry& entry,
-                        const NavigationController& controller,
+void MakeNavigateParams(const NavigationController& controller,
                         NavigationController::ReloadType reload_type,
                         ViewMsg_Navigate_Params* params) {
+  const NavigationEntry& entry = *controller.pending_entry();
   params->page_id = entry.page_id();
-  params->pending_history_list_offset = controller.GetIndexOfEntry(&entry);
+  params->pending_history_list_offset = controller.pending_entry_index();
   params->current_history_list_offset = controller.last_committed_entry_index();
   params->current_history_list_length = controller.entry_count();
   params->url = entry.url();
@@ -844,12 +844,8 @@ void TabContents::OpenURL(const GURL& url, const GURL& referrer,
 
 bool TabContents::NavigateToPendingEntry(
     NavigationController::ReloadType reload_type) {
-  return NavigateToEntry(*controller_.pending_entry(), reload_type);
-}
+  const NavigationEntry& entry = *controller_.pending_entry();
 
-bool TabContents::NavigateToEntry(
-    const NavigationEntry& entry,
-    NavigationController::ReloadType reload_type) {
   RenderViewHost* dest_render_view_host = render_manager_.Navigate(entry);
   if (!dest_render_view_host)
     return false;  // Unable to create the desired render view host.
@@ -881,7 +877,7 @@ bool TabContents::NavigateToEntry(
 
   // Navigate in the desired RenderViewHost.
   ViewMsg_Navigate_Params navigate_params;
-  MakeNavigateParams(entry, controller_, reload_type, &navigate_params);
+  MakeNavigateParams(controller_, reload_type, &navigate_params);
   dest_render_view_host->Navigate(navigate_params);
 
   if (entry.page_id() == -1) {
@@ -1988,15 +1984,8 @@ void TabContents::OnFindReply(int request_id,
 }
 
 void TabContents::GoToEntryAtOffset(int offset) {
-  if (!delegate_ || delegate_->OnGoToEntryOffset(offset)) {
-    NavigationEntry* entry = controller_.GetEntryAtOffset(offset);
-    if (!entry)
-      return;
-    // Note that we don't call NavigationController::GotToOffset() as we don't
-    // want to create a pending navigation entry (it might end up lingering
-    // http://crbug.com/51680).
-    NavigateToEntry(*entry, NavigationController::NO_RELOAD);
-  }
+  if (!delegate_ || delegate_->OnGoToEntryOffset(offset))
+    controller_.GoToOffset(offset);
 }
 
 void TabContents::OnMissingPluginStatus(int status) {
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 8bca66f..113bc72 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -818,12 +818,6 @@ class TabContents : public PageNavigator,
   // different and was therefore updated.
   bool UpdateTitleForEntry(NavigationEntry* entry, const std::wstring& title);
 
-  // Causes the TabContents to navigate in the right renderer to |entry|, which
-  // must be already part of the entries in the navigation controller.
-  // This does not change the NavigationController state.
-  bool NavigateToEntry(const NavigationEntry& entry,
-                       NavigationController::ReloadType reload_type);
-
   // Misc non-view stuff -------------------------------------------------------
 
   // Helper functions for sending notifications.
diff --git a/chrome/browser/tab_contents/test_tab_contents.cc b/chrome/browser/tab_contents/test_tab_contents.cc
index 2c1ba39..54a6873 100644
--- a/chrome/browser/tab_contents/test_tab_contents.cc
+++ b/chrome/browser/tab_contents/test_tab_contents.cc
@@ -45,7 +45,7 @@ void TestTabContents::Observe(NotificationType type,
   }
 }
 
-TestRenderViewHost* TestTabContents::pending_rvh() const {
+TestRenderViewHost* TestTabContents::pending_rvh() {
   return static_cast<TestRenderViewHost*>(
       render_manager_.pending_render_view_host_);
 }
@@ -70,34 +70,7 @@ void TestTabContents::NavigateAndCommit(const GURL& url) {
   bool reverse_on_redirect = false;
   BrowserURLHandler::RewriteURLIfNecessary(
       &loaded_url, profile(), &reverse_on_redirect);
-
-  // LoadURL created a navigation entry, now simulate the RenderView sending
-  // a notification that it actually navigated.
-  CommitPendingNavigation();
-}
-
-void TestTabContents::CommitPendingNavigation() {
-  // If we are doing a cross-site navigation, this simulates the current RVH
-  // notifying that it has unloaded so the pending RVH is resumed and can
-  // navigate.
-  ProceedWithCrossSiteNavigation();
-  TestRenderViewHost* rvh = pending_rvh();
-  if (!rvh)
-    rvh = static_cast<TestRenderViewHost*>(render_manager_.current_host());
-
-  const NavigationEntry* entry = controller().pending_entry();
-  DCHECK(entry);
-  int page_id = entry->page_id();
-  if (page_id == -1) {
-    // It's a new navigation, assign a never-seen page id to it.
-    page_id =
-        static_cast<MockRenderProcessHost*>(rvh->process())->max_page_id() + 1;
-  }
-  rvh->SendNavigate(page_id, entry->url());
-}
-
-void TestTabContents::ProceedWithCrossSiteNavigation() {
-  if (!pending_rvh())
-    return;
-  render_manager_.ShouldClosePage(true, true);
+  static_cast<TestRenderViewHost*>(render_view_host())->SendNavigate(
+      static_cast<MockRenderProcessHost*>(render_view_host()->process())->
+      max_page_id() + 1, loaded_url);
 }
diff --git a/chrome/browser/tab_contents/test_tab_contents.h b/chrome/browser/tab_contents/test_tab_contents.h
index 21bc096..f0750d7 100644
--- a/chrome/browser/tab_contents/test_tab_contents.h
+++ b/chrome/browser/tab_contents/test_tab_contents.h
@@ -20,7 +20,7 @@ class TestTabContents : public TabContents {
   // The render view host factory will be passed on to the
   TestTabContents(Profile* profile, SiteInstance* instance);
 
-  TestRenderViewHost* pending_rvh() const;
+  TestRenderViewHost* pending_rvh();
 
   // State accessor.
   bool cross_navigation_pending() {
@@ -62,15 +62,6 @@ class TestTabContents : public TabContents {
   // emulates what happens on a new navigation.
   void NavigateAndCommit(const GURL& url);
 
-  // Simulates the appropriate RenderView (pending if any, current otherwise)
-  // sending a navigate notification for the NavigationController pending entry.
-  void CommitPendingNavigation();
-
-  // Simulates the current RVH notifying that it has unloaded so that the
-  // pending RVH navigation can proceed.
-  // Does nothing if no cross-navigation is pending.
-  void ProceedWithCrossSiteNavigation();
-
   // Set by individual tests.
   bool transition_cross_site;