|
|
Created:
4 years, 2 months ago by jam Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, extensions-reviews_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix nested URL blocking needs with PlzNavigate.
Navigations from extension processes for chrome-extension URLs will get a response with a blob:chrome-extension scheme when using PlzNavigate. So store the fact that this is for an extension process with the navigate data so that we an refer to it later on the IO thread.
This fixes
ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed
ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked
BUG=504347, 647712
Patch Set 1 #
Total comments: 13
Messages
Total messages: 31 (20 generated)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347 ========== to ========== Fix nested URL blocking needs with PlzNavigate. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ==========
jam@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org
dcheng: content/renderer (since this is code you added in https://codereview.chromium.org/1303773002) alexmos: the rest
Description was changed from ========== Fix nested URL blocking needs with PlzNavigate. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ========== to ========== Fix nested URL blocking needs with PlzNavigate. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ==========
Description was changed from ========== Fix nested URL blocking needs with PlzNavigate. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ========== to ========== Fix nested URL blocking needs with PlzNavigate. Navigations from extension processes for chrome-extension URLs will get a response with a blob:chrome-extension scheme when using PlzNavigate. So store the fact that this is for an extension process with the navigate data so that we an refer to it later on the IO thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix nested URL blocking needs with PlzNavigate. Navigations from extension processes for chrome-extension URLs will get a response with a blob:chrome-extension scheme when using PlzNavigate. So store the fact that this is for an extension process with the navigate data so that we an refer to it later on the IO thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ========== to ========== Fix nested URL blocking needs with PlzNavigate. Navigations from extension processes for chrome-extension URLs will get a response with a blob:chrome-extension scheme when using PlzNavigate. So store the fact that this is for an extension process with the navigate data so that we an refer to it later on the IO thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked BUG=504347,647712 ==========
https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative 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.
https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative On 2016/10/06 02:34:00, dcheng wrote: > 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. I could be missing something. The callstack that tells the renderer to create the RenderView initially, from the browser is: > content.dll!content::RenderViewHostImpl::CreateRenderView(int opener_frame_route_id, int proxy_route_id, int max_page_id, const content::FrameReplicationState & replicated_frame_state, bool window_was_created_with_opener) Line 382 C++ 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?
https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative On 2016/10/06 04:33:48, jam wrote: > On 2016/10/06 02:34:00, dcheng wrote: > > 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. > > I could be missing something. > > The callstack that tells the renderer to create the RenderView initially, from > the browser is: > > content.dll!content::RenderViewHostImpl::CreateRenderView(int > opener_frame_route_id, int proxy_route_id, int max_page_id, const > content::FrameReplicationState & replicated_frame_state, bool > window_was_created_with_opener) Line 382 C++ > > 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? 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.
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/net/chro... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/net/chro... chrome/browser/net/chrome_extensions_network_delegate.cc:197: // https://crbug.com/652077. nit: maybe expand this comment to mention the new exception and why it's needed? https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); Wouldn't going via web_contents be wrong for OOP subframes? Feel like this should use something like GetRenderFrameHost()->GetSiteInstance(), but reading the comments on that (and chatting to Nasko), that seems like it might not be available at this point, since there's no response yet? One other question Nasko and I had was whether it's possible for PlzNavigate to use blob with some special inner url/origin, e.g. blob://plznav/{guid}, when creating the response stream [1], so we can distinguish blobs created by PlzNavigate from all other blob URLs. Not sure if Blink ever sees it though, and whether that might cause it to fail security checks. [1] https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resour... https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative On 2016/10/06 15:09:19, jam wrote: > On 2016/10/06 04:33:48, jam wrote: > > On 2016/10/06 02:34:00, dcheng wrote: > > > 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. > > > > I could be missing something. > > > > The callstack that tells the renderer to create the RenderView initially, from > > the browser is: > > > content.dll!content::RenderViewHostImpl::CreateRenderView(int > > opener_frame_route_id, int proxy_route_id, int max_page_id, const > > content::FrameReplicationState & replicated_frame_state, bool > > window_was_created_with_opener) Line 382 C++ > > > > > 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? > > 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. 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.
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); On 2016/10/06 22:37:28, alexmos wrote: > Wouldn't going via web_contents be wrong for OOP subframes? Feel like this > should use something like GetRenderFrameHost()->GetSiteInstance(), but reading > the comments on that (and chatting to Nasko), that seems like it might not be > available at this point, since there's no response yet? Right we wouldn't have a RenderFrameHost yet. Are we going to have OOP subframes for extensions webcontents? > > One other question Nasko and I had was whether it's possible for PlzNavigate to > use blob with some special inner url/origin, e.g. blob://plznav/{guid}, when > creating the response stream [1], so we can distinguish blobs created by > PlzNavigate from all other blob URLs. Not sure if Blink ever sees it though, > and whether that might cause it to fail security checks. > > [1] > https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resour... https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative On 2016/10/06 22:37:28, alexmos wrote: > On 2016/10/06 15:09:19, jam wrote: > > On 2016/10/06 04:33:48, jam wrote: > > > On 2016/10/06 02:34:00, dcheng wrote: > > > > 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. > > > > > > I could be missing something. > > > > > > The callstack that tells the renderer to create the RenderView initially, > from > > > the browser is: > > > > content.dll!content::RenderViewHostImpl::CreateRenderView(int > > > opener_frame_route_id, int proxy_route_id, int max_page_id, const > > > content::FrameReplicationState & replicated_frame_state, bool > > > window_was_created_with_opener) Line 382 C++ > > > > > > > > > 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? > > > > 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. > > 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. Ah ok, I'll wait for you then. 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. 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. > I'm > in the process of nailing down repro steps and filing a bug for that; I'll CC > you. This patch does repro this btw, if you remove the content/renderer change and then run ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with --enable-browser-side-navigation > > 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.
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); On 2016/10/06 22:54:31, jam wrote: > On 2016/10/06 22:37:28, alexmos wrote: > > Wouldn't going via web_contents be wrong for OOP subframes? Feel like this > > should use something like GetRenderFrameHost()->GetSiteInstance(), but reading > > the comments on that (and chatting to Nasko), that seems like it might not be > > available at this point, since there's no response yet? > > Right we wouldn't have a RenderFrameHost yet. > > Are we going to have OOP subframes for extensions webcontents? Yes, with --isolate-extensions, a top-level extensions page could embed a web iframe, which will be kicked into an OOPIF. If that iframe navigates under PlzNavigate, this seems like it will grab the main frame's SiteInstance, rather than the iframe's. https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/2401443002/diff/60001/content/renderer/render... content/renderer/render_widget.cc:330: // With PlzNavigate, the widget could already be created by the speculative On 2016/10/06 22:54:31, jam wrote: > On 2016/10/06 22:37:28, alexmos wrote: > > On 2016/10/06 15:09:19, jam wrote: > > > On 2016/10/06 04:33:48, jam wrote: > > > > On 2016/10/06 02:34:00, dcheng wrote: > > > > > 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. > > > > > > > > I could be missing something. > > > > > > > > The callstack that tells the renderer to create the RenderView initially, > > from > > > > the browser is: > > > > > content.dll!content::RenderViewHostImpl::CreateRenderView(int > > > > opener_frame_route_id, int proxy_route_id, int max_page_id, const > > > > content::FrameReplicationState & replicated_frame_state, bool > > > > window_was_created_with_opener) Line 382 C++ > > > > > > > > > > > > > > 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? > > > > > > 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. > > > > 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. > > Ah ok, I'll wait for you then. > > 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. > > 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. > > > I'm > > in the process of nailing down repro steps and filing a bug for that; I'll CC > > you. > > This patch does repro this btw, if you remove the content/renderer change and > then run ProcessManagerBrowserTest.NestedURLNavigationsToExtensionBlocked with > --enable-browser-side-navigation 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.
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); On 2016/10/07 01:47:47, alexmos wrote: > On 2016/10/06 22:54:31, jam wrote: > > On 2016/10/06 22:37:28, alexmos wrote: > > > Wouldn't going via web_contents be wrong for OOP subframes? Feel like this > > > should use something like GetRenderFrameHost()->GetSiteInstance(), but > reading > > > the comments on that (and chatting to Nasko), that seems like it might not > be > > > available at this point, since there's no response yet? > > > > Right we wouldn't have a RenderFrameHost yet. > > > > Are we going to have OOP subframes for extensions webcontents? > > > Yes, with --isolate-extensions, a top-level extensions page could embed a web > iframe, which will be kicked into an OOPIF. If that iframe navigates under > PlzNavigate, this seems like it will grab the main frame's SiteInstance, rather > than the iframe's. Do you have steps I can use to repro this? i.e. if an extensions page embeds a web iframe, wouldn't that be done with a webview and that would have a separate WebContents for the webview that would have the correct site instance?
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); On 2016/10/10 15:36:50, jam wrote: > On 2016/10/07 01:47:47, alexmos wrote: > > On 2016/10/06 22:54:31, jam wrote: > > > On 2016/10/06 22:37:28, alexmos wrote: > > > > Wouldn't going via web_contents be wrong for OOP subframes? Feel like > this > > > > should use something like GetRenderFrameHost()->GetSiteInstance(), but > > reading > > > > the comments on that (and chatting to Nasko), that seems like it might not > > be > > > > available at this point, since there's no response yet? > > > > > > Right we wouldn't have a RenderFrameHost yet. > > > > > > Are we going to have OOP subframes for extensions webcontents? > > > > > > Yes, with --isolate-extensions, a top-level extensions page could embed a web > > iframe, which will be kicked into an OOPIF. If that iframe navigates under > > PlzNavigate, this seems like it will grab the main frame's SiteInstance, > rather > > than the iframe's. > > Do you have steps I can use to repro this? > i.e. if an extensions page embeds a web iframe, wouldn't that be done with a > webview and that would have a separate WebContents for the webview that would > have the correct site instance? AFAIK extensions aren't allowed to use <webview>, that's only available in Chrome apps. Here's a sample extension where the extension options page embeds a web iframe: https://bugs.chromium.org/p/chromium/issues/detail?id=487872#c4
https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_navigation_ui_data.cc (right): https://codereview.chromium.org/2401443002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_navigation_ui_data.cc:36: web_contents->GetSiteInstance()->GetSiteURL()); On 2016/10/10 17:04:21, alexmos wrote: > On 2016/10/10 15:36:50, jam wrote: > > On 2016/10/07 01:47:47, alexmos wrote: > > > On 2016/10/06 22:54:31, jam wrote: > > > > On 2016/10/06 22:37:28, alexmos wrote: > > > > > Wouldn't going via web_contents be wrong for OOP subframes? Feel like > > this > > > > > should use something like GetRenderFrameHost()->GetSiteInstance(), but > > > reading > > > > > the comments on that (and chatting to Nasko), that seems like it might > not > > > be > > > > > available at this point, since there's no response yet? > > > > > > > > Right we wouldn't have a RenderFrameHost yet. > > > > > > > > Are we going to have OOP subframes for extensions webcontents? > > > > > > > > > Yes, with --isolate-extensions, a top-level extensions page could embed a > web > > > iframe, which will be kicked into an OOPIF. If that iframe navigates under > > > PlzNavigate, this seems like it will grab the main frame's SiteInstance, > > rather > > > than the iframe's. > > > > Do you have steps I can use to repro this? > > i.e. if an extensions page embeds a web iframe, wouldn't that be done with a > > webview and that would have a separate WebContents for the webview that would > > have the correct site instance? > > AFAIK extensions aren't allowed to use <webview>, that's only available in > Chrome apps. Here's a sample extension where the extension options page embeds > a web iframe: https://bugs.chromium.org/p/chromium/issues/detail?id=487872#c4 I should have been more precise, I meant guestview. there'll be a separate webcontents for the extensions options view
Message was sent while issue was closed.
closing this out in favor of https://codereview.chromium.org/2411693003/ |