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

content/browser/frame_host/frame_tree_node.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/frame_tree_node.h"
 
 #include <queue>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 382 matching lines @@
       navigation_request_->common_params().navigation_type);
 
   DidStartLoading(to_different_document, was_previously_loading);
 }
 
 void FrameTreeNode::ResetNavigationRequest(bool keep_state,
                                            bool inform_renderer) {
   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();
 
   if (keep_state)
     return;
 
   // The RenderFrameHostManager should clean up any speculative RenderFrameHost
   // it created for the navigation. Also register that the load stopped.
   DidStopLoading();
   render_manager_.CleanUpNavigation();
 
   // When reusing the same SiteInstance, a pending WebUI may have been created
   // on behalf of the navigation in the current RenderFrameHost. Clear it.
   if (site_instance_type ==
       NavigationRequest::AssociatedSiteInstanceType::CURRENT) {
     current_frame_host()->ClearPendingWebUI();
   }
 
   // If the navigation is renderer-initiated, the renderer should also be
   // informed that the navigation stopped if needed. In the case the renderer
   // 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()));
   }
 
 }
 
 bool FrameTreeNode::has_started_loading() const {
   return loading_progress_ != kLoadingProgressNotStarted;
 }
 
 void FrameTreeNode::reset_loading_progress() {
   loading_progress_ = kLoadingProgressNotStarted;
@@ skipping 121 matching lines @@
       }
       return parent_->child_at(i + relative_offset);
     }
   }
 
   NOTREACHED() << "FrameTreeNode not found in its parent's children.";
   return nullptr;
 }
 
 }  // namespace content