PlzNavigate: stop navigations when opening a document for write

content/browser/frame_host/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/navigator_impl.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 993 matching lines @@
   // 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.
   if (ongoing_navigation_request &&
       (ongoing_navigation_request->browser_initiated() ||
        ongoing_navigation_request->begin_params().has_user_gesture) &&
       !begin_params.has_user_gesture) {
     RenderFrameHost* current_frame_host =
         frame_tree_node->render_manager()->current_frame_host();
     current_frame_host->Send(
-        new FrameMsg_Stop(current_frame_host->GetRoutingID()));
+        new FrameMsg_DroppedNavigation(current_frame_host->GetRoutingID(),
+                                       begin_params.renderer_navigation_id));
     return;
   }
 
   // In all other cases the current navigation, if any, is canceled and a new
   // NavigationRequest is created for the node.
   frame_tree_node->CreatedNavigationRequest(
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params,
           controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount()));
@@ skipping 14 matching lines @@
 
   // For main frames, NavigationHandle will be created after the call to
   // |DidStartMainFrameNavigation|, so it receives the most up to date pending
   // entry from the NavigationController.
   NavigationEntry* pending_entry = controller_->GetPendingEntry();
   navigation_request->CreateNavigationHandle(
       pending_entry ? pending_entry->GetUniqueID() : 0);
   navigation_request->BeginNavigation();
 }
 
+void NavigatorImpl::OnAbortNavigation(FrameTreeNode* frame_tree_node) {
+  DCHECK(frame_tree_node);

[Charlie Reis, 2017/03/20 06:34:44]

nit: This seems unnecessary.  It doesn't seem likely to have a null
FrameTreeNode passed in, and we'd find out on the next line anyway.

[clamy, 2017/03/20 14:56:22]

Done.

+
+  NavigationRequest* ongoing_navigation_request =
+      frame_tree_node->navigation_request();
+  if (!ongoing_navigation_request ||

[Charlie Reis, 2017/03/20 06:34:44]

What happens if we've already sent the CommitNavigation IPC to the renderer by
the time we get this?  (It's naturally a race.)

Will ongoing_navigation_request still exist at that point?  What happens if we
cancel it after sending CommitNavigation and the renderer then sends up a
DidCommitProvisionalLoad?  (Will that be a problem when trying to match up the
commit with a NavigationHandle?)

More importantly, what happens in the renderer process if the CommitNavigation
IPC arrives and the renderer has already done its document.open, expecting the
load to be aborted?  Will that lead to problems, or will it safely overwrite
whatever the document.open started?

[clamy, 2017/03/20 14:56:22]

The ongoing navigation request will no longer exist after we've sent the
CommitNavigation IPC. There is still a possibility for a race, if the
CommitNavigation and DropNavigation are emitted at the same time and cross each
other. I don't think I can do much to prevent it, and it seems it just leads to
the navigation being committed. I'd like to point out that this kind of race is
already present: if we start a browser-initiated navigation in the current
renderer while it is doing the document.open, the navigation will go on.
Something similar happens in that case.

[Charlie Reis, 2017/03/20 23:02:49]

Great, that's a bit simpler.
Yes, I think it's fine if the navigation gets committed over whatever
document.open created.  (The same thing would also happen if document.open were
on a timer and the new commit just happened before it fired in the first place.)

I was concerned that something in the renderer might break if we've already
discarded renderer-side state, but it sounds like it works ok.

+      ongoing_navigation_request->browser_initiated()) {

[Charlie Reis, 2017/03/20 06:34:44]

The case we're looking at seems really similar to the logic in
OnBeginNavigation, which also checks for whether there's a user gesture.  Should
a document.write be able to cancel a link click (i.e., a renderer-initiated
navigation with a user gesture), for example?

Just wondering if we should be sharing the same policy (and possibly code)
instead of ending up with another variation here.

[clamy, 2017/03/20 14:56:22]

The existing behavior is that document.write will cancel a link click. I'm not
fully following the current behavior though, as currently we would cancel any
ongoing navigation in the frame of the renderer process, which includes
browser-initiated navigations. I thought restricting to only renderer-initiated
navigations made more sense (and emulating the current behavior in Plznavigate
just looks weird). If you think we should also restrict to non-user initiated
navigations, this can be changed.

[Charlie Reis, 2017/03/20 23:02:49]

It only bothered me in the number of different policies we have for different
cases.  Changing it to ignore user-initiated navigations does pose some
compatibility risk (e.g., if a page depended on doing a document.write in
response to link clicks (yuck!)), so it's probably safer to leave it as you have
it.

(I don't see as big a regression risk with excluding browser-initiated
navigations from this.)

+    return;
+  }
+
+  // Abort the renderer-initiated navigation request.
+  CancelNavigation(frame_tree_node);
+}
+
 // PlzNavigate
 void NavigatorImpl::CancelNavigation(FrameTreeNode* frame_tree_node) {
   CHECK(IsBrowserSideNavigationEnabled());
   frame_tree_node->ResetNavigationRequest(false);
   if (frame_tree_node->IsMainFrame())
     navigation_data_.reset();
 }
 
 void NavigatorImpl::LogResourceRequestTime(
     base::TimeTicks timestamp, const GURL& url) {
@@ skipping 235 matching lines @@
     if (navigation_handle)
       navigation_handle->update_entry_id_for_transfer(entry->GetUniqueID());
 
     controller_->SetPendingEntry(std::move(entry));
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content