Move isNavigationAllowed() check to main entry point for loads.

third_party/WebKit/Source/core/loader/NavigationScheduler.cpp

Index: third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
diff --git a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
index b33d21493967ed0059abfd29dba13408abff5137..06703f8453814e5de73f6003e4439f2273dec25d 100644
--- a/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
+++ b/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
@@ -278,6 +278,23 @@ bool NavigationScheduler::isNavigationScheduledWithin(double interval) const
     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

[dcheng, 2016/04/05 00:06:29]

Avi, given this description, do you think it still makes sense to have this? We
have the checkbox that should allow users to break out of the beforeunload loop
now... but maybe we still want to keep it? In that case, we probably need to be
better about checking this consistently (for example, scheduleFormSubmission
forgets to check this)

[Avi (use Gerrit), 2016/04/05 01:36:33]

I know about the dialog side of onbeforeunload, but not much about the
implementation side.

Unlike window.alert and friends, where the page's JavaScript has to be paused to
get the answer, onbeforeunload is triggered by a return value from a function
but the page's JavaScript doesn't have to run. Does onbeforeunload stop all
interaction from a page? Can it be that a page returns a string from
onbeforeunload, and while the dialog is up, the page has further navigation
interactions that we need to stop? Or does the fact that the dialog is up grind
the renderer to a halt waiting for an answer?

If the former, we would likely need to keep this.

[dcheng, 2016/04/05 04:47:22]

While the beforeunload dialog is open, no script can run. The issue that
NavigationDisablerForBeforeUnload addresses is code like this:

window.onbeforeunload = function() {
  location.reload();
  return "Mwhahahaha";
};

The reload gets queued, so if the user chooses to stay on the page, he/she is
immediately bombarded with another beforeunload dialog.

Though now that I think about this, maybe we can achieve the same effect by just
cancelling scheduled navigations after the beforeunload event runs. This means
that fragment navigations in beforeunload events now work but meh, that seems
OK? I'll try this in a followup patch.

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