Fix Self-Referencing OOPIF Infinite Loop

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index 829ab711cf79ad10ed0b0030d9d537cae1c81b7d..193fec9cb890de8c55c9d7734e4b93af7412f8ae 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -434,6 +434,12 @@ void NavigationHandleImpl::WillStartRequest(
   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())
@@ -447,6 +453,27 @@ void NavigationHandleImpl::WillStartRequest(
     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,
@@ -471,6 +498,12 @@ void NavigationHandleImpl::WillRedirectRequest(
   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();