Don't rely on the pending NavigationEntry for location.replace.

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 e24b58e84c6d09a62902022699c380921c95f04c..c22b00227e4432c96559a6faa987b97818a9a745 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -228,7 +228,6 @@ NavigationControllerImpl::NavigationControllerImpl(
     : browser_context_(browser_context),
       pending_entry_(NULL),
       failed_pending_entry_id_(0),
-      failed_pending_entry_should_replace_(false),
       last_committed_entry_index_(-1),
       pending_entry_index_(-1),
       transient_entry_index_(-1),
@@ -854,20 +853,8 @@ bool NavigationControllerImpl::RendererDidNavigate(
       pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE)
     pending_entry_->set_restore_type(NavigationEntryImpl::RESTORE_NONE);
 
-  // If we are doing a cross-site reload, we need to replace the existing
-  // navigation entry, not add another entry to the history. This has the side
-  // effect of removing forward browsing history, if such existed. Or if we are
-  // doing a cross-site redirect navigation, we will do a similar thing.
-  //
-  // If this is an error load, we may have already removed the pending entry
-  // when we got the notice of the load failure. If so, look at the copy of the
-  // pending parameters that were saved.
-  if (params.url_is_unreachable && failed_pending_entry_id_ != 0) {
-    details->did_replace_entry = failed_pending_entry_should_replace_;
-  } else {
-    details->did_replace_entry = pending_entry_ &&
-                                 pending_entry_->should_replace_entry();
-  }
+  // The renderer tells us whether the navigation replaces the current entry.
+  details->did_replace_entry = params.should_replace_current_entry;

[Avi (use Gerrit), 2016/03/14 20:49:59]

When we specify the error page load, do we copy over the value of
should-replace? You're removing failed_pending_entry_should_replace_ but it's
not immediately obvious to me that the value that you're putting in now is
correct.

[Charlie Reis, 2016/03/14 21:18:19]

In which context?  In general, if the browser process set
should_replace_current_entry on the pending entry before, it will now get passed
down to the renderer, used, and passed back up at commit time.

For error pages, we still do the intended replacement in
NavigationControllerBrowserTest.ErrorPageReplacement (and pass the test), which
is the test you added for failed_pending_entry_should_replace_ in
https://codereview.chromium.org/1048963002.

I'm not certain if that covers all the necessary cases for that bool, though. 
Let me know if it looks like there's something I'm missing.

 
   // Do navigation-type specific actions. These will make and commit an entry.
   details->type = ClassifyNavigation(rfh, params);
@@ -1012,6 +999,18 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
     return NAVIGATION_TYPE_NEW_SUBFRAME;
   }
 
+  // Cross-process location.replace navigations should be classified as New with
+  // replacement rather than ExistingPage, since it is not safe to reuse the
+  // NavigationEntry.
+  // TODO(creis): Have the renderer classify location.replace as
+  // did_create_new_entry for all cases and eliminate this special case.  This
+  // requires updating several test expectations.  See https://crbug.com/317872.
+  if (!rfh->GetParent() && GetLastCommittedEntry() &&
+      GetLastCommittedEntry()->site_instance() != rfh->GetSiteInstance() &&
+      params.should_replace_current_entry) {
+    return NAVIGATION_TYPE_NEW_PAGE;
+  }
+
   // We only clear the session history when navigating to a new page.
   DCHECK(!params.history_list_was_cleared);
 
@@ -1996,8 +1995,6 @@ void NavigationControllerImpl::DiscardPendingEntry(bool was_failure) {
 
   if (was_failure && pending_entry_) {
     failed_pending_entry_id_ = pending_entry_->GetUniqueID();
-    failed_pending_entry_should_replace_ =
-        pending_entry_->should_replace_entry();
   } else {
     failed_pending_entry_id_ = 0;
   }