PlzNavigate: Cancel ongoing history navigations in the browser when we receive a navigation request…

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 997805938531dd69285fc32763d94d83c67f1a6c..d99ffe98aa229049cfe19b1a896f4032d4d650b3 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -998,6 +998,17 @@ void NavigatorImpl::OnBeginNavigation(
   NavigationRequest* ongoing_navigation_request =
       frame_tree_node->navigation_request();
 
+  // Client redirects during the initial history navigation of a child frame
+  // should take precedence over the history navigation (despite being renderer-
+  // initiated).  See https://crbug.com/348447 and https://crbug.com/691168.
+  if (ongoing_navigation_request &&
+      ongoing_navigation_request->request_params()
+          .is_history_navigation_in_new_child) {
+    // Preemptively clear this local pointer before deleting the request.
+    ongoing_navigation_request = nullptr;
+    frame_tree_node->ResetNavigationRequest(false);

[jam, 2017/05/16 20:13:11]

btw I was tracing this code as part of
https://codereview.chromium.org/2889703002/, but I don't see it being hit for
the layout tests fixed here, or for any other layout or browser test. Is this
still needed?

[Charlie Reis, 2017/05/18 21:26:46]

Yep, it's still needed, but some races mean we aren't hitting it in tests.  We
should add a test for this branch of the race if we can-- I've filed
https://crbug.com/724271.

See https://codereview.chromium.org/2890613002/#msg15 for details.

+  }
+
   // The renderer-initiated navigation request is ignored iff a) there is an
   // ongoing request b) which is browser or user-initiated and c) the renderer
   // request is not user-initiated.
@@ -1166,8 +1177,12 @@ void NavigatorImpl::RequestNavigation(FrameTreeNode* frame_tree_node,
 
   // This value must be set here because creating a NavigationRequest might
   // change the renderer live/non-live status and change this result.
+  // We don't want to dispatch a beforeunload handler if
+  // is_history_navigation_in_new_child is true. This indicates a newly created
+  // child frame which does not have a beforunload handler.
   bool should_dispatch_beforeunload =
       !is_same_document_history_load &&
+      !is_history_navigation_in_new_child &&
       frame_tree_node->current_frame_host()->ShouldDispatchBeforeUnload();
   FrameMsg_Navigate_Type::Value navigation_type = GetNavigationType(
       frame_tree_node->current_url(),  // old_url