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 3013 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 cross-site coreferencing iframes
+// does not cause an infinite number of nested iframes to be created.
+// See https://crbug.com/650332 .

[alexmos, 2017/01/19 23:45:53]

nit: rewrap comment to 80 chars.

Also no space needed after bug number (same for all other places below)

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, CoReferencingFrames) {
+  // Load a page with a cross-site coreferencing iframe. "Coreferencing" here
+  // refers to two separate pages that contain subframes with URLs to each
+  // other.
+  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();
+
+  // The FrameTree contains two successful instances of each site plus an
+  // unsuccessfully-navigated third instance of B with a blank URL.  When not in
+  // site-per-process mode, the FrameTreeVisualizer depicts all nodes as
+  // referencing Site A because iframes are identified with their root site.
+  if (AreAllSitesIsolatedForTesting()) {
+    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 {
+    EXPECT_EQ(
+        " Site A\n"
+        "   +--Site A\n"
+        "        +--Site A\n"
+        "             +--Site A\n"
+        "                  +--Site A\n"
+        "Where A = http://a.com/",
+        FrameTreeVisualizer().DepictFrameTree(root));
+  }
+  FrameTreeNode* bottom_child =
+      root->child_at(0)->child_at(0)->child_at(0)->child_at(0);
+  EXPECT_TRUE(bottom_child->current_url().is_empty());
+  EXPECT_FALSE(bottom_child->has_committed_real_load());
+}
+
+// Ensures that nested subframes with the same URL but different fragments can
+// only be nested once.  See https://crbug.com/650332 .
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       SelfReferencingFragmentFrames) {
+  StartEmbeddedServer();
+  GURL url(
+      embedded_test_server()->GetURL("a.com", "/page_with_iframe.html#123"));
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+  FrameTreeNode* child = root->child_at(0);
+
+  // ExecuteScript is used here and once more below because it is important to
+  // use renderer-initiated navigations since browser-initiated navigations are
+  // bypassed in the self-referencing navigation check.
+  TestFrameNavigationObserver observer1(child);
+  EXPECT_TRUE(
+      ExecuteScript(child, "location.href = '" + url.spec() + "456" + "';"));
+  observer1.Wait();
+
+  FrameTreeNode* grandchild = child->child_at(0);
+

[alexmos, 2017/01/19 23:45:54]

I'd suggest moving up the definition of |expected_url| here, and doing the
EXPECT_EQ(expected_url, grandchild->current_url()) here, in addition to doing it
after the failed navigation below.  This way it's very clear that the grandchild
starts at this URL, and that this URL isn't expected to change.

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  // This navigation should be blocked.
+  GURL stalled_url(embedded_test_server()->GetURL(
+      "a.com", "/page_with_iframe.html#123456789"));
+  TestNavigationManager delayer(web_contents, stalled_url);

[alexmos, 2017/01/19 23:45:54]

Let's not call this "delayer", as we aren't actually delaying any requests here;
maybe just "manager" will do.  Similarly, let's rename |stalled_url| to
something like "blocked_url".  "Stalled" would imply that the request is going
to hang up waiting for a network reply, which isn't what you're doing here.

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  EXPECT_TRUE(ExecuteScript(grandchild,
+                            "location.href = '" + stalled_url.spec() + "';"));
+  EXPECT_FALSE(delayer.WaitForRequestStart());

[alexmos, 2017/01/19 23:45:54]

I'd suggest to add a comment to explain this a bit.  Something like "Wait for
WillStartRequest and verify that it aborts the request before starting it."

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  WaitForLoadStop(web_contents);
+
+  // The FrameTree contains two successful instances of the url plus an
+  // unsuccessfully-navigated third instance with a blank URL.
+  EXPECT_EQ(
+      " Site A\n"
+      "   +--Site A\n"
+      "        +--Site A\n"
+      "Where A = http://a.com/",
+      FrameTreeVisualizer().DepictFrameTree(root));
+
+  GURL expected_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  EXPECT_EQ(expected_url, grandchild->current_url());
+}
+
+// Ensure that loading a page with a meta refresh iframe does not cause an
+// infinite number of nested iframes to be created.  This test loads a page with
+// an about:blank iframe which injects html containing a meta refresh into the

