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

content/browser/frame_host/navigation_controller_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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 210 matching lines @@
   low_water_mark_ = high_water_mark_ = t;
   return t;
 }
 
 NavigationControllerImpl::NavigationControllerImpl(
     NavigationControllerDelegate* delegate,
     BrowserContext* browser_context)
     : 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),
       delegate_(delegate),
       max_restored_page_id_(-1),
       ssl_manager_(this),
       needs_reload_(false),
       is_initial_navigation_(true),
       in_navigate_to_pending_entry_(false),
       pending_reload_(NO_RELOAD),
@@ skipping 605 matching lines @@
 
   // If there is a pending entry at this point, it should have a SiteInstance,
   // except for restored entries.
   DCHECK(pending_entry_index_ == -1 ||
          pending_entry_->site_instance() ||
          pending_entry_->restore_type() != NavigationEntryImpl::RESTORE_NONE);
   if (pending_entry_ &&
       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);
 
   // is_in_page must be computed before the entry gets committed.
   details->is_in_page = IsURLInPageNavigation(
       params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
@@ skipping 124 matching lines @@
     // navigated on a popup navigated to about:blank (the iframe would be
     // written into the popup by script on the main page). For these cases,
     // there isn't any navigation stuff we can do, so just ignore it.
     if (!GetLastCommittedEntry())
       return NAVIGATION_TYPE_NAV_IGNORE;
 
     // Valid subframe navigation.
     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);
 
   if (rfh->GetParent()) {
     // All manual subframes would be did_create_new_entry and handled above, so
     // we know this is auto.
     if (GetLastCommittedEntry()) {
       return NAVIGATION_TYPE_AUTO_SUBFRAME;
     } else {
       // We ignore subframes created in non-committed pages; we'd appreciate if
@@ skipping 964 matching lines @@
 
 void NavigationControllerImpl::DiscardPendingEntry(bool was_failure) {
   // It is not safe to call DiscardPendingEntry while NavigateToEntry is in
   // progress, since this will cause a use-after-free.  (We only allow this
   // when the tab is being destroyed for shutdown, since it won't return to
   // NavigateToEntry in that case.)  http://crbug.com/347742.
   CHECK(!in_navigate_to_pending_entry_ || delegate_->IsBeingDestroyed());
 
   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;
   }
 
   if (pending_entry_index_ == -1)
     delete pending_entry_;
   pending_entry_ = NULL;
   pending_entry_index_ = -1;
 }
 
@@ skipping 60 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content