Fix flaky OOPIF tests by waiting for the old RenderFrame to be deleted.

content/browser/top_document_isolation_browsertest.cc

 // Copyright 2016 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 <string>
 
 #include "base/command_line.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/common/content_switches.h"
@@ skipping 260 matching lines @@
   EXPECT_EQ(
       " Site D\n"
       "   |--Site D\n"
       "   |--Site D\n"
       "   |--Site D\n"
       "   +--Site D\n"
       "Where D = http://c.com/",
       DepictFrameTree(root()));
 }
 
-// Flaky. See http://crbug.com/611300.
 IN_PROC_BROWSER_TEST_F(TopDocumentIsolationTest,
-                       DISABLED_NavigateToSubframeSiteWithPopup2) {
+                       NavigateToSubframeSiteWithPopup2) {
   if (content::AreAllSitesIsolatedForTesting())
     return;  // Top Document Isolation is disabled in this mode.
 
   // A(B, C) -> C(A, B), but while a separate C(A) popup exists.
   //
   // This test is constructed so that c.com is the second site to commit in the
   // default subframe SiteInstance, so the default subframe SiteInstance does
   // not have a "c.com" as the value of GetSiteURL().
   GURL abb_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(b, b)"));
 
   NavigateToURL(shell(), abb_url);
 
   EXPECT_EQ(
       " 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 = default subframe process",
       DepictFrameTree(root()));
 
   // A(B, B) -> A(B, C)
   GURL c_url(embedded_test_server()->GetURL(
       "c.com", "/cross_site_iframe_factory.html?c"));
   NavigateFrameToURL(root()->child_at(1), c_url);

[Łukasz Anforowicz, 2016/11/07 22:08:40]

No need for RenderFrameDeletedObserver here (in-between NavigateFrameToURL and
DepictFrameTree below)?

[Charlie Reis, 2016/11/07 22:58:08]

Interestingly, there's no process swap here, since we're in TDI mode.  The main
frame doesn't change, and the subframe stays in the "default subframe process"
despite going from B to C.

 
   EXPECT_EQ(
       " 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 = default subframe process",
       DepictFrameTree(root()));
 
   // This test exercises what happens when the SiteURL of the default subframe
@@ skipping 16 matching lines @@
 
   // The popup must stay with its opener, in the default subframe process.
   EXPECT_EQ(
       " Site B\n"
       "   +--Site B\n"
       "Where B = default subframe process",
       DepictFrameTree(popup_root));
 
   GURL cab_url(embedded_test_server()->GetURL(
       "c.com", "/cross_site_iframe_factory.html?c(a, b)"));
-  NavigateToURL(shell(), cab_url);
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    NavigateToURL(shell(), cab_url);
+    deleted_observer.WaitUntilDeleted();
+  }
 
   // This c.com navigation currently breaks out of the default subframe process,
   // even though that process houses a c.com pop-up.
   EXPECT_EQ(
       " Site C ------------ proxies for B\n"
       "   |--Site B ------- proxies for C\n"
       "   +--Site B ------- proxies for C\n"
       "Where B = default subframe process\n"
       "      C = http://c.com/",
       DepictFrameTree(root()));
 
   // c.com popup should remain where it was, in the subframe process.
   EXPECT_EQ(
       " Site B\n"
       "   +--Site B\n"
       "Where B = default subframe process",
       DepictFrameTree(popup_root));
   EXPECT_EQ(nullptr, popup_root->opener());
 
   // If we navigate the popup to a new site, it ought to transfer processes.
   GURL d_url(embedded_test_server()->GetURL(
       "d.com", "/cross_site_iframe_factory.html?d"));
-  NavigateToURL(popup, d_url);
+  {
+    RenderFrameDeletedObserver deleted_observer(
+        popup_root->current_frame_host());
+    NavigateToURL(popup, d_url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site D ------------ proxies for B\n"
       "Where B = default subframe process\n"
       "      D = http://d.com/",
       DepictFrameTree(popup_root));
-  NavigateToURL(shell(), d_url);
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    NavigateToURL(shell(), d_url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site D\n"
       "Where D = http://d.com/",
       DepictFrameTree(popup_root));
   EXPECT_EQ(
       " Site D\n"
       "Where D = http://d.com/",
       DepictFrameTree(root()));
 }
 
