Copy all the max page IDs to Instant/Prerender's TabContents

This fixes history problems that could arise with the previous approach.

BUG=109417
TEST=See bug for repro steps.

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

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

10 files changed, 87 insertions, 71 deletions

diff --git a/chrome/browser/instant/instant_loader.cc b/chrome/browser/instant/instant_loader.cc
index ec49ebd..629953f 100644
--- a/chrome/browser/instant/instant_loader.cc
+++ b/chrome/browser/instant/instant_loader.cc
@@ -1049,15 +1049,6 @@ void InstantLoader::SetupPreviewContents(TabContentsWrapper* tab_contents) {
   preview_contents_->core_tab_helper()->set_delegate(
       preview_tab_contents_delegate_.get());
 
-  // Propagate the max page id. That way if we end up merging the two
-  // NavigationControllers (which happens if we commit) none of the page ids
-  // will overlap.
-  int32 max_page_id = tab_contents->web_contents()->GetMaxPageID();
-  if (max_page_id != -1) {
-    preview_contents_->web_contents()->GetController().SetMaxRestoredPageID(
-        max_page_id + 1);
-  }
-
 #if defined(OS_MACOSX)
   // If |preview_contents_| does not currently have a RWHV, we will call
   // SetTakesFocusOnlyOnMouseDown() as a result of the
diff --git a/chrome/browser/prerender/prerender_contents.cc b/chrome/browser/prerender/prerender_contents.cc
index 81e7d35..ae196a1 100644
--- a/chrome/browser/prerender/prerender_contents.cc
+++ b/chrome/browser/prerender/prerender_contents.cc
@@ -213,7 +213,6 @@ PrerenderContents::PrerenderContents(
       prerendering_has_been_cancelled_(false),
       child_id_(-1),
       route_id_(-1),
-      starting_page_id_(-1),
       origin_(origin),
       experiment_id_(experiment_id) {
   DCHECK(prerender_manager != NULL);
@@ -247,30 +246,10 @@ void PrerenderContents::StartPrerendering(
     WebContents* source_wc =
         source_render_view_host->delegate()->GetAsWebContents();
     if (source_wc) {
-      // So that history merging will work, get the max page ID
-      // of the old page as a starting id.
-      starting_page_id_ = source_wc->GetMaxPageID();
-
       // Set the size of the new TC to that of the old TC.
       source_wc->GetView()->GetContainerBounds(&tab_bounds);
     }
   } else {
-    int max_page_id = -1;
-    // Get the largest page ID of all open tabs as a starting id.
-    for (BrowserList::BrowserVector::const_iterator browser_iter =
-            BrowserList::begin();
-         browser_iter != BrowserList::end();
-         ++browser_iter) {
-      const Browser* browser = *browser_iter;
-      int num_tabs = browser->tab_count();
-      for (int tab_index = 0; tab_index < num_tabs; ++tab_index) {
-        WebContents* web_contents = browser->GetWebContentsAt(tab_index);
-        if (web_contents != NULL)
-          max_page_id = std::max(max_page_id, web_contents->GetMaxPageID());
-      }
-    }
-    starting_page_id_ = max_page_id;
-
     // Try to get the active tab of the active browser and use that for tab
     // bounds. If the browser has never been active, we will fail to get a size
     // but we shouldn't be prerendering in that case anyway.
@@ -282,14 +261,6 @@ void PrerenderContents::StartPrerendering(
     }
   }
 
-  // Add a safety margin of kPrerenderPageIdOffset to the starting page id (for
-  // things such as redirects).
-  if (starting_page_id_ < 0)
-    starting_page_id_ = 0;
-  starting_page_id_ += kPrerenderPageIdOffset;
-  prerender_contents_->web_contents()->GetController().SetMaxRestoredPageID(
-      starting_page_id_);
-
   tab_contents_delegate_.reset(new TabContentsDelegateImpl(this));
   new_contents->SetDelegate(tab_contents_delegate_.get());
 
diff --git a/chrome/browser/prerender/prerender_contents.h b/chrome/browser/prerender/prerender_contents.h
index faa5ad7..2c8cdd8 100644
--- a/chrome/browser/prerender/prerender_contents.h
+++ b/chrome/browser/prerender/prerender_contents.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -170,8 +170,6 @@ class PrerenderContents : public content::NotificationObserver,
   // new tab.
   void CommitHistory(TabContentsWrapper* tab);
 
-  int32 starting_page_id() { return starting_page_id_; }
-
   base::Value* GetAsValue() const;
 
   // Returns whether a pending cross-site navigation is happening.
@@ -292,9 +290,6 @@ class PrerenderContents : public content::NotificationObserver,
   int child_id_;
   int route_id_;
 
-  // Page ID at which prerendering started.
-  int32 starting_page_id_;
-
   // Origin for this prerender.
   Origin origin_;
 
@@ -304,9 +299,6 @@ class PrerenderContents : public content::NotificationObserver,
   // List of all pages the prerendered page has tried to prerender.
   PendingPrerenderList pending_prerender_list_;
 
-  // Offset by which to offset prerendered pages
-  static const int32 kPrerenderPageIdOffset = 10;
-
   DISALLOW_COPY_AND_ASSIGN(PrerenderContents);
 };
 
diff --git a/chrome/browser/prerender/prerender_final_status.h b/chrome/browser/prerender/prerender_final_status.h
index 3b698d3..6e736fd 100644
--- a/chrome/browser/prerender/prerender_final_status.h
+++ b/chrome/browser/prerender/prerender_final_status.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -39,7 +39,7 @@ enum FinalStatus {
   FINAL_STATUS_WINDOW_PRINT = 24,
   FINAL_STATUS_RECENTLY_VISITED = 25,
   FINAL_STATUS_WINDOW_OPENER = 26,
-  FINAL_STATUS_PAGE_ID_CONFLICT = 27,
+  // Obsolete: FINAL_STATUS_PAGE_ID_CONFLICT = 27,
   FINAL_STATUS_SAFE_BROWSING = 28,
   FINAL_STATUS_FRAGMENT_MISMATCH = 29,
   FINAL_STATUS_SSL_CLIENT_CERTIFICATE_REQUESTED = 30,
diff --git a/chrome/browser/prerender/prerender_manager.cc b/chrome/browser/prerender/prerender_manager.cc
index 10cdaaf..ecf5eb0 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -572,12 +572,6 @@ bool PrerenderManager::MaybeUsePrerenderedPage(WebContents* web_contents,
     return false;
   }
 
-  if (prerender_contents->starting_page_id() <=
-      web_contents->GetMaxPageID()) {
-    prerender_contents.release()->Destroy(FINAL_STATUS_PAGE_ID_CONFLICT);
-    return false;
-  }
-
   // Don't use prerendered pages if debugger is attached to the tab.
   // See http://crbug.com/98541
   if (content::DevToolsAgentHostRegistry::IsDebuggerAttached(web_contents)) {
diff --git a/content/browser/tab_contents/navigation_controller_impl.cc b/content/browser/tab_contents/navigation_controller_impl.cc
index e3b2797..3c5ddee 100644
--- a/content/browser/tab_contents/navigation_controller_impl.cc
+++ b/content/browser/tab_contents/navigation_controller_impl.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -990,6 +990,11 @@ void NavigationControllerImpl::CopyStateFrom(
   session_storage_namespace_ = source.session_storage_namespace_->Clone();
 
   FinishRestore(source.last_committed_entry_index_, false);
+
+  // Copy the max page id map from the old tab to the new tab.  This ensures
+  // that new and existing navigations in the tab's current SiteInstances
+  // are identified properly.
+  tab_contents_->CopyMaxPageIDsFrom(source.tab_contents());
 }
 
 void NavigationControllerImpl::CopyStateFromAndPrune(
@@ -998,17 +1003,20 @@ void NavigationControllerImpl::CopyStateFromAndPrune(
       static_cast<NavigationControllerImpl*>(temp);
   // The SiteInstance and page_id of the last committed entry needs to be
   // remembered at this point, in case there is only one committed entry
-  // and it is pruned.
+  // and it is pruned.  We use a scoped_refptr to ensure the SiteInstance
+  // can't be freed during this time period.
   NavigationEntryImpl* last_committed =
       NavigationEntryImpl::FromNavigationEntry(GetLastCommittedEntry());
-  SiteInstance* site_instance =
-      last_committed ? last_committed->site_instance() : NULL;
+  scoped_refptr<SiteInstance> site_instance(
+      last_committed ? last_committed->site_instance() : NULL);
   int32 minimum_page_id = last_committed ? last_committed->GetPageID() : -1;
+  int32 max_page_id = last_committed ?
+      tab_contents_->GetMaxPageIDForSiteInstance(site_instance.get()) : -1;
 
   // This code is intended for use when the last entry is the active entry.
   DCHECK(
       (transient_entry_index_ != -1 &&
-      transient_entry_index_ == GetEntryCount() - 1) ||
+       transient_entry_index_ == GetEntryCount() - 1) ||
       (pending_entry_ && (pending_entry_index_ == -1 ||
                           pending_entry_index_ == GetEntryCount() - 1)) ||
       (!pending_entry_ && last_committed_entry_index_ == GetEntryCount() - 1));
@@ -1038,9 +1046,21 @@ void NavigationControllerImpl::CopyStateFromAndPrune(
       last_committed_entry_index_--;
   }
 
-  tab_contents_->SetHistoryLengthAndPrune(site_instance,
+  tab_contents_->SetHistoryLengthAndPrune(site_instance.get(),
                                           max_source_index,
                                           minimum_page_id);
+
+  // Copy the max page id map from the old tab to the new tab.  This ensures
+  // that new and existing navigations in the tab's current SiteInstances
+  // are identified properly.
+  tab_contents_->CopyMaxPageIDsFrom(source->tab_contents());
+
+  // If there is a last committed entry, be sure to include it in the new
+  // max page ID map.
+  if (max_page_id > -1) {
+    tab_contents_->UpdateMaxPageIDForSiteInstance(site_instance.get(),
+                                                  max_page_id);
+  }
 }
 
 void NavigationControllerImpl::PruneAllButActive() {
diff --git a/content/browser/tab_contents/navigation_controller_impl_unittest.cc b/content/browser/tab_contents/navigation_controller_impl_unittest.cc
index 2b8e813..340a19d 100644
--- a/content/browser/tab_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/tab_contents/navigation_controller_impl_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -63,6 +63,10 @@ void RegisterForAllNavNotifications(TestNotificationTracker* tracker,
                          controller));
 }
 
+SiteInstance* GetSiteInstanceFromEntry(NavigationEntry* entry) {
+  return NavigationEntryImpl::FromNavigationEntry(entry)->site_instance();
+}
+
 class TestWebContentsDelegate : public content::WebContentsDelegate {
  public:
   explicit TestWebContentsDelegate() :
@@ -2051,20 +2055,29 @@ TEST_F(NavigationControllerTest, SubframeWhilePending) {
 // Tests CopyStateFromAndPrune with 2 urls in source, 1 in dest.
 TEST_F(NavigationControllerTest, CopyStateFromAndPrune) {
   NavigationControllerImpl& controller = controller_impl();
-  const GURL url1("http://foo1");
-  const GURL url2("http://foo2");
-  const GURL url3("http://foo3");
+  const GURL url1("http://foo/1");
+  const GURL url2("http://foo/2");
+  const GURL url3("http://foo/3");
 
   NavigateAndCommit(url1);
   NavigateAndCommit(url2);
 
+  // First two entries should have the same SiteInstance.
+  SiteInstance* instance1 =
+      GetSiteInstanceFromEntry(controller.GetEntryAtIndex(0));
+  SiteInstance* instance2 =
+      GetSiteInstanceFromEntry(controller.GetEntryAtIndex(1));
+  EXPECT_EQ(instance1, instance2);
+  EXPECT_EQ(0, controller.GetEntryAtIndex(0)->GetPageID());
+  EXPECT_EQ(1, controller.GetEntryAtIndex(1)->GetPageID());
+  EXPECT_EQ(1, contents()->GetMaxPageIDForSiteInstance(instance1));
+
   scoped_ptr<TestTabContents> other_contents(CreateTestTabContents());
   NavigationControllerImpl& other_controller =
       other_contents->GetControllerImpl();
   other_contents->NavigateAndCommit(url3);
   other_contents->ExpectSetHistoryLengthAndPrune(
-      NavigationEntryImpl::FromNavigationEntry(
-          other_controller.GetEntryAtIndex(0))->site_instance(), 2,
+      GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0)), 2,
       other_controller.GetEntryAtIndex(0)->GetPageID());
   other_controller.CopyStateFromAndPrune(&controller);
 
@@ -2077,6 +2090,19 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune) {
   EXPECT_EQ(url1, other_controller.GetEntryAtIndex(0)->GetURL());
   EXPECT_EQ(url2, other_controller.GetEntryAtIndex(1)->GetURL());
   EXPECT_EQ(url3, other_controller.GetEntryAtIndex(2)->GetURL());
+  EXPECT_EQ(0, other_controller.GetEntryAtIndex(0)->GetPageID());
+  EXPECT_EQ(1, other_controller.GetEntryAtIndex(1)->GetPageID());
+  EXPECT_EQ(0, other_controller.GetEntryAtIndex(2)->GetPageID());
+
+  // A new SiteInstance should be used for the new tab.
+  SiteInstance* instance3 =
+      GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(2));
+  EXPECT_NE(instance3, instance1);
+
+  // The max page ID map should be copied over and updated with the max page ID
+  // from the current tab.
+  EXPECT_EQ(1, other_contents->GetMaxPageIDForSiteInstance(instance1));
+  EXPECT_EQ(0, other_contents->GetMaxPageIDForSiteInstance(instance3));
 }
 
 // Test CopyStateFromAndPrune with 2 urls, the first selected and nothing in
@@ -2104,6 +2130,13 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune2) {
   ASSERT_EQ(0, other_controller.GetCurrentEntryIndex());
 
   EXPECT_EQ(url1, other_controller.GetEntryAtIndex(0)->GetURL());
+  EXPECT_EQ(0, other_controller.GetEntryAtIndex(0)->GetPageID());
+
+  // The max page ID map should be copied over and updated with the max page ID
+  // from the current tab.
+  SiteInstance* instance1 =
+      GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0));
+  EXPECT_EQ(0, other_contents->GetMaxPageIDForSiteInstance(instance1));
 }
 
 // Test CopyStateFromAndPrune with 2 urls, the first selected and nothing in
