Followup comments from r460581.
BUG=365039, 705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2781383002
Cr-Commit-Position: refs/heads/master@{#461012}
diff --git a/content/browser/browser_side_navigation_browsertest.cc b/content/browser/browser_side_navigation_browsertest.cc
index 05e0195f..c7b3109 100644
--- a/content/browser/browser_side_navigation_browsertest.cc
+++ b/content/browser/browser_side_navigation_browsertest.cc
@@ -282,9 +282,8 @@
content::WindowedNotificationObserver close_observer(
content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
content::Source<content::WebContents>(shell()->web_contents()));
- GURL url(
- "data:text/html,<html><script>window.onbeforeunload=function(e)"
- "{}</script></html>");
+ GURL url =
+ embedded_test_server()->GetURL("/page_with_empty_beforeunload.html");
NavigateToURL(shell(), url);
shell()->LoadURL(GURL("chrome://resources/css/tabs.css"));
shell()->web_contents()->DispatchBeforeUnload();
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 7164463..b18b60c4 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -2732,7 +2732,7 @@
return false;
}
-bool RenderFrameHostImpl::ShouldDispatchUnload() {
+bool RenderFrameHostImpl::HasUnloadHandler() {
if (!IsRenderFrameLive())
return false;
diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h
index f3f7eee..72a51ef 100644
--- a/content/browser/frame_host/render_frame_host_impl.h
+++ b/content/browser/frame_host/render_frame_host_impl.h
@@ -433,7 +433,7 @@
// Returns true if the frame or any of its descendents have an onunload
// handler.
- bool ShouldDispatchUnload();
+ bool HasUnloadHandler();
// Update the frame's opener in the renderer process in response to the
// opener being modified (e.g., with window.open or being set to null) in
diff --git a/content/browser/frame_host/render_frame_host_manager_unittest.cc b/content/browser/frame_host/render_frame_host_manager_unittest.cc
index de19b320..b82b68e 100644
--- a/content/browser/frame_host/render_frame_host_manager_unittest.cc
+++ b/content/browser/frame_host/render_frame_host_manager_unittest.cc
@@ -1264,7 +1264,6 @@
// happen, but we have seen it when going back quickly across many entries
// (http://crbug.com/93427).
contents()->GetController().GoBack();
- contents()->GetMainFrame()->PrepareForCommit();
// The back navigation commits.
const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 1f5657a..848cd7d 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1421,10 +1421,12 @@
bool WebContentsImpl::NeedToFireBeforeUnload() {
// TODO(creis): Should we fire even for interstitial pages?
+ // TODO(nasko): it's confusing that this method, per comments and tests that
+ // depend on this behavior, needs to check unload handlers as well.
return WillNotifyDisconnection() && !ShowingInterstitialPage() &&
!GetRenderViewHost()->SuddenTerminationAllowed() &&
(GetMainFrame()->ShouldDispatchBeforeUnload() ||
- GetMainFrame()->ShouldDispatchUnload());
+ GetMainFrame()->HasUnloadHandler());
}
void WebContentsImpl::DispatchBeforeUnload() {
diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 92487362..a1b0f2c 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/content/browser/web_contents/web_contents_impl_browsertest.cc
@@ -1019,27 +1019,23 @@
EXPECT_TRUE(shell()->web_contents()->IsLoading());
}
-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);
+ ASSERT_TRUE(embedded_test_server()->Start());
+ GURL url = embedded_test_server()->GetURL("/simple_page.html");
+ NavigateToURL(shell(), url);
+ RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(
+ shell()->web_contents()->GetMainFrame());
+ EXPECT_FALSE(rfh->ShouldDispatchBeforeUnload());
}
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);
+ ASSERT_TRUE(embedded_test_server()->Start());
+ GURL url =
+ embedded_test_server()->GetURL("/page_with_empty_beforeunload.html");
+ NavigateToURL(shell(), url);
+ RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(
+ shell()->web_contents()->GetMainFrame());
+ EXPECT_TRUE(rfh->ShouldDispatchBeforeUnload());
}
namespace {
diff --git a/content/common/frame_messages.h b/content/common/frame_messages.h
index d5252062..93dc2c3f 100644
--- a/content/common/frame_messages.h
+++ b/content/common/frame_messages.h
@@ -1611,7 +1611,7 @@
// FrameMsg_RunFileChooserResponse message.
IPC_MESSAGE_ROUTED1(FrameHostMsg_RunFileChooser, content::FileChooserParams)
-// Messages to signal the presence or absence of BeforeUnload or Unload handlers
+// Messages to signal the presence or absence of beforeunload or unload handlers
// for a frame. |present| is true if there is at least one of the handlers for
// the frame.
IPC_MESSAGE_ROUTED1(FrameHostMsg_BeforeUnloadHandlersPresent,
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 9688e2f3..5b018aca 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4766,8 +4766,6 @@
case blink::WebFrameClient::UnloadHandler:
Send(new FrameHostMsg_UnloadHandlersPresent(routing_id_, present));
break;
- default:
- NOTREACHED();
}
}
diff --git a/content/test/data/page_with_empty_beforeunload.html b/content/test/data/page_with_empty_beforeunload.html
new file mode 100644
index 0000000..af92f7f
--- /dev/null
+++ b/content/test/data/page_with_empty_beforeunload.html
@@ -0,0 +1,7 @@
+<html>
+<head>
+<script>
+window.addEventListener("beforeunload", function(e) {});
+</script>
+</head>
+</html>
\ No newline at end of file
diff --git a/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp b/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
index cae0bb1..72352ee 100644
--- a/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
@@ -1425,6 +1425,10 @@
addUnloadEventListener(this);
} else if (eventType == EventTypeNames::beforeunload) {
UseCounter::count(document(), UseCounter::DocumentBeforeUnloadRegistered);
+ // This is confusingly named. It doesn't actually add the listener. It
+ // just increments a count so that we know we have listeners registered
+ // for the purposes of determining if we can fast terminate the renderer
+ // process.
addBeforeUnloadEventListener(this);
if (frame() && !frame()->isMainFrame())
UseCounter::count(document(), UseCounter::SubFrameBeforeUnloadRegistered);