-// Flaky on Mac. See https://crbug.com/611344.
-#if defined(OS_MACOSX)
-#define MAYBE_FramesForSitesInHistory DISABLED_FramesForSitesInHistory
-#else
-#define MAYBE_FramesForSitesInHistory FramesForSitesInHistory
-#endif
-
-IN_PROC_BROWSER_TEST_F(TopDocumentIsolationTest,
-                       MAYBE_FramesForSitesInHistory) {
+IN_PROC_BROWSER_TEST_F(TopDocumentIsolationTest, FramesForSitesInHistory) {
   if (content::AreAllSitesIsolatedForTesting())
     return;  // Top Document Isolation is disabled in this mode.
 
   // First, do a series of navigations.
   GURL a_url = embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a");
   GURL b_url = embedded_test_server()->GetURL(
       "b.com", "/cross_site_iframe_factory.html?b");
   GURL c_url = embedded_test_server()->GetURL(
       "c.com", "/cross_site_iframe_factory.html?c");
 
   // Browser-initiated navigation to a.com.
   NavigateToURL(shell(), a_url);
   EXPECT_EQ(
       " Site A\n"
       "Where A = http://a.com/",
       DepictFrameTree(root()));
 
   // Browser-initiated navigation to b.com.
-  NavigateToURL(shell(), b_url);
+  {
+    // For any cross-process navigations, we must wait for the old RenderFrame
+    // to be deleted before calling DepictFrameTree, or else there's a chance
+    // the old SiteInstance could be listed while pending deletion.
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    NavigateToURL(shell(), b_url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site B\n"
       "Where B = http://b.com/",
       DepictFrameTree(root()));
 
   // Renderer-initiated navigation back to a.com. This shouldn't swap processes.
   RendererInitiatedNavigateToURL(root(), a_url);
   EXPECT_EQ(
       " Site B\n"
       "Where B = http://b.com/",
       DepictFrameTree(root()));
 
   // Browser-initiated navigation to c.com.
-  NavigateToURL(shell(), c_url);
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    NavigateToURL(shell(), c_url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site C\n"
       "Where C = http://c.com/",
       DepictFrameTree(root()));
 
   // Now, navigate to a fourth site with iframes to the sites in the history.
-  NavigateToURL(shell(),
-                embedded_test_server()->GetURL(
-                    "d.com", "/cross_site_iframe_factory.html?d(a,b,c)"));
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    NavigateToURL(shell(),
+                  embedded_test_server()->GetURL(
+                      "d.com", "/cross_site_iframe_factory.html?d(a,b,c)"));
+    deleted_observer.WaitUntilDeleted();
+  }
 
   EXPECT_EQ(
       " Site D ------------ proxies for E\n"
       "   |--Site E ------- proxies for D\n"
       "   |--Site E ------- proxies for D\n"
       "   +--Site E ------- proxies for D\n"
       "Where D = http://d.com/\n"
       "      E = default subframe process",
       DepictFrameTree(root()));
 
   // Now try going back.
-  GoBack();
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    GoBack();
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site C\n"
       "Where C = http://c.com/",
       DepictFrameTree(root()));
-  GoBack();
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    GoBack();
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site B\n"
       "Where B = http://b.com/",
       DepictFrameTree(root()));
   GoBack();
   EXPECT_EQ(
       " Site B\n"
       "Where B = http://b.com/",
       DepictFrameTree(root()));
-  GoBack();
+  {
+    RenderFrameDeletedObserver deleted_observer(root()->current_frame_host());
+    GoBack();
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(
       " Site A\n"
       "Where A = http://a.com/",
       DepictFrameTree(root()));
 }
 
 IN_PROC_BROWSER_TEST_F(TopDocumentIsolationTest, CrossSiteAtLevelTwo) {
   if (content::AreAllSitesIsolatedForTesting())
     return;  // Top Document Isolation is disabled in this mode.
 
@@ skipping 97 matching lines @@
       "      C = http://adnetwork.com/",
       DepictFrameTree(root()));
   EXPECT_EQ(
       " Site C ------------ proxies for B\n"
       "Where B = default subframe process\n"
       "      C = http://adnetwork.com/",
       DepictFrameTree(popup_root));
 }
 
 }  // namespace content