Revert of Keep a copy of page id in RenderViewHost. (patchset #1 of...

Revert of Keep a copy of page id in RenderViewHost. (patchset #1 of https://codereview.chromium.org/493853002/)

Reason for revert:
This hit canary and we're getting instrumented crashes. That's what we were looking for so let's back it out.


Original issue's description:
> 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=99379, 369661
> TEST=this is gonna crash for a few people
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291077

TBR=rsesek@chromium.org,creis@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=99379, 369661

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

Cr-Commit-Position: refs/heads/master@{#291401}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@291401 0039d316-1c4b-4281-b951-d872f2087c98

chrome/common/crash_keys.cc

@@ -163,13 +163,6 @@ size_t RegisterChromeCrashKeys() {
     { "channel_error_bt", kMediumSize },
     { "remove_route_bt", kMediumSize },
     { "rwhvm_window", kMediumSize },
-    // The following keys are for diagnosing crashes in http://crbug.com/369661.
-    // They will not be permanent.
-    { "url1", kLargeSize },
-    { "url2", kLargeSize },
-    { "id1", kSmallSize },
-    { "id2", kSmallSize },
-    // End http://crbug.com/369661
     // media/:
     { "VideoCaptureDeviceQTKit", kSmallSize },
 #endif

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -1002,7 +1002,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(1, kExistingURL);
+  main_test_rfh()->SendNavigate(0, kExistingURL);
   EXPECT_EQ(1U, navigation_entry_committed_counter_);
   navigation_entry_committed_counter_ = 0;
 
@@ -1030,7 +1030,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

@@ -7,11 +7,9 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
-#include "base/debug/crash_logging.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/user_metrics_action.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "content/browser/accessibility/accessibility_mode_helper.h"
 #include "content/browser/accessibility/browser_accessibility_manager.h"
@@ -559,19 +557,8 @@ void RenderFrameHostImpl::OnDidRedirectProvisionalLoad(
     int32 page_id,
     const GURL& source_url,
     const GURL& target_url) {
-  if (render_view_host_->page_id_ != page_id) {
-    base::debug::SetCrashKeyValue("url1",
-                                  source_url.possibly_invalid_spec());
-    base::debug::SetCrashKeyValue("url2",
-                                  target_url.possibly_invalid_spec());
-    base::debug::SetCrashKeyValue(
-        "id1", base::IntToString(render_view_host_->page_id_));
-    base::debug::SetCrashKeyValue("id2",
-                                  base::IntToString(page_id));
-    CHECK(false);
-  }
   frame_tree_node_->navigator()->DidRedirectProvisionalLoad(
-      this, render_view_host_->page_id_, source_url, target_url);
+      this, page_id, source_url, target_url);
 }
 
 // Called when the renderer navigates.  For every frame loaded, we'll get this
@@ -591,10 +578,6 @@ void RenderFrameHostImpl::OnNavigate(const IPC::Message& msg) {
       Read(&msg, &iter, &validated_params))
     return;
 
-  // 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 ViewMsg_Stop message.
@@ -919,15 +902,6 @@ void RenderFrameHostImpl::OnUpdateTitle(
     int32 page_id,
     const base::string16& title,
     blink::WebTextDirection title_direction) {
-  if (render_view_host_->page_id_ != page_id) {
-    base::debug::SetCrashKeyValue(
-        "url1", GetLastCommittedURL().possibly_invalid_spec());
-    base::debug::SetCrashKeyValue(
-        "id1", base::IntToString(render_view_host_->page_id_));
-    base::debug::SetCrashKeyValue("id2",
-                                  base::IntToString(page_id));
-    CHECK(false);
-  }
   // 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) {
@@ -935,7 +909,7 @@ void RenderFrameHostImpl::OnUpdateTitle(
     return;
   }
 
-  delegate_->UpdateTitle(this, render_view_host_->page_id_, title,
+  delegate_->UpdateTitle(this, page_id, title,
                          WebTextDirectionToChromeTextDirection(
                              title_direction));
 }

content/browser/renderer_host/render_view_host_impl.cc

@@ -11,14 +11,12 @@
 
 #include "base/callback.h"
 #include "base/command_line.h"
-#include "base/debug/crash_logging.h"
 #include "base/debug/trace_event.h"
 #include "base/i18n/rtl.h"
 #include "base/json/json_reader.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
@@ -188,7 +186,6 @@ 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),
@@ -1053,13 +1050,6 @@ void RenderViewHostImpl::OnRenderProcessGone(int status, int exit_code) {
 }
 
 void RenderViewHostImpl::OnUpdateState(int32 page_id, const PageState& state) {
-  if (page_id_ != page_id) {
-    base::debug::SetCrashKeyValue(
-        "url1", GetMainFrame()->GetLastCommittedURL().possibly_invalid_spec());
-    base::debug::SetCrashKeyValue("id1", base::IntToString(page_id_));
-    base::debug::SetCrashKeyValue("id2", base::IntToString(page_id));
-    CHECK(false);
-  }
   // Without this check, the renderer can trick the browser into using
   // filenames it can't access in a future session restore.
   if (!CanAccessFilesOfPageState(state)) {
@@ -1067,18 +1057,12 @@ 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) {
-  if (page_id_ != page_id) {
-    base::debug::SetCrashKeyValue("url1", url.possibly_invalid_spec());
-    base::debug::SetCrashKeyValue("id1", base::IntToString(page_id_));
-    base::debug::SetCrashKeyValue("id2", base::IntToString(page_id));
-    CHECK(false);
-  }
   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

@@ -484,11 +484,6 @@ 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(-1, file_path);
+  test_rvh()->TestOnUpdateStateWithFile(process()->GetID(), file_path);
   EXPECT_EQ(1, process()->bad_msg_count());
 
   ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
       process()->GetID(), file_path);
-  test_rvh()->TestOnUpdateStateWithFile(-1, file_path);
+  test_rvh()->TestOnUpdateStateWithFile(process()->GetID(), file_path);
   EXPECT_EQ(1, process()->bad_msg_count());
 }
 

content/test/test_render_view_host.cc

@@ -346,9 +346,9 @@ void TestRenderViewHost::TestOnStartDragging(
 }
 
 void TestRenderViewHost::TestOnUpdateStateWithFile(
-    int page_id,
+    int process_id,
     const base::FilePath& file_path) {
-  OnUpdateState(page_id,
+  OnUpdateState(process_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 page_id, const base::FilePath& file_path);
+      int process_id, const base::FilePath& file_path);
 
   void TestOnStartDragging(const DropData& drop_data);