Fix Self-Referencing OOPIF Infinite Loop

content/browser/frame_host/render_frame_host_manager_browsertest.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 <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <set>
 
@@ skipping 3012 matching lines @@
   // With OOPIFs, this navigation used a cross-process transfer.  Ensure that
   // the iframe's old RFH still has correct origin, even though it's pending
   // deletion.
   if (AreAllSitesIsolatedForTesting()) {
     EXPECT_FALSE(child_rfh_b->is_active());
     EXPECT_NE(child_rfh_b, child->current_frame_host());
     EXPECT_EQ(url::Origin(url_b), child_rfh_b->GetLastCommittedOrigin());
   }
 }
 
+// Ensure that loading a page with a cross-site coreferencing iframe
+// does not cause an infinite number of nested iframes to be created.
+// TODO(davidsac): This test should be broken until issue 527367 is resolved.

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

I don't see how issue 527367 is related to this test.  That looks like a problem
with <meta> refresh, which this doesn't use.  

It'd be good to follow up on that issue separately and try out their repro to
see if your patch indeed fixes it.  If so, we can just add a test; if not, we
can figure out what's needed beyond this CL and whether we should do it.

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

Done.

+// Also, a decision of how to handle the issue will determine what FrameTree
+// string to expect
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, CoReferencingFrames) {
+  // Load a page with a cross-site coreferencing iframe.
+  StartEmbeddedServer();
+  GURL url_1(
+      embedded_test_server()->GetURL("a.com", "/coreferencingframe_1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), url_1));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+
+  if (AreAllSitesIsolatedForTesting()) {
+    // The Frame Tree contains two successful instances of each site plus an
+    // unsuccessfully-navigated third instance of B with a blank URL.
+    EXPECT_EQ(
+        " Site A ------------ proxies for B\n"
+        "   +--Site B ------- proxies for A\n"
+        "        +--Site A -- proxies for B\n"
+        "             +--Site B -- proxies for A\n"
+        "                  +--Site B -- proxies for A\n"
+        "Where A = http://a.com/\n"
+        "      B = http://b.com/",
+        FrameTreeVisualizer().DepictFrameTree(root));
+  } else {
+    // The Frame Tree contains two successful instances of each site plus an

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

nit: s/Frame Tree/FrameTree/g  (to match class name)

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

Done.

+    // unsuccessfully-navigated third instance of B with a blank URL.  The
+    // FrameTreeVisualizer depicts all nodes as referencing Site A because
+    // iframes

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

nit: unnecessary line break

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

Done.

+    // are identified with their root site when outside of site-per-process
+    // mode.

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

I think you can just put this comment above the if, and remove its other copy in
the other clause.  It explains both if branches pretty well.

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

Done.

+    EXPECT_EQ(
+        " Site A\n"
+        "   +--Site A\n"
+        "        +--Site A\n"
+        "             +--Site A\n"
+        "                  +--Site A\nWhere A = http://a.com/",

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

nit: line break after \n

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

Done.

+        FrameTreeVisualizer().DepictFrameTree(root));
+  }
+
+  EXPECT_EQ(
+      root->child_at(0)->child_at(0)->child_at(0)->child_at(0)->current_url(),

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

You can use GURL::is_empty() for this, along with EXPECT_TRUE.
It might also be nice to check that has_committed_real_load() is false on the
same bottom-most FTN.

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

Done.

+      "");
+}
+
 }  // namespace content