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

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
Committed: https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d
Cr-Commit-Position: refs/heads/master@{#385306}

[dcheng, 2016-04-05 00:06:28]

dcheng@chromium.org changed reviewers:
+ japhet@chromium.org

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

+japhet for primary review

+avi, +clamy, +kinuko for FYI

https://codereview.chromium.org/1858833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1858833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:897: if
(!m_frame->isNavigationAllowed())
Presumably, this will still be the main entry point after PlzNavigate, since we
still have to handle sync navigations somehow.

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:284: // 1.
NavigationDisablerForBeforeUnload is for blocking navigation scheduling
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)

[dcheng, 2016-04-05 00:06:55]

Also... no test because of plugin dependency. Working on it =(

[Avi (use Gerrit), 2016-04-05 01:36:32]

avi@chromium.org changed reviewers:
+ avi@chromium.org

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

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:284: // 1.
NavigationDisablerForBeforeUnload is for blocking navigation scheduling
On 2016/04/05 00:06:29, dcheng wrote:
> 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)

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]

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right):

https://codereview.chromium.org/1858833003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:284: // 1.
NavigationDisablerForBeforeUnload is for blocking navigation scheduling
On 2016/04/05 at 01:36:33, Avi wrote:
> On 2016/04/05 00:06:29, dcheng wrote:
> > 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)
> 
> 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.

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.

[Nate Chapin, 2016-04-05 20:05:59]

LGTM. Any hope of a test?

[dcheng, 2016-04-05 20:07:51]

On 2016/04/05 at 20:05:59, japhet wrote:
> LGTM. Any hope of a test?

Working on it in a followup =(
The plugin makes things messy.

[dcheng, 2016-04-05 21:06:10]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2016-04-05 21:06:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858833003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858833003/20001

[commit-bot: I haz the power, 2016-04-05 21:11:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-05 21:11:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2016-04-05 21:14:48]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-04-05 21:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1858833003/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-05 21:15:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858833003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858833003/40001

[commit-bot: I haz the power, 2016-04-05 22:39:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-05 22:41:04]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d
Cr-Commit-Position: refs/heads/master@{#385306}
==========

[commit-bot: I haz the power, 2016-04-05 22:41:05]

Patchset 3 (id:??) landed as
https://crrev.com/73563fee12defb21a8f955993b68907169e1ea6d
Cr-Commit-Position: refs/heads/master@{#385306}