Gather debugging information for the switch away from page id for titles and state.

content/browser/web_contents/web_contents_impl.cc

 // 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.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
+#include "base/debug/crash_logging.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/process/process.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ skipping 3834 matching lines @@
   FOR_EACH_OBSERVER(WebContentsObserver,
                     observers_,
                     RenderProcessGone(GetCrashedStatus()));
 }
 
 void WebContentsImpl::RenderViewDeleted(RenderViewHost* rvh) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, RenderViewDeleted(rvh));
 }
 
 void WebContentsImpl::UpdateState(RenderViewHost* rvh,
                                   int32 page_id,

[Charlie Reis, 2015/08/03 20:41:38]

In this case, we should probably also log both page_id and RVH::page_id_, as
well as RVI::nav_entry_id().

(See below for discussion.)

                                   const PageState& page_state) {
   // Ensure that this state update comes from a RenderViewHost that belongs to
   // this WebContents.
   // TODO(nasko): This should go through RenderFrameHost.
   if (rvh->GetDelegate()->GetAsWebContents() != this)
     return;
 
   // We must be prepared to handle state updates for any page. They occur
   // when the user is scrolling and entering form data, as well as when we're
   // leaving a page, in which case our state may have already been moved to
   // the next page. The navigation controller will look up the appropriate
   // NavigationEntry and update it when it is notified via the delegate.
   RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(rvh);
   NavigationEntryImpl* entry = controller_.GetEntryWithPageID(
       rvhi->GetSiteInstance(), page_id);
   if (!entry)
     return;
 
   NavigationEntryImpl* new_entry = controller_.GetEntryWithUniqueID(
       rvhi->nav_entry_id());
+  CHECK(new_entry);
 
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
     // TODO(creis): We can't properly update state for cross-process subframes
     // until PageState is decomposed into FrameStates. Until then, use the new
     // method.
     entry = new_entry;
   } else {
-    DCHECK_EQ(entry, new_entry);
+    base::debug::SetCrashKeyValue(
+        "oldindex", base::IntToString(controller_.GetIndexOfEntry(entry)));
+    base::debug::SetCrashKeyValue(
+        "newindex", base::IntToString(controller_.GetIndexOfEntry(new_entry)));
+    base::debug::SetCrashKeyValue(
+        "lastcommittedindex",
+        base::IntToString(controller_.GetLastCommittedEntryIndex()));
+    base::debug::SetCrashKeyValue("oldurl", entry->GetURL().spec());
+    base::debug::SetCrashKeyValue("newurl", new_entry->GetURL().spec());
+    base::debug::SetCrashKeyValue(
+        "updatedvalue", page_state.GetTopLevelUrlStringTemporaryForBug369661());
+    base::debug::SetCrashKeyValue("oldvalue", entry->GetURL().spec());
+    base::debug::SetCrashKeyValue("newvalue", new_entry->GetURL().spec());
+    CHECK_EQ(entry, new_entry);
   }
 
   if (page_state == entry->GetPageState())
     return;  // Nothing to update.
   entry->SetPageState(page_state);
   controller_.NotifyEntryChanged(entry);
 }
 
 void WebContentsImpl::UpdateTargetURL(RenderViewHost* render_view_host,
                                       const GURL& url) {
@@ skipping 143 matching lines @@
                     DocumentOnLoadCompletedInMainFrame());
 
   // TODO(avi): Remove. http://crbug.com/170921
   NotificationService::current()->Notify(
       NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
       Source<WebContents>(this),
       NotificationService::NoDetails());
 }
 
 void WebContentsImpl::UpdateTitle(RenderFrameHost* render_frame_host,
                                   int32 page_id,

[Charlie Reis, 2015/08/03 20:41:38]

Maybe we should report page_id and RVH::nav_entry_id() as well, so that we don't
have to dig into the minidump?

Also, it looks like we've already switched this page_id over to RVH's copy and
removed the one from the IPC.  (UpdateState still uses the one from the IPC.) 
That means we have three different ways to find the committed entry, none of
which agree (sigh):
1) From the IPC, as in UpdateState.
2) From RVH::page_id_, as here with |entry|.
3) From RVH::nav_entry_id(), as here with |new_entry|.

1 and 2 don't agree for UpdateState, though they seemed to agree for UpdateTitle
(hence why we switched this one over).  2 and 3 don't agree (hence the
UpdateTitle crashes we're seeing now).  1 and 3 don't agree (hence the
UpdateState crashes we're seeing now).

Maybe the clues you're collecting below will help us figure out why.

[Avi (use Gerrit), 2015/08/03 20:51:53]

Done.

                                   const base::string16& title,
                                   base::i18n::TextDirection title_direction) {
   // If we have a title, that's a pretty good indication that we've started
   // getting useful data.
   SetNotWaitingForResponse();
 
   // Try to find the navigation entry, which might not be the current one.
   // For example, it might be from a recently swapped out RFH.
   NavigationEntryImpl* entry = controller_.GetEntryWithPageID(
       render_frame_host->GetSiteInstance(), page_id);
 
   RenderViewHostImpl* rvhi =
       static_cast<RenderViewHostImpl*>(render_frame_host->GetRenderViewHost());
   NavigationEntryImpl* new_entry = controller_.GetEntryWithUniqueID(
       rvhi->nav_entry_id());
-  DCHECK_EQ(entry, new_entry);
+
+  base::debug::SetCrashKeyValue(
+      "oldindex", base::IntToString(controller_.GetIndexOfEntry(entry)));

[Charlie Reis, 2015/08/03 20:41:38]

Sanity check: Will this crash if entry is null?

[Avi (use Gerrit), 2015/08/03 20:51:53]

No, it doesn't care.

+  base::debug::SetCrashKeyValue(
+      "newindex", base::IntToString(controller_.GetIndexOfEntry(new_entry)));
+  base::debug::SetCrashKeyValue(
+      "lastcommittedindex",
+      base::IntToString(controller_.GetLastCommittedEntryIndex()));
+  base::debug::SetCrashKeyValue("oldurl", entry->GetURL().spec());
+  base::debug::SetCrashKeyValue("newurl", new_entry->GetURL().spec());
+  base::debug::SetCrashKeyValue("updatedvalue", base::UTF16ToUTF8(title));
+  base::debug::SetCrashKeyValue("oldvalue",
+                                base::UTF16ToUTF8(entry->GetTitle()));
+  base::debug::SetCrashKeyValue("newvalue",
+                                base::UTF16ToUTF8(new_entry->GetTitle()));
+  CHECK_EQ(entry, new_entry);

[Charlie Reis, 2015/08/03 20:41:38]

Note: In f2b5526a1c958891, |entry| is null and |new_entry| is valid.  

[Avi (use Gerrit), 2015/08/03 20:51:53]

Accounted for in the new patchset.

 
   // We can handle title updates when we don't have an entry in
   // UpdateTitleForEntry, but only if the update is from the current RVH.
   // TODO(avi): Change to make decisions based on the RenderFrameHost.
   if (!entry && render_frame_host != GetMainFrame())
     return;
 
   // TODO(evan): make use of title_direction.
   // http://code.google.com/p/chromium/issues/detail?id=27094
   if (!UpdateTitleForEntry(entry, title))
@@ skipping 525 matching lines @@
   player_map->erase(it);
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content