Create RenderFrameProxyHost at time of swap-out instead of commit.

Create RenderFrameProxyHost at time of swap-out instead of commit.

This CL moves the creation of RenderFrameProxyHost earlier in the timeline of cross-process navigation. Instead of waiting until commit time, it creates it at the time SwapOut message is sent to the old RenderFrameHost.
It also creates the renderer-side RenderFrameProxy object. Since now both sides have proxies, when in swapped out state, RenderFrameHost and RenderFrame send their messages through the proxies. This both helps verify proxies exist and paves the way of removing the notion of swapping out frames.

BUG=357747
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271325

[nasko, 2014-05-14 21:41:35]

Hey Charlie,
Can you review this CL for me?
Thanks,
Nasko

[nasko, 2014-05-14 21:48:45]

Adding site-isolation-reviews@.

[Charlie Reis, 2014-05-15 00:32:49]

Lots to look at, but seems like a good start!

At a high level, I'm torn about all the RenderView-related places that now have
to take a frame-specific proxy routing ID.  That seems a bit awkward, though I
suppose it parallels the swapped_out parameter.  Maybe it's ok if we keep them
together and eventually remove them?

Questions and comments below.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:96: void
set_render_frame_proxy(RenderFrameProxyHost* proxy) {
nit: set_render_frame_proxy_host

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:123: void
SwapOut(RenderFrameProxyHost* proxy);
Can you mention what |proxy| is for in the comment, and whether we expect its
corresponding RenderFrameProxy to exist in the renderer at the time this is
called?

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:271: // TODO(nasko): This
can be removed once we don't use swapping out of
There are a lot of comments like this, but the wording doesn't feel right to me.
 We should instead say something like "...once we don't have a swapped out state
on RenderFrameHosts," and cite http://crbug.com/357747.  Can you update the
others as well?

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:273: RenderFrameProxyHost*
render_frame_proxy_;
nit: render_frame_proxy_host_

That's more consistent with mentioning "host" in render_view_host_, and it's a
bit clearer to others trying to understand which object lives where.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
To be clear, render_frame_host_ (and its SiteInstance) now exist both as the
current RFH and on the proxy list.  Is that going to violate any assumptions in
the time period before commit?

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:921:
new_render_frame_host->set_render_frame_proxy(NULL);
Would it make sense to do this step inside PassFrameHostOwnership, or are we
intentionally skipping that in some call sites?

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:949: if (!proxy) {
We already know there's no proxy, since we're in the else block of the if on
line 914.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1131:
proxy_hosts_.find(old_site_instance_id);
nit: Wrong indent.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1148:
old_render_frame_host->GetSiteInstance())->active_view_count();
nit: Wrong indent.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1159: if
(!active_view_count) {
nit: No braces needed.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1377:
proxy_hosts_[site_instance->GetId()] = proxy;
I'm a little uncomfortable that we put the proxy into proxy_hosts_ before
calling SwapOut in one case and after calling it in the other.  SwapOut
synchronously calls back into SwappedOut, which might call RequestTransferURL,
so I think we should be consistent about the state of proxy_hosts_ during this
call if possible.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:380: int
proxy_routing_id);
It's a bit unclear what this is for.  Perhaps a comment would help.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:42: if
(render_frame_host_.get()) {
nit: No braces needed.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:73: // This method deletes
the RenderFrameProxy object in the renderer process,
nit: Remove comma

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:74: // which corresponds to
this RenderFrameProxyHost. Must be called before
I'm a bit concerned about missing cases where the host is deleted and leaking it
in the renderer process.  Is there a reason we can't send the IPC message from
the RFPH destructor?

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:77: void
DeleteRendererProxy();
nit: DeleteRenderFrameProxy()

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:84: RenderViewHostImpl*
render_view_host() {
This isn't a one liner anymore, so it should be in the .cc file.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:93:
scoped_ptr<RenderFrameHostImpl> PassFrameHostOwnership() {
nit: Move implementation to .cc file.

https://codereview.chromium.org/281663002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:236: int proxy_routing_id,
This feels strange to me.  A RenderFrameProxy is frame-specific, unlike
RenderView, and most RenderViews won't have them.  Is there another way to
handle this?  (We should at least have a comment about this similar to the one
in view_messages.h, but it would be nice to avoid passing it here if possible.)

minor nit: It looks a little strange to have route vs routing in the two
parameter names.

https://codereview.chromium.org/281663002/diff/60001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/common/frame_mes...
content/common/frame_messages.h:357: // Notifies the corresponding
RenderFrameProxy object to replace ifself with the
nit: itself

https://codereview.chromium.org/281663002/diff/60001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/common/view_mess...
content/common/view_messages.h:450: // used if |swapped_out| is true.
Maybe it should be declared just below swapped_out?

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:365: RenderFrameImpl*
RenderFrameImpl::FromRoutingID(int32 routing_id) {
This code looks fine, but any reason that it differs from FromWebFrame below?

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:650: // Some call sites use the associated
RenderViewImpl routing id to send
Whew, this is tricky!  Maybe start with a sentence like "In most cases, send
IPCs through the proxy when swapped out."

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:652: // The SwapOut_ACK message must be
delivered to the RenderFrameHost
nit: the

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:660: if
(!SwappedOutMessages::CanSendWhileSwappedOut(message)) {
I don't think this is right.  When CanSendWhileSwappedOut returns false, we'll
have already sent the message through the proxy on line 656.  We should probably
leave it in the initial if statement as it was before.

[Edit: I see that RenderFrameProxy::Send calls CanSendWhileSwappedOut as well,
and deletes the message if not.  Wouldn't that be a double free in most cases,
apart from SwapOut_ACK, etc?]

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:933: RenderFrameProxy* proxy =
RenderFrameProxy::CreateFrameProxy(
I'm unclear on why this is declared up here, rather than as part of the
set_render_frame_proxy call below.  Does it need to happen before we get swapped
out, similar to SyncNavigationState?  If so, let's put it in that block (of
things to do before swap out actually occurs).

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.h:533: RenderFrameProxy* render_frame_proxy_;
This should have a comment saying it's usually null unless we're swapped out. 
(Maybe declare it next to is_swapped_out_?)

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:32: CHECK(rf);
Not sure I understand why we need this function.  The RenderFrameProxy
constructor already does most of this, including looking up the RenderFrameImpl
and CHECKing that it exists.  Are there cases that we need to avoid the
set_render_frame call by calling the RenderFrameProxy constructor directly?

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:44:
RenderFrameProxy::RenderFrameProxy(int routing_id, int frame_routing_id)
Maybe we should check that routing_id is valid?

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.h:21: class CONTENT_EXPORT RenderFrameProxy
Can you add a class-level comment for this, mainly mentioning that it's a
placeholder for RenderFrames in other processes?  (We should probably have a
TODO somewhere in this file to remove the RenderFrame part of it as well.)

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_view_browsertest.cc:491:
view()->main_render_frame()->OnSwapOut(MSG_ROUTING_NONE);
Doesn't RenderFrameProxy's constructor assume that it has a valid routing ID for
the proxy, and call AddRoute with it?

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.h:184: int32 proxy_routing_id,
This should be documented (and perhaps moved near swapped_out) if we choose to
keep it.

[ncarter (slow), 2014-05-15 00:42:23]

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:156:
weak_ptr_factory_(this) {
Give that |render_frame_proxy_| an initializer. Pointers love initializers.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
What guarantees that there's not already something in the map at
proxy_hosts_[id]? It was clear in RenderFrameHostManager::CreateRenderFrame that
this was the case at the time of the assignment, but I get confused why it's not
a problem here.

[nasko, 2014-05-15 18:47:13]

I've addressed all the comments. Please take another look.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:156:
weak_ptr_factory_(this) {
On 2014/05/15 00:42:23, ncarter wrote:
> Give that |render_frame_proxy_| an initializer. Pointers love initializers.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:96: void
set_render_frame_proxy(RenderFrameProxyHost* proxy) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: set_render_frame_proxy_host

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:123: void
SwapOut(RenderFrameProxyHost* proxy);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Can you mention what |proxy| is for in the comment, and whether we expect its
> corresponding RenderFrameProxy to exist in the renderer at the time this is
> called?

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:271: // TODO(nasko): This
can be removed once we don't use swapping out of
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> There are a lot of comments like this, but the wording doesn't feel right to
me.
>  We should instead say something like "...once we don't have a swapped out
state
> on RenderFrameHosts," and cite http://crbug.com/357747.  Can you update the
> others as well?

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:273: RenderFrameProxyHost*
render_frame_proxy_;
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: render_frame_proxy_host_
> 
> That's more consistent with mentioning "host" in render_view_host_, and it's a
> bit clearer to others trying to understand which object lives where.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> To be clear, render_frame_host_ (and its SiteInstance) now exist both as the
> current RFH and on the proxy list.  Is that going to violate any assumptions
in
> the time period before commit?

It actually won't exist in two places. Here only a proxy is created, as we can't
have the RFH be in both places at the same time, this will be nasty.
Only at commit time we know that we will no longer use the RFH, so it is at that
point that we pass the ownership of the RFH from RFHM to the proxy.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
On 2014/05/15 00:42:23, ncarter wrote:
> What guarantees that there's not already something in the map at
> proxy_hosts_[id]? It was clear in RenderFrameHostManager::CreateRenderFrame
that
> this was the case at the time of the assignment, but I get confused why it's
not
> a problem here.

I don't think there is a guarantee and I found one test that hits this case -
CrossSiteTransferTest.ReplaceEntryInProcessThenTranfers. I've changed the code
to ensure it deletes the existing one before creating the new one.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:921:
new_render_frame_host->set_render_frame_proxy(NULL);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Would it make sense to do this step inside PassFrameHostOwnership, or are we
> intentionally skipping that in some call sites?

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:949: if (!proxy) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> We already know there's no proxy, since we're in the else block of the if on
> line 914.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1131:
proxy_hosts_.find(old_site_instance_id);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1148:
old_render_frame_host->GetSiteInstance())->active_view_count();
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1159: if
(!active_view_count) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:1377:
proxy_hosts_[site_instance->GetId()] = proxy;
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> I'm a little uncomfortable that we put the proxy into proxy_hosts_ before
> calling SwapOut in one case and after calling it in the other.  SwapOut
> synchronously calls back into SwappedOut, which might call RequestTransferURL,
> so I think we should be consistent about the state of proxy_hosts_ during this
> call if possible.

This was just a honest omission, not intentional. I've made all creations for
RFPH to use the same sequence.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:380: int
proxy_routing_id);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> It's a bit unclear what this is for.  Perhaps a comment would help.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:42: if
(render_frame_host_.get()) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: No braces needed.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:73: // This method deletes
the RenderFrameProxy object in the renderer process,
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: Remove comma

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:74: // which corresponds to
this RenderFrameProxyHost. Must be called before
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> I'm a bit concerned about missing cases where the host is deleted and leaking
it
> in the renderer process.  Is there a reason we can't send the IPC message from
> the RFPH destructor?

In the case of a renderer process crashing, we delete all proxies for that
process, so we will be sending IPCs to non-existing process.
After chatting about it, it can be moved to the destructor and gated on
GetProcess()->HasConnection().

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:77: void
DeleteRendererProxy();
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: DeleteRenderFrameProxy()

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:84: RenderViewHostImpl*
render_view_host() {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> This isn't a one liner anymore, so it should be in the .cc file.

This is no longer needed.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:93:
scoped_ptr<RenderFrameHostImpl> PassFrameHostOwnership() {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: Move implementation to .cc file.

Done.

https://codereview.chromium.org/281663002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:236: int proxy_routing_id,
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> This feels strange to me.  A RenderFrameProxy is frame-specific, unlike
> RenderView, and most RenderViews won't have them.  Is there another way to
> handle this?  (We should at least have a comment about this similar to the one
> in view_messages.h, but it would be nice to avoid passing it here if
possible.)
> 
> minor nit: It looks a little strange to have route vs routing in the two
> parameter names.

As discussed, there isn't any other way that will be much cleaner, so leaving it
in place. I've moved the proxy routing id to be next to swapped out state
wherever it is encountered and fixed the route vs routing here.

https://codereview.chromium.org/281663002/diff/60001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/common/frame_mes...
content/common/frame_messages.h:357: // Notifies the corresponding
RenderFrameProxy object to replace ifself with the
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: itself

Done.

https://codereview.chromium.org/281663002/diff/60001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/common/view_mess...
content/common/view_messages.h:450: // used if |swapped_out| is true.
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Maybe it should be declared just below swapped_out?

It made sense to group it with all routing ids. If you feel strongly, I'll move
it.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:365: RenderFrameImpl*
RenderFrameImpl::FromRoutingID(int32 routing_id) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> This code looks fine, but any reason that it differs from FromWebFrame below?

Yes, it is identical to the code in RenderViewImpl ;). I've changed it to
conform to the one in this file.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:650: // Some call sites use the associated
RenderViewImpl routing id to send
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Whew, this is tricky!  Maybe start with a sentence like "In most cases, send
> IPCs through the proxy when swapped out."

Done.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:652: // The SwapOut_ACK message must be
delivered to the RenderFrameHost
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> nit: the

Done.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:660: if
(!SwappedOutMessages::CanSendWhileSwappedOut(message)) {
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> I don't think this is right.  When CanSendWhileSwappedOut returns false, we'll
> have already sent the message through the proxy on line 656.  We should
probably
> leave it in the initial if statement as it was before.
> 
> [Edit: I see that RenderFrameProxy::Send calls CanSendWhileSwappedOut as well,
> and deletes the message if not.  Wouldn't that be a double free in most cases,
> apart from SwapOut_ACK, etc?]

If we ever get to call Send on the RenderFrameProxy, we will return and never
hit this statement or am I missing something?
The double free could happen if the call to RenderFrameProxy::Send didn't do an
early return out of this method.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:933: RenderFrameProxy* proxy =
RenderFrameProxy::CreateFrameProxy(
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> I'm unclear on why this is declared up here, rather than as part of the
> set_render_frame_proxy call below.  Does it need to happen before we get
swapped
> out, similar to SyncNavigationState?  If so, let's put it in that block (of
> things to do before swap out actually occurs).

I think it should be created prior to running the unload handler, which can
potentially run for a long time. The reason is that if the browser starts
sending IPCs, the routing id is registered and messages aren't dropped.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.h:533: RenderFrameProxy* render_frame_proxy_;
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> This should have a comment saying it's usually null unless we're swapped out. 
> (Maybe declare it next to is_swapped_out_?)

Done.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:32: CHECK(rf);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Not sure I understand why we need this function.  The RenderFrameProxy
> constructor already does most of this, including looking up the
RenderFrameImpl
> and CHECKing that it exists.  Are there cases that we need to avoid the
> set_render_frame call by calling the RenderFrameProxy constructor directly?

The code when through a bunch of iterations, so I didn't realize they do mostly
the same work :).

Overall, I think we need this method, as it will handle creating WebRemoteFrame,
connecting it to the RenderFrameProxy and inserting into the frame tree. It
isn't in this CL, since we can't do it all yet.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:44:
RenderFrameProxy::RenderFrameProxy(int routing_id, int frame_routing_id)
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Maybe we should check that routing_id is valid?

If it is invalid, what do we do? We can't fail construction or throw an
exception? Another justification for having CreateFrameProxy, but see my comment
in render_view_browsertest.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.h:21: class CONTENT_EXPORT RenderFrameProxy
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Can you add a class-level comment for this, mainly mentioning that it's a
> placeholder for RenderFrames in other processes?  (We should probably have a
> TODO somewhere in this file to remove the RenderFrame part of it as well.)

Done.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_view_browsertest.cc:491:
view()->main_render_frame()->OnSwapOut(MSG_ROUTING_NONE);
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> Doesn't RenderFrameProxy's constructor assume that it has a valid routing ID
for
> the proxy, and call AddRoute with it?

Contrary to its name, this is actually an unit test. There are no real IPCs
being dispatched between processes. I've replaced it with a constant, similar to
other unit tests.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.h:184: int32 proxy_routing_id,
On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> This should be documented (and perhaps moved near swapped_out) if we choose to
> keep it.

Done.

[ncarter (slow), 2014-05-15 20:49:44]

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:249: if
(render_view_host_->IsSwappedOut()) {
What guarantees that all frames sharing a RVH are swapped out at the same
moment? (Given how it's used here, I expected the IsSwappedOut state to be an
attribute of the RFH, not the RVH.)

[nasko, 2014-05-15 20:53:44]

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:249: if
(render_view_host_->IsSwappedOut()) {
On 2014/05/15 20:49:45, ncarter wrote:
> What guarantees that all frames sharing a RVH are swapped out at the same
> moment? (Given how it's used here, I expected the IsSwappedOut state to be an
> attribute of the RFH, not the RVH.)

Prior to frame tree replication, we always have only one RenderFrameHostImpl for
a swapped out RenderViewHostImpl. This will change once we start replicating the
frame tree, but so will this check.

[nasko, 2014-05-15 21:04:32]

On 2014/05/15 20:53:44, nasko wrote:
>
https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
> content/browser/frame_host/render_frame_host_impl.cc:249: if
> (render_view_host_->IsSwappedOut()) {
> On 2014/05/15 20:49:45, ncarter wrote:
> > What guarantees that all frames sharing a RVH are swapped out at the same
> > moment? (Given how it's used here, I expected the IsSwappedOut state to be
an
> > attribute of the RFH, not the RVH.)
> 
> Prior to frame tree replication, we always have only one RenderFrameHostImpl
for
> a swapped out RenderViewHostImpl. This will change once we start replicating
the
> frame tree, but so will this check.

Actually, I thought even more about this and with frame tree replication, we
will not be swapping out RenderFrameHosts for subframes, just the top-level
RF/RFH. For subframes I plan on using either full frame objects for active
frames or just proxies for frames in different process. So even in that world,
the check might be correct, though it will be more clear once the code is
written and can be reasoned about.

[Charlie Reis, 2014-05-15 22:54:59]

Great!  A few more questions and a couple nits below.  (I'll mention that my
comments ended up split between patch sets 4 and 5, just so you don't miss
them.)

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
On 2014/05/15 18:47:13, nasko wrote:
> On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> > To be clear, render_frame_host_ (and its SiteInstance) now exist both as the
> > current RFH and on the proxy list.  Is that going to violate any assumptions
> in
> > the time period before commit?
> 
> It actually won't exist in two places. Here only a proxy is created, as we
can't
> have the RFH be in both places at the same time, this will be nasty.
> Only at commit time we know that we will no longer use the RFH, so it is at
that
> point that we pass the ownership of the RFH from RFHM to the proxy.

In that case, should IsOnSwappedOutList(RFH) will return false if the proxy is
on the list but the RFH is the current one?  Looks like it will current return
true because it just compares against the proxy's SiteInstance.

Also, what about APIs that look up a host given a SiteInstance, like
GetSwappedOutRenderViewHost and GetRenderFrameProxyHost?  A given SiteInstance
can now be the current RFH and have a proxy on the list.  If we want to hide
that fact, we should probably update those and add a unit test for it.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:44:
RenderFrameProxy::RenderFrameProxy(int routing_id, int frame_routing_id)
On 2014/05/15 18:47:13, nasko wrote:
> On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> > Maybe we should check that routing_id is valid?
> 
> If it is invalid, what do we do? We can't fail construction or throw an
> exception? Another justification for having CreateFrameProxy, but see my
comment
> in render_view_browsertest.

A DCHECK would work fine, right?  It just seems like we shouldn't be creating
these (and calling AddRoute) with MSG_ROUTING_NONE, and now the test doesn't
attempt to.

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:521:
render_frame_host_->GetSiteInstance(), frame_tree_node_);
nit: Wrong indent.

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:32: Send(new
FrameMsg_DeleteProxy(routing_id_));
Ok for this to happen a second time when this object is deleted?  Or should we
just wait to do it in the destructor?

https://codereview.chromium.org/281663002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:236: int proxy_route_id,
Let's mention this in the comment (e.g., that it's only used when the RenderView
is created in a swapped out state).

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:645: bool
RenderFrameImpl::Send(IPC::Message* message) {
Thanks-- the changes here are much nicer.

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:659: return
render_frame_proxy_->Send(message);
nit: Wrong indent and no braces needed.

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.h:25: // RenderFrameProxy allow us to keep
existing window references valid over
nit: allows

[nasko, 2014-05-15 23:36:48]

Fixes based on the latest review from Charlie.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> On 2014/05/15 18:47:13, nasko wrote:
> > On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> > > To be clear, render_frame_host_ (and its SiteInstance) now exist both as
the
> > > current RFH and on the proxy list.  Is that going to violate any
assumptions
> > in
> > > the time period before commit?
> > 
> > It actually won't exist in two places. Here only a proxy is created, as we
> can't
> > have the RFH be in both places at the same time, this will be nasty.
> > Only at commit time we know that we will no longer use the RFH, so it is at
> that
> > point that we pass the ownership of the RFH from RFHM to the proxy.
> 
> In that case, should IsOnSwappedOutList(RFH) will return false if the proxy is
> on the list but the RFH is the current one?  Looks like it will current return
> true because it just compares against the proxy's SiteInstance.

The only way for IsOnSwappedOutList(RFH) to return true is for
(iter->second->render_frame_host() == rfh) to be true. The call to
render_frame_host() will return NULL prior to the proxy taking ownership, which
happens at commit time or when creating swapped out RFH. In both of these cases
it is indeed swapped out. What am I missing?
 
> Also, what about APIs that look up a host given a SiteInstance, like
> GetSwappedOutRenderViewHost and GetRenderFrameProxyHost?  A given SiteInstance
> can now be the current RFH and have a proxy on the list.  If we want to hide
> that fact, we should probably update those and add a unit test for it.

I'm not sure how we would hide the fact that there is a proxy and a frame for
short period of time. I'm open to ideas, but here is my analysis why I think it
is currently ok.

GetSwappedOutRenderViewHost will behave similarly to IsOnSwappedOutList(RFH),
though I had to restore one of my previous changes. See
RFPH::render_view_host().

For GetRenderFrameProxyHost, it is used in 3 locations:
* IsRVHOnSwappedOutList - already safe usage
* GetSwappedOutRenderViewHost - again, safe usage
* CreateRenderFrame - this one is a bit trickier, but there is already a
CHECK_NE ensuring that the current host and the new frame don't have the same
SiteInstance.

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/renderer/render_...
content/renderer/render_frame_proxy.cc:44:
RenderFrameProxy::RenderFrameProxy(int routing_id, int frame_routing_id)
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> On 2014/05/15 18:47:13, nasko wrote:
> > On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> > > Maybe we should check that routing_id is valid?
> > 
> > If it is invalid, what do we do? We can't fail construction or throw an
> > exception? Another justification for having CreateFrameProxy, but see my
> comment
> > in render_view_browsertest.
> 
> A DCHECK would work fine, right?  It just seems like we shouldn't be creating
> these (and calling AddRoute) with MSG_ROUTING_NONE, and now the test doesn't
> attempt to.

Done.

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:521:
render_frame_host_->GetSiteInstance(), frame_tree_node_);
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> nit: Wrong indent.

Done.

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:32: Send(new
FrameMsg_DeleteProxy(routing_id_));
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> Ok for this to happen a second time when this object is deleted?  Or should we
> just wait to do it in the destructor?

Thanks for the comment. I think we have a bug in ClearProxiesInSiteInstance,
which the new patch should be fixing.

https://codereview.chromium.org/281663002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/281663002/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:236: int proxy_route_id,
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> Let's mention this in the comment (e.g., that it's only used when the
RenderView
> is created in a swapped out state).

Done.

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:645: bool
RenderFrameImpl::Send(IPC::Message* message) {
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> Thanks-- the changes here are much nicer.

Done.

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:659: return
render_frame_proxy_->Send(message);
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> nit: Wrong indent and no braces needed.

Done.

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/281663002/diff/80001/content/renderer/render_...
content/renderer/render_frame_proxy.h:25: // RenderFrameProxy allow us to keep
existing window references valid over
On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> nit: allows

Done.

[Charlie Reis, 2014-05-16 18:54:07]

Cool-- LGTM.

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/281663002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:520:
proxy_hosts_[render_frame_host_->GetSiteInstance()->GetId()] = proxy;
On 2014/05/15 23:36:49, nasko wrote:
> On 2014/05/15 22:55:00, Charlie Reis(OOO as of May 17) wrote:
> > On 2014/05/15 18:47:13, nasko wrote:
> > > On 2014/05/15 00:32:50, Charlie Reis(OOO as of May 17) wrote:
> > > > To be clear, render_frame_host_ (and its SiteInstance) now exist both as
> the
> > > > current RFH and on the proxy list.  Is that going to violate any
> assumptions
> > > in
> > > > the time period before commit?
> > > 
> > > It actually won't exist in two places. Here only a proxy is created, as we
> > can't
> > > have the RFH be in both places at the same time, this will be nasty.
> > > Only at commit time we know that we will no longer use the RFH, so it is
at
> > that
> > > point that we pass the ownership of the RFH from RFHM to the proxy.
> > 
> > In that case, should IsOnSwappedOutList(RFH) will return false if the proxy
is
> > on the list but the RFH is the current one?  Looks like it will current
return
> > true because it just compares against the proxy's SiteInstance.
> 
> The only way for IsOnSwappedOutList(RFH) to return true is for
> (iter->second->render_frame_host() == rfh) to be true. The call to
> render_frame_host() will return NULL prior to the proxy taking ownership,
which
> happens at commit time or when creating swapped out RFH. In both of these
cases
> it is indeed swapped out. What am I missing?

I think I confused myself-- I must have been looking at
GetRenderFrameProxyHost(SiteInstance), which won't notice that the RFH is
missing during this time period.  We could detect whether the proxy has a null
RFH and not return it, but I think I agree with your comments below that it's
not necessary to hide it.

>  
> > Also, what about APIs that look up a host given a SiteInstance, like
> > GetSwappedOutRenderViewHost and GetRenderFrameProxyHost?  A given
SiteInstance
> > can now be the current RFH and have a proxy on the list.  If we want to hide
> > that fact, we should probably update those and add a unit test for it.
> 
> I'm not sure how we would hide the fact that there is a proxy and a frame for
> short period of time. I'm open to ideas, but here is my analysis why I think
it
> is currently ok.
> 
> GetSwappedOutRenderViewHost will behave similarly to IsOnSwappedOutList(RFH),
> though I had to restore one of my previous changes. See
> RFPH::render_view_host().
> 
> For GetRenderFrameProxyHost, it is used in 3 locations:
> * IsRVHOnSwappedOutList - already safe usage
> * GetSwappedOutRenderViewHost - again, safe usage
> * CreateRenderFrame - this one is a bit trickier, but there is already a
> CHECK_NE ensuring that the current host and the new frame don't have the same
> SiteInstance.

https://codereview.chromium.org/281663002/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/281663002/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.h:77: RenderViewHostImpl*
render_view_host();
If this can't be inlined anymore, it should be GetRenderViewHost().

[nasko, 2014-05-16 19:03:19]

Fixed method name.

sky@, can you do OWNERS review for components/?
Thanks!

[sky, 2014-05-16 19:47:12]

components LGTM

[nasko, 2014-05-16 20:35:14]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-16 20:36:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/281663002/160001

[ncarter (slow), 2014-05-16 20:56:17]

https://codereview.chromium.org/281663002/diff/160001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/281663002/diff/160001/content/common/frame_me...
content/common/frame_messages.h:359: IPC_MESSAGE_ROUTED0(FrameMsg_DeleteProxy)
Since this is routed to a RenderFrameProxy, should this message be named
FrameProxyMsg_Delete instead of FrameMsg_DeleteProxy ?

[nasko, 2014-05-16 23:10:42]

https://codereview.chromium.org/281663002/diff/160001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/281663002/diff/160001/content/common/frame_me...
content/common/frame_messages.h:359: IPC_MESSAGE_ROUTED0(FrameMsg_DeleteProxy)
On 2014/05/16 20:56:17, ncarter wrote:
> Since this is routed to a RenderFrameProxy, should this message be named
> FrameProxyMsg_Delete instead of FrameMsg_DeleteProxy ?

Probably. I've avoided introducing Proxy prefix for now, as a bunch of other
IPCs are now routed through the proxy objects as well. I'll do a rename pass
later on.

[commit-bot: I haz the power, 2014-05-17 02:38:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-17 02:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 02:57:56]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[nasko, 2014-05-17 04:12:59]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-17 04:14:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/281663002/160001

[commit-bot: I haz the power, 2014-05-17 06:45:02]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-05-17 07:07:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 07:07:22]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[nasko, 2014-05-18 22:14:56]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2014-05-18 22:15:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nasko@chromium.org/281663002/200001

[commit-bot: I haz the power, 2014-05-19 00:04:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-19 01:29:08]

Change committed as 271325