Keep a copy of page id in RenderViewHost.

This is an instrumented version of the patch that will be reverted in a few days. This is meant to catch crashes in edge cases and log enough for us to repro them.

BUG=407376
TEST=this may crash for a few people

Review URL: https://codereview.chromium.org/517813002

Cr-Commit-Position: refs/heads/master@{#292572}

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -1003,7 +1003,7 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   const GURL kExistingURL("http://foo/eh");
   controller.LoadURL(kExistingURL, content::Referrer(),
                      content::PAGE_TRANSITION_TYPED, std::string());
-  main_test_rfh()->SendNavigate(0, kExistingURL);
+  main_test_rfh()->SendNavigate(1, kExistingURL);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1031,7 +1031,7 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   const GURL kRedirectURL("http://foo/see");
   main_test_rfh()->OnMessageReceived(
       FrameHostMsg_DidRedirectProvisionalLoad(0,  // routing_id
-                                              -1,  // pending page_id
+                                              1,  // pending page_id
                                               kNewURL,  // old url
                                               kRedirectURL));  // new url
 

content/browser/frame_host/render_frame_host_impl.cc

@@ -569,8 +569,9 @@ void RenderFrameHostImpl::OnDidRedirectProvisionalLoad(
     int32 page_id,
     const GURL& source_url,
     const GURL& target_url) {
+  CHECK_EQ(render_view_host_->page_id_, page_id);
   frame_tree_node_->navigator()->DidRedirectProvisionalLoad(
-      this, page_id, source_url, target_url);
+      this, render_view_host_->page_id_, source_url, target_url);
 }
 
 // Called when the renderer navigates.  For every frame loaded, we'll get this
