Remove existing FrameNavigationEntry when new named frame is added.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index c49846cf376a6df6dada3efe4bbb8b171a98bf92..a438d186dc79646f60340fb0f33326d50f83d0a2 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -5426,4 +5426,149 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, PostViaOpenUrlMsg) {
   EXPECT_EQ("text=value\n", body);
 }
 
+// Tests that inserting a named subframe into the FrameTree clears any
+// previously existing FrameNavigationEntry objects for the same name.
+// See https://crbug.com/628677.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       EnsureFrameNavigationEntriesClearedOnMismatch) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  NavigationControllerImpl& controller = web_contents->GetController();
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+
+  // Start by navigating to a page with complex frame hierarchy.
+  GURL start_url(embedded_test_server()->GetURL("/frame_tree/top.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+  EXPECT_EQ(3U, root->child_count());
+  EXPECT_EQ(2U, root->child_at(0)->child_count());
+
+  NavigationEntryImpl* entry = controller.GetLastCommittedEntry();
+
+  // Verify only the parts of the NavigationEntry affected by this test.
+  {
+    // * Main frame has 3 subframes.
+    FrameNavigationEntry* root_entry = entry->GetFrameEntry(root);
+    EXPECT_NE(nullptr, root_entry);
+    EXPECT_EQ("", root_entry->frame_unique_name());
+    if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+      EXPECT_EQ(3U, entry->root_node()->children.size());
+
+      // * The first child of the main frame is named and has two more children.
+      FrameTreeNode* frame = root->child_at(0);
+      FrameNavigationEntry* frame_entry = entry->GetFrameEntry(frame);
+      EXPECT_NE(nullptr, frame_entry);
+      EXPECT_EQ("1-1-name", frame_entry->frame_unique_name());
+      EXPECT_EQ(2U, entry->root_node()->children[0]->children.size());
+    }
+  }
+
+  // Removing the first child of the main frame should remove the corresponding
+  // FrameTreeNode.
+  std::string remove_frame_script =
+      "var f = document.getElementById('1-1-id');"
+      "f.parentNode.removeChild(f);";
+  EXPECT_TRUE(ExecuteScript(root, remove_frame_script));
+  EXPECT_EQ(2U, root->child_count());
+
+  // However, the FrameNavigationEntry objects for the frame that was removed
+  // should still be around.
+  {
+    FrameNavigationEntry* root_entry = entry->GetFrameEntry(root);
+    EXPECT_NE(nullptr, root_entry);
+    if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+      EXPECT_EQ(3U, entry->root_node()->children.size());
+      EXPECT_EQ(2U, entry->root_node()->children[0]->children.size());
+    }
+  }
+
+  // Now, insert a frame with the same name as the previously removed one
+  // at a different layer of the frame tree.
+  FrameTreeNode* subframe = root->child_at(1)->child_at(1)->child_at(0);
+  EXPECT_EQ(2U, root->child_at(1)->child_count());
+  EXPECT_EQ(0U, subframe->child_count());
+  std::string add_named_frame_script =
+      "var f = document.createElement('iframe');"
+      "f.name = '1-1-name';"
+      "document.body.appendChild(f);";
+  EXPECT_TRUE(ExecuteScript(subframe, add_named_frame_script));
+  EXPECT_EQ(1U, subframe->child_count());
+
+  // Verify that the FrameNavigationEntry for the original frame is now gone.
+  {
+    FrameNavigationEntry* root_entry = entry->GetFrameEntry(root);
+    EXPECT_NE(nullptr, root_entry);
+    if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+      EXPECT_EQ(2U, entry->root_node()->children.size());
+    }
+  }
+}
+
+// Tests that sending a PageState update from a named subframe does not get
+// incorrectly set on previously existing FrameNavigationEntry for the same
+// name. It is similar to EnsureFrameNavigationEntriesClearedOnConflict, but

[Charlie Reis, 2016/08/01 21:04:25]

EnsureFrameNavigationEntriesClearedOnMismatch

[nasko, 2016/08/01 23:05:19]

Done.

+// doesn't navigate the iframes to real URLs when added to the DOM.
+// See https://crbug.com/628677.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       EnsureFrameNavigationEntriesClearedOnMismatchNoSrc) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+
+  GURL start_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+  NavigationEntryImpl* nav_entry =
+      web_contents->GetController().GetLastCommittedEntry();
+
+  std::string add_named_frame_script =
+      "var f = document.createElement('iframe');"
+      "f.name = 'foo';"
+      "document.body.appendChild(f);";
+  EXPECT_TRUE(ExecuteScript(root, add_named_frame_script));
+  EXPECT_EQ(1U, root->child_count());
+  EXPECT_EQ("foo", root->child_at(0)->frame_name());
+
+  std::string remove_frame_script =
+      "var f = document.querySelector('iframe');"
+      "f.parentNode.removeChild(f);";
+  EXPECT_TRUE(ExecuteScript(root, remove_frame_script));
+  EXPECT_EQ(0U, root->child_count());
+
+  // When a frame is removed from the page, the corresponding
+  // FrameNavigationEntry is not removed. This is done intentionally to support
+  // back-forward navigations in subframes and more intuitive UX on tab restore.
+  if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+    EXPECT_EQ(1U, nav_entry->root_node()->children.size());
+    FrameNavigationEntry* frame_entry =
+        nav_entry->root_node()->children[0]->frame_entry.get();
+    EXPECT_EQ("foo", frame_entry->frame_unique_name());
+  }
+
+  std::string add_frame_script =
+      "var f = document.createElement('iframe');"
+      "document.body.appendChild(f);";
+  EXPECT_TRUE(ExecuteScript(root, add_frame_script));
+  EXPECT_EQ(1U, root->child_count());
+  EXPECT_NE("foo", root->child_at(0)->frame_name());
+
+  // Add a nested frame with the previously used name.
+  EXPECT_TRUE(ExecuteScript(root->child_at(0), add_named_frame_script));
+  EXPECT_EQ(1U, root->child_at(0)->child_count());
+  EXPECT_EQ("foo", root->child_at(0)->child_at(0)->frame_name());
+
+  if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+    EXPECT_EQ(1U, nav_entry->root_node()->children.size());
+
+    NavigationEntryImpl::TreeNode* tree_node =
+        nav_entry->root_node()->children[0];
+    EXPECT_EQ(1U, tree_node->children.size());
+
+    tree_node = tree_node->children[0];
+    EXPECT_EQ(0U, tree_node->children.size());
+    EXPECT_EQ("foo", tree_node->frame_entry->frame_unique_name());
+  }
+
+  EXPECT_TRUE(ExecuteScript(root->child_at(0), remove_frame_script));
+  EXPECT_EQ(0U, root->child_at(0)->child_count());
+}
+
 }  // namespace content