Fix flakiness and enable RenderViewHostManagerTest on Linux/Mac.

content/browser/renderer_host/render_view_host_manager_browsertest.cc

 // Copyright (c) 2011 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 "base/file_util.h"
 #include "base/path_service.h"
 #include "base/ref_counted.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/profiles/profile.h"
@@ skipping 27 matching lines @@
         original_file_path, replacement_text, replacement_path);
   }
 };
 
 // Test for crbug.com/24447.  Following a cross-site link with rel=noreferrer
 // and target=_blank should create a new SiteInstance.
 IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest,
                        SwapProcessWithRelNoreferrerAndTargetBlank) {
   // Start two servers with different sites.
   ASSERT_TRUE(test_server()->Start());
-  net::TestServer https_server_(
+  net::TestServer https_server(
       net::TestServer::TYPE_HTTPS,
       FilePath(FILE_PATH_LITERAL("chrome/test/data")));
-  ASSERT_TRUE(https_server_.Start());
+  ASSERT_TRUE(https_server.Start());
 
   // Load a page with links that open in a new window.
   std::string replacement_path;
   ASSERT_TRUE(GetFilePathWithHostAndPortReplacement(
       "files/click-noreferrer-links.html",
-      https_server_.host_port_pair(),
+      https_server.host_port_pair(),
       &replacement_path));
   ui_test_utils::NavigateToURL(browser(),
                                test_server()->GetURL(replacement_path));
 
   // Get the original SiteInstance for later comparison.
   scoped_refptr<SiteInstance> orig_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_TRUE(orig_site_instance != NULL);
 
   // Test clicking a rel=noreferrer + target=blank link.
   bool success = false;
   EXPECT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
       browser()->GetSelectedTabContents()->render_view_host(), L"",
       L"window.domAutomationController.send(clickNoRefTargetBlankLink());",
       &success));
   EXPECT_TRUE(success);
   // Wait for the cross-site transition to finish.
-  ui_test_utils::WaitForLoadStop(
-      &(browser()->GetSelectedTabContents()->controller()));
+  if (browser()->GetSelectedTabContents()->is_loading())
+    ui_test_utils::WaitForLoadStop(

[Paweł Hajdan Jr., 2011/03/24 17:22:57]

Is it possibly a more general problem with WaitForLoadStop? In other words, does
every client of it has to check is_loading to avoid intermittent problems?

I'm just wondering whether we can put the is_loading() check inside
WaitForLoadStop to make it easy to use and hard to misuse.

[Charlie Reis, 2011/03/24 17:33:55]

Yes, that sounds like a good idea-- I'll give it a try.

The try bots are also being kind today and showing all the various forms of
flakiness here.  :)  The linux try bot just failed on the last line of this
test.  It looks like we need to wait for both the current and newly opened tabs
to finish their loads.  I'll make sure it clears the try servers before I send
you the updated patch.

Thanks!

[Charlie Reis, 2011/03/24 23:15:39]

Fixed.

I had to also check whether the tab has a pending RenderViewHost, since it's
possible that we get here even before the DidStartLoading message is sent from
the renderer (in addition to getting here after the DidStopLoading message is
sent).  That's even more of an argument to put this check in WaitForLoadStop, so
that everyone else doesn't have to worry about it.

+        &(browser()->GetSelectedTabContents()->controller()));
 
   // Opens in new tab.
   EXPECT_EQ(2, browser()->tab_count());
   EXPECT_EQ(1, browser()->selected_index());
-  EXPECT_EQ(L"Title Of Awesomeness",
-      browser()->GetSelectedTabContents()->GetTitle());
+  EXPECT_EQ("/files/title2.html",
+            browser()->GetSelectedTabContents()->GetURL().path());
 
   // Should have a new SiteInstance.
   scoped_refptr<SiteInstance> noref_blank_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_NE(orig_site_instance, noref_blank_site_instance);
 }
 
 // Test for crbug.com/24447.  Following a cross-site link with just
 // target=_blank should not create a new SiteInstance.
-// Disabled, http://crbug.com/67532.
 IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest,
-                       DISABLED_DontSwapProcessWithOnlyTargetBlank) {
+                       DontSwapProcessWithOnlyTargetBlank) {
   // Start two servers with different sites.
   ASSERT_TRUE(test_server()->Start());
-  net::TestServer https_server_(
+  net::TestServer https_server(
       net::TestServer::TYPE_HTTPS,
       FilePath(FILE_PATH_LITERAL("chrome/test/data")));
-  ASSERT_TRUE(https_server_.Start());
+  ASSERT_TRUE(https_server.Start());
 
   // Load a page with links that open in a new window.
   std::string replacement_path;
   ASSERT_TRUE(GetFilePathWithHostAndPortReplacement(
       "files/click-noreferrer-links.html",
-      https_server_.host_port_pair(),
+      https_server.host_port_pair(),
       &replacement_path));
   ui_test_utils::NavigateToURL(browser(),
                                test_server()->GetURL(replacement_path));
 
   // Get the original SiteInstance for later comparison.
   scoped_refptr<SiteInstance> orig_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_TRUE(orig_site_instance != NULL);
 
   // Test clicking a target=blank link.
   bool success = false;
   EXPECT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
       browser()->GetSelectedTabContents()->render_view_host(), L"",
       L"window.domAutomationController.send(clickTargetBlankLink());",
       &success));
   EXPECT_TRUE(success);
   // Wait for the cross-site transition to finish.
