Clear child FrameNavigationEntries if a history navigation redirects.

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 4179c2d32c673874a6888914dea717baf1821d0a..bb9b2c55cd6eb7ec0d430e889802e34f7ac407b3 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1181,6 +1181,13 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage(
   frame_entry->set_method(params.method);
   frame_entry->set_post_id(params.post_id);
 
+  // If the document sequence number has changed due to redirects or a
+  // location.replace, then the child FrameNavigationEntries that were there
+  // before no longer apply.  We can leave them around for in-page navigations.
+  if (frame_entry->document_sequence_number() !=
+      params.document_sequence_number)
+    entry->ClearChildren(rfh->frame_tree_node());

[Charlie Reis, 2016/07/15 22:42:59]

This handles the main frame case.  In the old path, we did it in
RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad, but doing it at
commit time is better: we don't have to worry about whether there's a pending
entry, and it handles location.replace as well.

+
   // Update the ISN and DSN in case this was a location.replace, which can cause
   // them to change.
   // TODO(creis): Classify location.replace as NEW_PAGE instead of EXISTING_PAGE
@@ -1296,13 +1303,16 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
   // handle navigation inside of a subframe in it without creating a new entry.
   DCHECK(GetLastCommittedEntry());
 
+  // For newly created subframes, we don't need to send a commit notification.
+  // This is only necessary for history navigations in subframes.
+  bool send_commit_notification = false;
+
+  // If the |nav_entry_id| is non-zero and matches an existing entry, this is
+  // a history navigation.  Update the last committed index accordingly.
+  // If we don't recognize the |nav_entry_id|, it might be a recently pruned
+  // entry.  We'll handle it below.
   if (params.nav_entry_id) {
     int entry_index = GetEntryIndexWithUniqueID(params.nav_entry_id);
-
-    // 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
@@ -1321,10 +1331,13 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
                                         bad_message::NC_AUTO_SUBFRAME);
       }
 
-      // TODO(creis): Update the FrameNavigationEntry in --site-per-process.

[Charlie Reis, 2016/07/15 22:42:59]

This was buggy for redirects before, and the new test covers it.  Now we update
the FNE below.

+      // We only need to discard the pending entry in this history navigation
+      // case.  For newly created subframes, there was no pending entry.
       last_committed_entry_index_ = entry_index;
       DiscardNonCommittedEntriesInternal();
-      return true;
+
+      // History navigations should send a commit notification.
+      send_commit_notification = true;
     }
   }
 
@@ -1337,21 +1350,9 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
         params.document_sequence_number, rfh->GetSiteInstance(), nullptr,
         params.url, params.referrer, params.page_state, params.method,
         params.post_id);
-
-    // Cross-process subframe navigations may leave a pending entry around.
-    // Clear it if it's actually for the subframe.
-    // TODO(creis): Don't use pending entries for subframe navigations.
-    // See https://crbug.com/495161.

[Charlie Reis, 2016/07/15 22:42:59]

This is just stale code now that https://crbug.com/495161 is fixed.

[nasko, 2016/07/15 23:21:54]

Nice!

-    if (pending_entry_ &&
-        pending_entry_->frame_tree_node_id() ==
-            rfh->frame_tree_node()->frame_tree_node_id()) {
-      DiscardPendingEntry(false);
-    }
   }
 
-  // We do not need to discard the pending entry in this case, since we will
-  // not generate commit notifications for this auto-subframe navigation.
-  return false;
+  return send_commit_notification;
 }
 
 int NavigationControllerImpl::GetIndexOfEntry(