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

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  * Copyright (C) 2009 Adam Barth. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
@@ skipping 260 matching lines @@
 bool NavigationScheduler::locationChangePending()
 {
     return m_redirect && m_redirect->isLocationChange();
 }
 
 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();
 }
 
 inline bool NavigationScheduler::shouldScheduleNavigation(const String& url) const
 {
     return m_frame->page() && m_frame->isNavigationAllowed() && (protocolIsJavaScript(url) || NavigationDisablerForBeforeUnload::isNavigationAllowed());
 }
 
@@ skipping 140 matching lines @@
     m_redirect.clear();
 }
 
 DEFINE_TRACE(NavigationScheduler)
 {
     visitor->trace(m_frame);
     visitor->trace(m_redirect);
 }
 
 } // namespace blink