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 9aacb49b0e91ef4143019bde521851eae795c131..1b4a48a9ec16b48d3c52fa57d853d75db51105b6 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -5397,4 +5397,120 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest, PostViaOpenUrlMsg) {
   EXPECT_EQ("text=value\n", body);
 }
 
+// Tests that sending a PageState update from a named subframe does not get
+// incorrectly set on previously existing FrameNavigationEntry for the same
+// name. See https://crbug.com/628677.
+// TODO(nasko): Move this to NavigationControllerBrowserTest once the default
+// for UseSubframeNavigationEntries is set to true.

[Charlie Reis, 2016/07/27 22:48:27]

Interesting.  I've just been making them normal tests and making sure they pass
on the Site Isolation Linux/Win FYI bots, which has the advantage that we can
verify the test passes in default Chrome as well (possibly skipping any subframe
FNE validation if needed).  Maybe that's worth doing here?

[nasko, 2016/07/29 15:52:21]

Sure. Makes sense. I was trying to ensure they run as expected on the default
bots.

+IN_PROC_BROWSER_TEST_F(NavigationControllerOopifBrowserTest,
+                       PageStateMismatch) {
+  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));
+
+  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());

[Charlie Reis, 2016/07/27 22:48:27]

Can you add an expectation after this that we still have a FrameNavigationEntry
for "foo" in the NavigationEntry?  (Only in UseSubframeNavigationEntries() mode,
if you take my suggestion above.)  Would help to mention the reasoning in a
comment, like what you have in the CL description.

[nasko, 2016/07/29 15:52:21]

Done.

+
+  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());
+
+  EXPECT_TRUE(ExecuteScript(root->child_at(0), add_named_frame_script));

[Charlie Reis, 2016/07/27 22:48:27]

// Add a nested frame with the previously used name.

[nasko, 2016/07/29 15:52:19]

Done.

+  EXPECT_EQ(1U, root->child_at(0)->child_count());
+  EXPECT_EQ("foo", root->child_at(0)->child_at(0)->frame_name());

[Charlie Reis, 2016/07/27 22:48:27]

Let's verify the tree of FrameNavigationEntries here, too (including the count
of children of the root FNE, so that we can tell the old one has been removed).

[nasko, 2016/07/29 15:52:21]

Done.

+
+  EXPECT_TRUE(ExecuteScript(root->child_at(0), remove_frame_script));
+  EXPECT_EQ(0U, root->child_at(0)->child_count());

[Charlie Reis, 2016/07/27 22:48:27]

I assume this crashed due to your NOTREACHED?

[nasko, 2016/07/29 15:52:19]

Indeed!

+}
+
+// Tests that inserting a named subframe into the FrameTree clears any
+// previously existing FrameNavigationEntry objects for the same name.
+// See https://crbug.com/628677.
+// TODO(nasko): Move this to NavigationControllerBrowserTest once the default
+// for UseSubframeNavigationEntries is set to true.
+IN_PROC_BROWSER_TEST_F(NavigationControllerOopifBrowserTest,
+                       EnsureFrameNavigationEntriesClearedOnNameConflict) {

[Charlie Reis, 2016/07/27 22:48:27]

This seems better documented than the one above, and has a lot of overlap.  Does
the first one cover a case that this doesn't?  I'm wondering if we can get by
with just this one.

[nasko, 2016/07/29 15:52:20]

This one actually navigates the iframes to real URLs whereas the other test
leaves them at about:blank. If you think this is a difference that isn't
significant, I'm ok with removing the other test case. I was just thinking that
initial empty document might have different behaviors, but they both should emit
a DidCommitProvisionalLoad IPC, so maybe the difference is not material.

[Charlie Reis, 2016/07/29 17:21:35]

Ah, that didn't come across from the comments or test names.  Since this one is
better documented, I'd suggest listing it first, and then describe the other one
as similar but with no src URLs.

Now that you mention it, there's actually 3 cases: real URL, no-src, and slow
URL.  The first two behave more or less the same, in that we normally get a
commit and won't get an UpdateState until after that.  I'm ok with having
separate tests for them.  In the slow URL case, we could see an UpdateState
before the first commit (e.g., if we put text into a form input and then wait a
second).  Before your fix, that UpdateState could end up on a stale FNE, and now
it should be fixed.

I wonder if it's worth having a test for that case as well?  The tricky part is
waiting long enough for the UpdateState message to arrive, since I don't think
we have anything to wait for.

[nasko, 2016/08/01 20:42:15]

Reordered and renamed to match a bit better.
I can try to write such a test. Maybe using a BrowserMessageFilter can help,
since I will be able to signal to the test when the UpdateState IPC has been
received. Are you ok with it being in a separate CL so we can have this landed
without waiting for that test?

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

Yes.  I'm curious how that works out, since I'd love to use such a thing in a
few other tests.

+  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());
+    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);
+    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);
+    EXPECT_EQ(2U, entry->root_node()->children.size());
+  }
+}
+
 }  // namespace content