[alexmos, 2017/01/19 23:45:54]

nit: s/which/where the page/  (it's the page injecting the html, not the iframe)

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+// iframe.  This test then checks that this does not cause infinite nested
+// iframes to be created. See https://crbug.com/527367 .
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       SelfReferencingMetaRefreshFrames) {
+  // Load a page with a blank iframe.
+  StartEmbeddedServer();
+  GURL url_1(embedded_test_server()->GetURL(
+      "a.com", "/page_with_meta_refresh_frame.html"));
+  // TODO(davidsac): add in an expect true here for something?

[alexmos, 2017/01/19 23:45:53]

There isn't a good way to expect anything from that helper at the moment. :(  I
think you can remove this TODO - you've got it covered with the two last EXPECTs
below.

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  NavigateToURLBlockUntilNavigationsComplete(shell(), url_1, 3);
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+
+  // The third navigation should fail and be cancelled, leaving a FrameTree with
+  // a height of 2.
+  EXPECT_EQ(
+      " Site A\n"
+      "   +--Site A\n"
+      "        +--Site A\n"
+      "Where A = http://a.com/",
+      FrameTreeVisualizer().DepictFrameTree(root));
+
+  EXPECT_EQ(GURL(url::kAboutBlankURL),
+            root->child_at(0)->child_at(0)->current_url());
+
+  EXPECT_FALSE(root->child_at(0)->child_at(0)->has_committed_real_load());
+}
+
+// Ensure that navigating a subframe to the same url as its parent twice in a

[alexmos, 2017/01/19 23:45:54]

nit: s/url/URL/

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+// row is not blocked by the self-reference check.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       SelfReferencingSameURLRenavigation) {
+  StartEmbeddedServer();
+  GURL url(embedded_test_server()->GetURL("a.com", "/page_with_iframe.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+  FrameTreeNode* child = root->child_at(0);
+
+  TestFrameNavigationObserver observer1(child);

[alexmos, 2017/01/19 23:45:53]

I'd suggest defining url_expected here, perhaps as GURL second_url(url.spec() +
"#123").  Then use it both in ExecuteScript and in the EXPECT_EQ below.  You
could also rename |url| to |first_url| and it will be clear that the child's URL
should've changed from |second_url| back to |first_url|.  (right now this was a
bit hard to follow for me.)

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  EXPECT_TRUE(
+      ExecuteScript(child, "location.href = '" + url.spec() + "#123';"));
+  observer1.Wait();
+
+  EXPECT_EQ(
+      " Site A\n"
+      "   +--Site A\n"
+      "        +--Site A\n"
+      "Where A = http://a.com/",
+      FrameTreeVisualizer().DepictFrameTree(root));
+
+  GURL url_expected(
+      embedded_test_server()->GetURL("a.com", "/page_with_iframe.html#123"));
+  EXPECT_EQ(child->current_url(), url_expected);
+
+  TestFrameNavigationObserver observer2(child);

[alexmos, 2017/01/19 23:45:53]

Perhaps add a comment that this navigation shouldn't be blocked, because the
blocking only happens when more than one ancestor has the same URL (excluding
fragments), and the frame's current URL shouldn't count toward that.

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Done.

+  EXPECT_TRUE(ExecuteScript(child, "location.href = '" + url.spec() + "';"));
+  observer2.Wait();
+
+  // The FrameTree doesn't change between both loads.
+  EXPECT_EQ(
+      " Site A\n"
+      "   +--Site A\n"
+      "        +--Site A\n"
+      "Where A = http://a.com/",
+      FrameTreeVisualizer().DepictFrameTree(root));

[alexmos, 2017/01/19 23:45:53]

I feel like the DepictFrameTree checks in this test aren't adding much here,
given all the coverage you've got for it in other tests.  If the second
navigation were to be incorrectly blocked, the frame tree would look the same
way.  I'd suggest removing both of them here -- just checking that the child
navigated to the URL correctly should be sufficient for what this test is doing.

[davidsac (gone - try alexmos), 2017/01/24 01:16:38]

Good point!  Done.

+
+  EXPECT_EQ(child->current_url(), url);
+}
+
 }  // namespace content