PlzNavigate: stop navigations when opening a document for write

content/renderer/render_frame_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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 1569 matching lines @@
     IPC_MESSAGE_HANDLER(FrameMsg_SetFrameOwnerProperties,
                         OnSetFrameOwnerProperties)
     IPC_MESSAGE_HANDLER(FrameMsg_AdvanceFocus, OnAdvanceFocus)
     IPC_MESSAGE_HANDLER(FrameMsg_SetFocusedFrame, OnSetFocusedFrame)
     IPC_MESSAGE_HANDLER(FrameMsg_SetTextTrackSettings,
                         OnTextTrackSettingsChanged)
     IPC_MESSAGE_HANDLER(FrameMsg_PostMessageEvent, OnPostMessageEvent)
     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,
                         OnGetSerializedHtmlWithLocalLinks)
     IPC_MESSAGE_HANDLER(FrameMsg_SerializeAsMHTML, OnSerializeAsMHTML)
     IPC_MESSAGE_HANDLER(FrameMsg_Find, OnFind)
     IPC_MESSAGE_HANDLER(FrameMsg_ClearActiveFindMatch, OnClearActiveFindMatch)
     IPC_MESSAGE_HANDLER(FrameMsg_StopFinding, OnStopFinding)
     IPC_MESSAGE_HANDLER(FrameMsg_EnableViewSourceMode, OnEnableViewSourceMode)
     IPC_MESSAGE_HANDLER(FrameMsg_SuppressFurtherDialogs,
@@ skipping 2482 matching lines @@
 
 void RenderFrameImpl::dispatchLoad() {
   Send(new FrameHostMsg_DispatchLoad(routing_id_));
 }
 
 blink::WebEffectiveConnectionType
 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_)
     return;
 
   if (is_empty_selection)
     selection_text_.clear();
 
   // UpdateTextInputState should be called before SyncSelectionIfRequired.
   // UpdateTextInputState may send TextInputStateChanged to notify the focus
@@ skipping 1163 matching lines @@
 
   browser_side_navigation_pending_ = false;
 }
 
 void RenderFrameImpl::OnReportContentSecurityPolicyViolation(
     const content::CSPViolationParams& violation_params) {
   frame_->reportContentSecurityPolicyViolation(
       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,
   // script, drag-n-drop operation, etc.
   // info.extraData is only non-null if this is a redirect. Use the extraData
   // initiation information for redirects, and check pending_navigation_params_
   // otherwise.
   bool is_content_initiated =
       info.extraData
           ? static_cast<DocumentState*>(info.extraData)
@@ skipping 1061 matching lines @@
                     net::LOAD_ONLY_FROM_CACHE | net::LOAD_DISABLE_CACHE);
     load_flags |= net::LOAD_BYPASS_CACHE;
   }
   BeginNavigationParams begin_navigation_params(
       GetWebURLRequestHeaders(info.urlRequest), load_flags,
       info.urlRequest.hasUserGesture(),
       info.urlRequest.getServiceWorkerMode() !=
           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);
     begin_navigation_params.searchable_form_url =
         web_searchable_form_data.url();
     begin_navigation_params.searchable_form_encoding =
         web_searchable_form_data.encoding().utf8();
   }
 
   if (info.isClientRedirect)
@@ skipping 545 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content