Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
404613005:
Start using RenderFrameProxyHost objects. (Closed)
|
Created:
6 years, 5 months ago by kenrb Modified:
6 years, 4 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionStart using RenderFrameProxyHost objects.
Instantiate RenderFrameProxyHost objects for remote frames when using
--site-per-process.
BUG=357747
Patch Set 1 #
Total comments: 19
Patch Set 2 : Rebase #Patch Set 3 : cosmetics #Patch Set 4 : Bug fix + ncarter review comments addressed #
Total comments: 88
Patch Set 5 : Rebase, bug fixes, addressed creis' comments #Patch Set 6 : Rebase #Patch Set 7 : Compile fix #
Total comments: 23
Patch Set 8 : more comments addressed #
Total comments: 5
Patch Set 9 : more tweaks #
Total comments: 3
Patch Set 10 : Removed unneeded CHECK #
Total comments: 1
Patch Set 11 : Rebased and fixed test failures #Patch Set 12 : Fix merge issues #Patch Set 13 : Disabled failing test #Patch Set 14 : Another test disabled #Messages
Total messages: 34 (0 generated)
This continues the work from https://codereview.chromium.org/241223002/. Originally Nasko's patch.
https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree.cc:61: if (source_node == node) Should this check be |source_node == node|, or |node->site_instance() == instance| ? I had expected the latter. Seems like we don't need proxies for in-process frames. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:370: DCHECK(GetProcess()->GetBrowserContext()); This doesn't look like a particularly useful DCHECK -- from what I can see, a RPH always has a browser context. Just checking HasConnection() is fine (Init() guarantees this) https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1040: int opener_route_id) { |opener_route_id| is unused (probably intentionally). Where is it headed? It only belongs as a parameter here, if we expect that we'll communicate |opener_route_id| to existing proxies (if this function is called when a proxy already exists) as well as new ones. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1263: } To be clear, this now means we're freeing old_render_frame_host upon return from this function (for the subframe case). https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1270: ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id); Is this function appropriately named after this CL? Should it be something like ShutdownRenderFrameProxyHostsInSiteInstance? https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1529: proxy_hosts_[site_instance->GetId()] = proxy; If it's really necessary to do this both here and on line 1525, it deserves a comment. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.cc:45: } These lines seem to have no effect. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.cc:101: DCHECK(GetProcess()->GetBrowserContext()); This DCHECK can be dropped; from what I can tell a renderprocesshost is never without a browsercontext, even before Init(). https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.h:66: // Initializaes the object and creates the RenderFrameProxy in the process zaes -> zes
Adding Charlie as a reviewer. Nick: do you mind taking a look again? https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree.cc:61: if (source_node == node) On 2014/07/21 20:31:56, ncarter wrote: > Should this check be |source_node == node|, or > |node->site_instance() == instance| ? > > I had expected the latter. Seems like we don't need proxies for in-process > frames. I think if we are populating a new process with a frame tree, then there should be only one ft node in that site instance. The CHECK in CreateRenderFrameProxy should catch if this is violated. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_impl.cc:370: DCHECK(GetProcess()->GetBrowserContext()); On 2014/07/21 20:31:56, ncarter wrote: > This doesn't look like a particularly useful DCHECK -- from what I can see, a > RPH always has a browser context. Just checking HasConnection() is fine (Init() > guarantees this) Removed. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1040: int opener_route_id) { On 2014/07/21 20:31:56, ncarter wrote: > |opener_route_id| is unused (probably intentionally). Where is it headed? It > only belongs as a parameter here, if we expect that we'll communicate > |opener_route_id| to existing proxies (if this function is called when a proxy > already exists) as well as new ones. I am removing it for now. I think Nasko intends to do something like that with it but he can re-add it later. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1263: } On 2014/07/21 20:31:56, ncarter wrote: > To be clear, this now means we're freeing old_render_frame_host upon return from > this function (for the subframe case). This appears to be correct, that we are destroying the old RFH when it will be replaced with a RFPH. The old one is still needed for main frame swap-out. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1270: ShutdownRenderFrameHostsInSiteInstance(old_site_instance_id); On 2014/07/21 20:31:56, ncarter wrote: > Is this function appropriately named after this CL? Should it be something like > ShutdownRenderFrameProxyHostsInSiteInstance? I think so; changed. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_host_manager.cc:1529: proxy_hosts_[site_instance->GetId()] = proxy; On 2014/07/21 20:31:56, ncarter wrote: > If it's really necessary to do this both here and on line 1525, it deserves a > comment. Removed. This looks like a no-op. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_proxy_host.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.cc:45: } On 2014/07/21 20:31:57, ncarter wrote: > These lines seem to have no effect. Removed. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.cc:101: DCHECK(GetProcess()->GetBrowserContext()); On 2014/07/21 20:31:57, ncarter wrote: > This DCHECK can be dropped; from what I can tell a renderprocesshost is never > without a browsercontext, even before Init(). Removed. https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/r... content/browser/frame_host/render_frame_proxy_host.h:66: // Initializaes the object and creates the RenderFrameProxy in the process On 2014/07/21 20:31:57, ncarter wrote: > zaes -> zes Done.
lgtm! https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/404613005/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree.cc:61: if (source_node == node) On 2014/07/24 17:53:53, kenrb wrote: > On 2014/07/21 20:31:56, ncarter wrote: > > Should this check be |source_node == node|, or > > |node->site_instance() == instance| ? > > > > I had expected the latter. Seems like we don't need proxies for in-process > > frames. > > I think if we are populating a new process with a frame tree, then there should > be only one ft node in that site instance. The CHECK in CreateRenderFrameProxy > should catch if this is violated. Acknowledged.
https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:711: if (!frame_->document().isNull()) I don't understand why we need this check. Under what circumstances are we going to end up with a null WebDocument for a WebLocalFrame? It doesn't really seem like this should ever happen, so I'm curious if it's because of our transition code or something else.
https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:711: if (!frame_->document().isNull()) On 2014/07/24 18:23:28, dcheng (OOO) wrote: > I don't understand why we need this check. Under what circumstances are we going > to end up with a null WebDocument for a WebLocalFrame? It doesn't really seem > like this should ever happen, so I'm curious if it's because of our transition > code or something else. Right now we have RenderFrameProxy wrapping a RenderFrameImpl, and in that situation there is a null document. This is only temporary, if I understand all of this correctly. I will add a comment for that. I rebased today, and now there is a crash bug that looks like a similar problem to that.
This is exciting. I'm eager to see us start creating WebRemoteFrames! Sorry for all the comments below, but I want to be sure I understand this. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:75: // given |site_instance| in every node except the |source| one. This is pretty ambiguous as written, so let's expand it a bit. What happens if there's already an active RenderFrameHost or an existing RenderFrameProxyHost for the SiteInstance in a given frame? Presumably it leaves the frame alone, rather than creating a RFPH? Maybe it would help to say what precondition we're expecting (e.g., the SiteInstance isn't present in any of the frames of the tree so far?). https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:78: const scoped_refptr<SiteInstance>& site_instance); Why pass the scoped_refptr& here when we're just passing it as a SiteInstance* in the body? Seems like the other methods on this class also use SiteInstance*, so I'm wondering whether there's a need for the scoped_refptr in this case. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_browsertest.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_browsertest.cc:174: IN_PROC_BROWSER_TEST_F(CrossProcessFrameTreeBrowserTest, VerifyProxyCreation) { nit: VerifyProxyCreation makes it sound like a network test, and it's just one aspect being tested here. Maybe CrossProcessSubframeProxies? I'm also a little unclear what the difference between this test class and SitePerProcessBrowserTest is. The latter already has a similar test called CrossSiteIframe which is verifying things like process creation. Do we only need this here because it's conceptually about the frame tree? (That might be fine, but it's worth thinking about before we split our --site-per-process tests into several places.) https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_browsertest.cc:210: EXPECT_TRUE(root->render_manager()->proxy_hosts_[site_instance->GetId()]); Are we creating the proxy in the subframe for the root's SiteInstance yet? It would be good to test that's there as well, if so. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:881: // FIXME (nasko, creis): An out-of-process child frame has no way of We use TODO(...) rather than FIXME in the Chrome repo. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:883: // semantics here should be re-evaluated during session history refactor. Can you add a mention of http://crbug.com/236848 to this sentence? https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:884: // For now, we assume this means the child frame loaded and proceed. "Note that this may do the wrong thing for cross-process AUTO_SUBFRAME navigations." https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:116: bool has_received_swap_out_ack() { return has_received_swap_out_ack_; } This doesn't appear to be called. Is it stale code? https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:943: // TODO(nasko): Remove this check once cross-site navigation no longer "this check" is ambiguous. Do this mean we want to always do the CHECK and remove the "if," or does it mean we should eventually remove the CHECK? https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); When do for_main_frame and frame_tree_node_->IsMainFrame() differ? It makes this code kind of hard to follow, since it's not clear why we're checking one vs the other. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1326: // which expects properly ref-counted object, so use a scoped_refptr for it. Why does it expect that? As mentioned earlier, it's just passing it to CreateRenderFrameProxy, which takes a SiteInstance*. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:261: // Helper method to create and initialize a RenderFrameProxyHost. Please say what this returns. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:316: int GetRoutingIdForSiteInstance(SiteInstance* site_instance); This name is confusing, since routing IDs are used for many things but SiteInstances don't have them. Is it for the RenderFrameHost with the given SiteInstance in this RenderFrameHostManager? What does it return if there is none? Let's add a comment and maybe change the name to something like GetFrameRoutingIdForSiteInstance. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: bool InitRenderFrame(RenderFrameHost* render_frame_host); This needs a comment. Is the behavior similar to InitRenderView above? https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:438: // swapped out. Nice! We're getting closer to where we won't have RenderFrameHosts that are swapped out. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:63: return site_instance_->HasProcess() ? site_instance_->GetProcess() : NULL; When does this matter? I'd like to avoid adding another variation on the meaning of "GetProcess" if we can. https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:4100: // RenderWidgetHostView or a new one should be created here. Is this the reason we need this method to be in WebContentsImpl? It looks like the current code could just live in RFHM, but it's fine to have it here if we know there's going to be widget/view stuff coming soon. https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... File content/common/frame_messages.h (right): https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:337: // The new frame should be created as a child of the object idendified by identified https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:338: // |parent_routing_id| or as top level if the id is MSG_ROUTING_NONE. "the id" -> that (Assuming we're talking about parent_routing_id. "The id" could refer to either of the IDs.) https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:345: // idendified by |parent_routing_id| or as top level if the id is Same as above. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:373: CHECK_NE(MSG_ROUTING_NONE, parent_routing_id); Wait, the comments on this function and the IPC message said this is allowed. Which one is wrong? https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:377: // If the browser is sending a valid parent routing id, it better be created it better be -> it should already be https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1003: is_swapped_out_ = true; Do we need RenderFrameImpl::is_swapped_out_ anymore? Sounds like we delete the RenderFrameImpl instead of putting it in a swapped out state. (I'm ok leaving that for another CL, but we should at least have a TODO if that's true.) https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1049: if (is_site_per_process) { When would this be false? We don't swap out subframes unless --site-per-process is passed. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1050: // TODO(nasko): delete the frame here, since we've replaced it with a Can we do this now? (Is it a leak if we don't?) https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1749: WebNavigationPolicy value = DecidePolicyForNavigation( Why is this change needed? (Leftover from an earlier change?) https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1857: routing_id_, ds->request().url())); This indent looks wrong. I think it was fine before, or it should need another 4 spaces. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2427: // This is broken and should be fixed to propagate the URL. URL -> first party https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2437: // TODO(nasko): Hack around asking about top-frame data source. This could lead to some nasty bugs. We should elaborate a bit here (e.g., mention site-per-process or something) to make sure we don't forget about this. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:3260: // There's no reason to ignore navigations on subframes (why again?) "(why again?)"... ? Perhaps this is because subframes get deleted rather than swapped out? https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.h:95: // identified by |parent_routing_id| or as the top-level frame if if the latter is https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_thread_impl.cc:1277: void RenderThreadImpl::OnSetZoomLevelForCurrentURL(const std::string& scheme, Is this from a merge conflict? It seems unrelated to the rest. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_thread_impl.cc:1321: // TODO(nasko): For now, this message is only sent for subframes, as the "For now" -> do we expect that to change? Why? https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_view_impl.cc:3434: for (WebFrame* frame = main_frame; frame; This loop doesn't make sense to me. Wouldn't it end up at the last local frame in the tree? Why are we looking for that?
On 2014/07/24 at 21:06:17, kenrb wrote: > https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... > content/renderer/render_frame_impl.cc:711: if (!frame_->document().isNull()) > On 2014/07/24 18:23:28, dcheng (OOO) wrote: > > I don't understand why we need this check. Under what circumstances are we going > > to end up with a null WebDocument for a WebLocalFrame? It doesn't really seem > > like this should ever happen, so I'm curious if it's because of our transition > > code or something else. > > Right now we have RenderFrameProxy wrapping a RenderFrameImpl, and in that situation there is a null document. This is only temporary, if I understand all of this correctly. I will add a comment for that. > > I rebased today, and now there is a crash bug that looks like a similar problem to that. Why is the document null in those cases? I thought that in those cases, the frame would be navigated to swappedout://, so there should still be a non-null Document.
A drive-by to address some of Charlie's comments and provide some context. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:78: const scoped_refptr<SiteInstance>& site_instance); On 2014/07/24 22:36:29, Charlie Reis wrote: > Why pass the scoped_refptr& here when we're just passing it as a SiteInstance* > in the body? Seems like the other methods on this class also use SiteInstance*, > so I'm wondering whether there's a need for the scoped_refptr in this case. See my response in RFHM. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_browsertest.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_browsertest.cc:210: EXPECT_TRUE(root->render_manager()->proxy_hosts_[site_instance->GetId()]); On 2014/07/24 22:36:29, Charlie Reis wrote: > Are we creating the proxy in the subframe for the root's SiteInstance yet? It > would be good to test that's there as well, if so. That should be created by the virtue of cross-process navigation. Once we swap out the old renderer, we will get a proxy for the root SiteInstance. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:116: bool has_received_swap_out_ack() { return has_received_swap_out_ack_; } On 2014/07/24 22:36:29, Charlie Reis wrote: > This doesn't appear to be called. Is it stale code? I added this to know at time of CommitPending whether to delete the RFH or not. I don't recall what happened to that code. I think it is safe to remove for now and we can always add it later. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:943: // TODO(nasko): Remove this check once cross-site navigation no longer On 2014/07/24 22:36:29, Charlie Reis wrote: > "this check" is ambiguous. Do this mean we want to always do the CHECK and > remove the "if," or does it mean we should eventually remove the CHECK? Remove the whole block, as I expect the "swapped_out" param will just disappear. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1326: // which expects properly ref-counted object, so use a scoped_refptr for it. On 2014/07/24 22:36:29, Charlie Reis wrote: > Why does it expect that? As mentioned earlier, it's just passing it to > CreateRenderFrameProxy, which takes a SiteInstance*. I think I wrote the initial code for CreateProxiesForSiteInstance to accept scoped_refptr and made this change due to that. I think you are right that we can just pass in the SiteInstance pointer and it should work fine. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:316: int GetRoutingIdForSiteInstance(SiteInstance* site_instance); On 2014/07/24 22:36:30, Charlie Reis wrote: > This name is confusing, since routing IDs are used for many things but > SiteInstances don't have them. Is it for the RenderFrameHost with the given > SiteInstance in this RenderFrameHostManager? What does it return if there is > none? > > Let's add a comment and maybe change the name to something like > GetFrameRoutingIdForSiteInstance. It is scoped to RFHM and is supposed to return the routing id of either RFH or RFPH, which is associated with |site_instance|. This method is used to find out the routing id for the parent object to send for creation IPCs. The reason I didn't add Frame to the name is that the parent might be in a different process, at which point there will exist a proxy for the SiteInstance of the object we are creating, not a frame. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: bool InitRenderFrame(RenderFrameHost* render_frame_host); On 2014/07/24 22:36:30, Charlie Reis wrote: > This needs a comment. Is the behavior similar to InitRenderView above? Yes, it is. In general, I've made RFH/RFPH creation mimick how RVHs are created. Similarly named methods should behave similarly. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:63: return site_instance_->HasProcess() ? site_instance_->GetProcess() : NULL; On 2014/07/24 22:36:30, Charlie Reis wrote: > When does this matter? I'd like to avoid adding another variation on the > meaning of "GetProcess" if we can. Initially it was meant to not create a process if there wasn't one. I think in our pair code review with Nick we analyzed it and concluded that the HasProcess() check isn't needed and should just return the result of the GetProcess() call. https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:4100: // RenderWidgetHostView or a new one should be created here. On 2014/07/24 22:36:30, Charlie Reis wrote: > Is this the reason we need this method to be in WebContentsImpl? It looks like > the current code could just live in RFHM, but it's fine to have it here if we > know there's going to be widget/view stuff coming soon. The reason it is here is to deal with widgets and views, which are unknown at the RFHM level. Just like the RVH case above. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1003: is_swapped_out_ = true; On 2014/07/24 22:36:31, Charlie Reis wrote: > Do we need RenderFrameImpl::is_swapped_out_ anymore? Sounds like we delete the > RenderFrameImpl instead of putting it in a swapped out state. > > (I'm ok leaving that for another CL, but we should at least have a TODO if > that's true.) I think a TODO to revisit this is good. I recall trying to get rid of swapped out on RFH, but there was something that prevented me from doing it yet. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1049: if (is_site_per_process) { On 2014/07/24 22:36:31, Charlie Reis wrote: > When would this be false? We don't swap out subframes unless --site-per-process > is passed. It shouldn't be false. We will only have proxies for subframes with --site-per-process. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1050: // TODO(nasko): delete the frame here, since we've replaced it with a On 2014/07/24 22:36:30, Charlie Reis wrote: > Can we do this now? (Is it a leak if we don't?) It likely is a leak. I left this piece for a follow up CL, since --site-per-process is not the default and I didn't want to make this even more complex. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1749: WebNavigationPolicy value = DecidePolicyForNavigation( On 2014/07/24 22:36:31, Charlie Reis wrote: > Why is this change needed? (Leftover from an earlier change?) Most likely leftover from debugging statements using the return value. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:3260: // There's no reason to ignore navigations on subframes (why again?) On 2014/07/24 22:36:31, Charlie Reis wrote: > "(why again?)"... ? > > Perhaps this is because subframes get deleted rather than swapped out? Yes, subframes don't use swapout logic, so the existing code block doesn't apply. The code can be written a bit better, this was just to unblock things. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_thread_impl.cc (left): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_thread_impl.cc:1277: void RenderThreadImpl::OnSetZoomLevelForCurrentURL(const std::string& scheme, On 2014/07/24 22:36:32, Charlie Reis wrote: > Is this from a merge conflict? It seems unrelated to the rest. I moved it to be ordered according to the .h file. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_thread_impl.cc:1321: // TODO(nasko): For now, this message is only sent for subframes, as the On 2014/07/24 22:36:32, Charlie Reis wrote: > "For now" -> do we expect that to change? Why? Yes. Once we eliminate RenderView(Host), we need to create the top-level frame somehow. I can imagine it done just like the others using this IPC.
https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:116: bool has_received_swap_out_ack() { return has_received_swap_out_ack_; } On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:29, Charlie Reis wrote: > > This doesn't appear to be called. Is it stale code? > > I added this to know at time of CommitPending whether to delete the RFH or not. > I don't recall what happened to that code. I think it is safe to remove for now > and we can always add it later. I think if we remove the getter then we can also remove the member too.
https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:75: // given |site_instance| in every node except the |source| one. On 2014/07/24 22:36:29, Charlie Reis wrote: > This is pretty ambiguous as written, so let's expand it a bit. What happens if > there's already an active RenderFrameHost or an existing RenderFrameProxyHost > for the SiteInstance in a given frame? Presumably it leaves the frame alone, > rather than creating a RFPH? > > Maybe it would help to say what precondition we're expecting (e.g., the > SiteInstance isn't present in any of the frames of the tree so far?). I have tried to clarify this. Please review the new comment. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:78: const scoped_refptr<SiteInstance>& site_instance); On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:29, Charlie Reis wrote: > > Why pass the scoped_refptr& here when we're just passing it as a SiteInstance* > > in the body? Seems like the other methods on this class also use > SiteInstance*, > > so I'm wondering whether there's a need for the scoped_refptr in this case. > > See my response in RFHM. I have changed it to a SiteInstance*, but it needs to be scoped_refptr because base::Bind requires it for any refcounted types. I am making it create one within the method, though, to simplify the interface. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_browsertest.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_browsertest.cc:174: IN_PROC_BROWSER_TEST_F(CrossProcessFrameTreeBrowserTest, VerifyProxyCreation) { On 2014/07/24 22:36:29, Charlie Reis wrote: > nit: VerifyProxyCreation makes it sound like a network test, and it's just one > aspect being tested here. Maybe CrossProcessSubframeProxies? > > I'm also a little unclear what the difference between this test class and > SitePerProcessBrowserTest is. The latter already has a similar test called > CrossSiteIframe which is verifying things like process creation. Do we only > need this here because it's conceptually about the frame tree? (That might be > fine, but it's worth thinking about before we split our --site-per-process tests > into several places.) I've changed the test name but left the class name alone. This can be worked out later as we improve tests. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:881: // FIXME (nasko, creis): An out-of-process child frame has no way of On 2014/07/24 22:36:29, Charlie Reis wrote: > We use TODO(...) rather than FIXME in the Chrome repo. Fixed. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:883: // semantics here should be re-evaluated during session history refactor. On 2014/07/24 22:36:29, Charlie Reis wrote: > Can you add a mention of http://crbug.com/236848 to this sentence? Done. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigation_controller_impl.cc:884: // For now, we assume this means the child frame loaded and proceed. On 2014/07/24 22:36:29, Charlie Reis wrote: > "Note that this may do the wrong thing for cross-process AUTO_SUBFRAME > navigations." Done. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_impl.h:116: bool has_received_swap_out_ack() { return has_received_swap_out_ack_; } On 2014/07/25 17:18:16, ncarter wrote: > On 2014/07/25 07:13:20, nasko (in Munich) wrote: > > On 2014/07/24 22:36:29, Charlie Reis wrote: > > > This doesn't appear to be called. Is it stale code? > > > > I added this to know at time of CommitPending whether to delete the RFH or > not. > > I don't recall what happened to that code. I think it is safe to remove for > now > > and we can always add it later. > > I think if we remove the getter then we can also remove the member too. Removed. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:943: // TODO(nasko): Remove this check once cross-site navigation no longer On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:29, Charlie Reis wrote: > > "this check" is ambiguous. Do this mean we want to always do the CHECK and > > remove the "if," or does it mean we should eventually remove the CHECK? > > Remove the whole block, as I expect the "swapped_out" param will just disappear. I capitalized CHECK to remove the ambiguity. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); On 2014/07/24 22:36:30, Charlie Reis wrote: > When do for_main_frame and frame_tree_node_->IsMainFrame() differ? It makes > this code kind of hard to follow, since it's not clear why we're checking one vs > the other. Can you follow up on this with Nasko? I am not clear on the answer, but it is clear that there is a distinction. for_main_frame indicates that this RenderView will get a platform RenderWidgetHostView rather than a RenderWidgetHostViewChildFrame, but I see cases where for_main_frame is false and IsMainFrame() is true. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1326: // which expects properly ref-counted object, so use a scoped_refptr for it. On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:29, Charlie Reis wrote: > > Why does it expect that? As mentioned earlier, it's just passing it to > > CreateRenderFrameProxy, which takes a SiteInstance*. > > I think I wrote the initial code for CreateProxiesForSiteInstance to accept > scoped_refptr and made this change due to that. I think you are right that we > can just pass in the SiteInstance pointer and it should work fine. Changed to SiteInstance*. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:261: // Helper method to create and initialize a RenderFrameProxyHost. On 2014/07/24 22:36:30, Charlie Reis wrote: > Please say what this returns. Done. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:316: int GetRoutingIdForSiteInstance(SiteInstance* site_instance); On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > This name is confusing, since routing IDs are used for many things but > > SiteInstances don't have them. Is it for the RenderFrameHost with the given > > SiteInstance in this RenderFrameHostManager? What does it return if there is > > none? > > > > Let's add a comment and maybe change the name to something like > > GetFrameRoutingIdForSiteInstance. > > It is scoped to RFHM and is supposed to return the routing id of either RFH or > RFPH, which is associated with |site_instance|. This method is used to find out > the routing id for the parent object to send for creation IPCs. The reason I > didn't add Frame to the name is that the parent might be in a different process, > at which point there will exist a proxy for the SiteInstance of the object we > are creating, not a frame. I have added a comment but left the name the same. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:429: bool InitRenderFrame(RenderFrameHost* render_frame_host); On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > This needs a comment. Is the behavior similar to InitRenderView above? > > Yes, it is. In general, I've made RFH/RFPH creation mimick how RVHs are created. > Similarly named methods should behave similarly. I have added a short comment. Let me know if you want something different. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_proxy_host.h:63: return site_instance_->HasProcess() ? site_instance_->GetProcess() : NULL; On 2014/07/25 07:13:20, nasko (in Munich) wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > When does this matter? I'd like to avoid adding another variation on the > > meaning of "GetProcess" if we can. > > Initially it was meant to not create a process if there wasn't one. I think in > our pair code review with Nick we analyzed it and concluded that the > HasProcess() check isn't needed and should just return the result of the > GetProcess() call. Changed. https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... File content/common/frame_messages.h (right): https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:337: // The new frame should be created as a child of the object idendified by On 2014/07/24 22:36:30, Charlie Reis wrote: > identified Done. https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:338: // |parent_routing_id| or as top level if the id is MSG_ROUTING_NONE. On 2014/07/24 22:36:30, Charlie Reis wrote: > "the id" -> that > > (Assuming we're talking about parent_routing_id. "The id" could refer to either > of the IDs.) Done. https://codereview.chromium.org/404613005/diff/60001/content/common/frame_mes... content/common/frame_messages.h:345: // idendified by |parent_routing_id| or as top level if the id is On 2014/07/24 22:36:30, Charlie Reis wrote: > Same as above. Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:373: CHECK_NE(MSG_ROUTING_NONE, parent_routing_id); On 2014/07/24 22:36:30, Charlie Reis wrote: > Wait, the comments on this function and the IPC message said this is allowed. > Which one is wrong? I don't know what is intended here, so I left a TODO for nasko. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:377: // If the browser is sending a valid parent routing id, it better be created On 2014/07/24 22:36:31, Charlie Reis wrote: > it better be -> it should already be Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1003: is_swapped_out_ = true; On 2014/07/25 07:13:21, nasko (in Munich) wrote: > On 2014/07/24 22:36:31, Charlie Reis wrote: > > Do we need RenderFrameImpl::is_swapped_out_ anymore? Sounds like we delete > the > > RenderFrameImpl instead of putting it in a swapped out state. > > > > (I'm ok leaving that for another CL, but we should at least have a TODO if > > that's true.) > > I think a TODO to revisit this is good. I recall trying to get rid of swapped > out on RFH, but there was something that prevented me from doing it yet. TODO added. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1049: if (is_site_per_process) { On 2014/07/25 07:13:21, nasko (in Munich) wrote: > On 2014/07/24 22:36:31, Charlie Reis wrote: > > When would this be false? We don't swap out subframes unless > --site-per-process > > is passed. > > It shouldn't be false. We will only have proxies for subframes with > --site-per-process. I will leave these for Nasko's follow-up CL. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1749: WebNavigationPolicy value = DecidePolicyForNavigation( On 2014/07/25 07:13:21, nasko (in Munich) wrote: > On 2014/07/24 22:36:31, Charlie Reis wrote: > > Why is this change needed? (Leftover from an earlier change?) > > Most likely leftover from debugging statements using the return value. Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:1857: routing_id_, ds->request().url())); On 2014/07/24 22:36:31, Charlie Reis wrote: > This indent looks wrong. I think it was fine before, or it should need another > 4 spaces. Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2427: // This is broken and should be fixed to propagate the URL. On 2014/07/24 22:36:31, Charlie Reis wrote: > URL -> first party Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:2437: // TODO(nasko): Hack around asking about top-frame data source. On 2014/07/24 22:36:31, Charlie Reis wrote: > This could lead to some nasty bugs. We should elaborate a bit here (e.g., > mention site-per-process or something) to make sure we don't forget about this. Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.cc:3260: // There's no reason to ignore navigations on subframes (why again?) On 2014/07/25 07:13:21, nasko (in Munich) wrote: > On 2014/07/24 22:36:31, Charlie Reis wrote: > > "(why again?)"... ? > > > > Perhaps this is because subframes get deleted rather than swapped out? > > Yes, subframes don't use swapout logic, so the existing code block doesn't > apply. The code can be written a bit better, this was just to unblock things. Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_frame_impl.h:95: // identified by |parent_routing_id| or as the top-level frame if On 2014/07/24 22:36:31, Charlie Reis wrote: > if the latter is Done. https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/renderer/render_... content/renderer/render_view_impl.cc:3434: for (WebFrame* frame = main_frame; frame; On 2014/07/24 22:36:32, Charlie Reis wrote: > This loop doesn't make sense to me. Wouldn't it end up at the last local frame > in the tree? Why are we looking for that? Changed to find the first localFrame in the tree. Also I added a comment since this is another hack.
Thanks-- next round of comments below. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:78: const scoped_refptr<SiteInstance>& site_instance); On 2014/07/25 23:42:04, kenrb wrote: > On 2014/07/25 07:13:20, nasko (in Munich) wrote: > > On 2014/07/24 22:36:29, Charlie Reis wrote: > > > Why pass the scoped_refptr& here when we're just passing it as a > SiteInstance* > > > in the body? Seems like the other methods on this class also use > > SiteInstance*, > > > so I'm wondering whether there's a need for the scoped_refptr in this case. > > > > See my response in RFHM. > > I have changed it to a SiteInstance*, but it needs to be scoped_refptr because > base::Bind requires it for any refcounted types. I am making it create one > within the method, though, to simplify the interface. Oh, there was a reason. Darn. This approach should work, but let's mention that it creates a scoped_refptr for |site_instance| in the declaration comment, since that can affect behavior (e.g., deleting it if there are no other references to it when you call this method). https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); On 2014/07/25 23:42:05, kenrb wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > When do for_main_frame and frame_tree_node_->IsMainFrame() differ? It makes > > this code kind of hard to follow, since it's not clear why we're checking one > vs > > the other. > > Can you follow up on this with Nasko? I am not clear on the answer, but it is > clear that there is a distinction. for_main_frame indicates that this RenderView > will get a platform RenderWidgetHostView rather than a > RenderWidgetHostViewChildFrame, but I see cases where for_main_frame is false > and IsMainFrame() is true. I think we've worked it out (mostly). See my later comments. https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.h:316: int GetRoutingIdForSiteInstance(SiteInstance* site_instance); On 2014/07/25 23:42:05, kenrb wrote: > On 2014/07/25 07:13:20, nasko (in Munich) wrote: > > On 2014/07/24 22:36:30, Charlie Reis wrote: > > > This name is confusing, since routing IDs are used for many things but > > > SiteInstances don't have them. Is it for the RenderFrameHost with the given > > > SiteInstance in this RenderFrameHostManager? What does it return if there > is > > > none? > > > > > > Let's add a comment and maybe change the name to something like > > > GetFrameRoutingIdForSiteInstance. > > > > It is scoped to RFHM and is supposed to return the routing id of either RFH or > > RFPH, which is associated with |site_instance|. This method is used to find > out > > the routing id for the parent object to send for creation IPCs. The reason I > > didn't add Frame to the name is that the parent might be in a different > process, > > at which point there will exist a proxy for the SiteInstance of the object we > > are creating, not a frame. > > I have added a comment but left the name the same. Acknowledged. https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:4100: // RenderWidgetHostView or a new one should be created here. On 2014/07/25 07:13:21, nasko (in Munich) wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > Is this the reason we need this method to be in WebContentsImpl? It looks > like > > the current code could just live in RFHM, but it's fine to have it here if we > > know there's going to be widget/view stuff coming soon. > > The reason it is here is to deal with widgets and views, which are unknown at > the RFHM level. Just like the RVH case above. Acknowledged. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree.h:74: // When a new process is being created to render content from a specific site This is still a bit subtle-- it's assuming that "a new process being created" implies there's no proxies with |site_instance| yet. What if the process had crashed and we're recreating it? If the code is making an assumption that none of the frames in the tree have a proxy for |site_instance| yet, we should state that. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree_browsertest.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree_browsertest.cc:211: EXPECT_TRUE(root->render_manager()->proxy_hosts_[site_instance->GetId()]); Nasko mentioned we should have a proxy in the subframe. Can you add a check like this? // Also ensure that the child has a proxy for the root node's SiteInstance. EXPECT_TRUE(child->render_manager()->proxy_hosts_[root->current_frame_host()->GetSiteInstance()]); https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); Ok, Nick and I looked into this, and I can confirm that for_main_frame and IsMainFrame are different in the case below. I'm not 100% sure if it behaves correctly, but it might be. During a cross-site iframe navigation, the subframe's RFHM::UpdateStateForNavigate will call RFHM::CreateRenderFrame on the *root* RFHM, passing for_main_frame==false. That's pretty confusing but I can kind of see how it's "not for a main frame navigation." The goal is just to create a swapped out RVH / RFH in the top-level FrameTreeNode. From here, we pass |for_main_frame| into InitRenderView, which passes it to WebContents::CreateRenderViewForRenderManager, which creates a RenderWidgetHostViewChildFrame. Is that what we want? Are we supposed to be creating a RWHVCF for the top-level swapped out RFH/RVH? (It's *not* what we do for CreateSwappedOutRenderView, where we call CreateViewForWidget rather than creating a RWHVCF. But maybe it's what we want here?) Assuming that's correct, I still think it's too confusing to have different for_main_frame and IsMainFrame concepts here. Let's at least change the name of the parameter to for_main_frame_navigation, unless you see any better ways to make it simpler. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1372: // Create the swapped out RVH for the new SiteInstance. This will create Nick had a good suggestion here. This first block is awkward to follow because a subframe RFHM is directly telling the main frame RFHM to do some detailed operations (creating a RVH/RFH). It then tells the FrameTree to make sure there's a proxy in each node, which makes more sense. It seems preferable for this call site *only* tells the FrameTree to get the proxies created. FrameTree::CreateProxiesForSiteInstance should do this block (lines 1372-1386) internally, such that it creates the RVH if needed before walking the tree. That cleans up this call site and seems easier to follow because we don't have one RFHM directly taking action on another. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1390: frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance( We should mention that this call creates a scoped_refptr for new_instance, so it's important to make the call *after* the CreateRenderFrame call. (Otherwise the refcount could go to zero before we actually use it.) https://codereview.chromium.org/404613005/diff/120001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/404613005/diff/120001/content/common/frame_me... content/common/frame_messages.h:356: // idendified by |parent_routing_id| or as top level if that is identified https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:379: // TODO(nasko): The IPC documentation for this message says Let's replace this with the comment from RenderThreadImpl. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:720: // FIXME: document() should not be null, but as a transitional step nit: TODO(kenrb): https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:3296: // There's no reason to ignore navigations on subframes, swap out logic nit: subframes, since the swap out logic no longer applies to them. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_thread_impl.cc:1346: // ViewMsg_New IPC. This answers my question. The IPC documentation says this is allowed, but this check says we don't support it yet (but will in the future). That's fine. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_thread_impl.cc:1347: CHECK(parent_routing_id != MSG_ROUTING_NONE); This is redundant with the CHECK_NE in RenderFrameImpl::CreateFrame. Let's delete this CHECK and move the comment to RenderFrameImpl::CreateFrame.
https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree.h:74: // When a new process is being created to render content from a specific site On 2014/07/28 19:24:28, Charlie Reis wrote: > This is still a bit subtle-- it's assuming that "a new process being created" > implies there's no proxies with |site_instance| yet. What if the process had > crashed and we're recreating it? > > If the code is making an assumption that none of the frames in the tree have a > proxy for |site_instance| yet, we should state that. Done. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree_browsertest.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree_browsertest.cc:211: EXPECT_TRUE(root->render_manager()->proxy_hosts_[site_instance->GetId()]); On 2014/07/28 19:24:28, Charlie Reis wrote: > Nasko mentioned we should have a proxy in the subframe. Can you add a check > like this? > > // Also ensure that the child has a proxy for the root node's SiteInstance. > EXPECT_TRUE(child->render_manager()->proxy_hosts_[root->current_frame_host()->GetSiteInstance()]); Done. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); On 2014/07/28 19:24:28, Charlie Reis wrote: > Ok, Nick and I looked into this, and I can confirm that for_main_frame and > IsMainFrame are different in the case below. I'm not 100% sure if it behaves > correctly, but it might be. > > During a cross-site iframe navigation, the subframe's > RFHM::UpdateStateForNavigate will call RFHM::CreateRenderFrame on the *root* > RFHM, passing for_main_frame==false. That's pretty confusing but I can kind of > see how it's "not for a main frame navigation." The goal is just to create a > swapped out RVH / RFH in the top-level FrameTreeNode. > > From here, we pass |for_main_frame| into InitRenderView, which passes it to > WebContents::CreateRenderViewForRenderManager, which creates a > RenderWidgetHostViewChildFrame. Is that what we want? Are we supposed to be > creating a RWHVCF for the top-level swapped out RFH/RVH? (It's *not* what we do > for CreateSwappedOutRenderView, where we call CreateViewForWidget rather than > creating a RWHVCF. But maybe it's what we want here?) > > Assuming that's correct, I still think it's too confusing to have different > for_main_frame and IsMainFrame concepts here. Let's at least change the name of > the parameter to for_main_frame_navigation, unless you see any better ways to > make it simpler. As discussed offline, this looks like it is working properly. I changed the name as you suggest. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1372: // Create the swapped out RVH for the new SiteInstance. This will create On 2014/07/28 19:24:28, Charlie Reis wrote: > Nick had a good suggestion here. This first block is awkward to follow because > a subframe RFHM is directly telling the main frame RFHM to do some detailed > operations (creating a RVH/RFH). It then tells the FrameTree to make sure > there's a proxy in each node, which makes more sense. > > It seems preferable for this call site *only* tells the FrameTree to get the > proxies created. FrameTree::CreateProxiesForSiteInstance should do this block > (lines 1372-1386) internally, such that it creates the RVH if needed before > walking the tree. That cleans up this call site and seems easier to follow > because we don't have one RFHM directly taking action on another. Done. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1390: frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance( On 2014/07/28 19:24:28, Charlie Reis wrote: > We should mention that this call creates a scoped_refptr for new_instance, so > it's important to make the call *after* the CreateRenderFrame call. (Otherwise > the refcount could go to zero before we actually use it.) This is no longer necessary after moving the above code into CreateProxiesForSiteInstance, correct? https://codereview.chromium.org/404613005/diff/120001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/404613005/diff/120001/content/common/frame_me... content/common/frame_messages.h:356: // idendified by |parent_routing_id| or as top level if that is On 2014/07/28 19:24:28, Charlie Reis wrote: > identified Done. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:379: // TODO(nasko): The IPC documentation for this message says On 2014/07/28 19:24:28, Charlie Reis wrote: > Let's replace this with the comment from RenderThreadImpl. Done. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:720: // FIXME: document() should not be null, but as a transitional step On 2014/07/28 19:24:28, Charlie Reis wrote: > nit: TODO(kenrb): Done. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_frame_impl.cc:3296: // There's no reason to ignore navigations on subframes, swap out logic On 2014/07/28 19:24:29, Charlie Reis wrote: > nit: subframes, since the swap out logic no longer applies to them. Done. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_thread_impl.cc:1346: // ViewMsg_New IPC. On 2014/07/28 19:24:29, Charlie Reis wrote: > This answers my question. The IPC documentation says this is allowed, but this > check says we don't support it yet (but will in the future). That's fine. Done. https://codereview.chromium.org/404613005/diff/120001/content/renderer/render... content/renderer/render_thread_impl.cc:1347: CHECK(parent_routing_id != MSG_ROUTING_NONE); On 2014/07/28 19:24:29, Charlie Reis wrote: > This is redundant with the CHECK_NE in RenderFrameImpl::CreateFrame. Let's > delete this CHECK and move the comment to RenderFrameImpl::CreateFrame. Done.
https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree.h:78: const scoped_refptr<SiteInstance>& site_instance); On 2014/07/28 19:24:28, Charlie Reis wrote: > On 2014/07/25 23:42:04, kenrb wrote: > > On 2014/07/25 07:13:20, nasko (in Munich) wrote: > > > On 2014/07/24 22:36:29, Charlie Reis wrote: > > > > Why pass the scoped_refptr& here when we're just passing it as a > > SiteInstance* > > > > in the body? Seems like the other methods on this class also use > > > SiteInstance*, > > > > so I'm wondering whether there's a need for the scoped_refptr in this > case. > > > > > > See my response in RFHM. > > > > I have changed it to a SiteInstance*, but it needs to be scoped_refptr because > > base::Bind requires it for any refcounted types. I am making it create one > > within the method, though, to simplify the interface. > > Oh, there was a reason. Darn. > > This approach should work, but let's mention that it creates a scoped_refptr for > |site_instance| in the declaration comment, since that can affect behavior > (e.g., deleting it if there are no other references to it when you call this > method). Wouldn't base::Unretained work if you want it to be a raw ptr? https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); On 2014/07/25 23:42:05, kenrb wrote: > On 2014/07/24 22:36:30, Charlie Reis wrote: > > When do for_main_frame and frame_tree_node_->IsMainFrame() differ? It makes > > this code kind of hard to follow, since it's not clear why we're checking one > vs > > the other. > > Can you follow up on this with Nasko? I am not clear on the answer, but it is > clear that there is a distinction. for_main_frame indicates that this RenderView > will get a platform RenderWidgetHostView rather than a > RenderWidgetHostViewChildFrame, but I see cases where for_main_frame is false > and IsMainFrame() is true. In the code there are two competing notions of "main frame". There is, of course, the main frame of the frame tree. But there is also a "main frame" of a renderviewhost (RVHI::main_frame_routing_id()), which is the frame that triggered the view's creation, and this RFH is always in the same SI as the RVH, and not necessarily on the root frametreenode. I thought that |for_main_frame| here refers to the root frametreenode sense, but ...
https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/60001/content/browser/frame_ho... content/browser/frame_host/render_frame_host_manager.cc:1008: render_view_host, opener_route_id, proxy_routing_id, for_main_frame); On 2014/07/28 22:30:49, ncarter wrote: > On 2014/07/25 23:42:05, kenrb wrote: > > On 2014/07/24 22:36:30, Charlie Reis wrote: > > > When do for_main_frame and frame_tree_node_->IsMainFrame() differ? It makes > > > this code kind of hard to follow, since it's not clear why we're checking > one > > vs > > > the other. > > > > Can you follow up on this with Nasko? I am not clear on the answer, but it is > > clear that there is a distinction. for_main_frame indicates that this > RenderView > > will get a platform RenderWidgetHostView rather than a > > RenderWidgetHostViewChildFrame, but I see cases where for_main_frame is false > > and IsMainFrame() is true. > > In the code there are two competing notions of "main frame". There is, of > course, the main frame of the frame tree. But there is also a "main frame" of a > renderviewhost (RVHI::main_frame_routing_id()), which is the frame that > triggered the view's creation, and this RFH is always in the same SI as the RVH, > and not necessarily on the root frametreenode. > > I thought that |for_main_frame| here refers to the root frametreenode sense, but > ... Stale comment -- ignore.
LGTM apart from Daniel's question in RenderFrameImpl. You'll also need owners approval for the extensions file. https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1390: frame_tree_node_->frame_tree()->CreateProxiesForSiteInstance( On 2014/07/28 21:07:21, kenrb wrote: > On 2014/07/28 19:24:28, Charlie Reis wrote: > > We should mention that this call creates a scoped_refptr for new_instance, so > > it's important to make the call *after* the CreateRenderFrame call. > (Otherwise > > the refcount could go to zero before we actually use it.) > > This is no longer necessary after moving the above code into > CreateProxiesForSiteInstance, correct? Acknowledged. https://codereview.chromium.org/404613005/diff/160001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/404613005/diff/160001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:159: source->frame_tree()->root()->render_manager() This can be simplified to root_->render_manager(), since source->frame_tree() is |this|. (Then we can get rid of the line wrap before the arrow, as well.) https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... content/renderer/render_frame_impl.cc:722: // to this method. In that case there is a null document. Daniel and I are still bit confused by this. If RenderFrameProxy wraps a RenderFrameImpl (presumably in the main frame case), why wouldn't the RenderFrameImpl have a document?
kalman@: Can you please review the change in extensions/? https://codereview.chromium.org/404613005/diff/160001/content/browser/frame_h... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/404613005/diff/160001/content/browser/frame_h... content/browser/frame_host/frame_tree.cc:159: source->frame_tree()->root()->render_manager() On 2014/07/28 22:32:28, Charlie Reis wrote: > This can be simplified to root_->render_manager(), since source->frame_tree() is > |this|. (Then we can get rid of the line wrap before the arrow, as well.) Done. https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... content/renderer/render_frame_impl.cc:722: // to this method. In that case there is a null document. On 2014/07/28 22:32:28, Charlie Reis wrote: > Daniel and I are still bit confused by this. If RenderFrameProxy wraps a > RenderFrameImpl (presumably in the main frame case), why wouldn't the > RenderFrameImpl have a document? I have expanded the comment a bit. It happens for the RenderView's mainframe in the child frame process -- the RenderView creates a RenderFrameImpl, then it is swapped to a RenderFrameProxy while still document-less. I don't know exactly what Nasko had in mind to change this behavior in future, but it is part of the current hacky swap mechanism.
LGTM with nit. https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/404613005/diff/160001/content/renderer/render... content/renderer/render_frame_impl.cc:722: // to this method. In that case there is a null document. On 2014/07/29 16:38:36, kenrb wrote: > On 2014/07/28 22:32:28, Charlie Reis wrote: > > Daniel and I are still bit confused by this. If RenderFrameProxy wraps a > > RenderFrameImpl (presumably in the main frame case), why wouldn't the > > RenderFrameImpl have a document? > > I have expanded the comment a bit. It happens for the RenderView's mainframe in > the child frame process -- the RenderView creates a RenderFrameImpl, then it is > swapped to a RenderFrameProxy while still document-less. I don't know exactly > what Nasko had in mind to change this behavior in future, but it is part of the > current hacky swap mechanism. Acknowledged. https://codereview.chromium.org/404613005/diff/180001/content/renderer/render... File content/renderer/render_frame_proxy.cc (right): https://codereview.chromium.org/404613005/diff/180001/content/renderer/render... content/renderer/render_frame_proxy.cc:167: CHECK(!render_frame || frame_routing_id_ != MSG_ROUTING_NONE); nit: Wrong indent.
lgtm
https://codereview.chromium.org/404613005/diff/180001/content/renderer/render... File content/renderer/render_frame_proxy.cc (right): https://codereview.chromium.org/404613005/diff/180001/content/renderer/render... content/renderer/render_frame_proxy.cc:167: CHECK(!render_frame || frame_routing_id_ != MSG_ROUTING_NONE); On 2014/07/29 16:46:35, Charlie Reis wrote: > nit: Wrong indent. That was just a test line, I had removed it but forgotten to save the file. Thanks for catching it.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/404613005/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
https://codereview.chromium.org/404613005/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:963: routing_id = proxy->GetRoutingID(); This line is the bug. Revert it. This function (super confusingly!) is supposed to return the RVH routing ID -- and in other paths through the function, we do.
https://codereview.chromium.org/404613005/diff/180001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/404613005/diff/180001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1332: SiteInstance* new_instance = current_instance; If you make this line: scoped_refptr<SiteInstance> new_instance = current_instance; Then the CrossSiteTransferTest passes.
The CQ bit was checked by kenrb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/404613005/280001
The CQ bit was unchecked by nasko@chromium.org
The CQ bit was checked by nick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kenrb@chromium.org/404613005/280001
The CQ bit was unchecked by nick@chromium.org
This has one remaining trybot failure, and Android-only failure of the new test CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies. The fix for that is likely a change to Blink, so I'll disable it and try landing this patch under TBR via issue 444503002. |
