Make page IDs be specific to a RenderView rather than global to its process.

This avoids races that cause the browser to kill the renderer.

BUG=106616
TEST=Restore Chrome with an extension's options page showing.

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

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

6 files changed, 90 insertions, 67 deletions

diff --git a/content/browser/tab_contents/interstitial_page.cc b/content/browser/tab_contents/interstitial_page.cc
index 778ef77..ed7f3c6 100644
--- a/content/browser/tab_contents/interstitial_page.cc
+++ b/content/browser/tab_contents/interstitial_page.cc
@@ -412,7 +412,9 @@ TabContentsView* InterstitialPage::CreateTabContentsView() {
   render_view_host_->SetView(view);
   render_view_host_->AllowBindings(content::BINDINGS_POLICY_DOM_AUTOMATION);
 
-  render_view_host_->CreateRenderView(string16());
+  int32 max_page_id =
+      tab()->GetMaxPageIDForSiteInstance(render_view_host_->site_instance());
+  render_view_host_->CreateRenderView(string16(), max_page_id);
   view->SetSize(tab_contents_view->GetContainerSize());
   // Don't show the interstitial until we have navigated to it.
   view->Hide();
diff --git a/content/browser/tab_contents/render_view_host_manager_unittest.cc b/content/browser/tab_contents/render_view_host_manager_unittest.cc
index bcc359e..b327253 100644
--- a/content/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -127,11 +127,10 @@ class RenderViewHostManagerTest : public RenderViewHostTestHarness {
     if (old_rvh != active_rvh())
       old_rvh->SendShouldCloseACK(true);
 
-    // Commit the navigation.
-    active_rvh()->SendNavigate(
-        static_cast<MockRenderProcessHost*>(active_rvh()->process())->
-            max_page_id() + 1,
-        url);
+    // Commit the navigation with a new page ID.
+    int32 max_page_id =
+        contents()->GetMaxPageIDForSiteInstance(active_rvh()->site_instance());
+    active_rvh()->SendNavigate(max_page_id + 1, url);
 
     // Simulate the SwapOut_ACK that fires if you commit a cross-site navigation
     // without making any network requests.
@@ -237,8 +236,8 @@ TEST_F(RenderViewHostManagerTest, AlwaysSendEnableViewSourceMode) {
       ViewHostMsg_ShouldClose_ACK(rvh()->routing_id(), true));
   ASSERT_TRUE(pending_rvh());  // New pending RenderViewHost will be created.
   RenderViewHost* last_rvh = pending_rvh();
-  int new_id = static_cast<MockRenderProcessHost*>(pending_rvh()->process())->
-               max_page_id() + 1;
+  int32 new_id = contents()->GetMaxPageIDForSiteInstance(
+      active_rvh()->site_instance()) + 1;
   pending_rvh()->SendNavigate(new_id, kUrl);
   EXPECT_EQ(controller().last_committed_entry_index(), 1);
   ASSERT_TRUE(controller().GetLastCommittedEntry());
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index a3ffb43..390acb4 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -196,7 +196,6 @@ TabContents::TabContents(content::BrowserContext* browser_context,
       crashed_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       crashed_error_code_(0),
       waiting_for_response_(false),
-      max_page_id_(-1),
       load_state_(net::LOAD_STATE_IDLE, string16()),
       upload_size_(0),
       upload_position_(0),
@@ -391,19 +390,24 @@ const string16& TabContents::GetTitle() const {
 }
 
 int32 TabContents::GetMaxPageID() {
-  if (GetSiteInstance())
-    return GetSiteInstance()->max_page_id();
-  else
-    return max_page_id_;
+  return GetMaxPageIDForSiteInstance(GetSiteInstance());
+}
+
+int32 TabContents::GetMaxPageIDForSiteInstance(SiteInstance* site_instance) {
+  if (max_page_ids_.find(site_instance->id()) == max_page_ids_.end())
+    max_page_ids_[site_instance->id()] = -1;
+
+  return max_page_ids_[site_instance->id()];
 }
 
 void TabContents::UpdateMaxPageID(int32 page_id) {
-  // Ensure both the SiteInstance and RenderProcessHost update their max page
-  // IDs in sync. Only TabContents will also have site instances, except during
-  // testing.
-  if (GetSiteInstance())
-    GetSiteInstance()->UpdateMaxPageID(page_id);
-  GetRenderProcessHost()->UpdateMaxPageID(page_id);
+  UpdateMaxPageIDForSiteInstance(GetSiteInstance(), page_id);
+}
+
+void TabContents::UpdateMaxPageIDForSiteInstance(SiteInstance* site_instance,
+                                                 int32 page_id) {
+  if (GetMaxPageIDForSiteInstance(site_instance) < page_id)
+    max_page_ids_[site_instance->id()] = page_id;
 }
 
 SiteInstance* TabContents::GetSiteInstance() const {
@@ -1328,30 +1332,16 @@ void TabContents::DidNavigateAnyFramePostCommit(
                     DidNavigateAnyFrame(details, params));
 }
 
-void TabContents::UpdateMaxPageIDIfNecessary(SiteInstance* site_instance,
-                                             RenderViewHost* rvh) {
-  // If we are creating a RVH for a restored controller, then we might
-  // have more page IDs than the SiteInstance's current max page ID.  We must
-  // make sure that the max page ID is larger than any restored page ID.
-  // Note that it is ok for conflicting page IDs to exist in another tab
-  // (i.e., NavigationController), but if any page ID is larger than the max,
-  // the back/forward list will get confused.
+void TabContents::UpdateMaxPageIDIfNecessary(RenderViewHost* rvh) {
+  // If we are creating a RVH for a restored controller, then we need to make
+  // sure the RenderView starts with a next_page_id_ larger than the number
+  // of restored entries.  This must be called before the RenderView starts
+  // navigating (to avoid a race between the browser updating max_page_id and
+  // the renderer updating next_page_id_).  Because of this, we only call this
+  // from CreateRenderView and allow that to notify the RenderView for us.
   int max_restored_page_id = controller_.max_restored_page_id();
-  if (max_restored_page_id > 0) {
-    int curr_max_page_id = site_instance->max_page_id();
-    if (max_restored_page_id > curr_max_page_id) {
-      // Need to update the site instance immediately.
-      site_instance->UpdateMaxPageID(max_restored_page_id);
-
-      // Also tell the renderer to update its internal representation.  We
-      // need to reserve enough IDs to make all restored page IDs less than
-      // the max.
-      if (curr_max_page_id < 0)
-        curr_max_page_id = 0;
-      rvh->Send(new ViewMsg_ReservePageIDRange(
-          rvh->routing_id(), max_restored_page_id - curr_max_page_id));
-    }
-  }
+  if (max_restored_page_id > GetMaxPageIDForSiteInstance(rvh->site_instance()))
+    UpdateMaxPageIDForSiteInstance(rvh->site_instance(), max_restored_page_id);
 }
 
 bool TabContents::UpdateTitleForEntry(NavigationEntry* entry,
@@ -2018,7 +2008,12 @@ bool TabContents::CreateRenderViewForRenderManager(
   if (rwh_view)
     rwh_view->SetSize(view_->GetContainerSize());
 
-  if (!render_view_host->CreateRenderView(string16()))
+  // Make sure we use the correct starting page_id in the new RenderView.
+  UpdateMaxPageIDIfNecessary(render_view_host);
+  int32 max_page_id =
+      GetMaxPageIDForSiteInstance(render_view_host->site_instance());
+
+  if (!render_view_host->CreateRenderView(string16(), max_page_id))
     return false;
 
 #if defined(OS_LINUX) || defined(OS_OPENBSD)
@@ -2030,8 +2025,6 @@ bool TabContents::CreateRenderViewForRenderManager(
   }
 #endif
 
-  UpdateMaxPageIDIfNecessary(render_view_host->site_instance(),
-                             render_view_host);
   return true;
 }
 
diff --git a/content/browser/tab_contents/tab_contents.h b/content/browser/tab_contents/tab_contents.h
index 3f70a58..21fa71f 100644
--- a/content/browser/tab_contents/tab_contents.h
+++ b/content/browser/tab_contents/tab_contents.h
@@ -147,19 +147,27 @@ class CONTENT_EXPORT TabContents : public PageNavigator,
   virtual const GURL& GetURL() const OVERRIDE;
   virtual const string16& GetTitle() const;
 
-  // The max PageID of any page that this TabContents has loaded.  PageIDs
-  // increase with each new page that is loaded by a tab.  If this is a
-  // TabContents, then the max PageID is kept separately on each SiteInstance.
-  // Returns -1 if no PageIDs have yet been seen.
+  // The max page ID for any page that the current SiteInstance has loaded in
+  // this TabContents.  Page IDs are specific to a given SiteInstance and
+  // TabContents, corresponding to a specific RenderView in the renderer.
+  // Page IDs increase with each new page that is loaded by a tab.
   int32 GetMaxPageID();
 
-  // Updates the max PageID to be at least the given PageID.
+  // The max page ID for any page that the given SiteInstance has loaded in
+  // this TabContents.
+  int32 GetMaxPageIDForSiteInstance(SiteInstance* site_instance);
+
+  // Updates the max page ID for the current SiteInstance in this TabContents
+  // to be at least |page_id|.
   void UpdateMaxPageID(int32 page_id);
 
-  // Returns the site instance associated with the current page. By default,
-  // there is no site instance. TabContents overrides this to provide proper
-  // access to its site instance.
-  virtual SiteInstance* GetSiteInstance() const;
+  // Updates the max page ID for the given SiteInstance in this TabContents
+  // to be at least |page_id|.
+  void UpdateMaxPageIDForSiteInstance(SiteInstance* site_instance,
+                                      int32 page_id);
+
+  // Returns the SiteInstance associated with the current page.
+  SiteInstance* GetSiteInstance() const;
 
   // Returns the SiteInstance for the pending navigation, if any.  Otherwise
   // returns the current SiteInstance.
@@ -679,11 +687,11 @@ class CONTENT_EXPORT TabContents : public PageNavigator,
       const content::LoadCommittedDetails& details,
       const ViewHostMsg_FrameNavigate_Params& params);
 
-  // If our controller was restored and the page id is > than the site
-  // instance's page id, the site instances page id is updated as well as the
-  // renderers max page id.
-  void UpdateMaxPageIDIfNecessary(SiteInstance* site_instance,
-                                  RenderViewHost* rvh);
+  // If our controller was restored, update the max page ID associated with the
+  // given RenderViewHost to be larger than the number of restored entries.
+  // This is called in CreateRenderView before any navigations in the RenderView
+  // have begun, to prevent any races in updating RenderView::next_page_id.
+  void UpdateMaxPageIDIfNecessary(RenderViewHost* rvh);
 
   // Saves the given title to the navigation entry and does associated work. It
   // will update history and the view for the new title, and also synthesize
@@ -789,11 +797,10 @@ class CONTENT_EXPORT TabContents : public PageNavigator,
   // See waiting_for_response() above.
   bool waiting_for_response_;
 
-  // Indicates the largest PageID we've seen.  This field is ignored if we are
-  // a TabContents, in which case the max page ID is stored separately with
-  // each SiteInstance.
-  // TODO(brettw) this seems like it can be removed according to the comment.
-  int32 max_page_id_;
+  // Map of SiteInstance ID to max page ID for this tab. A page ID is specific
+  // to a given tab and SiteInstance, and must be valid for the lifetime of the
+  // TabContents.
+  std::map<int32, int32> max_page_ids_;
 
   // System time at which the current load was started.
   base::TimeTicks current_load_start_;
diff --git a/content/browser/tab_contents/tab_contents_unittest.cc b/content/browser/tab_contents/tab_contents_unittest.cc
index d175cf3e..f743e7d 100644
--- a/content/browser/tab_contents/tab_contents_unittest.cc
+++ b/content/browser/tab_contents/tab_contents_unittest.cc
@@ -267,6 +267,29 @@ TEST_F(TabContentsTest, NTPViewSource) {
   EXPECT_EQ(ASCIIToUTF16(kUrl), contents()->GetTitle());
 }
 
+// Test to ensure UpdateMaxPageID is working properly.
+TEST_F(TabContentsTest, UpdateMaxPageID) {
+  SiteInstance* instance1 = contents()->GetSiteInstance();
+  scoped_refptr<SiteInstance> instance2(SiteInstance::CreateSiteInstance(NULL));
+
+  // Starts at -1.
+  EXPECT_EQ(-1, contents()->GetMaxPageID());
+  EXPECT_EQ(-1, contents()->GetMaxPageIDForSiteInstance(instance1));
+  EXPECT_EQ(-1, contents()->GetMaxPageIDForSiteInstance(instance2));
+
+  // Make sure max_page_id_ is monotonically increasing per SiteInstance.
+  contents()->UpdateMaxPageID(3);
+  contents()->UpdateMaxPageID(1);
+  EXPECT_EQ(3, contents()->GetMaxPageID());
+  EXPECT_EQ(3, contents()->GetMaxPageIDForSiteInstance(instance1));
+  EXPECT_EQ(-1, contents()->GetMaxPageIDForSiteInstance(instance2));
+
+  contents()->UpdateMaxPageIDForSiteInstance(instance2, 7);
+  EXPECT_EQ(3, contents()->GetMaxPageID());
+  EXPECT_EQ(3, contents()->GetMaxPageIDForSiteInstance(instance1));
+  EXPECT_EQ(7, contents()->GetMaxPageIDForSiteInstance(instance2));
+}
+
 // Test simple same-SiteInstance navigation.
 TEST_F(TabContentsTest, SimpleNavigation) {
   TestRenderViewHost* orig_rvh = rvh();
diff --git a/content/browser/tab_contents/test_tab_contents.cc b/content/browser/tab_contents/test_tab_contents.cc
index 99a2f57..f0b6cc4 100644
--- a/content/browser/tab_contents/test_tab_contents.cc
+++ b/content/browser/tab_contents/test_tab_contents.cc
@@ -75,7 +75,7 @@ void TestTabContents::TestDidNavigateWithReferrer(
 bool TestTabContents::CreateRenderViewForRenderManager(
     RenderViewHost* render_view_host) {
   // This will go to a TestRenderViewHost.
-  render_view_host->CreateRenderView(string16());
+  render_view_host->CreateRenderView(string16(), -1);
   return true;
 }
 
@@ -114,8 +114,7 @@ void TestTabContents::CommitPendingNavigation() {
   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;
+    page_id = GetMaxPageIDForSiteInstance(rvh->site_instance()) + 1;
   }
   rvh->SendNavigate(page_id, entry->url());