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();