Do not reset NavigationHandle when navigating same-page

content/browser/frame_host/interstitial_page_navigator_impl.cc

Index: content/browser/frame_host/interstitial_page_navigator_impl.cc
diff --git a/content/browser/frame_host/interstitial_page_navigator_impl.cc b/content/browser/frame_host/interstitial_page_navigator_impl.cc
index a19deefa127366eb78f867e56bc8c4c4d7fd73b0..548b4d1a6892d8aa9b06dedc16b0dad55bfecac0 100644
--- a/content/browser/frame_host/interstitial_page_navigator_impl.cc
+++ b/content/browser/frame_host/interstitial_page_navigator_impl.cc
@@ -45,8 +45,9 @@ void InterstitialPageNavigatorImpl::DidStartProvisionalLoad(
 
 void InterstitialPageNavigatorImpl::DidNavigate(
     RenderFrameHostImpl* render_frame_host,
-    const FrameHostMsg_DidCommitProvisionalLoad_Params& input_params) {
-  if (navigation_handle_) {
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& input_params,
+    std::unique_ptr<NavigationHandleImpl> navigation_handle) {
+  if (!navigation_handle->IsSamePage() && navigation_handle_) {

[Charlie Reis, 2016/11/04 17:30:53]

Hmm, I think we should look more closely at this.

The problem happens when navigation_handle != navigation_handle_, in which case
we might be calling DidCommitNavigation on the wrong handle.  It seems fragile
to assume that's only true if navigation_handle->IsSamePage() returns false.  If
we are making that assumption, it's safer to do it as a DCHECK or CHECK, with
something like:
if (navigation_handle == navigation_handle_) {
  // existing block
} else {
  CHECK(navigation_handle->IsSamePage());
}

Additionally, in the navigation_handle->IsSamePage() case, aren't we missing the
DidCommitNavigation call on it?

[clamy, 2016/11/07 14:10:51]

Actually, it is impossible for navigation_handle_ to be navigation_handle. In
the case of interstitials, the NavigationHandle is stored in the
InterstitialPageNavigatorImpl, so a brand new NavigationHandle would be created
in RenderFrameHostImpl ta commit time.

I've moved the storage of the NavigationHandle to the RenderFrameHost, so this
should solve the issue of having two NavigationHandles in that case.

[Charlie Reis, 2016/11/08 06:38:53]

Much better-- thanks!

     navigation_handle_->DidCommitNavigation(input_params, false,
                                             render_frame_host);
     navigation_handle_.reset();