Skip BeforeUnload on cross-site navigations when there are no handlers

content/browser/web_contents/web_contents_impl_unittest.cc

Index: content/browser/web_contents/web_contents_impl_unittest.cc
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index ab40bcd466f7431fb7b60094387f79825a8ff74e..c12ca7a4656a291b1cef3a5a058ee0e2795489e7 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -489,6 +489,7 @@ TEST_F(WebContentsImplTest, CrossSiteBoundaries) {
   // Keep the number of active frames in orig_rfh's SiteInstance non-zero so
   // that orig_rfh doesn't get deleted when it gets swapped out.
   orig_rfh->GetSiteInstance()->increment_active_frame_count();
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
 
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(orig_rfh->GetRenderViewHost(), contents()->GetRenderViewHost());
@@ -528,6 +529,7 @@ TEST_F(WebContentsImplTest, CrossSiteBoundaries) {
   // non-zero so that orig_rfh doesn't get deleted when it gets
   // swapped out.
   pending_rfh->GetSiteInstance()->increment_active_frame_count();
+  pending_rfh->SendBeforeUnloadHandlersPresent(true);
 
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(pending_rfh, contents()->GetMainFrame());
@@ -731,6 +733,7 @@ TEST_F(WebContentsImplTest, NavigateFromSitelessUrl) {
   // non-zero so that orig_rfh doesn't get deleted when it gets
   // swapped out.
   orig_rfh->GetSiteInstance()->increment_active_frame_count();
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
 
   EXPECT_EQ(orig_instance, contents()->GetSiteInstance());
   EXPECT_TRUE(
@@ -955,9 +958,9 @@ TEST_F(WebContentsImplTest, CrossSiteComparesAgainstCurrentPage) {
 }
 
 // Test that the onbeforeunload and onunload handlers run when navigating
-// across site boundaries.
+// across site boundaries if needed.
 TEST_F(WebContentsImplTest, CrossSiteUnloadHandlers) {
-  TestRenderFrameHost* orig_rfh = contents()->GetMainFrame();
+  TestRenderFrameHost* rfh1 = contents()->GetMainFrame();
   SiteInstance* instance1 = contents()->GetSiteInstance();
 
   // Navigate to URL.  First URL should use first RenderViewHost.
@@ -965,42 +968,61 @@ TEST_F(WebContentsImplTest, CrossSiteUnloadHandlers) {
   controller().LoadURL(
       url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
   contents()->GetMainFrame()->PrepareForCommit(url);
-  contents()->TestDidNavigate(orig_rfh, 1, url, ui::PAGE_TRANSITION_TYPED);
+  contents()->TestDidNavigate(rfh1, 1, url, ui::PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());
-  EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  EXPECT_EQ(rfh1, contents()->GetMainFrame());
+  rfh1->SendBeforeUnloadHandlersPresent(true);
 
   // Navigate to new site, but simulate an onbeforeunload denial.
   const GURL url2("http://www.yahoo.com");
   controller().LoadURL(
       url2, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
-  EXPECT_TRUE(orig_rfh->IsWaitingForBeforeUnloadACK());
+  EXPECT_TRUE(rfh1->IsWaitingForBeforeUnloadACK());
   base::TimeTicks now = base::TimeTicks::Now();
-  orig_rfh->OnMessageReceived(
+  rfh1->OnMessageReceived(
       FrameHostMsg_BeforeUnload_ACK(0, false, now, now));
-  EXPECT_FALSE(orig_rfh->IsWaitingForBeforeUnloadACK());
+  EXPECT_FALSE(rfh1->IsWaitingForBeforeUnloadACK());
   EXPECT_FALSE(contents()->cross_navigation_pending());
-  EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  EXPECT_EQ(rfh1, contents()->GetMainFrame());
 
   // Navigate again, but simulate an onbeforeunload approval.
   controller().LoadURL(
       url2, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
-  EXPECT_TRUE(orig_rfh->IsWaitingForBeforeUnloadACK());
+  EXPECT_TRUE(rfh1->IsWaitingForBeforeUnloadACK());
   now = base::TimeTicks::Now();
-  orig_rfh->PrepareForCommit(url2);
-  EXPECT_FALSE(orig_rfh->IsWaitingForBeforeUnloadACK());
+  rfh1->PrepareForCommit(url2);
+  EXPECT_FALSE(rfh1->IsWaitingForBeforeUnloadACK());
   EXPECT_TRUE(contents()->cross_navigation_pending());
-  TestRenderFrameHost* pending_rfh = contents()->GetPendingMainFrame();
-
-  // We won't hear DidNavigate until the onunload handler has finished running.
+  TestRenderFrameHost* rfh2 = contents()->GetPendingMainFrame();
 
   // DidNavigate from the pending page.
   contents()->TestDidNavigate(
-      pending_rfh, 1, url2, ui::PAGE_TRANSITION_TYPED);
+      rfh2, 1, url2, ui::PAGE_TRANSITION_TYPED);
   SiteInstance* instance2 = contents()->GetSiteInstance();
   EXPECT_FALSE(contents()->cross_navigation_pending());
-  EXPECT_EQ(pending_rfh, contents()->GetMainFrame());
+  EXPECT_EQ(rfh2, contents()->GetMainFrame());
   EXPECT_NE(instance1, instance2);
   EXPECT_TRUE(contents()->GetPendingMainFrame() == NULL);
+

[carlosk, 2015/02/17 12:52:09]

Suggestion: split it here into a new test.

[clamy, 2015/02/18 13:27:23]

I think it makes sense for it to be in this test, which is testing beforeUnload
in general.

+  // Now navigate to a new site. There is no beforeUnload handlers, so we should

[Charlie Reis, 2015/02/17 23:04:31]

nit: There are

[clamy, 2015/02/18 13:27:23]

Done.

+  // not be waiting for a BeforeUnloadACK.
+  const GURL url3("http://www.chromium.org");
+  rfh2->GetSiteInstance()->increment_active_frame_count();
+  controller().LoadURL(
+      url3, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  EXPECT_FALSE(rfh2->IsWaitingForBeforeUnloadACK());
+  rfh2->PrepareForCommit(url3);
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderFrameHost* rfh3 = contents()->GetPendingMainFrame();
+
+  // DidNavigate from the pending page.
+  contents()->TestDidNavigate(
+      rfh3, 1, url3, ui::PAGE_TRANSITION_TYPED);
+  SiteInstance* instance3 = contents()->GetSiteInstance();
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(rfh3, contents()->GetMainFrame());
+  EXPECT_NE(instance2, instance3);
+  EXPECT_TRUE(contents()->GetPendingMainFrame() == NULL);
 }
 
 // Test that during a slow cross-site navigation, the original renderer can
@@ -1018,6 +1040,7 @@ TEST_F(WebContentsImplTest, CrossSiteNavigationPreempted) {
   contents()->TestDidNavigate(orig_rfh, 1, url, ui::PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
 
   // Navigate to new site, simulating an onbeforeunload approval.
   const GURL url2("http://www.yahoo.com");
@@ -1057,6 +1080,7 @@ TEST_F(WebContentsImplTest, CrossSiteNavigationBackPreempted) {
             NavigationEntryImpl::FromNavigationEntry(entry1)->site_instance());
   EXPECT_TRUE(ntp_rfh->GetRenderViewHost()->GetEnabledBindings() &
               BINDINGS_POLICY_WEB_UI);
+  ntp_rfh->SendBeforeUnloadHandlersPresent(true);
 
   // Navigate to new site.
   const GURL url2("http://www.google.com");
@@ -1104,6 +1128,7 @@ TEST_F(WebContentsImplTest, CrossSiteNavigationBackPreempted) {
   EXPECT_EQ(url3, entry3->GetURL());
   EXPECT_EQ(instance3,
             NavigationEntryImpl::FromNavigationEntry(entry3)->site_instance());
+  google_rfh->SendBeforeUnloadHandlersPresent(true);
 
   // Go back within the site.
   controller().GoBack();
@@ -1155,6 +1180,7 @@ TEST_F(WebContentsImplTest, CrossSiteNavigationNotPreemptedByFrame) {
   contents()->TestDidNavigate(orig_rfh, 1, url, ui::PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
 
   // Start navigating to new site.
   const GURL url2("http://www.yahoo.com");
@@ -1185,6 +1211,7 @@ TEST_F(WebContentsImplTest, CrossSiteNotPreemptedDuringBeforeUnload) {
   controller().LoadURL(
       url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
   TestRenderFrameHost* orig_rfh = contents()->GetMainFrame();
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
   EXPECT_FALSE(contents()->cross_navigation_pending());
 
   // Navigate to new site, with the beforeunload request in flight.
@@ -1223,6 +1250,7 @@ TEST_F(WebContentsImplTest, CrossSiteNavigationCanceled) {
   contents()->TestDidNavigate(orig_rfh, 1, url, ui::PAGE_TRANSITION_TYPED);
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(orig_rfh, contents()->GetMainFrame());
+  orig_rfh->SendBeforeUnloadHandlersPresent(true);
 
   // Navigate to new site, simulating an onbeforeunload approval.
   const GURL url2("http://www.yahoo.com");