Fix DisownOpener and related tests.

content/browser/frame_host/render_frame_host_manager_browsertest.cc

Index: content/browser/frame_host/render_frame_host_manager_browsertest.cc
diff --git a/content/browser/frame_host/render_frame_host_manager_browsertest.cc b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
index 3f96c31ce6889edeb07ff5b40d44da492999f12f..5b7f191219f7bcd03e04cad981d8523d23fd9645 100644
--- a/content/browser/frame_host/render_frame_host_manager_browsertest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_browsertest.cc
@@ -411,10 +411,8 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest,
 
 // Test that setting the opener to null in a window affects cross-process
 // navigations, including those to existing entries.  http://crbug.com/156669.
-// Flaky on android: http://crbug.com/397185
-// Flaky on windows and mac: http://crbug.com/291249
-// This test also crashes under ThreadSanitizer, http://crbug.com/356758.
-#if defined(OS_ANDROID) || defined(OS_WIN) || defined(THREAD_SANITIZER) || defined(OS_MACOSX)
+// This test crashes under ThreadSanitizer, http://crbug.com/356758.
+#if defined(THREAD_SANITIZER)

[Charlie Reis, 2014/08/22 19:41:23]

I don't think I've fixed the ThreadSanitizer issue.  I could re-enable just to
see how it goes, or I could leave it disabled there.

 #define MAYBE_DisownOpener DISABLED_DisownOpener
 #else
 #define MAYBE_DisownOpener DisownOpener
@@ -444,6 +442,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MAYBE_DisownOpener) {
       &success));
   EXPECT_TRUE(success);
   Shell* new_shell = new_shell_observer.GetShell();
+  EXPECT_TRUE(new_shell->web_contents()->HasOpener());
 
   // Wait for the navigation in the new tab to finish, if it hasn't.
   WaitForLoadStop(new_shell->web_contents());
@@ -460,10 +459,12 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MAYBE_DisownOpener) {
   scoped_refptr<SiteInstance> new_site_instance(
       new_shell->web_contents()->GetSiteInstance());
   EXPECT_NE(orig_site_instance, new_site_instance);
+  EXPECT_TRUE(new_shell->web_contents()->HasOpener());
 
   // Now disown the opener.
   EXPECT_TRUE(ExecuteScript(new_shell->web_contents(),
                             "window.opener = null;"));
+  EXPECT_FALSE(new_shell->web_contents()->HasOpener());

[Charlie Reis, 2014/08/22 19:41:23]

I'm still not sure why the original test was failing the way it was.  The
IsSwappedOut check could have failed if we tried to notify the pending RVH
(e.g., before we moved to RenderFrameProxyHosts), but I think the ExecuteScript
line will wait for the opener to be cleared before we move on to the back
navigation below.

(Reasoning: the DidDisownOpener IPC will be sent by the renderer before the
DomAutomationController reply.)

I've removed the IsSwappedOut check, but there's a chance the test will fail
this new expectation if there's still a race.

[nasko, 2014/08/22 20:24:34]

ExecuteScript waits until the script is executed before returning, due to how it
is implemented (waiting for the DomAutomation to send a message back). I expect
we get a didDisownOpener synchronously as part of the script exectution, hence
the IPC comes before the ExecuteScript has returned, which allows the DidDisown
IPC to be processed.

 
   // Go back and ensure the opener is still null.
   {
@@ -477,6 +478,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MAYBE_DisownOpener) {
       "window.domAutomationController.send(window.opener == null);",
       &success));
   EXPECT_TRUE(success);
+  EXPECT_FALSE(new_shell->web_contents()->HasOpener());
 
   // Now navigate forward again (creating a new process) and check opener.
   NavigateToURL(new_shell, GetCrossSiteURL("files/title1.html"));
@@ -486,6 +488,7 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MAYBE_DisownOpener) {
       "window.domAutomationController.send(window.opener == null);",
       &success));
   EXPECT_TRUE(success);
+  EXPECT_FALSE(new_shell->web_contents()->HasOpener());
 }
 
 // Test that subframes can disown their openers.  http://crbug.com/225528.