Don't load subframe history items if a client redirect occurs during load.

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 4940 matching lines @@
             GetRequestBodyForWebURLRequest(info.urlRequest), referrer,
             info.defaultPolicy, info.replacesCurrentHistoryItem, false);
     return blink::WebNavigationPolicyIgnore;  // Suppress the load here.
   }
 
   // In OOPIF-enabled modes, back/forward navigations in newly created subframes
   // should be sent to the browser in case there is a matching
   // FrameNavigationEntry.  If none is found, fall back to the default url.
   if (SiteIsolationPolicy::UseSubframeNavigationEntries() &&
       info.isHistoryNavigationInNewChildFrame && is_content_initiated) {
-    OpenURL(url, IsHttpPost(info.urlRequest),
-            GetRequestBodyForWebURLRequest(info.urlRequest), referrer,
-            info.defaultPolicy, info.replacesCurrentHistoryItem, true);
-    // Suppress the load in Blink but mark the frame as loading.
-    return blink::WebNavigationPolicyHandledByClient;
+    // Don't do this if |info| also says it is a client redirect, in which case
+    // JavaScript on the page is trying to interrupt the history navigation.

[nasko, 2016/08/04 16:00:38]

nit: s/on the page/in a different frame/ to be more precise that it isn't this
specific frame and in general I'd like us to avoid "page" as we are now in frame
granularity almost everywhere.

[Charlie Reis, 2016/08/04 16:46:27]

Will fix in the followup CL with OOPIF test, as you suggested to me offline.

+    if (!info.isClientRedirect) {

[Charlie Reis, 2016/08/04 14:48:46]

Before, we were punting both the original iframe load and the subsequent JS
navigation (which is treated as a client redirect) to the browser via OpenURL in
the new navigation path.  Now we avoid punting the client redirect and let it
proceed, and we'll try to ignore the history item when we get it in
NavigateInternal below.

[Nate Chapin, 2016/08/04 16:40:42]

It surprised me that info.isHistoryNavigationInNewChildFrame is true for the
client redirect. I see why, but that name fooled me :/

[Charlie Reis, 2016/08/04 16:46:27]

Yeah, I was surprised at first, too.  I suppose we could use the
isClientRedirect hint in FrameLoaderClientImpl::decidePolicyForNavigation to
avoid setting that to true, but then we wouldn't know to send the cancel IPC
below.

I'll think about ways to clarify that for the followup.

+      OpenURL(url, IsHttpPost(info.urlRequest),
+              GetRequestBodyForWebURLRequest(info.urlRequest), referrer,
+              info.defaultPolicy, info.replacesCurrentHistoryItem, true);
+      // Suppress the load in Blink but mark the frame as loading.
+      return blink::WebNavigationPolicyHandledByClient;
+    } else {
+      // Client redirects during an initial history load should attempt to
+      // cancel the history navigation.  They will create a provisional document
+      // loader, causing the history load to be ignored in NavigateInternal, and
+      // this IPC will try to cancel any cross-process history load.
+      Send(new FrameHostMsg_CancelInitialHistoryLoad(routing_id_));

[Charlie Reis, 2016/08/04 14:48:46]

This is only necessary if the history item was being sent to a different
process.  We'll try to cancel it before it commits (if possible).

+    }
   }
 
   // Use the frame's original request's URL rather than the document's URL for
   // subsequent checks.  For a popup, the document's URL may become the opener
   // window's URL if the opener has called document.write().
   // See http://crbug.com/93517.
   GURL old_url(frame_->dataSource()->request().url());
 
   // Detect when we're crossing a permission-based boundary (e.g. into or out of
   // an extension or app origin, leaving a WebUI page, etc). We only care about
@@ skipping 585 matching lines @@
         // PageState.
         item_for_history_navigation = entry->root();
         history_load_type = request_params.is_same_document_history_load
                                 ? blink::WebHistorySameDocumentLoad
                                 : blink::WebHistoryDifferentDocumentLoad;
         load_type = request_params.is_history_navigation_in_new_child
                         ? blink::WebFrameLoadType::InitialHistoryLoad
                         : blink::WebFrameLoadType::BackForward;
         should_load_request = true;
 
+        // If this navigation is to a history item for a new child frame, we
+        // should cancel it if we were interrupted by a Javascript navigation
+        // that has already committed or has started a provisional loader.
+        if (request_params.is_history_navigation_in_new_child &&
+            (!current_history_item_.isNull() ||
+             frame_->provisionalDataSource())) {
+          should_load_request = false;
+          has_history_navigation_in_frame = false;
+        }
+
         // Generate the request for the load from the HistoryItem.
         // PlzNavigate: use the data sent by the browser for the url and the
         // HTTP state. The restoration of user state such as scroll position
         // will be done based on the history item during the load.
-        if (!browser_side_navigation) {
+        if (!browser_side_navigation && should_load_request) {
           request = frame_->requestFromHistoryItem(item_for_history_navigation,
                                                    cache_policy);
         }
       }
     }
   } else {
     // Navigate to the given URL.
     if (!start_params.extra_headers.empty() && !browser_side_navigation) {
       for (net::HttpUtil::HeadersIterator i(start_params.extra_headers.begin(),
                                             start_params.extra_headers.end(),
@@ skipping 761 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