Fix DisownOpener and related tests.

content/browser/frame_host/render_frame_host_manager_browsertest.cc

 // Copyright 2013 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 <set>
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
@@ skipping 393 matching lines @@
   EXPECT_TRUE(ExecuteScriptAndExtractBool(
       shell()->web_contents(),
       "window.domAutomationController.send(testCloseWindow());",
       &success));
   EXPECT_TRUE(success);
   close_watcher.Wait();
 }
 
 // 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
 #endif
 IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, MAYBE_DisownOpener) {
   StartServer();
 
   // Load a page with links that open in a new window.
   std::string replacement_path;
   ASSERT_TRUE(GetFilePathWithHostAndPortReplacement(
       "files/click-noreferrer-links.html",
       foo_host_port_,
       &replacement_path));
   NavigateToURL(shell(), test_server()->GetURL(replacement_path));
 
   // Get the original SiteInstance for later comparison.
   scoped_refptr<SiteInstance> orig_site_instance(
       shell()->web_contents()->GetSiteInstance());
   EXPECT_TRUE(orig_site_instance.get() != NULL);
 
   // Test clicking a target=_blank link.
   ShellAddedObserver new_shell_observer;
   bool success = false;
   EXPECT_TRUE(ExecuteScriptAndExtractBool(
       shell()->web_contents(),
       "window.domAutomationController.send(clickSameSiteTargetBlankLink());",
       &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());
   EXPECT_EQ("/files/title2.html",
             new_shell->web_contents()->GetLastCommittedURL().path());
 
   // Should have the same SiteInstance.
   scoped_refptr<SiteInstance> blank_site_instance(
       new_shell->web_contents()->GetSiteInstance());
   EXPECT_EQ(orig_site_instance, blank_site_instance);
 
   // Now navigate the new tab to a different site.
   NavigateToURL(new_shell, GetCrossSiteURL("files/title1.html"));
   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.
   {
     TestNavigationObserver back_nav_load_observer(new_shell->web_contents());
     new_shell->web_contents()->GetController().GoBack();
     back_nav_load_observer.Wait();
   }
   success = false;
   EXPECT_TRUE(ExecuteScriptAndExtractBool(
       new_shell->web_contents(),
       "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"));
   success = false;
   EXPECT_TRUE(ExecuteScriptAndExtractBool(
       new_shell->web_contents(),
       "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.
 IN_PROC_BROWSER_TEST_F(RenderFrameHostManagerTest, DisownSubframeOpener) {
   const GURL frame_url("data:text/html,<iframe name=\"foo\"></iframe>");
   NavigateToURL(shell(), frame_url);
 
   // Give the frame an opener using window.open.
   EXPECT_TRUE(ExecuteScript(shell()->web_contents(),
                             "window.open('about:blank','foo');"));
@@ skipping 958 matching lines @@
   // Navigate to url1 and check bindings.
   NavigateToURL(new_shell, url1);
   // The navigation should have used the first SiteInstance, otherwise
   // |initial_rvh| did not have a chance to be used.
   EXPECT_EQ(new_web_contents->GetSiteInstance(), site_instance1);
   EXPECT_EQ(BINDINGS_POLICY_WEB_UI,
       new_web_contents->GetRenderViewHost()->GetEnabledBindings());
 }
 
 }  // namespace content