OOPIF: Fix subframe back/forward after recreating FTNs.

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 a6aef93555b1e3c66be8b3febefcc3bca5261bc6..ad46cf541d70aae42dcb59321520086d26f7eae1 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -729,7 +729,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   // ... and replace it with a failed load.
   // TODO(creis): Make this be NEW_PAGE along with the other location.replace
   // cases.  There isn't much impact to having this be EXISTING_PAGE for now.
-  // See https://crbug.com/317872.
+  // See https://crbug.com/596707.
   {
     FrameNavigateParamsCapturer capturer(root);
     NavigateToURLAndReplace(shell(), error_url);
@@ -972,7 +972,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   {
     // location.replace().
     // TODO(creis): Change this to be NEW_PAGE with replacement in
-    // https://crbug.com/317872.
+    // https://crbug.com/596707.
     FrameNavigateParamsCapturer capturer(root);
     GURL frame_url(embedded_test_server()->GetURL(
         "/navigation_controller/simple_page_1.html"));
@@ -2285,6 +2285,9 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
     EXPECT_EQ(0U, entry4->root_node()->children.size());
   }
 
+  // Inject a JS value so that we can check for it later.
+  EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), "foo=3;"));
+
   // 7. Go back again, to the data URL in the nested iframe.
   {
     TestNavigationObserver back_load_observer(shell()->web_contents());
@@ -2316,6 +2319,15 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
     EXPECT_EQ(0U, entry3->root_node()->children.size());
   }
 
+  // Verify that we did not reload the main frame. See https://crbug.com/586234.
+  {
+    int value = 0;
+    EXPECT_TRUE(ExecuteScriptAndExtractInt(root->current_frame_host(),
+                                           "domAutomationController.send(foo)",
+                                           &value));
+    EXPECT_EQ(3, value);
+  }
+
   // 8. Go back again, to the data URL in the first subframe.
   {
     TestNavigationObserver back_load_observer(shell()->web_contents());
@@ -2711,6 +2723,73 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_EQ(named_subframe_name, foo_subframe_entry->frame_unique_name());
 }
 
+// Ensures that FrameNavigationEntries for dynamically added iframes can be
+// found correctly.  If we don't find them based on unique name in
+// AddOrUpdateEntry, we'll end up using the old entry (with a PageState) during

[alexmos, 2016/04/06 17:14:22]

nit: s/AddOrUpdateEntry/AddOrUpdateFrameEntry/ ?

[Charlie Reis, 2016/04/12 16:25:51]

Done.

+// the transfer, causing the renderer to crash.  See https://crbug.com/568768.

[alexmos, 2016/04/06 17:14:22]

In 568768, the second problem was "restoring the slashdot.org history item
multiple times before load stop" - is that covered somehow by this fix/test, or
is it a separate issue?

[alexmos, 2016/04/06 17:14:22]

I'm not sure I fully understand this bit: "we'll end up using the old entry
(with a PageState) during the transfer, causing the renderer to crash" - out of
curiosity, where would the renderer crash?  Is there an explicit way to check
that the subframe's FNE was found correctly at the end of the test?

[Charlie Reis, 2016/04/12 16:25:51]

Ah, yes, that comment could have been clearer.  I've updated it to make it more
explicit about what was going wrong.  The failures were in patch set 3 in the
following tests:
PasswordManagerBrowserTestBase.CrossSiteIframeNotFillTest
BrowserViewTest.AvoidUnnecessaryVisibilityChanges
ServiceWorkerTest.WebAccessibleResourcesIframeSrc

Unfortunately, I couldn't find an easy way to verify the specific problem of
duplicating the FNE in the test.  It would almost be possible to clone the last
committed entry and call AddOrUpdateFrameEntry on it, but that requires the
FrameTreeNode to be created first, which leads to the transfer.  Instead, I'm
just letting the test crash during the transfer if it fails.

[Charlie Reis, 2016/04/12 16:25:51]

I'm not able to repro that part of the bug anymore without my fix.  I think it
might have been fixed by some other change in the interim.

My guess is that we were treating further button clicks as part of the
back/forward navigation (since load stop hadn't happened yet), and now we don't.

[alexmos, 2016/04/12 18:55:40]

Acknowledged.  The new comment is great.

[alexmos, 2016/04/12 18:55:40]

Acknowledged.

+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       FrameNavigationEntry_RepeatCreatedFrame) {
+  const NavigationControllerImpl& controller =
+      static_cast<const NavigationControllerImpl&>(
+          shell()->web_contents()->GetController());
+
+  // 1. Navigate the main frame.
+  GURL url(embedded_test_server()->GetURL(
+      "/navigation_controller/page_with_links.html"));
+  NavigateToURL(shell(), url);

[alexmos, 2016/04/06 17:14:22]

nit: EXPECT_TRUE

[Charlie Reis, 2016/04/12 16:25:51]

Done.

+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  SiteInstance* main_site_instance =
+      root->current_frame_host()->GetSiteInstance();
+
+  // 2. Add a cross-site subframe.
+  GURL frame_url(embedded_test_server()->GetURL(
+      "foo.com", "/navigation_controller/simple_page_1.html"));
+  std::string script = "var iframe = document.createElement('iframe');"
+                       "iframe.src = '" + frame_url.spec() + "';"
+                       "document.body.appendChild(iframe);";
+  {
+    LoadCommittedCapturer capturer(shell()->web_contents());
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
+  }
+
+  FrameTreeNode* subframe = root->child_at(0);
+  if (AreAllSitesIsolatedForTesting()) {
+    EXPECT_NE(main_site_instance,
+              subframe->current_frame_host()->GetSiteInstance());
+  }
+  if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
+    FrameNavigationEntry* subframe_entry =
+        controller.GetLastCommittedEntry()->GetFrameEntry(subframe);
+    EXPECT_EQ(frame_url, subframe_entry->url());
+  }
+
+  // 3. Reload the main frame.
+  {
+    FrameNavigateParamsCapturer capturer(root);
+    shell()->web_contents()->GetController().Reload(false);
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_RELOAD, capturer.params().transition);
+    EXPECT_EQ(NAVIGATION_TYPE_EXISTING_PAGE, capturer.details().type);
+    EXPECT_FALSE(capturer.details().is_in_page);
+  }
+
+  // 4. Add the iframe again.
+  {
+    LoadCommittedCapturer capturer(shell()->web_contents());
+    EXPECT_TRUE(content::ExecuteScript(root->current_frame_host(), script));
+    capturer.Wait();
+    EXPECT_EQ(ui::PAGE_TRANSITION_AUTO_SUBFRAME, capturer.transition_type());
+  }
+  if (AreAllSitesIsolatedForTesting()) {
+    EXPECT_NE(main_site_instance,
+              root->child_at(0)->current_frame_host()->GetSiteInstance());
+  }
+}
+
 // Verifies that item sequence numbers and document sequence numbers update
 // properly for main frames and subframes.
 IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,