Issue AboutToNavigateRenderFrame notification after beforeunload handling.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index bc8a487c3db31f79e03dd1a36856969126ff648e..2749283b53bdb77c8a735f68a20ed08946ab961d 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -556,6 +556,13 @@ void RenderFrameHostManager::OnBeforeUnloadACK(
       // is ok to do nothing here.
       if (pending_render_frame_host_ &&
           pending_render_frame_host_->are_navigations_suspended()) {
+        NavigatorDelegate* navigator_delegate =
+            frame_tree_node_->navigator()->GetDelegate();
+        if (navigator_delegate &&
+            pending_render_frame_host_->ShouldDispatchBeforeUnload()) {

[dgozman, 2015/09/22 20:54:30]

This unfortunate check is here to not issue notification twice for subframes (we
immediately cancel beforeunload handling for them).

[Charlie Reis, 2015/09/22 23:12:10]

This is almost certainly not what you want to do.  Why would it matter whether
the pending (destination) RFH has a beforeunload handler?  In most cases, we've
just created that RFH and it hasn't loaded a page yet.

I also don't understand how it relates to the subframe case you describe.

[dgozman, 2015/09/22 23:27:55]

See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Charlie Reis, 2015/09/22 23:56:42]

The current implementation of ShouldDispatchBeforeUnload just temporary code
until we support beforeunload on subframes.  If you're trying to check whether
this is a live subframe, calling ShouldDispatchBeforeUnload is not a good way to
do it.  Just call frame_tree_node_->IsMainFrame() and IsRenderFrameLive()
directly if that's the condition you're after.

However, I don't understand why that check is needed, especially on the pending
RFH.  You might want to take a closer look at why the notification is happening
twice in some cases, to help us understand how to resolve it.

+          navigator_delegate->AboutToNavigateRenderFrame(

[Charlie Reis, 2015/09/22 23:12:10]

What if we moved AboutToNavigateRenderFrame to RenderFrameHostDelegate (which is
WebContents, just like NavigatorDelegate) and moved all of the calls inside
RenderFrameHostImpl?  We could even use a helper function to do the call and the
Send, as yurys@ suggests.

That would avoid the problem above with ShouldCloseTabOnUnresponsiveRenderer.

[dgozman, 2015/09/22 23:27:56]

I was hesitant about moving notifications between delegates. But if it's ok, I
will surely do that. Just needed a blessing from content owner.

[Charlie Reis, 2015/09/22 23:56:42]

Yep, it's fine.  And even better if we can remove the method from
WebContentsObserver, as you suggest.

+              render_frame_host_.get(), pending_render_frame_host_.get());
+        }
         pending_render_frame_host_->SetNavigationsSuspended(false,
                                                             proceed_time);
       }