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

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 50787510ac49e81c23c39e6ca723f4163e07075d..ee7d9328ae1a89e3be8b1e6027b9938a2caba2db 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -1058,8 +1058,8 @@ void NavigatorImpl::RequestNavigation(FrameTreeNode* frame_tree_node,
   // is synchronous and same-site) then NavigationRequest::BeginNavigation

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

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?

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

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.
 

   // should be directly called instead.
   if (should_dispatch_beforeunload &&
-      ShouldMakeNetworkRequestForURL(
-          navigation_request->common_params().url)) {
+      !navigation_request->common_params().url.SchemeIs(
+          url::kJavaScriptScheme)) {

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

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 22:42:07]

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)
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 23:11:41]

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

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

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.)

     navigation_request->SetWaitingForRendererResponse();
     frame_tree_node->current_frame_host()->DispatchBeforeUnload(
         true, reload_type != ReloadType::NONE);