Run unload handlers when navigating to about:blank using PlzNavigate.

Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler if ShouldMakeNetworkRequestForURL returns true for the URL. However ShouldMakeNetworkRequestForURL returns false for some urls that cause navigation, e.g. about:blank. So specifically check for javascript scheme, as that's the only one we don't want to run unload handlers for as it's not a real navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b618e0bb2169495e61072983f2a101c97fab9b63
Cr-Commit-Position: refs/heads/master@{#422302}

[jam, 2016-09-28 07:06:37]

Description was changed from

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns true for urls that cause navigation, e.g.
about:blank. So specifically check for javascript scheme, as that's the only one
we don't want to run unload handlers for as it's not a real navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
==========

to

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns true for urls that cause navigation, e.g.
about:blank. So specifically check for javascript scheme, as that's the only one
we don't want to run unload handlers for as it's not a real navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-09-28 07:08:06]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 07:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 08:08:43]

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

[commit-bot: I haz the power, 2016-09-28 08:08:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-29 15:34:59]

jam@chromium.org changed reviewers:
+ clamy@chromium.org

[jam, 2016-09-29 15:35:00]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 15:35:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 16:53:42]

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

[commit-bot: I haz the power, 2016-09-29 16:53:43]

Dry run: Exceeded global retry quota

[jam, 2016-09-29 17:00:54]

jam@chromium.org changed reviewers:
+ clamy@chromium.org

[jam, 2016-09-29 21:03:17]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 21:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 21:43:57]

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

[commit-bot: I haz the power, 2016-09-29 21:43:58]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-29 22:39:48]

Patchset #3 (id:40001) has been deleted

[jam, 2016-09-29 22:40:07]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:40:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-29 23:13:33]

jam@chromium.org changed reviewers:
+ creis@chromium.org
- clamy@chromium.org

[jam, 2016-09-29 23:13:34]

Switching to Charlie since Camille is OOO.

[commit-bot: I haz the power, 2016-09-29 23:31:16]

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

[commit-bot: I haz the power, 2016-09-29 23:31:17]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-30 17:36:10]

Thanks!  A few thoughts on getting the condition right.

Is there a typo in the CL description?  I think you mean
"ShouldMakeNetworkRequestForURL returns *false* for *some* urls that cause
navigation."

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:914: // Test that when a page has an
onunload handler, reloading a page shows a
nit: onbeforeunload? (Looks like a typo from long ago.)

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:926: // call below tries to put up an
unload dialog synchronously in the
nit: a beforeunload dialog

Also, I'm curious about the "synchronously" part.  Doesn't the dialog come from
the renderer (asynchronously)?

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1058: // is synchronous and
same-site) then NavigationRequest::BeginNavigation
This comment is a bit confusing to me.  What does "synchronous and same-site"
mean?  about:blank should trigger beforeunload, and same-site pages should
trigger it as well.

It looks like same-page and IsRendererDebugURL might be the only ones that
don't?

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1062: url::kJavaScriptScheme)) {
I think we need to account for debug URLs like chrome://kill as well, right? 
Those shouldn't trigger beforeunload (or at least, don't seem to today).

There's a IsRendererDebugURL that we consult in similar places that might be
useful here, if it's the condition we're after.

Do we also need to make an exception for in-page navigations, at least
browser-initiated ones?  I imagine the renderer will handle
pushState/replaceState just fine, along with renderer-initiated fragment
navigations.  Browser-initiated fragment navigations (like typing #foo in the
address bar) don't seem to trigger beforeunload, though, and we might need to
account for that here.

[jam, 2016-09-30 20:14:21]

Description was changed from

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns true for urls that cause navigation, e.g.
about:blank. So specifically check for javascript scheme, as that's the only one
we don't want to run unload handlers for as it's not a real navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns false for some urls that cause
navigation, e.g. about:blank. So specifically check for javascript scheme, as
that's the only one we don't want to run unload handlers for as it's not a real
navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2016-09-30 22:40:17]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 22:40:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-30 22:42:07]

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:914: // Test that when a page has an
onunload handler, reloading a page shows a
On 2016/09/30 17:36:10, Charlie Reis wrote:
> nit: onbeforeunload? (Looks like a typo from long ago.)

Done.

https://codereview.chromium.org/2373273002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:926: // call below tries to put up an
unload dialog synchronously in the
On 2016/09/30 17:36:10, Charlie Reis wrote:
> nit: a beforeunload dialog
> 
> Also, I'm curious about the "synchronously" part.  Doesn't the dialog come
from
> the renderer (asynchronously)?

