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

[alexmos, 2017/01/06 02:27:29]

nit: s/a cross-site coreferencing iframe/cross-site coreferencing iframes/ 
(since there are lots of them :))

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+// 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, 2017/01/06 02:27:29]

I think we can also remove the TODO, right?  I don't think this test should be
referencing 527367.  Or maybe I'm not understanding what you meant by the TODO?

Instead, I think you can add a reference to https://crbug.com/650332, which is
exactly what the test is for.

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, CoReferencingFrames) {
+  // Load a page with a cross-site coreferencing iframe.

[alexmos, 2017/01/06 02:27:29]

Perhaps add a comment here to clarify what "coreferencing" means.  I.e., the
page's cross-site iframe also has an iframe with its source set to the top
frame's URL.

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+  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.  The

[alexmos, 2017/01/06 02:27:29]

nit: I'd move "when outside of site-per-process mode" to the beginning of the
sentence.

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+  // FrameTreeVisualizer depicts all nodes as referencing Site A because iframes
+  // are identified with their root site when outside of site-per-process mode.
+  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));
+  }
+
+  EXPECT_TRUE(root->child_at(0)

[alexmos, 2017/01/06 02:27:29]

You could declare "FrameTreeNode* bottom_child =
root->child_at(0)->child_at(0)->child_at(0)->child_at(0);" and use in both
places below to save a bunch of lines :)

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+                  ->child_at(0)
+                  ->child_at(0)
+                  ->child_at(0)
+                  ->current_url()
+                  .is_empty());
+
+  EXPECT_FALSE(root->child_at(0)
+                   ->child_at(0)
+                   ->child_at(0)
+                   ->child_at(0)
+                   ->has_committed_real_load());
+}
+
+// Ensure that loading a page with fragments

[alexmos, 2017/01/06 02:27:29]

Finish the comment to explain what the test is doing :)

And similarly to above, reference 650332 and remove the TODO.

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+// TODO(davidsac): This test should be broken until issue 527367 is resolved.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
+                       SelfReferencingFragmentFrames) {
+  StartEmbeddedServer();
+  GURL url(embedded_test_server()->GetURL("a.com", "/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);
+
+  TestFrameNavigationObserver observer1(child);

[alexmos, 2017/01/06 02:27:29]

Let's add a comment that it's important to use renderer-initiated navigations,
as the self-referencing URL check is bypassed for browser-initiated ones.

[davidsac (gone - try alexmos), 2017/01/19 18:26:23]

Done.

+  EXPECT_TRUE(
+      ExecuteScript(child, "location.href = '" + url.spec() + "456" + "';"));
+  observer1.Wait();
+
+  FrameTreeNode* grandchild = child->child_at(0);
+
+  TestFrameNavigationObserver observer2(grandchild);

[alexmos, 2017/01/06 02:27:29]

Add a comment that this navigation should be blocked.

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+  EXPECT_TRUE(ExecuteScript(
+      grandchild, "location.href = '" + url.spec() + "456789" + "';"));
+  observer2.Wait();
+
+  // 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));
+

[alexmos, 2017/01/06 02:27:29]

Can you include an EXPECT_EQ() for the grandchild's URL?  Presumably it should
be about:blank, i.e. GURL(url::kAboutBlankURL)?

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+  EXPECT_FALSE(root->child_at(0)->child_at(0)->has_committed_real_load());
+}
+
+// Ensure that loading a page with a meta refresh iframe

[alexmos, 2017/01/06 02:27:29]

nit: rewrap comment to 80 lines.  Your editor should be able to do this
automatically on the fly for you. :)

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

Done.

+// 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, 2017/01/06 02:27:29]

Instead of TODO, just reference https://crbug.com/527367.  Also, might be nice
to give a bit more detail, something like "This test loads a page with an
about:blank iframe, injects html containing a meta refresh to the iframe, and
ensures that this does not cause..."

[davidsac (gone - try alexmos), 2017/01/19 18:26:23]

Done.

+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MetaRefreshFrames) {

[alexmos, 2017/01/06 02:27:29]

Perhaps add "SelfReferencing" prefix to the test name? so it's clear that this
is related to the other tests?

[davidsac (gone - try alexmos), 2017/01/19 18:26:23]

Done.

+  // Load a page with a cross-site coreferencing 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?
+  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_FALSE(root->child_at(0)->current_url().is_empty());

[alexmos, 2017/01/06 02:27:29]

Instead of this, can we check that current_url() is equal to about:blank?

[davidsac (gone - try alexmos), 2017/01/19 18:26:23]

Done.

+
+  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
+// row is not blocked by the self-reference check.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, SameOriginRenavigation) {
+  StartEmbeddedServer();
+  GURL url(embedded_test_server()->GetURL("a.com", "/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);
+  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", "/iframe.html#123"));
+  EXPECT_EQ(child->current_url(), url_expected);
+
+  TestFrameNavigationObserver observer2(child);

[alexmos, 2017/01/06 02:27:29]

This setup is very similar to the test in SelfReferencingFragmentFrames.  Can we
just move lines 3184-3196 there (before the grandchild checks), and check both
things in one test?  (And then you won't need this test.)

[davidsac (gone - try alexmos), 2017/01/19 18:26:24]

I feel like that would make it a bit confusing what was getting tested. 
Although both tests use URL fragments, this test checks a pretty different bug. 
That being said, I have renamed this test to be a bit more clear that it is
related to the other tests.

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

Acknowledged.

+  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));
+
+  EXPECT_EQ(child->current_url(), url);
+}
+
 }  // namespace content