Fix nested URL blocking needs with PlzNavigate.

content/renderer/render_widget.cc

Index: content/renderer/render_widget.cc
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index c135f14e277771d4a38bcc09ac48ccd0f120784a..030e4c70720def7b48f5fc14d6a93f07cd539f54 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -32,6 +32,7 @@
 #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"
@@ -326,8 +327,12 @@ RenderWidget* RenderWidget::CreateForFrame(
   // 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(