Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
310413004:
Move ownership of CrossProcessFrameConnector to RenderFrameProxyHost. (Closed)
|
Created:
6 years, 6 months ago by nasko Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove ownership of CrossProcessFrameConnector to RenderFrameProxyHost.
BUG=357747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278532
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Add checks for CPFC to cross-site iframe test. #Patch Set 3 : Cleanup. #
Total comments: 24
Patch Set 4 : Fixes based on Ken's review. #Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:283: frame_tree_node_->render_manager()->ProxyToParent(); I think this is the problem. This message intercept here is intended to intercept messages coming *from* the parent frame renderer and going *to* the child frame renderer. This is currently exercised when the current RenderFrameHostImpl is swapped out and has a CPFC -- indicating that it is responsible for passing input events and iframe size information to its child. In the current form, do we still have a parent-renderer RenderFrameHost for the swapped out child, or has that been replaced with RenderFrameProxyHost yet? If it has been replaced (I don't think it has, but I haven't been following closely), this intercept just goes there -- the RFPH should be receiving the message from the RenderFrameProxy. Otherwise, I'm not sure what the current configuration of RenderFrameProxy and RenderFrameProxyHost looks like for a swapepd out child frame.
https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1184: proxy->SetView(render_frame_host_->render_view_host()->GetView()); Is this the wrong view being set? We are trying to link the CPFC to the RenderWidgetHostViewChildFrame that is attached to the child frame's RenderFrameHost (i.e. the real one, for the child process). Is this linking it to the parent's view?
https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/310413004/diff/20001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1184: proxy->SetView(render_frame_host_->render_view_host()->GetView()); On 2014/06/06 16:10:56, kenrb wrote: > Is this the wrong view being set? We are trying to link the CPFC to the > RenderWidgetHostViewChildFrame that is attached to the child frame's > RenderFrameHost (i.e. the real one, for the child process). Is this linking it > to the parent's view? Ah, never mind. This should be happening in the child frame's RenderFrameHostManager, so this looks correct.
Ken, can you review this CL for real now? It isn't much different than before, but it is cleaned up and hopefully ready to go.
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.cc:79: frame_proxy_in_parent_renderer_->GetRoutingID(), params)); Did the style change on this? Accessors and mutators were supposed to match variable names, I thought, and so looked different from other method names. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/cross_process_frame_connector.h (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:61: // in the child frame's RenderFrameHostManager corresponding to the parent's This is a bit hard to follow, probably good to clarify that it is A2 in the above illustration. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:62: // SiteInstance. When a child frame swaps, set_view() is called to update to the Are we still using the term 'swaps'? Maybe 'navigates and is moved to a new process' or something similar. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:111: // TODO(kenrb): The type becomes RenderFrameProxyHost when that class comes This TODO can go. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) This doesn't look right to me. The original line here existed for intercepting messages coming from the parent frame renderer, going to the child frame renderer (e.g. input events, resize). This appears to be going the wrong way. Really, this should be in RenderFrameProxyHost, and use its CPFC pointer, but I don't know if RFPH actually receives messages yet. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.cc:29: ->GetSiteInstance() == site_instance) { Is this condition correct? We should be creating a new CPFC if this RFPH is in a different process than its parent. For now I think this means NOT the same SiteInstance as its parent, right? Later on I think we will need a better way to determine whether an RFPH is cross-process. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:74: void SetView(RenderWidgetHostView* view); Nit: 'SetView' is a confusing name, because is makes it sound like a mutator. The 'view' is really the view for the RenderWidgetHost of the child frame (hence there is RenderWidgetHostImpl::SetView). 'SetChildRWHView' or something similar would be a better alternative. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:109: // When a RrenderFrameHost is in a different process from its parent in the Nit: typo RrenderFrameHost
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.cc:79: frame_proxy_in_parent_renderer_->GetRoutingID(), params)); On 2014/06/19 14:49:04, kenrb wrote: > Did the style change on this? Accessors and mutators were supposed to match > variable names, I thought, and so looked different from other method names. RenderFrameProxyHost already has GetRoutingID, so I didn't think having routing_id() was warranted. We can do a mass find/replace in another CL, since I don't want to derail the focus on this one. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/cross_process_frame_connector.h (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:61: // in the child frame's RenderFrameHostManager corresponding to the parent's On 2014/06/19 14:49:04, kenrb wrote: > This is a bit hard to follow, probably good to clarify that it is A2 in the > above illustration. Done. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:62: // SiteInstance. When a child frame swaps, set_view() is called to update to the On 2014/06/19 14:49:04, kenrb wrote: > Are we still using the term 'swaps'? Maybe 'navigates and is moved to a new > process' or something similar. Done. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/cross_process_frame_connector.h:111: // TODO(kenrb): The type becomes RenderFrameProxyHost when that class comes On 2014/06/19 14:49:04, kenrb wrote: > This TODO can go. Done. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 14:49:04, kenrb wrote: > This doesn't look right to me. The original line here existed for intercepting > messages coming from the parent frame renderer, going to the child frame > renderer (e.g. input events, resize). This appears to be going the wrong way. I don't think this is correct and is the reason I've given feedback before on the "child" naming on the method and IPC. The IPC comes from the RenderFrame that is about to navigate cross-process, not from its parent frame. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > Really, this should be in RenderFrameProxyHost, and use its CPFC pointer, but I > don't know if RFPH actually receives messages yet. I contemplated this before and had it coded, but decided to be more explicit that only messages related to CPFC need to be forwarded, not any other IPC. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.cc:29: ->GetSiteInstance() == site_instance) { On 2014/06/19 14:49:04, kenrb wrote: > Is this condition correct? We should be creating a new CPFC if this RFPH is in a > different process than its parent. For now I think this means NOT the same > SiteInstance as its parent, right? Later on I think we will need a better way to > determine whether an RFPH is cross-process. I believe it is. The proxy being created is connecting an RFH in a different process from its parent, right? This means that the connecting object lives in the process of the parent frame, which is the same SiteInstance. What will be in a separate process is the RenderFrameHost, not the proxy. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:74: void SetView(RenderWidgetHostView* view); On 2014/06/19 14:49:04, kenrb wrote: > Nit: 'SetView' is a confusing name, because is makes it sound like a mutator. > The 'view' is really the view for the RenderWidgetHost of the child frame (hence > there is RenderWidgetHostImpl::SetView). 'SetChildRWHView' or something similar > would be a better alternative. Done. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:109: // When a RrenderFrameHost is in a different process from its parent in the On 2014/06/19 14:49:04, kenrb wrote: > Nit: typo RrenderFrameHost Done.
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 15:46:41, nasko wrote: > On 2014/06/19 14:49:04, kenrb wrote: > > This doesn't look right to me. The original line here existed for intercepting > > messages coming from the parent frame renderer, going to the child frame > > renderer (e.g. input events, resize). This appears to be going the wrong way. > > I don't think this is correct and is the reason I've given feedback before on > the "child" naming on the method and IPC. The IPC comes from the RenderFrame > that is about to navigate cross-process, not from its parent frame. > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > I feel like I am missing something here. Does input event forwarding work with this patch in place? https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.cc:29: ->GetSiteInstance() == site_instance) { On 2014/06/19 15:46:41, nasko wrote: > On 2014/06/19 14:49:04, kenrb wrote: > > Is this condition correct? We should be creating a new CPFC if this RFPH is in > a > > different process than its parent. For now I think this means NOT the same > > SiteInstance as its parent, right? Later on I think we will need a better way > to > > determine whether an RFPH is cross-process. > > I believe it is. The proxy being created is connecting an RFH in a different > process from its parent, right? This means that the connecting object lives in > the process of the parent frame, which is the same SiteInstance. What will be in > a separate process is the RenderFrameHost, not the proxy. Ok, I think I might get it. site_instance is the SiteInstance of the process that the RFPH's RenderFrameProxy, and not the SiteInstance of the RenderFrameHostManager for its corresponding frame? That would make sense.
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 16:16:32, kenrb wrote: > On 2014/06/19 15:46:41, nasko wrote: > > On 2014/06/19 14:49:04, kenrb wrote: > > > This doesn't look right to me. The original line here existed for > intercepting > > > messages coming from the parent frame renderer, going to the child frame > > > renderer (e.g. input events, resize). This appears to be going the wrong > way. > > > > I don't think this is correct and is the reason I've given feedback before on > > the "child" naming on the method and IPC. The IPC comes from the RenderFrame > > that is about to navigate cross-process, not from its parent frame. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > I feel like I am missing something here. Does input event forwarding work with > this patch in place? Yes : ). Input events are delivered to the child frame in the parent process, then sent through IPC up to the browser, right? So in the picture of CPFC header, they are delivered to A2, which is the proxy/swapped out frame. This is A2 and it forwards those events to CPFC. Similar code will exist in the proxy object, once the IPCs flow to it instead of the swapped out RFHs. https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.cc:29: ->GetSiteInstance() == site_instance) { On 2014/06/19 16:16:32, kenrb wrote: > On 2014/06/19 15:46:41, nasko wrote: > > On 2014/06/19 14:49:04, kenrb wrote: > > > Is this condition correct? We should be creating a new CPFC if this RFPH is > in > > a > > > different process than its parent. For now I think this means NOT the same > > > SiteInstance as its parent, right? Later on I think we will need a better > way > > to > > > determine whether an RFPH is cross-process. > > > > I believe it is. The proxy being created is connecting an RFH in a different > > process from its parent, right? This means that the connecting object lives in > > the process of the parent frame, which is the same SiteInstance. What will be > in > > a separate process is the RenderFrameHost, not the proxy. > > Ok, I think I might get it. site_instance is the SiteInstance of the process > that the RFPH's RenderFrameProxy, and not the SiteInstance of the > RenderFrameHostManager for its corresponding frame? That would make sense. Cool! I'm glad I could help clarify it.
lgtm https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 16:24:35, nasko wrote: > On 2014/06/19 16:16:32, kenrb wrote: > > On 2014/06/19 15:46:41, nasko wrote: > > > On 2014/06/19 14:49:04, kenrb wrote: > > > > This doesn't look right to me. The original line here existed for > > intercepting > > > > messages coming from the parent frame renderer, going to the child frame > > > > renderer (e.g. input events, resize). This appears to be going the wrong > > way. > > > > > > I don't think this is correct and is the reason I've given feedback before > on > > > the "child" naming on the method and IPC. The IPC comes from the RenderFrame > > > that is about to navigate cross-process, not from its parent frame. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > > > > I feel like I am missing something here. Does input event forwarding work with > > this patch in place? > > Yes : ). Input events are delivered to the child frame in the parent process, > then sent through IPC up to the browser, right? So in the picture of CPFC > header, they are delivered to A2, which is the proxy/swapped out frame. This is > A2 and it forwards those events to CPFC. Similar code will exist in the proxy > object, once the IPCs flow to it instead of the swapped out RFHs. Okay, I find GetProxyToParent() is confusing because this RenderFrameHost is already for a RenderFrame in the parent process. I guess this gets cleared up when RenderFrameHostProxy replaces the A2 RenderFrameHost, rather than coexisting with it? Will GetProxyToParent() be removed at that point? I feel like we shouldn't need that function.
John, can you do an OWNERS review? https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 17:07:08, kenrb wrote: > On 2014/06/19 16:24:35, nasko wrote: > > On 2014/06/19 16:16:32, kenrb wrote: > > > On 2014/06/19 15:46:41, nasko wrote: > > > > On 2014/06/19 14:49:04, kenrb wrote: > > > > > This doesn't look right to me. The original line here existed for > > > intercepting > > > > > messages coming from the parent frame renderer, going to the child frame > > > > > renderer (e.g. input events, resize). This appears to be going the wrong > > > way. > > > > > > > > I don't think this is correct and is the reason I've given feedback before > > on > > > > the "child" naming on the method and IPC. The IPC comes from the > RenderFrame > > > > that is about to navigate cross-process, not from its parent frame. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > > > > > > > I feel like I am missing something here. Does input event forwarding work > with > > > this patch in place? > > > > Yes : ). Input events are delivered to the child frame in the parent process, > > then sent through IPC up to the browser, right? So in the picture of CPFC > > header, they are delivered to A2, which is the proxy/swapped out frame. This > is > > A2 and it forwards those events to CPFC. Similar code will exist in the proxy > > object, once the IPCs flow to it instead of the swapped out RFHs. > > Okay, I find GetProxyToParent() is confusing because this RenderFrameHost is > already for a RenderFrame in the parent process. The goal is to get the proxy, which is in the same SiteInstance as the parent. Do you have any suggestion on how to rename it to be more clear? > I guess this gets cleared up > when RenderFrameHostProxy replaces the A2 RenderFrameHost, rather than > coexisting with it? > > Will GetProxyToParent() be removed at that point? I feel like we shouldn't need > that function. I still think it will be needed in RenderFrameHostManager, since we want to update the RWHV on the proper proxy, right? Though at that point it will be internal to RFHM and should not be needed outside of it I think.
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 17:12:38, nasko wrote: > On 2014/06/19 17:07:08, kenrb wrote: > > On 2014/06/19 16:24:35, nasko wrote: > > > On 2014/06/19 16:16:32, kenrb wrote: > > > > On 2014/06/19 15:46:41, nasko wrote: > > > > > On 2014/06/19 14:49:04, kenrb wrote: > > > > > > This doesn't look right to me. The original line here existed for > > > > intercepting > > > > > > messages coming from the parent frame renderer, going to the child > frame > > > > > > renderer (e.g. input events, resize). This appears to be going the > wrong > > > > way. > > > > > > > > > > I don't think this is correct and is the reason I've given feedback > before > > > on > > > > > the "child" naming on the method and IPC. The IPC comes from the > > RenderFrame > > > > > that is about to navigate cross-process, not from its parent frame. > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > > > > > > > > > > I feel like I am missing something here. Does input event forwarding work > > with > > > > this patch in place? > > > > > > Yes : ). Input events are delivered to the child frame in the parent > process, > > > then sent through IPC up to the browser, right? So in the picture of CPFC > > > header, they are delivered to A2, which is the proxy/swapped out frame. This > > is > > > A2 and it forwards those events to CPFC. Similar code will exist in the > proxy > > > object, once the IPCs flow to it instead of the swapped out RFHs. > > > > Okay, I find GetProxyToParent() is confusing because this RenderFrameHost is > > already for a RenderFrame in the parent process. > > The goal is to get the proxy, which is in the same SiteInstance as the parent. > Do you have any suggestion on how to rename it to be more clear? > > > I guess this gets cleared up > > when RenderFrameHostProxy replaces the A2 RenderFrameHost, rather than > > coexisting with it? > > > > Will GetProxyToParent() be removed at that point? I feel like we shouldn't > need > > that function. > > I still think it will be needed in RenderFrameHostManager, since we want to > update the RWHV on the proper proxy, right? Though at that point it will be > internal to RFHM and should not be needed outside of it I think. Actually, even at the end, we still need this code. The reason is that when executing OnSwapOut in RenderFrame, we mark the WebFrame as remote, which results in calling back into RenderFrame to send out the sizing information. That will still need to be forwarded to the CPFC, so this code will have to stay. If we want to eliminate this code here, sizing data will have to be sent directly to the proxy, potentially handled all within RenderFrame (though I don't have enough data at this point to see if it is possible).
https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/310413004/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.cc:285: proxy->cross_process_frame_connector()->OnMessageReceived(msg)) On 2014/06/19 17:52:05, nasko wrote: > Actually, even at the end, we still need this code. The reason is that when > executing OnSwapOut in RenderFrame, we mark the WebFrame as remote, which > results in calling back into RenderFrame to send out the sizing information. > That will still need to be forwarded to the CPFC, so this code will have to > stay. > If we want to eliminate this code here, sizing data will have to be sent > directly to the proxy, potentially handled all within RenderFrame (though I > don't have enough data at this point to see if it is possible). My intent was that everything would go through the proxy, rather than having two code paths for the in-process child frame to out-of-process child frame communication channel. I think that would be preferable if possible.
lgtm for site_per_process_browsertest.cc, you're an owner for frame_host so no need for me to review that (you know this a lot more than me too!)
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/310413004/80001
The CQ bit was checked by nasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nasko@chromium.org/310413004/80001
Message was sent while issue was closed.
Change committed as 278532 |
