How to fix WebContentsImplBrowserTest.NoResetOnBeforeUnloadCanceledOnCommit.

content/browser/web_contents/web_contents_impl_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 "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/pattern.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ skipping 23 matching lines @@
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_resource_dispatcher_host_delegate.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace content {
 
+namespace {
+
 void ResizeWebContentsView(Shell* shell, const gfx::Size& size,
                            bool set_start_page) {
   // Shell::SizeTo is not implemented on Aura; WebContentsView::SizeContents
   // works on Win and ChromeOS but not Linux - we need to resize the shell
   // window on Linux because if we don't, the next layout of the unchanged shell
   // window will resize WebContentsView back to the previous size.
   // SizeContents is a hack and should not be relied on.
 #if defined(OS_MACOSX)
   shell->SizeTo(size);
   // If |set_start_page| is true, start with blank page to make sure resize
@@ skipping 137 matching lines @@
   int loadingStateChangedCount() const { return loadingStateChangedCount_; }
   int loadingStateToDifferentDocumentCount() const {
     return loadingStateToDifferentDocumentCount_;
   }
 
  private:
   int loadingStateChangedCount_;
   int loadingStateToDifferentDocumentCount_;
 };
 
+class TestJavaScriptDialogManager : public JavaScriptDialogManager,
+                                    public WebContentsDelegate {
+ public:
+  TestJavaScriptDialogManager() : message_loop_runner_(new MessageLoopRunner) {}
+  ~TestJavaScriptDialogManager() override {}
+
+  void Wait() {
+    message_loop_runner_->Run();
+    message_loop_runner_ = new MessageLoopRunner;
+  }
+
+  std::string last_message() { return last_message_; }
+  bool did_get_dialog() { return did_get_dialog_; }
+  bool did_get_beforeunload() { return did_get_beforeunload_; }
+
+  // WebContentsDelegate
+
+  JavaScriptDialogManager* GetJavaScriptDialogManager(
+      WebContents* source) override {
+    return this;
+  }
+
+  // JavaScriptDialogManager
+
+  void RunJavaScriptDialog(WebContents* web_contents,
+                           const GURL& origin_url,
+                           JavaScriptDialogType dialog_type,
+                           const base::string16& message_text,
+                           const base::string16& default_prompt_text,
+                           const DialogClosedCallback& callback,
+                           bool* did_suppress_message) override {
+    did_get_dialog_ = true;
+    last_message_ = base::UTF16ToUTF8(message_text);
+    *did_suppress_message = true;
+
+    message_loop_runner_->Quit();
+  };
+
+  void RunBeforeUnloadDialog(WebContents* web_contents,
+                             bool is_reload,
+                             const DialogClosedCallback& callback) override {
+    did_get_beforeunload_ = true;
+
+    message_loop_runner_->Quit();
+  }
+
+  bool HandleJavaScriptDialog(WebContents* web_contents,
+                              bool accept,
+                              const base::string16* prompt_override) override {
+    return true;
+  }
+
+  void CancelDialogs(WebContents* web_contents, bool reset_state) override {}
+
+ private:
+  std::string last_message_;
+
+  bool did_get_dialog_ = false;
+  bool did_get_beforeunload_ = false;
+
+  // The MessageLoopRunner used to spin the message loop.
+  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestJavaScriptDialogManager);
+};
+
+}  // namespace
+
 // Test that DidStopLoading includes the correct URL in the details.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, DidStopLoadingDetails) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
   LoadStopNotificationObserver load_observer(
       &shell()->web_contents()->GetController());
   NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html"));
   load_observer.Wait();
 
   EXPECT_EQ("/title1.html", load_observer.url_.path());
@@ skipping 762 matching lines @@
 
     Shell* new_shell = new_shell_observer.GetShell();
     WaitForLoadStop(new_shell->web_contents());
 
     EXPECT_EQ("foo",
               static_cast<WebContentsImpl*>(new_shell->web_contents())
                   ->GetFrameTree()->root()->frame_name());
   }
 }
 
