Issue AboutToNavigateRenderFrame notification after beforeunload handling.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 534 matching lines @@
     DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kEnableBrowserSideNavigation));
     // Ignore if we're not in a cross-process navigation.
     if (!pending_render_frame_host_)
       return;
 
     if (proceed) {
       // Ok to unload the current page, so proceed with the cross-process
       // navigation.  Note that if navigations are not currently suspended, it
       // might be because the renderer was deemed unresponsive and this call was
       // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it

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

What about this case (on line 533)?  If the unresponsiveness timer fires, we'll
call SetNavigationsSuspended up there and allow the navigation to proceed.

       // 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);
       }
     } else {
       // Current page says to cancel.
       CancelPending();
     }
   } else {
     // Non-cross-process transition means closing the entire tab.
     bool proceed_to_fire_unload;
@@ skipping 2025 matching lines @@
 int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
   if (!frame_tree_node_->opener())
     return MSG_ROUTING_NONE;
 
   return frame_tree_node_->opener()
       ->render_manager()
       ->GetRoutingIdForSiteInstance(instance);
 }
 
 }  // namespace content