Keep a copy of page id in RenderViewHost.

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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.
 
 #include "content/browser/frame_host/render_frame_host_impl.h"
 
 #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"
 #include "content/browser/accessibility/browser_accessibility_state_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/frame_host/cross_process_frame_connector.h"
 #include "content/browser/frame_host/cross_site_transferring_request.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigator.h"
@@ skipping 527 matching lines @@
   GetProcess()->FilterURL(false, &validated_url);
 
   frame_tree_node_->navigator()->DidFailLoadWithError(
       this, validated_url, error_code, error_description);
 }
 
 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",

[Charlie Reis, 2014/08/20 20:22:30]

Once we have the data, what will the URL tell us?  I ask because it seems
unlikely to lead to repro steps (from past experience on bugs like this) and is
data that probably shouldn't be logged without a reason.

I'm guessing that the page ID values will be more useful.  Do we have any
hypotheses so far that we could test with additional state (e.g., whether
there's a last committed entry)?

[Avi (use Gerrit), 2014/08/20 20:29:20]

I have no experience in debugging situations like this, so if you have
experience and suggestions as to what might be useful in this scenario I'm all
ears.

I was logging the URLs in the attempt to make a repro case out of them. I don't
have hypotheses right now. The scenario that I can imagine is like last time,
where we were re-creating a RenderView, and it sent out a message before
navigation (5 != -1 or the like), but right now I have no idea what is going on
here, or even if all of these crashes are even related.

[Charlie Reis, 2014/08/20 20:44:47]

Well, I could be wrong-- the URL might lead to a repro in this case, if it's a
deterministic page behavior that's to blame.  Since we don't expect it to be
live for more than one Canary, it seems ok to give it a try.  Let's be sure to
revert it once it's out there for one build.

[Avi (use Gerrit), 2014/08/20 21:00:02]

No disagreement there.

+                                  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, 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
 // notification containing parameters identifying the navigation.
 //
 // Subframes are identified by the page transition type.  For subframes loaded
 // as part of a wider page load, the page_id will be the same as for the top
 // level frame.  If the user explicitly requests a subframe navigation, we will
 // get a new page_id because we need to create a new navigation entry for that
 // action.
 void RenderFrameHostImpl::OnNavigate(const IPC::Message& msg) {
   // Read the parameters out of the IPC message directly to avoid making another
   // copy when we filter the URLs.
   PickleIterator iter(msg);
   FrameHostMsg_DidCommitProvisionalLoad_Params validated_params;
   if (!IPC::ParamTraits<FrameHostMsg_DidCommitProvisionalLoad_Params>::
       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.
   // We do not want to cancel the pending navigation in this case, since the
   // old page will soon be stopped.  Instead, treat this as a beforeunload ack
   // to allow the pending navigation to continue.
   if (render_view_host_->is_waiting_for_beforeunload_ack_ &&
       render_view_host_->unload_ack_is_for_cross_site_transition_ &&
       PageTransitionIsMainFrame(validated_params.transition)) {
     OnBeforeUnloadACK(true, send_before_unload_start_time_,
@@ skipping 304 matching lines @@
 void RenderFrameHostImpl::OnDidDisownOpener() {
   // This message is only sent for top-level frames. TODO(avi): when frame tree
   // mirroring works correctly, add a check here to enforce it.
   delegate_->DidDisownOpener(this);
 }
 
 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());

[Charlie Reis, 2014/08/20 20:22:30]

It's possible for GetLastCommittedURL to return null in a new tab, and I bet we
could construct a case that we would even get an UpdateTitle message in that
situation (e.g., w=window.open(slow_url);w.document.title=foo).

[Avi (use Gerrit), 2014/08/20 20:29:20]

Would that be a failure, though? Before the commit, the page ids would match and
we wouldn't fail. And GetLastCommittedURL can't return null; it returns GURL.

[Charlie Reis, 2014/08/20 20:44:47]

Ah, you're right, GetLastCommittedEntry can return null but not
GetLastCommittedURL.

+    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) {
     NOTREACHED() << "Renderer sent too many characters in title.";
     return;
   }
 
-  delegate_->UpdateTitle(this, page_id, title,
+  delegate_->UpdateTitle(this, render_view_host_->page_id_, title,
                          WebTextDirectionToChromeTextDirection(
                              title_direction));
 }
 
 void RenderFrameHostImpl::OnUpdateEncoding(const std::string& encoding_name) {
   // This message is only sent for top-level frames. TODO(avi): when frame tree
   // mirroring works correctly, add a check here to enforce it.
   delegate_->UpdateEncoding(this, encoding_name);
 }
 
@@ skipping 313 matching lines @@
 }
 
 void RenderFrameHostImpl::CancelSuspendedNavigations() {
   // Clear any state if a pending navigation is canceled or preempted.
   if (suspended_nav_params_)
     suspended_nav_params_.reset();
   navigations_suspended_ = false;
 }
 
 }  // namespace content