@@ -2139,6 +2172,12 @@ TEST_F(NavigationControllerTest, CopyStateFromAndPrune3) {
   ASSERT_TRUE(other_controller.GetPendingEntry());
 
   EXPECT_EQ(url3, other_controller.GetPendingEntry()->GetURL());
+
+  // The max page ID map should be copied over and updated with the max page ID
+  // from the current tab.
+  SiteInstance* instance1 =
+      GetSiteInstanceFromEntry(other_controller.GetEntryAtIndex(0));
+  EXPECT_EQ(0, other_contents->GetMaxPageIDForSiteInstance(instance1));
 }
 
 // Tests that navigations initiated from the page (with the history object)
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index a3fb7d1..ce3abd1 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -494,6 +494,10 @@ void TabContents::UpdateMaxPageIDForSiteInstance(SiteInstance* site_instance,
     max_page_ids_[site_instance->id()] = page_id;
 }
 
+void TabContents::CopyMaxPageIDsFrom(TabContents* tab_contents) {
+  max_page_ids_ = tab_contents->max_page_ids_;
+}
+
 SiteInstance* TabContents::GetSiteInstance() const {
   return render_manager_.current_host()->site_instance();
 }
diff --git a/content/browser/tab_contents/tab_contents.h b/content/browser/tab_contents/tab_contents.h
index 7dcd5be..12bad9d 100644
--- a/content/browser/tab_contents/tab_contents.h
+++ b/content/browser/tab_contents/tab_contents.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -73,6 +73,11 @@ class CONTENT_EXPORT TabContents
   void UpdateMaxPageIDForSiteInstance(SiteInstance* site_instance,
                                       int32 page_id);
 
+  // Copy the current map of SiteInstance ID to max page ID from another tab.
+  // This is necessary when this tab adopts the NavigationEntries from
+  // |tab_contents|.
+  void CopyMaxPageIDsFrom(TabContents* tab_contents);
+
   // Called by the NavigationController to cause the TabContents to navigate to
   // the current pending entry. The NavigationController should be called back
   // with RendererDidNavigate on success or DiscardPendingEntry on failure.
diff --git a/content/public/browser/navigation_controller.h b/content/public/browser/navigation_controller.h
index 1bd0cf0..0e18514 100644
--- a/content/public/browser/navigation_controller.h
+++ b/content/public/browser/navigation_controller.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -247,7 +247,7 @@ class NavigationController {
   // active entry. For example:
   // source: A B *C* D
   // this:   E F *G*   (last must be active or pending)
-  // result: A B *G*
+  // result: A B C *G*
   // This ignores the transient index of the source and honors that of 'this'.
   virtual void CopyStateFromAndPrune(NavigationController* source) = 0;