Fix nested URL blocking needs with PlzNavigate.

content/renderer/render_widget.cc

 // Copyright (c) 2012 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/renderer/render_widget.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 14 matching lines @@
 #include "build/build_config.h"
 #include "cc/output/compositor_frame_sink.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/scheduler/begin_frame_source.h"
 #include "content/common/content_switches_internal.h"
 #include "content/common/input/synthetic_gesture_packet.h"
 #include "content/common/input_messages.h"
 #include "content/common/swapped_out_messages.h"
 #include "content/common/text_input_state.h"
 #include "content/common/view_messages.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/context_menu_params.h"
 #include "content/renderer/cursor_utils.h"
 #include "content/renderer/devtools/render_widget_screen_metrics_emulator.h"
 #include "content/renderer/external_popup_menu.h"
 #include "content/renderer/gpu/frame_swap_message_queue.h"
 #include "content/renderer/gpu/queue_message_swap_promise.h"
 #include "content/renderer/gpu/render_widget_compositor.h"
 #include "content/renderer/ime_event_guard.h"
@@ skipping 274 matching lines @@
     bool hidden,
     const ScreenInfo& screen_info,
     CompositorDependencies* compositor_deps,
     blink::WebLocalFrame* frame) {
   CHECK_NE(routing_id, MSG_ROUTING_NONE);
   // TODO(avi): Before RenderViewImpl has-a RenderWidget, the browser passes the
   // same routing ID for both the view routing ID and the main frame widget
   // routing ID. https://crbug.com/545684
   RenderViewImpl* view = RenderViewImpl::FromRoutingID(routing_id);
   if (view) {
-    view->AttachWebFrameWidget(
-        RenderWidget::CreateWebFrameWidget(view->GetWidget(), frame));
+    // With PlzNavigate, the widget could already be created by the speculative

[dcheng, 2016/10/06 02:34:00]

I'm a bit confused about this comment. Could you help me understand what's going
on? I guess this getting called twice for a main frame, but I don't understand
how that's happening: this happens on the RenderViewImpl::Initialize() path, and
I wouldn't expect this to be called twice for the same RenderView.

[jam, 2016/10/06 04:33:48]

I could be missing something.

The callstack that tells the renderer to create the RenderView initially, from
the browser is:
 
content.dll!content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost
* render_view_host, int opener_frame_routing_id, int proxy_routing_id, const
content::FrameReplicationState & replicated_frame_state) Line 4928  C++
 
content.dll!content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl
* render_view_host, content::RenderFrameProxyHost * proxy) Line 1847  C++
 
content.dll!content::RenderFrameHostManager::CreateRenderFrame(content::SiteInstance
* instance, bool hidden, int * view_routing_id_ptr) Line 1694  C++
 
content.dll!content::RenderFrameHostManager::CreateSpeculativeRenderFrameHost(content::SiteInstance
* old_instance, content::SiteInstance * new_instance) Line 1650 C++
  content.dll!content::RenderFrameHostManager::GetFrameHostForNavigation(const
content::NavigationRequest & request) Line 803 C++
 
content.dll!content::RenderFrameHostManager::DidCreateNavigationRequest(content::NavigationRequest
* request) Line 714  C++
 
content.dll!content::FrameTreeNode::CreatedNavigationRequest(std::unique_ptr<content::NavigationRequest,std::default_delete<content::NavigationRequest>
> navigation_request) Line 351  C++
  content.dll!content::NavigatorImpl::OnBeginNavigation(content::FrameTreeNode *
frame_tree_node, const content::CommonNavigationParams & common_params, const
content::BeginNavigationParams & begin_params) Line 922  C++
  content.dll!content::RenderFrameHostImpl::OnBeginNavigation(const
content::CommonNavigationParams & common_params, const
content::BeginNavigationParams & begin_params) Line 1788 C++