I don't actually remember why I wrote it that way. I've removed the comment for
now. When PlzNavigate is the default the if statement would be gone.

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1058: // is synchronous and
same-site) then NavigationRequest::BeginNavigation
On 2016/09/30 17:36:10, Charlie Reis wrote:
> This comment is a bit confusing to me.  What does "synchronous and same-site"
> mean?  about:blank should trigger beforeunload, and same-site pages should
> trigger it as well.

I'm not sure what it was supposed to mean as well, especially since there are no
checks there for sync or samesite. i've removed that comment.
 
> 
> It looks like same-page and IsRendererDebugURL might be the only ones that
> don't?

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1062: url::kJavaScriptScheme)) {
On 2016/09/30 17:36:10, Charlie Reis wrote:
> I think we need to account for debug URLs like chrome://kill as well, right? 
> Those shouldn't trigger beforeunload (or at least, don't seem to today).
> 
> There's a IsRendererDebugURL that we consult in similar places that might be
> useful here, if it's the condition we're after.

Good point, yes this behavior is currently broken (before this cl). I'm
surprised this had no tests, I've added one.

I've also moved this to the JS specific codepath above (line 1040)

> 
> Do we also need to make an exception for in-page navigations, at least
> browser-initiated ones?  I imagine the renderer will handle
> pushState/replaceState just fine, along with renderer-initiated fragment
> navigations.  Browser-initiated fragment navigations (like typing #foo in the
> address bar) don't seem to trigger beforeunload, though, and we might need to
> account for that here.

So I first agreed and changed the code and added a test. But then I realized
there are lots of corner cases. i.e. if I checked if the urls are the same
without fragments, this would then not run unload handlers if the user clicked
on a link to the same url, or if the renderer navigated to the same url. we
would need more information plumbed to this method to differentiate this. it
seemed out of scope for this cl at least.

[jam, 2016-09-30 22:42:12]

The CQ bit was unchecked by jam@chromium.org

[Charlie Reis, 2016-09-30 22:54:42]

Thanks!  LGTM with nit.

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:414: IsRendererDebugURL(dest_url))
{
Interesting.  This is somewhat independent of the rest of the CL, but I think
you're right.  I'm ok including it.

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1037: // Navigation to a debug URLs
is not a "real" navigation so there is no
nit: debug URL

[jam, 2016-09-30 23:07:46]

The CQ bit was checked by jam@chromium.org

[jam, 2016-09-30 23:07:46]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2373273002/#ps100001
(title: "review nit")

[jam, 2016-09-30 23:07:50]

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:414: IsRendererDebugURL(dest_url))
{
On 2016/09/30 22:54:42, Charlie Reis wrote:
> Interesting.  This is somewhat independent of the rest of the CL, but I think
> you're right.  I'm ok including it.

yeah I looked for other usages of javascript scheme to see other possible
problematic uses. i figured as long as no tests fail, this is an improvement.

https://codereview.chromium.org/2373273002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1037: // Navigation to a debug URLs
is not a "real" navigation so there is no
On 2016/09/30 22:54:42, Charlie Reis wrote:
> nit: debug URL

Done.

[commit-bot: I haz the power, 2016-09-30 23:08:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-30 23:11:41]

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1062: url::kJavaScriptScheme)) {
On 2016/09/30 22:42:07, jam wrote:
> On 2016/09/30 17:36:10, Charlie Reis wrote:
> > I think we need to account for debug URLs like chrome://kill as well, right?

> > Those shouldn't trigger beforeunload (or at least, don't seem to today).
> > 
> > There's a IsRendererDebugURL that we consult in similar places that might be
> > useful here, if it's the condition we're after.
> 
> Good point, yes this behavior is currently broken (before this cl). I'm
> surprised this had no tests, I've added one.
> 
> I've also moved this to the JS specific codepath above (line 1040)
> 
> > 
> > Do we also need to make an exception for in-page navigations, at least
> > browser-initiated ones?  I imagine the renderer will handle
> > pushState/replaceState just fine, along with renderer-initiated fragment
> > navigations.  Browser-initiated fragment navigations (like typing #foo in
the
> > address bar) don't seem to trigger beforeunload, though, and we might need
to
> > account for that here.
> 
> So I first agreed and changed the code and added a test. But then I realized
> there are lots of corner cases. i.e. if I checked if the urls are the same
> without fragments, this would then not run unload handlers if the user clicked
> on a link to the same url, or if the renderer navigated to the same url. we
> would need more information plumbed to this method to differentiate this. it
> seemed out of scope for this cl at least.

