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

content/browser/site_per_process_browsertest.cc

 // Copyright (c) 2012 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 "content/browser/site_per_process_browsertest.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 619 matching lines @@
             GetFrameDeviceScaleFactor(web_contents()));
 
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
   FrameTreeNode* child = root->child_at(0);
   EXPECT_EQ(SitePerProcessHighDPIBrowserTest::kDeviceScaleFactor,
             GetFrameDeviceScaleFactor(child));
 }
 
 // Ensure that navigating subframes in --site-per-process mode works and the
 // correct documents are committed.
-#if defined(OS_WIN)
-// This test is flaky on Windows, see https://crbug.com/629419.
-#define MAYBE_CrossSiteIframe DISABLED_CrossSiteIframe
-#else
-#define MAYBE_CrossSiteIframe CrossSiteIframe
-#endif
-IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, MAYBE_CrossSiteIframe) {
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CrossSiteIframe) {
   GURL main_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(a,a(a,a(a)))"));
   NavigateToURL(shell(), main_url);
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
 
   TestNavigationObserver observer(shell()->web_contents());
 
   // Load same-site page into iframe.
   FrameTreeNode* child = root->child_at(0);
   GURL http_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
   NavigateFrameToURL(child, http_url);

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

No need for RenderFrameDeletedObserver in-between this NavigateFrameToURL and
DepictFrameTree?

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

This one is same-site, so there's no deletion of a RenderFrame and thus nothing
to wait for.

   EXPECT_EQ(http_url, observer.last_navigation_url());
   EXPECT_TRUE(observer.last_navigation_succeeded());
   {
     // There should be only one RenderWidgetHost when there are no
     // cross-process iframes.
     std::set<RenderWidgetHostView*> views_set =
         web_contents()->GetRenderWidgetHostViewsInTree();
     EXPECT_EQ(1U, views_set.size());
   }
 
   EXPECT_EQ(
       " Site A\n"
       "   |--Site A\n"
       "   +--Site A\n"
       "        |--Site A\n"
       "        +--Site A\n"
       "             +--Site A\n"
       "Where A = http://a.com/",
       DepictFrameTree(root));
 
   // Load cross-site page into iframe.
   GURL url = embedded_test_server()->GetURL("foo.com", "/title2.html");
-  NavigateFrameToURL(root->child_at(0), url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());

[Charlie Reis, 2016/11/07 18:20:07]

The general rule is that we need this around each cross-process navigation, at
least when followed by DepictFrameTree (but wouldn't hurt to do always).

+    NavigateFrameToURL(root->child_at(0), url);
+    deleted_observer.WaitUntilDeleted();
+  }
   // Verify that the navigation succeeded and the expected URL was loaded.
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
 
   // Ensure that we have created a new process for the subframe.
   ASSERT_EQ(2U, root->child_count());
   SiteInstance* site_instance = child->current_frame_host()->GetSiteInstance();
   RenderViewHost* rvh = child->current_frame_host()->render_view_host();
   RenderProcessHost* rph = child->current_frame_host()->GetProcess();
   EXPECT_NE(shell()->web_contents()->GetRenderViewHost(), rvh);
@@ skipping 23 matching lines @@
       "   +--Site A ------- proxies for B\n"
       "        |--Site A -- proxies for B\n"
       "        +--Site A -- proxies for B\n"
       "             +--Site A -- proxies for B\n"
       "Where A = http://a.com/\n"
       "      B = http://foo.com/",
       DepictFrameTree(root));
 
   // Load another cross-site page into the same iframe.
   url = embedded_test_server()->GetURL("bar.com", "/title3.html");
