Don't make a copy of the pending entry if it is for a different process.

BUG=178242
TEST=See bug for repro steps.


Review URL: https://chromiumcodereview.appspot.com/12313113

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

2 files changed, 58 insertions, 4 deletions

diff --git a/content/browser/web_contents/navigation_controller_impl.cc b/content/browser/web_contents/navigation_controller_impl.cc
index 72ca128..8596182 100644
--- a/content/browser/web_contents/navigation_controller_impl.cc
+++ b/content/browser/web_contents/navigation_controller_impl.cc
@@ -1088,10 +1088,12 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
    const ViewHostMsg_FrameNavigate_Params& params, bool replace_entry) {
   NavigationEntryImpl* new_entry;
   bool update_virtual_url;
-  if (pending_entry_) {
-    // TODO(brettw) this assumes that the pending entry is appropriate for the
-    // new page that was just loaded. I don't think this is necessarily the
-    // case! We should have some more tracking to know for sure.
+  // Only make a copy of the pending entry if it is appropriate for the new page
+  // that was just loaded.  We verify this at a coarse grain by checking that
+  // the SiteInstance hasn't been assigned to something else.
+  if (pending_entry_ &&
+      (!pending_entry_->site_instance() ||
+       pending_entry_->site_instance() == web_contents_->GetSiteInstance())) {
     new_entry = new NavigationEntryImpl(*pending_entry_);
 
     // Don't use the page type from the pending entry. Some interstitial page
diff --git a/content/browser/web_contents/navigation_controller_impl_unittest.cc b/content/browser/web_contents/navigation_controller_impl_unittest.cc
index 464bb2f8..8ddf7da5 100644
--- a/content/browser/web_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_controller_impl_unittest.cc
@@ -667,6 +667,58 @@ TEST_F(NavigationControllerTest, LoadURL_ExistingPending) {
   EXPECT_EQ(kNewURL, controller.GetActiveEntry()->GetURL());
 }
 
+// Tests navigating to a new URL when there is a pending back/forward
+// navigation to a cross-process, privileged URL. This will happen if the user
+// hits back, but before that commits, they navigate somewhere new.
+TEST_F(NavigationControllerTest, LoadURL_PrivilegedPending) {
+  NavigationControllerImpl& controller = controller_impl();
+  TestNotificationTracker notifications;
+  RegisterForAllNavNotifications(&notifications, &controller);
+
+  // First make some history, starting with a privileged URL.
+  const GURL kExistingURL1("http://privileged");
+  controller.LoadURL(
+      kExistingURL1, Referrer(), PAGE_TRANSITION_TYPED, std::string());
+  // Pretend it has bindings so we can tell if we incorrectly copy it.
+  test_rvh()->AllowBindings(2);
+  test_rvh()->SendNavigate(0, kExistingURL1);
+  EXPECT_TRUE(notifications.Check1AndReset(NOTIFICATION_NAV_ENTRY_COMMITTED));
+
+  // Navigate cross-process to a second URL.
+  const GURL kExistingURL2("http://foo/eh");
+  controller.LoadURL(
+      kExistingURL2, Referrer(), PAGE_TRANSITION_TYPED, std::string());
+  test_rvh()->SendShouldCloseACK(true);
+  TestRenderViewHost* foo_rvh = static_cast<TestRenderViewHost*>(
+      contents()->GetPendingRenderViewHost());
+  foo_rvh->SendNavigate(1, kExistingURL2);
+  EXPECT_TRUE(notifications.Check1AndReset(NOTIFICATION_NAV_ENTRY_COMMITTED));
+
+  // Now make a pending back/forward navigation to a privileged entry.
+  // The zeroth entry should be pending.
+  controller.GoBack();
+  foo_rvh->SendShouldCloseACK(true);
+  EXPECT_EQ(0U, notifications.size());
+  EXPECT_EQ(0, controller.GetPendingEntryIndex());
+  EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(2, NavigationEntryImpl::FromNavigationEntry(
+                controller.GetPendingEntry())->bindings());
+
+  // Before that commits, do a new navigation.
+  const GURL kNewURL("http://foo/bee");
+  LoadCommittedDetails details;
+  foo_rvh->SendNavigate(3, kNewURL);
+
+  // There should no longer be any pending entry, and the third navigation we
+  // just made should be committed.
+  EXPECT_TRUE(notifications.Check1AndReset(NOTIFICATION_NAV_ENTRY_COMMITTED));
+  EXPECT_EQ(-1, controller.GetPendingEntryIndex());
+  EXPECT_EQ(2, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(kNewURL, controller.GetActiveEntry()->GetURL());
+  EXPECT_EQ(0, NavigationEntryImpl::FromNavigationEntry(
+                controller.GetLastCommittedEntry())->bindings());
+}
+
 // Tests navigating to an existing URL when there is a pending new navigation.
 // This will happen if the user enters a URL, but before that commits, the
 // current page fires history.back().