I should also add, it's not clear to me what the right behavior is :)

[Charlie Reis, 2016-09-30 23:20:08]

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2373273002/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:1062: url::kJavaScriptScheme)) {
On 2016/09/30 23:11:41, jam wrote:
> On 2016/09/30 22:42:07, jam wrote:
> > On 2016/09/30 17:36:10, Charlie Reis wrote:
> > > I think we need to account for debug URLs like chrome://kill as well,
right?
> 
> > > Those shouldn't trigger beforeunload (or at least, don't seem to today).
> > > 
> > > There's a IsRendererDebugURL that we consult in similar places that might
be
> > > useful here, if it's the condition we're after.
> > 
> > Good point, yes this behavior is currently broken (before this cl). I'm
> > surprised this had no tests, I've added one.
> > 
> > I've also moved this to the JS specific codepath above (line 1040)
> > 
> > > 
> > > Do we also need to make an exception for in-page navigations, at least
> > > browser-initiated ones?  I imagine the renderer will handle
> > > pushState/replaceState just fine, along with renderer-initiated fragment
> > > navigations.  Browser-initiated fragment navigations (like typing #foo in
> the
> > > address bar) don't seem to trigger beforeunload, though, and we might need
> to
> > > account for that here.
> > 
> > So I first agreed and changed the code and added a test. But then I realized
> > there are lots of corner cases. i.e. if I checked if the urls are the same
> > without fragments, this would then not run unload handlers if the user
clicked
> > on a link to the same url, or if the renderer navigated to the same url. we
> > would need more information plumbed to this method to differentiate this. it
> > seemed out of scope for this cl at least.
> 
> I should also add, it's not clear to me what the right behavior is :)

Yeah, I was just pointing out how it appeared to behave today from some quick
manual testing.  It's fine to put some more thought into how to handle that
case.  (Ideally we wouldn't run beforeunload for browser-initiated fragment
navigations, but it's not a major regression if it happens.)

[commit-bot: I haz the power, 2016-10-01 00:57:18]

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

[commit-bot: I haz the power, 2016-10-01 00:57:19]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-10-01 03:59:20]

Patchset #6 (id:120001) has been deleted

[jam, 2016-10-01 03:59:58]

The CQ bit was checked by jam@chromium.org

[jam, 2016-10-01 03:59:58]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2373273002/#ps140001
(title: "undo incorrect change that broke chrome://crash")

[commit-bot: I haz the power, 2016-10-01 04:00:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 05:28:41]

Description was changed from

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns false for some urls that cause
navigation, e.g. about:blank. So specifically check for javascript scheme, as
that's the only one we don't want to run unload handlers for as it's not a real
navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns false for some urls that cause
navigation, e.g. about:blank. So specifically check for javascript scheme, as
that's the only one we don't want to run unload handlers for as it's not a real
navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-01 05:28:43]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-10-01 05:30:54]

Description was changed from

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns false for some urls that cause
navigation, e.g. about:blank. So specifically check for javascript scheme, as
that's the only one we don't want to run unload handlers for as it's not a real
navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Run unload handlers when navigating to about:blank using PlzNavigate.

Previously, NavigatorImpl::RequestNavigation would only run the unload handler
if ShouldMakeNetworkRequestForURL returns true for the URL. However
ShouldMakeNetworkRequestForURL returns false for some urls that cause
navigation, e.g. about:blank. So specifically check for javascript scheme, as
that's the only one we don't want to run unload handlers for as it's not a real
navigation.

This fixes the following tests:
BrowserTest.BeforeUnloadVsBeforeReload
BrowserTest.CancelBeforeUnloadResetsURL
BrowserTest.ReloadThenCancelBeforeUnload

BUG=504347
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/b618e0bb2169495e61072983f2a101c97fab9b63
Cr-Commit-Position: refs/heads/master@{#422302}
==========

[commit-bot: I haz the power, 2016-10-01 05:30:58]

Patchset 6 (id:??) landed as
https://crrev.com/b618e0bb2169495e61072983f2a101c97fab9b63
Cr-Commit-Position: refs/heads/master@{#422302}