Fix Self-Referencing OOPIF Infinite Loop

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/navigation_handle_impl.h"
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
 #include "content/browser/appcache/appcache_service_impl.h"
@@ skipping 416 matching lines @@
   if (method_ == "POST")
     resource_request_body_ = resource_request_body;
   sanitized_referrer_ = sanitized_referrer;
   has_user_gesture_ = has_user_gesture;
   transition_ = transition;
   is_external_protocol_ = is_external_protocol;
   request_context_type_ = request_context_type;
   state_ = WILL_SEND_REQUEST;
   complete_callback_ = callback;
 
+  LOG(INFO) << "Will start request";

[alexmos, 2016/12/28 00:25:59]

Don't forget to remove the LOGs, here and below

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+  if (isURLBlocked()) {
+    RunCompleteCallback(NavigationThrottle::CANCEL);
+    return;
+  }
+
   RegisterNavigationThrottles();
 
   if (IsBrowserSideNavigationEnabled())
     navigation_ui_data_ = GetDelegate()->GetNavigationUIData(this);
 
   // Notify each throttle of the request.
   NavigationThrottle::ThrottleCheckResult result = CheckWillStartRequest();
 
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER)
     RunCompleteCallback(result);
 }
 
+bool NavigationHandleImpl::isURLBlocked() {
+  // return false;

[alexmos, 2016/12/28 00:25:59]

Not needed

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+  if (url_.SchemeIs("about"))

[alexmos, 2016/12/28 00:25:59]

Let's keep the comment about excluding about: from the self-reference checks. 
The original one in Blink wasn't very helpful, but I found that it was added in
https://codereview.chromium.org/1410093006, but https://crbug.com/341858.  Let's
add that bug number (and/or some of the description in that CL) to the comment.

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+    return false;
+
+  // We allow one level of self-reference because some sites depend on that,
+  // but we don't allow more than one.
+  bool foundSelfReference = false;

[alexmos, 2016/12/28 00:25:59]

nit: rename this according to Chromium style, i.e., found_self_reference.

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+  for (const FrameTreeNode* node = frame_tree_node_; node;

[alexmos, 2016/12/28 00:25:59]

I think there might be a bug here, in that this should start on
frame_tree_node_'s parent instead.  The original isURLAllowed was called on the
parent frame in HTMLFrameElementBase::isURLAllowed, but this will start from the
actual navigating frame, so the first comparison will be incorrect.

The specific scenario in which this would fail would be if the navigating
frame's current (last committed) URL is the same as the target URL.  Then the
new check might disallow the new navigation (if the URL also occurs in one of
the ancestors) unnecessarily, whereas the previous check would've allowed it. 
It'd be nice to add a test for this.

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+       node = node->parent()) {
+    if (node->current_url() == url_) {

[alexmos, 2016/12/28 00:25:59]

This also doesn't look the same as the old check.  The check in Blink used
equalIgnoringFragmentIdentifier, which ignores the part of the URL after the #,
but this won't do that.  Let's add that functionality along with a test.  (ref()
is the GURL way of getting the part after the #; you can look into
GURL::ReplaceComponents/ClearRef to clear that out in both URLs before
comparison.)

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+      if (foundSelfReference) {
+        LOG(INFO) << "Blocked URL: " << url_;
+        return true;
+      }
+      foundSelfReference = true;
+    }
+  }
+  return false;
+}
+
 void NavigationHandleImpl::WillRedirectRequest(
     const GURL& new_url,
     const std::string& new_method,
     const GURL& new_referrer_url,
     bool new_is_external_protocol,
     scoped_refptr<net::HttpResponseHeaders> response_headers,
     net::HttpResponseInfo::ConnectionInfo connection_info,
     const ThrottleChecksFinishedCallback& callback) {
   // Update the navigation parameters.
   url_ = new_url;
   method_ = new_method;
   sanitized_referrer_.url = new_referrer_url;
   sanitized_referrer_ = Referrer::SanitizeForRequest(url_, sanitized_referrer_);
   is_external_protocol_ = new_is_external_protocol;
   response_headers_ = response_headers;
   connection_info_ = connection_info;
   was_redirected_ = true;
   redirect_chain_.push_back(new_url);
   if (new_method != "POST")
     resource_request_body_ = nullptr;
 
   state_ = WILL_REDIRECT_REQUEST;
   complete_callback_ = callback;
 
+  LOG(INFO) << "Will redirect request";
+  if (isURLBlocked()) {
+    RunCompleteCallback(NavigationThrottle::CANCEL);

[alexmos, 2016/12/28 00:25:59]

Looking at how CheckWill(Start|Redirect)Request works, I wonder if we should
also add a "state_ = CANCELING;" before this, both here in and WillStartRequest.
 It'd be good to double-check with Nasko on this; I'm not sure if it's important
to keep state_ correct for canceling.

[davidsac (gone - try alexmos), 2017/01/06 00:44:57]

Done.

+    return;
+  }
+
   // Notify each throttle of the request.
   NavigationThrottle::ThrottleCheckResult result = CheckWillRedirectRequest();
 
   // If the navigation is not deferred, run the callback.
   if (result != NavigationThrottle::DEFER)
     RunCompleteCallback(result);
 }
 
 void NavigationHandleImpl::WillProcessResponse(
     RenderFrameHostImpl* render_frame_host,
@@ skipping 297 matching lines @@
     throttles_to_register.push_back(std::move(clear_site_data_throttle));
 
   if (throttles_to_register.size() > 0) {
     throttles_.insert(throttles_.begin(), throttles_to_register.begin(),
                       throttles_to_register.end());
     throttles_to_register.weak_clear();
   }
 }
 
 }  // namespace content