Fix the commit type for out-of-process iframes.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index f0adc0eec86961fe7178874628ede1c5f31b198f..4c482fef462157b1124ba1f11e50ee8990fcfd3e 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -422,6 +422,12 @@ NavigationEntryImpl* NavigationControllerImpl::GetEntryWithPageID(
   return (index != -1) ? entries_[index].get() : NULL;
 }
 
+bool NavigationControllerImpl::HasCommittedRealLoad(
+    FrameTreeNode* frame_tree_node) const {
+  NavigationEntryImpl* last_committed = GetLastCommittedEntry();
+  return last_committed && last_committed->HasFrameEntry(frame_tree_node);
+}
+
 void NavigationControllerImpl::LoadEntry(NavigationEntryImpl* entry) {
   // When navigating to a new page, we don't know for sure if we will actually
   // end up leaving the current page.  The new page load could for example
@@ -833,13 +839,9 @@ bool NavigationControllerImpl::RendererDidNavigate(
                      new_type == NAVIGATION_TYPE_EXISTING_PAGE;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kSitePerProcess)) {
-    // In site-per-process mode, the new classifier sometimes correctly
-    // classifies things as auto-subframe where the old classifier incorrectly
-    // ignored them or called them NEW_SUBFRAME.
-    ignore_mismatch |= details->type == NAVIGATION_TYPE_NAV_IGNORE &&
-                       new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
-    ignore_mismatch |= details->type == NAVIGATION_TYPE_NEW_SUBFRAME &&
-                       new_type == NAVIGATION_TYPE_AUTO_SUBFRAME;
+    // We know that the old classifier is wrong for OOPIFs, so use the new one
+    // in --site-per-process mode.
+    details->type = new_type;

[Avi (use Gerrit), 2015/05/27 19:26:05]

One way of doing it... :)

You will also need to force ignore_mismatch to true, so you don't trigger the
[D]CHECK during all this mishegas.

[Charlie Reis, 2015/05/27 19:31:19]

Yeah, I started to fix the old one in patch set 1, but it was kind of a mess. 
Seems like you're close enough to the switch that we can start using the new one
in --site-per-process.  :)
The CHECK will never fail in --site-per-process, since this line made the two
values equal.  But you're right that the crash keys aren't needed either, so
I'll add that.

[Avi (use Gerrit), 2015/05/27 19:34:15]

Duh, yeah. My brain is running on the local track today.

   }
   if (!ignore_mismatch) {
     base::debug::SetCrashKeyValue("oldtype", base::IntToString(details->type));
@@ -949,10 +951,6 @@ bool NavigationControllerImpl::RendererDidNavigate(
 NavigationType NavigationControllerImpl::ClassifyNavigation(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
-  // Hack; remove once http://crbug.com/464014 is fixed.
-  if (rfh->IsCrossProcessSubframe())
-    return NAVIGATION_TYPE_NEW_SUBFRAME;
-
   if (params.page_id == -1) {
     // The renderer generates the page IDs, and so if it gives us the invalid
     // page ID (-1) we know it didn't actually navigate. This happens in a few
@@ -1102,10 +1100,6 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
 NavigationType NavigationControllerImpl::ClassifyNavigationWithoutPageID(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {
-  // Hack; remove once http://crbug.com/464014 is fixed.
-  if (rfh->IsCrossProcessSubframe())
-    return NAVIGATION_TYPE_NEW_SUBFRAME;
-
   if (params.did_create_new_entry) {
     // A new entry. We may or may not have a pending entry for the page, and
     // this may or may not be the main frame.
@@ -1456,18 +1450,13 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
   DCHECK(GetLastCommittedEntry());
 
   if (params.nav_entry_id) {
-    // If the |nav_entry_id| is non-zero, this is a browser-initiated navigation
-    // and is thus a "history auto" navigation. TODO(creis): Implement
-    // back/forward this way for site-per-process.
-
     int entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id);
-    if (entry_index == -1) {
-      NOTREACHED();
-      return false;
-    }
 
-    // Update the current navigation entry in case we're going back/forward.
-    if (entry_index != last_committed_entry_index_) {
+    // If the |nav_entry_id| is non-zero and matches an existing entry, this is
+    // a history auto" navigation.  Update the last committed index accordingly.
+    // If we don't recognize the |nav_entry_id|, it might be either a pending
+    // entry for a transfer or a recently pruned entry.  We'll handle it below.
+    if (entry_index != -1 && entry_index != last_committed_entry_index_) {
       // Make sure that a subframe commit isn't changing the main frame's
       // origin. Otherwise the renderer process may be confused, leading to a
       // URL spoof. We can't check the path since that may change
@@ -1480,6 +1469,8 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
         // kill the renderer process with bad_message::NC_AUTO_SUBFRAME.
         NOTREACHED() << "Unexpected main frame origin change on AUTO_SUBFRAME.";
       }
+
+      // TODO(creis): Update the FrameNavigationEntry in --site-per-process.
       last_committed_entry_index_ = entry_index;
       DiscardNonCommittedEntriesInternal();
       return true;
@@ -1494,6 +1485,10 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
     last_committed->AddOrUpdateFrameEntry(rfh->frame_tree_node(),
                                           rfh->GetSiteInstance(), params.url,
                                           params.referrer);
+
+    // Cross-process subframe navigations may leave a pending entry around.
+    // TODO(creis): Don't use pending entries for subframe navigations.
+    DiscardNonCommittedEntriesInternal();
   }
 
   // We do not need to discard the pending entry in this case, since we will