Do not reset NavigationHandle when navigating same-page

content/browser/frame_host/render_frame_host_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/render_frame_host_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 1161 matching lines @@
   process->FilterURL(true, &validated_params.searchable_form_url);
 
   // Without this check, the renderer can trick the browser into using
   // filenames it can't access in a future session restore.
   if (!CanAccessFilesOfPageState(validated_params.page_state)) {
     bad_message::ReceivedBadMessage(
         GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE);
     return;
   }
 
-  // If the URL or |was_within_same_page| does not match what the
-  // NavigationHandle expects, treat the commit as a new navigation. This can
-  // happen if an ongoing slow same-process navigation is interwoven with a
-  // synchronous renderer-initiated navigation.
-  // TODO(csharrison): Data navigations loaded with LoadDataWithBaseURL get
-  // reset here, because the NavigationHandle tracks the URL but the
-  // validated_params.url tracks the data. The trick of saving the old entry ids
-  // for these navigations should go away when this is properly handled. See
-  // crbug.com/588317.
-  int entry_id_for_data_nav = 0;
-  bool is_renderer_initiated = true;
-  if (navigation_handle_ &&
-      ((navigation_handle_->GetURL() != validated_params.url) ||
-       navigation_handle_->IsSamePage() !=
-           validated_params.was_within_same_page)) {
-    // Make sure that the pending entry was really loaded via
-    // LoadDataWithBaseURL and that it matches this handle.
-    NavigationEntryImpl* pending_entry =
-        NavigationEntryImpl::FromNavigationEntry(
-            frame_tree_node()->navigator()->GetController()->GetPendingEntry());
-    bool pending_entry_matches_handle =
-        pending_entry &&
-        pending_entry->GetUniqueID() ==
-            navigation_handle_->pending_nav_entry_id();
-    // TODO(csharrison): The pending entry's base url should equal
-    // |validated_params.base_url|. This is not the case for loads with invalid
-    // base urls.

[Charlie Reis, 2016/11/03 22:02:22]

Is there a reason not to preserve this TODO?

[clamy, 2016/11/04 14:18:21]

Nope, it got lost in code reordering. I brought it back.

-    if (navigation_handle_->GetURL() == validated_params.base_url &&
-        pending_entry_matches_handle &&
-        !pending_entry->GetBaseURLForDataURL().is_empty()) {
-      entry_id_for_data_nav = navigation_handle_->pending_nav_entry_id();
-      is_renderer_initiated = pending_entry->is_renderer_initiated();
+  // PlzNavigate
+  if (!navigation_handle_ && IsBrowserSideNavigationEnabled()) {
+    // PlzNavigate: the browser has not been notified about the start of the
+    // load in this renderer yet. Do it now.

[Charlie Reis, 2016/11/03 22:02:22]

nit: Add something like: (e.g., for same-page navigations that start in the
renderer).

[clamy, 2016/11/04 14:18:21]

Done.

+    if (!is_loading()) {
+      bool was_loading = frame_tree_node()->frame_tree()->IsLoading();
+      is_loading_ = true;
+      frame_tree_node()->DidStartLoading(true, was_loading);
     }
-    navigation_handle_.reset();
+    pending_commit_ = false;
   }
 
-  // Synchronous renderer-initiated navigations will send a
-  // DidCommitProvisionalLoad IPC without a prior DidStartProvisionalLoad
-  // message. Or in addition, the if block above can reset the NavigationHandle
-  // in cases it doesn't match the expected commit.
-  if (!navigation_handle_) {
-    // There is no pending NavigationEntry in these cases, so pass 0 as the
-    // nav_id. If the previous handle was a prematurely aborted navigation
-    // loaded via LoadDataWithBaseURL, propogate the entry id.
-    navigation_handle_ = NavigationHandleImpl::Create(
-        validated_params.url, frame_tree_node_, is_renderer_initiated,
-        validated_params.was_within_same_page, validated_params.is_srcdoc,
-        base::TimeTicks::Now(), entry_id_for_data_nav,
-        false);  // started_from_context_menu
-    // PlzNavigate
-    if (IsBrowserSideNavigationEnabled()) {
-      // PlzNavigate: synchronous loads happen in the renderer, and the browser
-      // has not been notified about the start of the load yet. Do it now.
-      if (!is_loading()) {
-        bool was_loading = frame_tree_node()->frame_tree()->IsLoading();
-        is_loading_ = true;
-        frame_tree_node()->DidStartLoading(true, was_loading);
-      }
-      pending_commit_ = false;
-    }
-  }
+  // Find the appropriate NavigationHandle for this navigation.
+  std::unique_ptr<NavigationHandleImpl> navigation_handle =
+      FindNavigationHandleForCommit(validated_params);
+  DCHECK(navigation_handle);
 
   accessibility_reset_count_ = 0;
-  frame_tree_node()->navigator()->DidNavigate(this, validated_params);
+  frame_tree_node()->navigator()->DidNavigate(this, validated_params,
+                                              std::move(navigation_handle));
 
   // Since we didn't early return, it's safe to keep the commit state.
   commit_state_resetter.disable();
 
   // For a top-level frame, there are potential security concerns associated
   // with displaying graphics from a previously loaded page after the URL in
   // the omnibar has been changed. It is unappealing to clear the page
   // immediately, but if the renderer is taking a long time to issue any
   // compositor output (possibly because of script deliberately creating this
   // situation) then we clear it after a while anyway.
@@ skipping 1926 matching lines @@
   // handler after it's destroyed so it can't run after the RFHI is destroyed.
   web_bluetooth_service_->SetClientConnectionErrorHandler(base::Bind(
       &RenderFrameHostImpl::DeleteWebBluetoothService, base::Unretained(this)));
   return web_bluetooth_service_.get();
 }
 
 void RenderFrameHostImpl::DeleteWebBluetoothService() {
   web_bluetooth_service_.reset();
 }
 
+std::unique_ptr<NavigationHandleImpl>
+RenderFrameHostImpl::FindNavigationHandleForCommit(
+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
+  // If this is a same-page navigation, there isn't an existing NavigationHandle
+  // to use for the navigation. Create one, but don't reset any NavigationHandle
+  // tracking an ongoing navigation, since this may lead to the cancellation of
+  // the navigation.
+  if (params.was_within_same_page) {
+    // There is no pending NavigationEntry in these cases, so pass 0 as the
+    // nav_id.

[Charlie Reis, 2016/11/03 22:02:22]

nit: pending_nav_entry_id

[clamy, 2016/11/04 14:18:21]

Done.

+    return NavigationHandleImpl::Create(
+        params.url, frame_tree_node_, true,  // is_renderer_initiated

[Charlie Reis, 2016/11/03 22:02:22]

Hmm, is_renderer_initiated isn't necessarily true for all same-page navigations.
 You can type "#foo" in the address bar to trigger one.

Of course, in that case we would already have a matching NavigationHandle, in
which case it's wrong to get inside this block and create another one.  Maybe
the condition on line 3155 should include a check for whether the current handle
matches this?  Or rather, move the block from line 3164 to the top of the
method?

We should include a test for this case.

[clamy, 2016/11/04 14:18:21]

That's a good point, though in this case we don't have a NavigationHandle, since
it is only created when we receive a DidStartProvisionalLoad. However we do have
a pending NavigationEntry, so I'm checking against that. We would still
miscategorize it if we happened to delete the pending entry in the meantime,
though I'm not sure what I can do in that case.

I've added a test to check that the NavigationHandle has the right values in
this particular case.

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

Good point.  I think missing the canceled-pending-entry case is probably ok.
Thanks for the test!  Why is it disabled in PlzNavigate, though?

+        params.was_within_same_page, params.is_srcdoc, base::TimeTicks::Now(),
+        0, false);  // started_from_context_menu
+  }
+
+  // Determine if the current NavigationHandle can be used.
+  if (navigation_handle_ && navigation_handle_->GetURL() == params.url) {
+    DCHECK(!navigation_handle_->IsSamePage());
+    return std::move(navigation_handle_);
+  }
+
+  // If the URL does not match what the NavigationHandle expects, treat the
+  // commit as a new navigation. This can happen when loading a Data
+  // navigation with LoadDataWithBaseURL.
+  //
+  // TODO(csharrison): Data navigations loaded with LoadDataWithBaseURL get
+  // reset here, because the NavigationHandle tracks the URL but the params.url
+  // tracks the data. The trick of saving the old entry ids for these
+  // navigations should go away when this is properly handled.
+  // See crbug.com/588317.
+  int entry_id_for_data_nav = 0;
+  bool is_renderer_initiated = true;
+
+  // Make sure that the pending entry was really loaded via LoadDataWithBaseURL
+  // and that it matches this handle.  TODO(csharrison): The pending entry's
+  // base url should equal |params.base_url|. This is not the case for loads
+  // with invalid base urls.
+  if (navigation_handle_) {
+    NavigationEntryImpl* pending_entry =
+        NavigationEntryImpl::FromNavigationEntry(
+            frame_tree_node()->navigator()->GetController()->GetPendingEntry());
+    bool pending_entry_matches_handle =
+        pending_entry &&
+        pending_entry->GetUniqueID() ==
+            navigation_handle_->pending_nav_entry_id();
+    if (navigation_handle_->GetURL() == params.base_url &&
+        pending_entry_matches_handle &&
+        !pending_entry->GetBaseURLForDataURL().is_empty()) {
+      entry_id_for_data_nav = navigation_handle_->pending_nav_entry_id();
+      is_renderer_initiated = pending_entry->is_renderer_initiated();
+    }
+
+    // Reset any existing NavigationHandle.
+    navigation_handle_.reset();
+  }
+
+  // There is no pending NavigationEntry in these cases, so pass 0 as the
+  // nav_id. If the previous handle was a prematurely aborted navigation loaded

[Charlie Reis, 2016/11/03 22:02:22]

nit: pending_nav_entry_id

[clamy, 2016/11/04 14:18:21]

Done.

+  // via LoadDataWithBaseURL, propagate the entry id.
+  return NavigationHandleImpl::Create(
+      params.url, frame_tree_node_, is_renderer_initiated,
+      params.was_within_same_page, params.is_srcdoc, base::TimeTicks::Now(),
+      entry_id_for_data_nav, false);  // started_from_context_menu
+}
+
 }  // namespace content