Solidify Entry discarding logic (NavigationHandle keeps its ID)

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 408314a6dd9d14cfba848db31d86851fa19b1317..59f962afe896e49f1582b5ef18c5592675b6a4ec 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -57,6 +57,7 @@
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_entry_screenshot_manager.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"  // Temporary
 #include "content/browser/site_instance_impl.h"
@@ -1084,12 +1085,17 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
   scoped_ptr<NavigationEntryImpl> new_entry;
   bool update_virtual_url;
   // Only make a copy of the pending entry if it is appropriate for the new page
-  // that was just loaded.  We verify this at a coarse grain by checking that
-  // the SiteInstance hasn't been assigned to something else, and by making sure
-  // that the pending entry was intended as a new entry (rather than being a
-  // history navigation that was interrupted by an unrelated, renderer-initiated
-  // navigation).
-  if (pending_entry_ && pending_entry_index_ == -1 &&
+  // that was just loaded. Verify this by checking if the entry corresponds

[Charlie Reis, 2016/02/10 00:25:42]

Glad we're adding this!  Can you add a mention of it to the CL description? 
(Maybe we can remove the 280512 logic from DidFailProvisionalLoad after all this
lands.)

[Charlie Harrison, 2016/02/11 20:23:05]

Done.

+  // to the current navigation handle. Note that in some tests the render frame
+  // host does not have a valid handle. Additionally, coarsely check that:
+  // 1. The SiteInstance hasn't been assigned to something else.
+  // 2. The pending entry was intended as a new entry, rather than being a
+  // history navigation that was interrupted by an unreleated,

[Charlie Reis, 2016/02/10 00:25:42]

nit: unrelated

[Charlie Harrison, 2016/02/11 20:23:05]

Done.

+  // renderer-initiated navigation.
+  NavigationHandleImpl* handle = rfh->navigation_handle();
+  if (pending_entry_ && handle &&
+      handle->pending_nav_entry_id() == pending_entry_->GetUniqueID() &&
+      pending_entry_index_ == -1 &&
       (!pending_entry_->site_instance() ||
        pending_entry_->site_instance() == rfh->GetSiteInstance())) {

[Charlie Reis, 2016/02/10 00:25:42]

Side note: I'm not sure if the SiteInstances can differ if the NavHandle and
pending entry have the same unique ID, can they?

That said, I'm happy leaving this as part of the check for now (to be safe), and
maybe later turning it into an assertion.

[Charlie Harrison, 2016/02/11 20:23:05]

I wanted to leave them in to be sure. I'm happy to leave a TODO :)

     new_entry = pending_entry_->Clone();