Allow navigations to opener's site to go back into opener SiteInstance.

Previously, when a popup and its opener were cross-site, and neither
site required a dedicated process, navigating the popup back to the
opener's site did not swap processes, so the two ended up in different
processes and couldn't script each other.  Even though
DetermineSiteInstanceForURL contains logic to place same-site popups
into their opener's SiteInstance (added as part of the fix for a
similar issue 796912), this logic was never reached because we never
attempted a process transfer in this case, thanks to
IsRendererTransferNeededForNavigation() returning false.

This CL tweaks IsRendererTransferNeededForNavigation so that a
cross-process transfer is always attempted is such cases.  This allows
DetermineSiteInstanceForURL to always place the popup that's going back
to its opener site into the opener's SiteInstance.

Bug: 807184
Change-Id: Ia4d8d8b2107d3b9e3b9329dd473c06d49b7bd5ec
Reviewed-on: https://chromium-review.googlesource.com/894369Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533410}

content/browser/frame_host/render_frame_host_manager.cc

@@ -1316,6 +1316,8 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
   // isn't a strict invariant but rather a heuristic to avoid unnecessary
   // OOPIFs; see https://crbug.com/711006.  Note that this shouldn't apply to
   // TopDocumentIsolation, so do this after TDI checks above.
+  //
+  // TODO(alexmos): Remove this check after fixing https://crbug.com/787576.
   if (!frame_tree_node_->IsMainFrame()) {
     RenderFrameHostImpl* parent =
         frame_tree_node_->parent()->current_frame_host();
@@ -1382,6 +1384,20 @@ bool RenderFrameHostManager::IsRendererTransferNeededForNavigation(
     return true;
   }
 
+  // If the destination URL is not same-site with current RenderFrameHost and
+  // doesn't require a dedicated process (see above), but it is same-site with
+  // the opener RenderFrameHost, attempt a transfer so that the destination URL
+  // can go back to the opener SiteInstance.  This avoids breaking scripting in
+  // some cases when only a subset of sites is isolated
+  // (https://crbug.com/807184).
+  //
+  // TODO(alexmos): This is a temporary workaround and should be removed after
+  // fixing https://crbug.com/787576.
+  FrameTreeNode* opener = frame_tree_node_->opener();
+  if (opener && IsCurrentlySameSite(opener->current_frame_host(), dest_url) &&
+      opener->current_frame_host()->GetSiteInstance() != rfh->GetSiteInstance())
+    return true;
+
   return false;
 }
 

content/browser/isolated_origin_browsertest.cc

@@ -421,6 +421,77 @@ IN_PROC_BROWSER_TEST_F(IsolatedOriginTest,
   EXPECT_EQ(bar_url2.spec(), popup_location);
 }
 
+// Check that when a non-isolated-origin page opens a popup, navigates it
+// to an isolated origin, and then the popup navigates to a third non-isolated
+// origin and finally back to its opener's origin, the popup and the opener
+// iframe end up in the same process and can script each other:
+//
+//   foo.com
+//      |
+//  window.open()
+//      |
+//      V
+//  about:blank -> isolated.foo.com -> bar.com -> foo.com
+//
+// This is a variant of PopupNavigatesToIsolatedOriginAndBack where the popup
+// navigates to a third site before coming back to the opener's site. See
+// https://crbug.com/807184.
+IN_PROC_BROWSER_TEST_F(IsolatedOriginTest,
+                       PopupNavigatesToIsolatedOriginThenToAnotherSiteAndBack) {
+  // Start on www.foo.com.
+  GURL foo_url(embedded_test_server()->GetURL("www.foo.com", "/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), foo_url));
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+
+  // Open a blank popup.
+  ShellAddedObserver new_shell_observer;
+  EXPECT_TRUE(ExecuteScript(root, "window.w = window.open();"));
+  Shell* new_shell = new_shell_observer.GetShell();
+
+  // Have the opener navigate the popup to an isolated origin.
+  GURL isolated_url(
+      embedded_test_server()->GetURL("isolated.foo.com", "/title1.html"));
+  {
+    TestNavigationManager manager(new_shell->web_contents(), isolated_url);
+    EXPECT_TRUE(ExecuteScript(
+        root, "window.w.location.href = '" + isolated_url.spec() + "';"));
+    manager.WaitForNavigationFinished();
+  }
+
+  // Simulate the isolated origin in the popup navigating to bar.com.
+  GURL bar_url(embedded_test_server()->GetURL("bar.com", "/title2.html"));
+  {
+    TestNavigationManager manager(new_shell->web_contents(), bar_url);
+    EXPECT_TRUE(
+        ExecuteScript(new_shell, "location.href = '" + bar_url.spec() + "';"));
+    manager.WaitForNavigationFinished();
+  }
+
+  // At this point, the popup and the opener should still be in separate
+  // SiteInstances.
+  EXPECT_NE(new_shell->web_contents()->GetMainFrame()->GetSiteInstance(),
+            root->current_frame_host()->GetSiteInstance());
+
+  // Simulate the isolated origin in the popup navigating to www.foo.com.
+  {
+    TestNavigationManager manager(new_shell->web_contents(), foo_url);
+    EXPECT_TRUE(
+        ExecuteScript(new_shell, "location.href = '" + foo_url.spec() + "';"));
+    manager.WaitForNavigationFinished();
+  }
+
+  // The popup should now be in the same SiteInstance as its same-site opener.
+  EXPECT_EQ(new_shell->web_contents()->GetMainFrame()->GetSiteInstance(),
+            root->current_frame_host()->GetSiteInstance());
+
+  // Check that the popup can script the opener.
+  std::string opener_location;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      new_shell, "domAutomationController.send(window.opener.location.href);",
+      &opener_location));
+  EXPECT_EQ(foo_url.spec(), opener_location);
+}
+
 // Check that with an ABA hierarchy, where B is an isolated origin, the root
 // and grandchild frames end up in the same process and can script each other.
 // See https://crbug.com/796912.

content/browser/top_document_isolation_browsertest.cc

@@ -596,20 +596,20 @@ IN_PROC_BROWSER_TEST_F(TopDocumentIsolationTest, PopupAndRedirection) {
       DepictFrameTree(root()));
 
   // The popup redirects itself to the advertiser's website (ad.com).
+  RenderFrameDeletedObserver deleted_observer(popup_root->current_frame_host());
   RendererInitiatedNavigateToURL(popup_root, ad_url);
+  deleted_observer.WaitUntilDeleted();
 
   // This must join its same-site opener, in the default subframe SiteInstance.
   EXPECT_EQ(
-      " Site A ------------ proxies for B C\n"
-      "   +--Site B ------- proxies for A C\n"
+      " Site A ------------ proxies for B\n"
+      "   +--Site B ------- proxies for A\n"
       "Where A = http://page.com/\n"
-      "      B = default subframe process\n"
-      "      C = http://adnetwork.com/",
+      "      B = default subframe process",
       DepictFrameTree(root()));
   EXPECT_EQ(
-      " Site C ------------ proxies for B\n"
-      "Where B = default subframe process\n"
-      "      C = http://adnetwork.com/",
+      " Site B\n"
+      "Where B = default subframe process",
       DepictFrameTree(popup_root));
 }