and then later another speculative RenderView is created

 	content.dll!content::RenderFrameHostImpl::CreateRenderFrame(int
proxy_routing_id, int opener_routing_id, int parent_routing_id, int
previous_sibling_routing_id) Line 875	C++

	content.dll!content::WebContentsImpl::CreateRenderFrameForRenderManager(content::RenderFrameHost
* render_frame_host, int proxy_routing_id, int opener_routing_id, int
parent_routing_id, int previous_sibling_routing_id) Line 4963	C++

	content.dll!content::RenderFrameHostManager::InitRenderFrame(content::RenderFrameHostImpl
* render_frame_host) Line 1997	C++

	content.dll!content::RenderFrameHostManager::CreateRenderFrame(content::SiteInstance
* instance, bool hidden, int * view_routing_id_ptr) Line 1718	C++

	content.dll!content::RenderFrameHostManager::CreateSpeculativeRenderFrameHost(content::SiteInstance
* old_instance, content::SiteInstance * new_instance) Line 1650	C++
 	content.dll!content::RenderFrameHostManager::GetFrameHostForNavigation(const
content::NavigationRequest & request) Line 803	C++

	content.dll!content::RenderFrameHostManager::DidCreateNavigationRequest(content::NavigationRequest
* request) Line 714	C++

	content.dll!content::FrameTreeNode::CreatedNavigationRequest(std::unique_ptr<content::NavigationRequest,std::default_delete<content::NavigationRequest>
> navigation_request) Line 351	C++
 	content.dll!content::NavigatorImpl::OnBeginNavigation(content::FrameTreeNode *
frame_tree_node, const content::CommonNavigationParams & common_params, const
content::BeginNavigationParams & begin_params) Line 922	C++
 	content.dll!content::RenderFrameHostImpl::OnBeginNavigation(const
content::CommonNavigationParams & common_params, const
content::BeginNavigationParams & begin_params) Line 1788	C++


That one reuses the same RenderWidget (it's the same WebContents). So then
RenderFrameImpl::Create notices that widget_params.routing_id is set and so
calls RenderWidget::CreateForFrame with that routing id. The method then checks
if there's an existing RenderView for that route, and if so calls
view->AttachWebFrameWidget, and that's where the assert fires. So is the issue
then that the first RenderFrame should have been detached which would reset
RenderViewImpl's frame_widget_ first?

[jam, 2016/10/06 15:09:19]

actually, hold off on reviewing this file. I'll work on a reduced test case
first, and also so that I can check the right visual behavior.

[alexmos, 2016/10/06 22:37:28]

The crash that I think you're hitting sounds a lot like
https://crbug.com/651980, which isn't a PlzNavigate-specific problem, but rather
an OOPIF problem that we'll be investigating and fixing soon.  Also, and perhaps
related, looking at DiscardUnusedFrame, I think the fallback case where the
active frame count is > 1 and the proxy doesn't exist is broken and needs to be
fixed, which causes problems when canceling a pending or speculative RFH.  I'm
in the process of nailing down repro steps and filing a bug for that; I'll CC
you.

I'd suggest to maybe hold off on this until we can fix these bugs?  I suspect
you'll be able to remove this part of the fix from this CL.

[jam, 2016/10/06 22:54:31]

Ah ok, I'll wait for you then.

  Also, and perhaps
Yes that's as far as I got, that method seemed suspicious that it would keep the
RVH alive when the main speculative goes away.
This patch does repro this btw, if you remove the content/renderer change and
then run ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with
--enable-browser-side-navigation

[alexmos, 2016/10/07 01:47:47]

Yes, I added that as a repro to issue 651980, and in addition I also filed issue
653746 to show how proxy creation is broken in DiscardUnusedFrame.

+    // render frame.
+    if (!IsBrowserSideNavigationEnabled() || !view->GetWebFrameWidget()) {
+      view->AttachWebFrameWidget(
+          RenderWidget::CreateWebFrameWidget(view->GetWidget(), frame));
+    }
     return view->GetWidget();
   }
   scoped_refptr<RenderWidget> widget(
       g_create_render_widget
           ? g_create_render_widget(compositor_deps, blink::WebPopupTypeNone,
                                    screen_info, false, hidden, false)
           : new RenderWidget(compositor_deps, blink::WebPopupTypeNone,
                              screen_info, false, hidden, false));
   widget->SetRoutingID(routing_id);
   widget->for_oopif_ = true;
@@ skipping 1825 matching lines @@
 bool RenderWidget::isPointerLocked() {
   return mouse_lock_dispatcher_->IsMouseLockedTo(
       webwidget_mouse_lock_target_.get());
 }
 
 blink::WebWidget* RenderWidget::GetWebWidget() const {
   return webwidget_internal_;
 }
 
 }  // namespace content