Ensure the NavigationHandle's nav entry ID is updated during transfers.

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 836 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 the pending entry matches this commit, check if it says to replace the
+  // current entry (which preserves forward history).  This is used for things
+  // like cross-process client redirects or cross-process reloads.

[Charlie Harrison, 2016/03/11 15:15:14]

Maybe reference location.replace(<cross-site>). I was initially confused reading
this comment about why you would ever need a cross-process reload.

[Charlie Reis, 2016/03/11 20:12:48]

Done.

   //
   // 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) {
+  // pending parameters that were saved and see if they match the handle.
+  NavigationHandleImpl* handle = rfh->navigation_handle();
+  DCHECK(handle);
+  if (params.url_is_unreachable && failed_pending_entry_id_ != 0 &&
+      handle->pending_nav_entry_id() == failed_pending_entry_id_) {
     details->did_replace_entry = failed_pending_entry_should_replace_;
+  } else if (pending_entry_ &&
+             pending_entry_->GetUniqueID() == handle->pending_nav_entry_id() &&

[Charlie Harrison, 2016/03/11 15:15:14]

Suggestion: This logic is duplicated in a bunch of places. It might be nice to
consolidate it. I'm mainly thinking of the check in
|RendererDidNavigateToNewPage|. Up to you though, it's arguably less clean to
thread a bool through that function.

[Charlie Reis, 2016/03/11 20:12:48]

Good idea.  I wrote a helper function for it.

We can now be sure that the handle isn't null in the
RendererDidNavigateToNewPage case as well, thanks to the unit test changes.

+             pending_entry_->should_replace_entry()) {
+    details->did_replace_entry = pending_entry_->should_replace_entry();
   } else {
-    details->did_replace_entry = pending_entry_ &&
-                                 pending_entry_->should_replace_entry();
+    details->did_replace_entry = false;
   }
 
   // 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) {
@@ skipping 1191 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content