Move isNavigationAllowed() check to main entry point for loads.
Also document the difference between the two types of navigation
disablers and how they should be used.
BUG=600182
Review URL: https://codereview.chromium.org/1858833003
Cr-Commit-Position: refs/heads/master@{#385306}
diff --git a/third_party/WebKit/Source/core/frame/LocalFrame.cpp b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
index 971bdcd5..daa5dc8 100644
--- a/third_party/WebKit/Source/core/frame/LocalFrame.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalFrame.cpp
@@ -292,8 +292,6 @@
void LocalFrame::navigate(const FrameLoadRequest& request)
{
- if (!isNavigationAllowed())
- return;
m_loader.load(request);
}
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index e0e5e416..b9f010c 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -884,6 +884,9 @@
{
ASSERT(m_frame->document());
+ if (!m_frame->isNavigationAllowed())
+ return;
+
if (m_inStopAllLoaders)
return;
diff --git a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
index da4158c..94e7e6f 100644
--- a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
+++ b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
@@ -278,6 +278,23 @@
return m_redirect && m_redirect->delay() <= interval;
}
+// TODO(dcheng): There are really two different load blocking concepts at work
+// here and they have been incorrectly tangled together.
+//
+// 1. NavigationDisablerForBeforeUnload is for blocking navigation scheduling
+// during a beforeunload event. Scheduled navigations during beforeunload
+// would make it possible to get trapped in an endless loop of beforeunload
+// dialogs.
+//
+// Checking Frame::isNavigationAllowed() doesn't make sense in this context:
+// NavigationScheduler is always cleared when a new load commits, so it's
+// impossible for a scheduled navigation to clobber a navigation that just
+// committed.
+//
+// 2. FrameNavigationDisabler / LocalFrame::isNavigationAllowed() are intended
+// to prevent Documents from being reattached during destruction, since it
+// can cause bugs with security origin confusion. This is primarily intended
+// to block /synchronous/ navigations during things lke Document::detach().
inline bool NavigationScheduler::shouldScheduleReload() const
{
return m_frame->page() && m_frame->isNavigationAllowed() && NavigationDisablerForBeforeUnload::isNavigationAllowed();