PlzNavigate: stop navigations when opening a document for write

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 7ae0fbf22cffbab58631186cabe423a7404f9a6c..ad1fb0ce7286bbbf628f0f2992bdaff05bce4be5 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1587,6 +1587,7 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(FrameMsg_FailedNavigation, OnFailedNavigation)
     IPC_MESSAGE_HANDLER(FrameMsg_ReportContentSecurityPolicyViolation,
                         OnReportContentSecurityPolicyViolation)
+    IPC_MESSAGE_HANDLER(FrameMsg_DroppedNavigation, OnDroppedNavigation)
     IPC_MESSAGE_HANDLER(FrameMsg_GetSavableResourceLinks,
                         OnGetSavableResourceLinks)
     IPC_MESSAGE_HANDLER(FrameMsg_GetSerializedHtmlWithLocalLinks,
@@ -4089,6 +4090,10 @@ RenderFrameImpl::getEffectiveConnectionType() {
   return effective_connection_type_;
 }
 
+void RenderFrameImpl::abortClientNavigation() {
+  Send(new FrameHostMsg_AbortNavigation(routing_id_));
+}
+
 void RenderFrameImpl::didChangeSelection(bool is_empty_selection) {
   if (!GetRenderWidget()->input_handler().handling_input_event() &&
       !handling_select_range_)
@@ -5272,6 +5277,16 @@ void RenderFrameImpl::OnReportContentSecurityPolicyViolation(
       BuildWebContentSecurityPolicyViolation(violation_params));
 }
 
+void RenderFrameImpl::OnDroppedNavigation(int navigation_id) {
+  // Check that the navigation dropped was the last one sent to the browser
+  // process. If not, do nothing as the browser process still has some more
+  // navigations to process.
+  if (navigation_id != navigation_id_)
+    return;
+  browser_side_navigation_pending_ = false;
+  frame_->clientDroppedNavigation();

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

Why does this code need to wait until we hear back from the browser process? 
Could we just do this at the time of abortClientNavigation, knowing that the
navigation will be canceled (or already has been canceled in favor of a
browser-initiated navigation)?

I'm wondering if we can skip the extra IPC back to the renderer, which seems
like it would eliminate any need for navigation_id_ and cut out some of the
races being introduced here.

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

The patch is fixing two issues: the first one is cancelling any ongoing
navigation when we do a document.open. This is what the IPC from the renderer to
the browser is doing. This exposed another more general issue: what happens when
the browser stops processing a navigation that the renderer sent, and that the
renderer is waiting for? This IPC here tackles this second problem, and is not
limited to the document.open case.

When the browser decided not to proceed with a renderer-initiated navigation, we
used to send a Stop IPC. This had the side effect of cancelling any ongoing
load. In particular, if the renderer was not done with the load of the current
page, the load would stop. Not proceeding with a navigation should not cause the
current load to stop, so I had to introduce this new IPC, DroppedNavigation.
However, it was pointed that this could lead to potential races, if the renderer
had issued several navigations.

Let's imagine the following scenario:
- the renderer wants to navigate to foo.com
- the browser decides to drop the navigation
- the renderer wants to navigate to bar.com
- the renderer receives the DroppedNavigation IPC from the navigation to foo.com
and thinks the navigation to bar.com is no longer ongoing
- the browser did in fact go on with the navigation to bar.com

This may lead to bad behavior, such as the load event being fired in the parent
frame when it should not.

In order to avoid this kind of race, Nasko proposed differentiating based on the
urls. However, if we take the example above and replace the second navigation to
bar.com with one to foo.com, we still have the same issue. This is why I
proposed introducing an id, since with the id the race condition goes away.

It may be easier if I split this patch in two: one part to fix the issue with
cancelled renderer-initiated navigations, and one part to fix the issue with
document.open.

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

Right.
Yes, I do like that you're trying to add something here which avoids stopping
all loaders.
In my suggestion, I was hoping that we could skip the DroppedNavigation IPC in
the document.open case because the renderer could assume the navigation would be
canceled and preemptively clear its navigation state.  That way there's no race
with the renderer subsequently starting a new navigation, potentially making
things much simpler to reason about.

I suppose that suggestion won't help in the other cases that the browser
independently decides to drop the navigation, though.
How bad is the issue with the FrameMsg_Stop IPC today?  While I do like the idea
of fixing that, I'm not sure it it's worth adding a navigation ID for.

For your "split the patch" idea, what if we landed the document.open fix first,
doing the preemptive cancel of the renderer's navigation state rather than
having the browser send back an IPC?  Then we could separately decide whether
the issues with the Stop IPC are big enough to justify the ID.  (And you may be
able to convince me that it's worthwhile!  But at least that way it's
independent of the document.open fix.)

+}
+
 WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation(
     const NavigationPolicyInfo& info) {
   // A content initiated navigation may have originated from a link-click,
@@ -6353,7 +6368,7 @@ void RenderFrameImpl::BeginNavigation(const NavigationPolicyInfo& info) {
           blink::WebURLRequest::ServiceWorkerMode::All,
       GetRequestContextTypeForWebURLRequest(info.urlRequest),
       GetMixedContentContextTypeForWebURLRequest(info.urlRequest),
-      initiator_origin);
+      initiator_origin, ++navigation_id_);
 
   if (!info.form.isNull()) {
     WebSearchableFormData web_searchable_form_data(info.form);