Do not reset NavigationHandle when navigating same-page

content/browser/frame_host/interstitial_page_navigator_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.
 
 #include "content/browser/frame_host/interstitial_page_navigator_impl.h"
 
 #include "content/browser/frame_host/interstitial_page_impl.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ skipping 27 matching lines @@
                                    false,  // is_renderer_initiated
                                    false,  // is_synchronous
                                    false,  // is_srcdoc
                                    navigation_start,
                                    0,       // pending_nav_entry_id
                                    false);  // started_in_context_menu
 }
 
 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();
   }
 
   // TODO(nasko): Move implementation here, but for the time being call out
   // to the interstitial page code.
   interstitial_->DidNavigate(render_frame_host->render_view_host(),
                              input_params);
 }
 
 NavigationHandleImpl*
 InterstitialPageNavigatorImpl::GetNavigationHandleForFrameHost(
     RenderFrameHostImpl* render_frame_host) {
   return navigation_handle_.get();
 }
 
 }  // namespace content