Solidify Entry discarding logic (NavigationHandle keeps its ID)

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 39 matching lines @@
 #include "cc/base/switches.h"
 #include "components/mime_util/mime_util.h"
 #include "content/browser/bad_message.h"
 #include "content/browser/browser_url_handler_impl.h"
 #include "content/browser/dom_storage/dom_storage_context_wrapper.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/frame_host/debug_urls.h"
 #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"
 #include "content/common/frame_messages.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/ssl_status_serialization.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/invalidate_type.h"
@@ skipping 1007 matching lines @@
   return NAVIGATION_TYPE_EXISTING_PAGE;
 }
 
 void NavigationControllerImpl::RendererDidNavigateToNewPage(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool replace_entry) {
   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();
 
     update_virtual_url = new_entry->update_virtual_url_with_url();
   } else {
     new_entry = make_scoped_ptr(new NavigationEntryImpl);
 
     // Find out whether the new entry needs to update its virtual URL on URL
     // change and set up the entry accordingly. This is needed to correctly
     // update the virtual URL when replaceState is called after a pushState.
     GURL url = params.url;
@@ skipping 939 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content