PlzNavigate: stop navigations when opening a document for write

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 ad1a19a5eb33de3be38b9eb3335cf595e60c6f51..d0727647b7227f634677049a80df68bd990b0c2e 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -1011,7 +1011,8 @@ void NavigatorImpl::OnBeginNavigation(
     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;
   }
 
@@ -1046,6 +1047,20 @@ void NavigatorImpl::OnBeginNavigation(
   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());