-// TODO(clamy): Make the test work on Windows and on Mac. On Mac and Windows,
-// there seem to be an issue with the ShellJavascriptDialogManager.
-// Flaky on all platforms: https://crbug.com/655628
 // Test that if a BeforeUnload dialog is destroyed due to the commit of a
 // cross-site navigation, it will not reset the loading state.
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
-                       DISABLED_NoResetOnBeforeUnloadCanceledOnCommit) {
+                       NoResetOnBeforeUnloadCanceledOnCommit) {
+  WebContentsImpl* contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  TestJavaScriptDialogManager dialog_manager;
+  contents->SetDelegate(&dialog_manager);
+
   ASSERT_TRUE(embedded_test_server()->Start());
   const GURL kStartURL(
       embedded_test_server()->GetURL("/hang_before_unload.html"));
   const GURL kCrossSiteURL(
       embedded_test_server()->GetURL("bar.com", "/title1.html"));
 
   // Navigate to a first web page with a BeforeUnload event listener.
   EXPECT_TRUE(NavigateToURL(shell(), kStartURL));
+  // Disable the hang monitor, otherwise there will be a race between the
+  // beforeunload dialog and the beforeunload hang timer.
+  contents->GetMainFrame()->DisableBeforeUnloadHangMonitorForTesting();
 
   // Start a cross-site navigation that will not commit for the moment.
-  TestNavigationManager cross_site_delayer(shell()->web_contents(),
-                                           kCrossSiteURL);
+  TestNavigationManager cross_site_delayer(contents, kCrossSiteURL);
   shell()->LoadURL(kCrossSiteURL);
   EXPECT_TRUE(cross_site_delayer.WaitForRequestStart());
 
   // Click on a link in the page. This will show the BeforeUnload dialog.
   // Ensure the dialog is not dismissed, which will cause it to still be
   // present when the cross-site navigation later commits.
   // Note: the javascript function executed will not do the link click but
   // schedule it for afterwards. Since the BeforeUnload event is synchronous,
   // clicking on the link right away would cause the ExecuteScript to never
   // return.
-  SetShouldProceedOnBeforeUnload(shell(), false);
   EXPECT_TRUE(ExecuteScript(shell(), "clickLinkSoon()"));
-  WaitForAppModalDialog(shell());
+  dialog_manager.Wait();
+  EXPECT_TRUE(dialog_manager.did_get_beforeunload());
 
   // Have the cross-site navigation commit. The main RenderFrameHost should
   // still be loading after that.
   cross_site_delayer.WaitForNavigationFinished();
   EXPECT_TRUE(shell()->web_contents()->IsLoading());
+
+  contents->SetDelegate(nullptr);
+  contents->SetJavaScriptDialogManagerForTesting(nullptr);
 }
 
 namespace {
 void NavigateToDataURLAndExpectBeforeUnload(Shell* shell,
                                             const std::string& html,
                                             bool expect_onbeforeunload) {
   NavigateToURL(shell, GURL("data:text/html," + html));
   RenderFrameHostImpl* rfh =
       static_cast<RenderFrameHostImpl*>(shell->web_contents()->GetMainFrame());
   EXPECT_EQ(expect_onbeforeunload, rfh->ShouldDispatchBeforeUnload());
 }
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, NoOnBeforeUnload) {
   const std::string NO_BEFORE_UNLOAD_HTML = "<html><body>foo</body></html>";
   NavigateToDataURLAndExpectBeforeUnload(shell(), NO_BEFORE_UNLOAD_HTML, false);
 }
 
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, OnBeforeUnload) {
   const std::string BEFORE_UNLOAD_HTML =
       "<html><body><script>window.onbeforeunload=function(e) {}</script>"
       "</body></html>";
   NavigateToDataURLAndExpectBeforeUnload(shell(), BEFORE_UNLOAD_HTML, true);
 }
 
-namespace {
-
-class TestJavaScriptDialogManager : public JavaScriptDialogManager,
-                                    public WebContentsDelegate {
- public:
-  TestJavaScriptDialogManager() : message_loop_runner_(new MessageLoopRunner) {}
-  ~TestJavaScriptDialogManager() override {}
-
-  void Wait() {
-    message_loop_runner_->Run();
-    message_loop_runner_ = new MessageLoopRunner;
-  }
-
-  std::string last_message() { return last_message_; }
-
-  // WebContentsDelegate
-
-  JavaScriptDialogManager* GetJavaScriptDialogManager(
-      WebContents* source) override {
-    return this;
-  }
-
-  // JavaScriptDialogManager
-
-  void RunJavaScriptDialog(WebContents* web_contents,
-                           const GURL& origin_url,
-                           JavaScriptDialogType dialog_type,
-                           const base::string16& message_text,
-                           const base::string16& default_prompt_text,
-                           const DialogClosedCallback& callback,
-                           bool* did_suppress_message) override {
-    last_message_ = base::UTF16ToUTF8(message_text);
-    *did_suppress_message = true;
-
-    message_loop_runner_->Quit();
-  };
-
-  void RunBeforeUnloadDialog(WebContents* web_contents,
-                             bool is_reload,
-                             const DialogClosedCallback& callback) override {}
-
-  bool HandleJavaScriptDialog(WebContents* web_contents,
-                              bool accept,
-                              const base::string16* prompt_override) override {
-    return true;
-  }
-
-  void CancelDialogs(WebContents* web_contents,
-                     bool reset_state) override {}
-
- private:
-  std::string last_message_;
-
-  // The MessageLoopRunner used to spin the message loop.
-  scoped_refptr<MessageLoopRunner> message_loop_runner_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestJavaScriptDialogManager);
-};
-
-}  // namespace
-
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
                        JavaScriptDialogsInMainAndSubframes) {
   WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell()->web_contents());
   TestJavaScriptDialogManager dialog_manager;
   wc->SetDelegate(&dialog_manager);
 
   host_resolver()->AddRule("*", "127.0.0.1");
   ASSERT_TRUE(embedded_test_server()->Start());
 
   NavigateToURL(shell(),
@@ skipping 380 matching lines @@
   ASSERT_TRUE(saw_override);
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&ResourceDispatcherHost::SetDelegate,
                  base::Unretained(ResourceDispatcherHostImpl::Get()),
                  old_delegate));
 }
 
 }  // namespace content