PlzNavigate: don't stop all loaders when renderer-initiated nav fails

content/browser/frame_host/frame_tree_node.cc

Index: content/browser/frame_host/frame_tree_node.cc
diff --git a/content/browser/frame_host/frame_tree_node.cc b/content/browser/frame_host/frame_tree_node.cc
index ee394a607ab5e7cadf5abfc96f44a58bddddd9d9..f3ce0aa7bd649a72a35b91c9ebbe57cf6216e03d 100644
--- a/content/browser/frame_host/frame_tree_node.cc
+++ b/content/browser/frame_host/frame_tree_node.cc
@@ -400,7 +400,13 @@ void FrameTreeNode::ResetNavigationRequest(bool keep_state,
   CHECK(IsBrowserSideNavigationEnabled());
   if (!navigation_request_)
     return;
-  bool was_renderer_initiated = !navigation_request_->browser_initiated();
+
+  // The renderer should be informed if the caller allows to do so and the
+  // navigation came from a BeginNavigation IPC. This corresponds to
+  // may_transfer being set to false on the NavigationRequest.
+  int need_to_inform_renderer =
+      inform_renderer && !navigation_request_->may_transfer();

[Charlie Reis, 2017/03/23 16:52:53]

Hmm, I'm not sure I understand this change.  Why was !browser_initiated() wrong?
 It was more clearly about the case that the renderer would have some state that
needs clearing.  Using may_transfer() instead is subtle and indirect-- without
looking into the code, I don't know why it's equivalent.  (That also means it's
at risk of changing for unrelated reasons, which could break this check.)

[clamy, 2017/04/24 13:53:44]

We should only inform the renderer if the navigation was started through the
BeginNavigation path because that;s the only case where it is expecting the
browser process to be doing a navigation (which corresponds to
!navigation_request_->may_transfer()). browser_initiated may be false for
navigations started via RequestNavigation, if they ultimately come from OpenURL.

[Charlie Reis, 2017/04/24 20:13:38]

Ok, so the OpenURL case means the renderer doesn't need to be notified, and thus
!browser_initiated() isn't the right thing to use.  (I guess the renderer
doesn't go into a loading state in that case?)

But how is "may_transfer" connected to BeginNavigation?  A renderer-initiated
navigation via BeginNavigation seems like it could still transfer (e.g., for a
redirect to the Chrome Web Store).  Maybe I'm just having trouble with the
name-- the comment on NavigationRequest::may_transfer_ is similarly confusing to
me, since it says that a transfer may happen even when may_transfer_ is false. 
Maybe we can rename that to be clearer, to help this case make sense as well.

Is may_transfer() actually about whether we send the navigation through
RenderFrameHostManager to do a process swap for cross-site navigations (and thus
preserve the current process model in PlzNavigate)?

If so, there's two ways we might try to address this:

1) Make the name (and comment) closer to that case, with something like
should_swap_if_cross_site().  That reduces some of the ambiguity around the word
"transfer," since we do transfers already in non-PlzNavigate on
renderer-initiated navigations (like the CWS case).  However, even this name is
a bit odd in the current context, since "whether the renderer has navigation
state" and "whether we should swap processes for cross-site navigations" are two
different concepts that just happen to be the same at the moment.

2) Make this member of NavigationRequest explicitly be about whether it started
with a BeginNavigation (e.g., from_begin_navigation()?).  Then we can build
policies on top of that state, including whether to do a process swap for
cross-site navigations and whether the renderer needs to be notified when a
navigation is reset.

I'm leaning towards the latter.  What do you think?

[clamy, 2017/04/25 12:24:08]

I agree that the naming is confusing. I think we named in that way because it
was only used in one particular place for which the naming made more sense (and
I wanted to keep it short).

Since we're using it in other contexts than RenderFrameHostmanager, I like
option 2 better as well.

+
   NavigationRequest::AssociatedSiteInstanceType site_instance_type =
       navigation_request_->associated_site_instance_type();
   navigation_request_.reset();
@@ -425,9 +431,9 @@ void FrameTreeNode::ResetNavigationRequest(bool keep_state,
   // process asked for the navigation to be aborted, e.g. following a
   // document.open, do not send an IPC to the renderer process as it already
   // expects the navigation to stop.
-  if (was_renderer_initiated && inform_renderer) {
+  if (need_to_inform_renderer) {
     current_frame_host()->Send(
-        new FrameMsg_Stop(current_frame_host()->GetRoutingID()));
+        new FrameMsg_DroppedNavigation(current_frame_host()->GetRoutingID()));
   }
 
 }