-  NavigateFrameToURL(root->child_at(0), url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateFrameToURL(root->child_at(0), url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
 
   // Check again that a new process is created and is different from the
   // top level one and the previous one.
   ASSERT_EQ(2U, root->child_count());
   child = root->child_at(0);
   EXPECT_NE(shell()->web_contents()->GetRenderViewHost(),
             child->current_frame_host()->render_view_host());
   EXPECT_NE(rvh, child->current_frame_host()->render_view_host());
@@ skipping 973 matching lines @@
   EXPECT_EQ(
       " Site A ------------ proxies for B C\n"
       "   |--Site B ------- proxies for A C\n"
       "   +--Site C ------- proxies for A B\n"
       "Where A = http://a.com/\n"
       "      B = http://foo.com/ (no process)\n"
       "      C = http://bar.com/",
       DepictFrameTree(root));
 }
 
-#if defined(OS_WIN)
-// This test is flaky on Windows, see https://crbug.com/629419.
-#define MAYBE_NavigateRemoteFrame DISABLED_NavigateRemoteFrame
-#else
-#define MAYBE_NavigateRemoteFrame NavigateRemoteFrame
-#endif
-IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, MAYBE_NavigateRemoteFrame) {
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigateRemoteFrame) {
   GURL main_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(a,a(a,a(a)))"));
   NavigateToURL(shell(), main_url);
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
 
   TestNavigationObserver observer(shell()->web_contents());
 
   // Load same-site page into iframe.
   FrameTreeNode* child = root->child_at(0);
   GURL http_url(embedded_test_server()->GetURL("a.com", "/title1.html"));
   NavigateFrameToURL(child, http_url);

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

I think I understand no need for RenderFrameDeletedObserver here, because we
wait for the *same* child just below.  Still - maybe for consistency it might
also make sense to have a RenderFrameDeletedObserver here?

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

We can't, for the same reason as above-- it would just wait forever, since
there's no RenderFrame deleted for same-site navigations.

   EXPECT_EQ(http_url, observer.last_navigation_url());
   EXPECT_TRUE(observer.last_navigation_succeeded());
 
   // Load cross-site page into iframe.
   GURL url = embedded_test_server()->GetURL("foo.com", "/title2.html");
-  NavigateFrameToURL(root->child_at(0), url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateFrameToURL(root->child_at(0), url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
 
   // Ensure that we have created a new process for the subframe.
   EXPECT_EQ(
       " Site A ------------ proxies for B\n"
       "   |--Site B ------- proxies for A\n"
       "   +--Site A ------- proxies for B\n"
       "        |--Site A -- proxies for B\n"
       "        +--Site A -- proxies for B\n"
       "             +--Site A -- proxies for B\n"
       "Where A = http://a.com/\n"
       "      B = http://foo.com/",
       DepictFrameTree(root));
   SiteInstance* site_instance = child->current_frame_host()->GetSiteInstance();
   EXPECT_NE(shell()->web_contents()->GetSiteInstance(), site_instance);
 
   // Emulate the main frame changing the src of the iframe such that it
   // navigates cross-site.
   url = embedded_test_server()->GetURL("bar.com", "/title3.html");
-  NavigateIframeToURL(shell()->web_contents(), "child-0", url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateIframeToURL(shell()->web_contents(), "child-0", url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
 
   // Check again that a new process is created and is different from the
   // top level one and the previous one.
   EXPECT_EQ(
       " Site A ------------ proxies for C\n"
       "   |--Site C ------- proxies for A\n"
       "   +--Site A ------- proxies for C\n"
       "        |--Site A -- proxies for C\n"
       "        +--Site A -- proxies for C\n"
       "             +--Site A -- proxies for C\n"
       "Where A = http://a.com/\n"
       "      C = http://bar.com/",
       DepictFrameTree(root));
 
   // Navigate back to the parent's origin and ensure we return to the
   // parent's process.
-  NavigateFrameToURL(child, http_url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateFrameToURL(child, http_url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(http_url, observer.last_navigation_url());
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(shell()->web_contents()->GetSiteInstance(),
             child->current_frame_host()->GetSiteInstance());
 }
 
 IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
                        NavigateRemoteFrameToBlankAndDataURLs) {
   GURL main_url(embedded_test_server()->GetURL(
       "a.com", "/cross_site_iframe_factory.html?a(a,a(a))"));
@@ skipping 2445 matching lines @@
   // wait for reply.
   PostMessageAndWaitForReply(child1, "postToSibling('subframe-msg', 2)",
                              "\"done-1-2-name\"");
 
   // Verify the total number of received messages for each subframe.
   EXPECT_EQ(1, GetReceivedMessages(child0));
   EXPECT_EQ(2, GetReceivedMessages(child1));
   EXPECT_EQ(1, GetReceivedMessages(child2));
 }
 
-#if defined(OS_WIN)
-// This test is flaky on Windows, see https://crbug.com/629419.
-#define MAYBE_RFPHDestruction DISABLED_RFPHDestruction
-#else
-#define MAYBE_RFPHDestruction RFPHDestruction
-#endif
-IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, MAYBE_RFPHDestruction) {
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, RFPHDestruction) {
   GURL main_url(embedded_test_server()->GetURL("/site_per_process_main.html"));
   NavigateToURL(shell(), main_url);
 
   // It is safe to obtain the root frame tree node here, as it doesn't change.
   FrameTreeNode* root = web_contents()->GetFrameTree()->root();
 
   TestNavigationObserver observer(shell()->web_contents());
 
   // Load cross-site page into iframe.
   FrameTreeNode* child = root->child_at(0);
   GURL url = embedded_test_server()->GetURL("foo.com", "/title2.html");
-  NavigateFrameToURL(root->child_at(0), url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateFrameToURL(root->child_at(0), url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
   EXPECT_EQ(
       " Site A ------------ proxies for B\n"
       "   |--Site B ------- proxies for A\n"
       "   +--Site A ------- proxies for B\n"
       "        |--Site A -- proxies for B\n"
       "        +--Site A -- proxies for B\n"
       "             +--Site A -- proxies for B\n"
       "Where A = http://127.0.0.1/\n"
       "      B = http://foo.com/",
       DepictFrameTree(root));
 
   // Load another cross-site page.
   url = embedded_test_server()->GetURL("bar.com", "/title3.html");
-  NavigateIframeToURL(shell()->web_contents(), "test", url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    NavigateIframeToURL(shell()->web_contents(), "test", url);
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_TRUE(observer.last_navigation_succeeded());
   EXPECT_EQ(url, observer.last_navigation_url());
   EXPECT_EQ(
       " Site A ------------ proxies for C\n"
       "   |--Site C ------- proxies for A\n"
       "   +--Site A ------- proxies for C\n"
       "        |--Site A -- proxies for C\n"
       "        +--Site A -- proxies for C\n"
       "             +--Site A -- proxies for C\n"
       "Where A = http://127.0.0.1/\n"
       "      C = http://bar.com/",
       DepictFrameTree(root));
 
   // Navigate back to the parent's origin.
-  RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
-  url = embedded_test_server()->GetURL("/title1.html");
-  NavigateFrameToURL(child, url);
+  {
+    RenderFrameDeletedObserver deleted_observer(child->current_frame_host());
+    url = embedded_test_server()->GetURL("/title1.html");
+    NavigateFrameToURL(child, url);
+    // Wait for the old process to exit, to verify that the proxies go away.
+    deleted_observer.WaitUntilDeleted();
+  }
   EXPECT_EQ(url, observer.last_navigation_url());
   EXPECT_TRUE(observer.last_navigation_succeeded());
 
-  // Wait for the old process to exit, to verify that the proxies go away.
-  deleted_observer.WaitUntilDeleted();
   EXPECT_EQ(
       " Site A\n"
       "   |--Site A\n"
       "   +--Site A\n"
       "        |--Site A\n"
       "        +--Site A\n"
       "             +--Site A\n"
       "Where A = http://127.0.0.1/",
       DepictFrameTree(root));
 }
@@ skipping 4208 matching lines @@
   EXPECT_TRUE(NavigateToURL(shell(), b_url));
 
   base::string16 expected_title(base::UTF8ToUTF16("foo"));
   TitleWatcher title_watcher(popup2->web_contents(), expected_title);
   EXPECT_TRUE(ExecuteScript(
       shell(), "window.open('','popup2').postMessage('foo', '*');"));
   EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
 }
 
 }  // namespace content