Do not reset navigation state when BeforeUnload is cancelled by a commit

content/browser/web_contents/web_contents_impl_browsertest.cc

Index: content/browser/web_contents/web_contents_impl_browsertest.cc
diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 61043a399b99f44fa0342c5f1bb3df55eb353612..301360c535b34d434523e17623f7c29b69678836 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/content/browser/web_contents/web_contents_impl_browsertest.cc
@@ -815,4 +815,47 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest, NewNamedWindow) {
   }
 }
 
+// TODO(clamy): Make the test work on Windows and on Mac.
+#if defined(OS_WIN) || defined(OS_MACOSX)

[clamy, 2016/03/29 13:01:50]

Apparently the test has some issues on Windows and MacOS. Since this a fix for a
crash I'd like to land it ASAP, and fix the test for other platforms in a second
patch.

[Charlie Reis, 2016/03/29 18:33:21]

Hmm, this concerns me, because the crash affects those platforms and it's not
clear to me why this would be platform specific.  Is the test going to be flaky
on Linux?

If you're confident it's due to a test-only issue on Windows and Mac, then
please elaborate in the comment a bit and I'll be ok with fixing it in a
followup.

[clamy, 2016/03/30 13:31:11]

From what I see on the logs, there is an issue with the navigation not being
caught. I think it may be due to the way the ShellJavascriptDialogManager is
implemented on Windows and Mac, which is different than on Linux. In particular,
I'm not sure it says proceed by default on those platforms, unlike on Linux.

+#define MAYBE_NoResetOnBeforeUnloadCanceledOnCommit \
+  DISABLED_NoResetOnBeforeUnloadCanceledOnCommit
+#else
+#define MAYBE_NoResetOnBeforeUnloadCanceledOnCommit \
+  NoResetOnBeforeUnloadCanceledOnCommit
+#endif
+// This CL tests that if a BeforeUnload dialog is destroyed due to the commit

[nasko, 2016/03/29 20:19:06]

nit: drop "CL". This is a test case, it just happens to be part of a CL, which
once committed won't exist as a concept.

[clamy, 2016/03/30 13:31:11]

Done. I did meant to write tests don't know what happened there.

+// of a cross-site navigation, it will not reset the loading state.
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       MAYBE_NoResetOnBeforeUnloadCanceledOnCommit) {
+  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));
+
+  // Start a cross-site navigation that will not commit for the moment.
+  NavigationDelayer cross_site_delayer(shell()->web_contents(), kCrossSiteURL);
+  shell()->LoadURL(kCrossSiteURL);
+  cross_site_delayer.WaitForNavigationPaused();
+
+  // Click on a link in the page. This will show the BeforeUnload dialog. Have

[nasko, 2016/03/29 20:19:06]

nit: "Have it hang" doesn't convey why or what hangs? Something along the lines
of "Ensure that dialog is not dismissed, which will cause ... to wait ...".

[clamy, 2016/03/30 13:31:11]

Done.

+  // it hang.
+  // 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.
+  SetProceedByDefaultOnBeforeUnload(shell(), false);
+  EXPECT_TRUE(ExecuteScript(shell()->web_contents(), "clickLinkSoon()"));
+  WaitForAppModalDialog(shell());
+
+  // Have the cross-site navigation commit. The main RenderFrameHost should
+  // still be loading after that.
+  cross_site_delayer.ResumeNavigation();
+  cross_site_delayer.WaitForNavigationFinished();
+  EXPECT_TRUE(shell()->web_contents()->IsLoading());
+}
+
 }  // namespace content