@@ -592,6 +593,10 @@ void RenderFrameHostImpl::OnNavigate(const IPC::Message& msg) {
   TRACE_EVENT1("navigation", "RenderFrameHostImpl::OnNavigate",
                "url", validated_params.url.possibly_invalid_spec());
 
+  // Update the RVH's current page ID so that future IPCs from the renderer
+  // correspond to the new page.
+  render_view_host_->page_id_ = validated_params.page_id;
+
   // If we're waiting for a cross-site beforeunload ack from this renderer and
   // we receive a Navigate message from the main frame, then the renderer was
   // navigating already and sent it before hearing the FrameMsg_Stop message.
@@ -925,6 +930,7 @@ void RenderFrameHostImpl::OnUpdateTitle(
     int32 page_id,
     const base::string16& title,
     blink::WebTextDirection title_direction) {
+  CHECK_EQ(render_view_host_->page_id_, page_id);
   // This message is only sent for top-level frames. TODO(avi): when frame tree
   // mirroring works correctly, add a check here to enforce it.
   if (title.length() > kMaxTitleChars) {
@@ -932,7 +938,7 @@ void RenderFrameHostImpl::OnUpdateTitle(
     return;
   }
 
-  delegate_->UpdateTitle(this, page_id, title,
+  delegate_->UpdateTitle(this, render_view_host_->page_id_, title,
                          WebTextDirectionToChromeTextDirection(
                              title_direction));
 }

content/browser/renderer_host/render_view_host_impl.cc

@@ -184,6 +184,7 @@ RenderViewHostImpl::RenderViewHostImpl(
       instance_(static_cast<SiteInstanceImpl*>(instance)),
       waiting_for_drag_context_response_(false),
       enabled_bindings_(0),
+      page_id_(-1),
       main_frame_routing_id_(main_frame_routing_id),
       run_modal_reply_msg_(NULL),
       run_modal_opener_id_(MSG_ROUTING_NONE),
@@ -1073,6 +1074,7 @@ void RenderViewHostImpl::OnRenderProcessGone(int status, int exit_code) {
 }
 
 void RenderViewHostImpl::OnUpdateState(int32 page_id, const PageState& state) {
+  CHECK_EQ(page_id_, page_id);
   // Without this check, the renderer can trick the browser into using
   // filenames it can't access in a future session restore.
   if (!CanAccessFilesOfPageState(state)) {
@@ -1080,12 +1082,13 @@ void RenderViewHostImpl::OnUpdateState(int32 page_id, const PageState& state) {
     return;
   }
 
-  delegate_->UpdateState(this, page_id, state);
+  delegate_->UpdateState(this, page_id_, state);
 }
 
 void RenderViewHostImpl::OnUpdateTargetURL(int32 page_id, const GURL& url) {
+  CHECK_EQ(page_id_, page_id);
   if (IsRVHStateActive(rvh_state_))
-    delegate_->UpdateTargetURL(page_id, url);
+    delegate_->UpdateTargetURL(page_id_, url);
 
   // Send a notification back to the renderer that we are ready to
   // receive more target urls.

content/browser/renderer_host/render_view_host_impl.h

@@ -466,6 +466,11 @@ class CONTENT_EXPORT RenderViewHostImpl
   int enabled_bindings_;
 
 
+  // The most recent page ID we've heard from the renderer process.  This is
+  // used as context when other session history related IPCs arrive.
+  // TODO(creis): Allocate this in WebContents/NavigationController instead.
+  int32 page_id_;
+
   // The current state of this RVH.
   // TODO(nasko): Move to RenderFrameHost, as this is per-frame state.
   RenderViewHostImplState rvh_state_;

content/browser/renderer_host/render_view_host_unittest.cc

@@ -214,12 +214,12 @@ TEST_F(RenderViewHostTest, MessageWithBadHistoryItemFiles) {
   EXPECT_TRUE(PathService::Get(base::DIR_TEMP, &file_path));
   file_path = file_path.AppendASCII("foo");
   EXPECT_EQ(0, process()->bad_msg_count());
-  test_rvh()->TestOnUpdateStateWithFile(process()->GetID(), file_path);
+  test_rvh()->TestOnUpdateStateWithFile(-1, file_path);
   EXPECT_EQ(1, process()->bad_msg_count());
 
   ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
       process()->GetID(), file_path);
-  test_rvh()->TestOnUpdateStateWithFile(process()->GetID(), file_path);
+  test_rvh()->TestOnUpdateStateWithFile(-1, file_path);
   EXPECT_EQ(1, process()->bad_msg_count());
 }
 

content/renderer/render_frame_impl.cc

@@ -1997,6 +1997,7 @@ void RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad(
   std::vector<GURL> redirects;
   GetRedirectChain(data_source, &redirects);
   if (redirects.size() >= 2) {
+    CHECK(!render_view_->page_id_not_yet_reported_);
     Send(new FrameHostMsg_DidRedirectProvisionalLoad(
         routing_id_,
         render_view_->page_id_,
@@ -2146,6 +2147,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   if (is_new_navigation) {
     // We bump our Page ID to correspond with the new session history entry.
     render_view_->page_id_ = render_view_->next_page_id_++;
+    render_view_->page_id_not_yet_reported_ = true;
 
     // Don't update history_page_ids_ (etc) for kSwappedOutURL, since
     // we don't want to forget the entry that was there, and since we will
@@ -2182,6 +2184,7 @@ void RenderFrameImpl::didCommitProvisionalLoad(
         !navigation_state->request_committed()) {
       // This is a successful session history navigation!
       render_view_->page_id_ = navigation_state->pending_page_id();
+      render_view_->page_id_not_yet_reported_ = true;
 
       render_view_->history_list_offset_ =
           navigation_state->pending_history_list_offset();
@@ -2272,6 +2275,7 @@ void RenderFrameImpl::didReceiveTitle(blink::WebLocalFrame* frame,
         routing_id_, base::UTF16ToUTF8(title16));
 
     base::string16 shortened_title = title16.substr(0, kMaxTitleChars);
+    CHECK(!render_view_->page_id_not_yet_reported_);
     Send(new FrameHostMsg_UpdateTitle(routing_id_,
                                       render_view_->page_id_,
                                       shortened_title, direction));
@@ -3358,6 +3362,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(blink::WebFrame* frame) {
     if (!is_swapped_out())
       Send(new FrameHostMsg_DidCommitProvisionalLoad(routing_id_, params));
   }
+  render_view_->page_id_not_yet_reported_ = false;
 
   render_view_->last_page_id_sent_to_browser_ =
       std::max(render_view_->last_page_id_sent_to_browser_,

content/renderer/render_view_impl.cc

@@ -666,6 +666,7 @@ RenderViewImpl::RenderViewImpl(RenderViewImplParams* params)
       opener_suppressed_(false),
       suppress_dialogs_until_swap_out_(false),
       page_id_(-1),
+      page_id_not_yet_reported_(false),
       last_page_id_sent_to_browser_(-1),
       next_page_id_(params->next_page_id),
       history_list_offset_(-1),
@@ -1549,6 +1550,7 @@ void RenderViewImpl::SendUpdateState(HistoryEntry* entry) {
   if (entry->root().urlString() == WebString::fromUTF8(kSwappedOutURL))
     return;
 
+  CHECK(!page_id_not_yet_reported_);
   Send(new ViewHostMsg_UpdateState(
       routing_id_, page_id_, HistoryEntryToPageState(entry)));
 }
@@ -1867,6 +1869,7 @@ void RenderViewImpl::UpdateTargetURL(const GURL& url,
     // see |ParamTraits<GURL>|.
     if (latest_url.possibly_invalid_spec().size() > GetMaxURLChars())
       latest_url = GURL();
+    CHECK(!page_id_not_yet_reported_);
     Send(new ViewHostMsg_UpdateTargetURL(routing_id_, page_id_, latest_url));
     target_url_ = latest_url;
     target_url_status_ = TARGET_INFLIGHT;

content/renderer/render_view_impl.h

@@ -917,6 +917,11 @@ class CONTENT_EXPORT RenderViewImpl
   // See documentation in RenderView.
   int32 page_id_;
 
+  // TEMPORARY <http://crbug.com/407376>. If true, then |page_id_| has been
+  // changed but not yet reported to the browser process. No IPCs should be sent
+  // with a page_id_ while this is true.
+  bool page_id_not_yet_reported_;
+
   // Indicates the ID of the last page that we sent a FrameNavigate to the
   // browser for. This is used to determine if the most recent transition
   // generated a history entry (less than page_id_), or not (equal to or

content/test/test_render_view_host.cc

@@ -346,9 +346,9 @@ void TestRenderViewHost::TestOnStartDragging(
 }
 
 void TestRenderViewHost::TestOnUpdateStateWithFile(
-    int process_id,
+    int page_id,
     const base::FilePath& file_path) {
-  OnUpdateState(process_id,
+  OnUpdateState(page_id,
                 PageState::CreateForTesting(GURL("http://www.google.com"),
                                             false,
                                             "data",

content/test/test_render_view_host.h

@@ -249,7 +249,7 @@ class TestRenderViewHost
       FrameHostMsg_DidCommitProvisionalLoad_Params* params);
 
   void TestOnUpdateStateWithFile(
-      int process_id, const base::FilePath& file_path);
+      int page_id, const base::FilePath& file_path);
 
   void TestOnStartDragging(const DropData& drop_data);