Move the RenderProcessGone IPC from RenderViewHost to RenderFrameHost

Move the RenderProcessGone IPC from RenderViewHost to RenderFrameHost

BUG=304341, 450799
Committed: https://crrev.com/b985af1c7d2647db123a76370ea5a09e41151e61
Cr-Commit-Position: refs/heads/master@{#314974}

[nasko, 2015-02-03 00:02:40]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2015-02-03 00:02:40]

Hey Charlie,
Can you review this CL for me? It moves RenderProcessGone handling from
RenderViewHost to RenderFrameHost. It is a part of the work to cleanup WCO
interfaces and needed in order to notify for RenderFrameDeleted properly.
Thanks in advance!
Nasko

[Charlie Reis, 2015-02-03 01:06:35]

Thanks-- a few thoughts below.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (left):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:314:
render_frame_host->render_view_host()->GetSiteInstance();
Is this unrelated?  We should probably do GetSiteInstance cleanup in a separate
CL, since the rest of this CL has a non-zero chance of being reverted.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1005:
frame_tree_node_->frame_tree()->RenderProcessGone(render_view_host_);
Does OnRenderProcessGone get called once per frame?  If so, should we really be
calling this multiple times?

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1010:
render_view_host_->render_view_termination_status_ =
Sanity check: This used to be set before the FrameTree::RenderProcessGone call. 
Can you make sure it's not accessed from within there if we're going to reorder
it?

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1013: // Our base class
RenderWidgetHost needs to reset some stuff.
nit: Not our base class anymore.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:15: #include
"base/process/kill.h"
What's this needed for?  Maybe it just goes in the .cc file?

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2074:
IDMap<IPC::Listener>::iterator iter(&listeners_);
I'm guessing we don't have a list of the frames?  Seems unfortunate to send it
to every IPC listener, but I suppose it works.

Edit: Oh, widgets process it as well...  Maybe we should mention that it's
processed by both frames and widgets in a comment?

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1055: //
RenderViewHostImpl and destroy itself.
At some point, we'll need RenderWidgetHostImpl to know whether it's for a frame
(main frame or subframe) or not, in order to get rid of RVH.  But I suppose this
approach works for now.

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:448:
IPC_MESSAGE_HANDLER(FrameHostMsg_RenderProcessGone, OnRenderProcessGone)
Huh.  I guess we'll need to handle some FrameHostMsgs here?  Feels weird, but I
don't have a better suggestion.

[nasko, 2015-02-04 16:51:31]

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (left):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:314:
render_frame_host->render_view_host()->GetSiteInstance();
On 2015/02/03 01:06:35, Charlie Reis wrote:
> Is this unrelated?  We should probably do GetSiteInstance cleanup in a
separate
> CL, since the rest of this CL has a non-zero chance of being reverted.

Done.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1005:
frame_tree_node_->frame_tree()->RenderProcessGone(render_view_host_);
On 2015/02/03 01:06:35, Charlie Reis wrote:
> Does OnRenderProcessGone get called once per frame?  If so, should we really
be
> calling this multiple times?

It is indeed called once per frame. Ideally it will be called one per process,
but I don't know of a clean way to do it. It should be the case that any
subsequent calls for frames in the same process will just execute without making
any further changes to the FrameTree.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1010:
render_view_host_->render_view_termination_status_ =
On 2015/02/03 01:06:35, Charlie Reis wrote:
> Sanity check: This used to be set before the FrameTree::RenderProcessGone
call. 
> Can you make sure it's not accessed from within there if we're going to
reorder
> it?

I don't think I can reasonably guarantee this. Therefore I'm splitting the if
statement to preserve the ordering.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1013: // Our base class
RenderWidgetHost needs to reset some stuff.
On 2015/02/03 01:06:35, Charlie Reis wrote:
> nit: Not our base class anymore.

Done.

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:15: #include
"base/process/kill.h"
On 2015/02/03 01:06:35, Charlie Reis wrote:
> What's this needed for?  Maybe it just goes in the .cc file?

Done.

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2074:
IDMap<IPC::Listener>::iterator iter(&listeners_);
On 2015/02/03 01:06:35, Charlie Reis wrote:
> I'm guessing we don't have a list of the frames?  Seems unfortunate to send it
> to every IPC listener, but I suppose it works.
> 
> Edit: Oh, widgets process it as well...  Maybe we should mention that it's
> processed by both frames and widgets in a comment?

Does it matter what the types of the listeners are? Each listener associated
with this process must be notified of its death. I'm not sure what a comment
will add here, other than possibly stale data.

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1055: //
RenderViewHostImpl and destroy itself.
On 2015/02/03 01:06:35, Charlie Reis wrote:
> At some point, we'll need RenderWidgetHostImpl to know whether it's for a
frame
> (main frame or subframe) or not, in order to get rid of RVH.  But I suppose
this
> approach works for now.