-  ui_test_utils::WaitForLoadStop(
-      &(browser()->GetSelectedTabContents()->controller()));
+  if (browser()->GetSelectedTabContents()->is_loading())
+    ui_test_utils::WaitForLoadStop(
+        &(browser()->GetSelectedTabContents()->controller()));
 
   // Opens in new tab.
   EXPECT_EQ(2, browser()->tab_count());
   EXPECT_EQ(1, browser()->selected_index());
-  EXPECT_EQ(L"Title Of Awesomeness",
-      browser()->GetSelectedTabContents()->GetTitle());
+  EXPECT_EQ("/files/title2.html",
+            browser()->GetSelectedTabContents()->GetURL().path());
 
   // Should have the same SiteInstance.
   scoped_refptr<SiteInstance> blank_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_EQ(orig_site_instance, blank_site_instance);
 }
 
 // Test for crbug.com/24447.  Following a cross-site link with rel=noreferrer
 // and no target=_blank should not create a new SiteInstance.
 IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest,
                        DontSwapProcessWithOnlyRelNoreferrer) {
   // Start two servers with different sites.
   ASSERT_TRUE(test_server()->Start());
-  net::TestServer https_server_(
+  net::TestServer https_server(
       net::TestServer::TYPE_HTTPS,
       FilePath(FILE_PATH_LITERAL("chrome/test/data")));
-  ASSERT_TRUE(https_server_.Start());
+  ASSERT_TRUE(https_server.Start());
 
   // Load a page with links that open in a new window.
   std::string replacement_path;
   ASSERT_TRUE(GetFilePathWithHostAndPortReplacement(
       "files/click-noreferrer-links.html",
-      https_server_.host_port_pair(),
+      https_server.host_port_pair(),
       &replacement_path));
   ui_test_utils::NavigateToURL(browser(),
                                test_server()->GetURL(replacement_path));
 
   // Get the original SiteInstance for later comparison.
   scoped_refptr<SiteInstance> orig_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_TRUE(orig_site_instance != NULL);
 
   // Test clicking a rel=noreferrer link.
   bool success = false;
   EXPECT_TRUE(ui_test_utils::ExecuteJavaScriptAndExtractBool(
       browser()->GetSelectedTabContents()->render_view_host(), L"",
       L"window.domAutomationController.send(clickNoRefLink());",
       &success));
   EXPECT_TRUE(success);
   // Wait for the cross-site transition to finish.
-  ui_test_utils::WaitForLoadStop(
-      &(browser()->GetSelectedTabContents()->controller()));
+  if (browser()->GetSelectedTabContents()->is_loading())
+    ui_test_utils::WaitForLoadStop(
+        &(browser()->GetSelectedTabContents()->controller()));
 
   // Opens in same tab.
   EXPECT_EQ(1, browser()->tab_count());
   EXPECT_EQ(0, browser()->selected_index());
-  EXPECT_EQ(L"Title Of Awesomeness",
-      browser()->GetSelectedTabContents()->GetTitle());
+  EXPECT_EQ("/files/title2.html",
+            browser()->GetSelectedTabContents()->GetURL().path());
 
   // Should have the same SiteInstance.
   scoped_refptr<SiteInstance> noref_site_instance(
       browser()->GetSelectedTabContents()->GetSiteInstance());
   EXPECT_EQ(orig_site_instance, noref_site_instance);
 }
 
 // Hangs flakily in Win, http://crbug.com/45040.
 #if defined(OS_WIN)
 #define MAYBE_ChromeURLAfterDownload DISABLED_ChromeURLAfterDownload
@@ skipping 38 matching lines @@
   }
 
   // NotificationObserver
   virtual void Observe(NotificationType type,
                        const NotificationSource& source,
                        const NotificationDetails& details) {
     switch (type.value) {
       case NotificationType::BROWSER_CLOSED:
         MessageLoopForUI::current()->Quit();
         break;
+      default:
+        NOTREACHED();
+        break;
     }
   }
 
  private:
   NotificationRegistrar registrar_;
 };
 
 // Test for crbug.com/12745. This tests that if a download is initiated from
 // a chrome:// page that has registered and onunload handler, the browser
 // will be able to close.
@@ skipping 20 matching lines @@
       &result));
   EXPECT_TRUE(result);
   ui_test_utils::NavigateToURL(browser(), zip_url);
 
   ui_test_utils::WaitForDownloadCount(
     browser()->profile()->GetDownloadManager(), 1);
 
   browser()->CloseWindow();
   BrowserClosedObserver wait_for_close(browser());
 }