Acknowledged.

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:448:
IPC_MESSAGE_HANDLER(FrameHostMsg_RenderProcessGone, OnRenderProcessGone)
On 2015/02/03 01:06:35, Charlie Reis wrote:
> Huh.  I guess we'll need to handle some FrameHostMsgs here?  Feels weird, but
I
> don't have a better suggestion.

Well, it is not a frame specific message to be honest. It is a listener message.
We can rename it, if it makes more sense, as it is a message sent to each IPC
listener registered for the specific process.

[Charlie Reis, 2015-02-05 00:53:14]

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

https://codereview.chromium.org/894843003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1013:
frame_tree_node_->frame_tree()->RenderProcessGone(render_view_host_);
Looking closer, FrameTree::RenderProcessGone walks the frame tree looking for
FrameTreeNodes whose current RFH uses this RVH.  For each of those, it calls
ResetForNewProcess and InvalidateMojoConnection.

That's exactly the same set of RFHs that will receive this message.  Sounds like
we can skip the middle man (FrameTree) and just have this RFH call
InvalidateMojoConnection and frame_tree_node_->ResetForNewProcess().

(Even ResetForNewProcess can get a bit clearner, since we can move the
SetRenderFrameCreated(false) call to here.)

[nasko, 2015-02-05 23:02:00]

Patchset #4 (id:60001) has been deleted

[nasko, 2015-02-05 23:02:15]

Patchset #4 (id:60001) has been deleted

[nasko, 2015-02-05 23:07:33]

Patchset #4 (id:80001) has been deleted

[nasko, 2015-02-05 23:08:45]

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

https://codereview.chromium.org/894843003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1013:
frame_tree_node_->frame_tree()->RenderProcessGone(render_view_host_);
On 2015/02/05 00:53:13, Charlie Reis wrote:
> Looking closer, FrameTree::RenderProcessGone walks the frame tree looking for
> FrameTreeNodes whose current RFH uses this RVH.  For each of those, it calls
> ResetForNewProcess and InvalidateMojoConnection.
> 
> That's exactly the same set of RFHs that will receive this message.  Sounds
like
> we can skip the middle man (FrameTree) and just have this RFH call
> InvalidateMojoConnection and frame_tree_node_->ResetForNewProcess().
> 
> (Even ResetForNewProcess can get a bit clearner, since we can move the
> SetRenderFrameCreated(false) call to here.)

Good point! Inlined those two calls.

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1043: // therefore it
doesn't need to reset FrameTreeNode state.
I accidentally slipped a rebase in, so the intra-patch diff is messed up. This
is the only relevant part that changed - don't reset the FTN if the RFH is
swapped out.

[Charlie Reis, 2015-02-06 00:04:48]

A few bugs below?

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:293:
render_frame_host->render_view_host()->GetSiteInstance();
Looks like a failed merge here.

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1043: // therefore it
doesn't need to reset FrameTreeNode state.
On 2015/02/05 23:08:45, nasko wrote:
> I accidentally slipped a rebase in, so the intra-patch diff is messed up. This
> is the only relevant part that changed - don't reset the FTN if the RFH is
> swapped out.

I don't quite understand the comment.  Why does "owned by RFPH" imply "don't
need to reset FTN state?"  It seems like being owned by RFPH isn't the key
point; the fact that some other RFH is the current one is more important.

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1044: if (is_swapped_out())
Isn't this backwards?  This only resets it if it's swapped out.

[nasko, 2015-02-06 01:10:48]

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:293:
render_frame_host->render_view_host()->GetSiteInstance();
On 2015/02/06 00:04:47, Charlie Reis wrote:
> Looks like a failed merge here.

Oh, that was auto merged :(. Thanks for catching it!

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1043: // therefore it
doesn't need to reset FrameTreeNode state.
On 2015/02/06 00:04:48, Charlie Reis wrote:
> On 2015/02/05 23:08:45, nasko wrote:
> > I accidentally slipped a rebase in, so the intra-patch diff is messed up.
This
> > is the only relevant part that changed - don't reset the FTN if the RFH is
> > swapped out.
> 
> I don't quite understand the comment.  Why does "owned by RFPH" imply "don't
> need to reset FTN state?"  It seems like being owned by RFPH isn't the key
> point; the fact that some other RFH is the current one is more important.

Done.

https://codereview.chromium.org/894843003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1044: if (is_swapped_out())
On 2015/02/06 00:04:48, Charlie Reis wrote:
> Isn't this backwards?  This only resets it if it's swapped out.

Indeed it is, I had different check before so slipped between the different
versions I worked with.

[Charlie Reis, 2015-02-06 03:13:57]

LGTM

[Charlie Reis, 2015-02-06 03:14:05]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-02-06 03:15:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894843003/180001

[commit-bot: I haz the power, 2015-02-06 04:16:02]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-02-06 04:17:48]

Patchset 8 (id:??) landed as
https://crrev.com/b985af1c7d2647db123a76370ea5a09e41151e61
Cr-Commit-Position: refs/heads/master@{#314974}