OOPIF: Don't resurrect a dead process just to create proxies.

OOPIF: Don't resurrect a dead process just to create proxies.

This fixes another cause of crashing without a valid parent proxy in
RenderFrameProxy::CreateFrameProxy.  This occurred when a renderer
crashed, and another renderer added a child frame, which triggered a
new proxy for that frame to be created for the crashed process.  The
crashed process was recreated just to create the proxy, and the proxy
creation crashed because its parent proxy didn't exist.

This CL fixes InitRenderFrameProxy to not recreate a process just to
create proxies.  The process should only come back if it ever needs to
host a RenderFrame, and all the proxies should already be created
then.

BUG=476846
Committed: https://crrev.com/a3988993743393edd93d1a72ca4cb9454973ab92
Cr-Commit-Position: refs/heads/master@{#329972}

[alexmos, 2015-05-14 16:59:17]

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

[alexmos, 2015-05-14 17:01:43]

Nasko, can you please take a look?

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:147:
frame_tree_node_->parent()->render_manager()->GetRenderFrameProxyHost(
Using GetRenderFrameProxyHost instead of GetRoutingIdForSiteInstance should be
safe, since FrameMsg_NewFrameProxy expects the parent to always be a
RenderFrameProxy.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:153:
DCHECK(parent_proxy->is_render_frame_proxy_live());
I don't think we need my previous recursively-initialize-the-parent fix with the
way this is currently called.  We can revisit that if we ever hit this (which
would also yield another repro for 476846).

[nasko, 2015-05-14 21:26:29]

Thanks for fixing this! The iterative fix didn't make as much sense, so this
looks very nice! Few comments.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:137: // RenderFrame.  In
that case, CreateRenderView will reinitialize the
As we are trying to kill off RenderView, having CreateRenderView be in charge of
initializing the process is a bit fragile. Let's ensure that we init the process
at the time we know we will be creating a RenderFrame in it. Namely, the callers
of CreateProxiesForSiteInstance (or actually CreateOpenerRenderViewsIfNeeded).

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:147:
frame_tree_node_->parent()->render_manager()->GetRenderFrameProxyHost(
On 2015/05/14 17:01:43, alexmos wrote:
> Using GetRenderFrameProxyHost instead of GetRoutingIdForSiteInstance should be
> safe, since FrameMsg_NewFrameProxy expects the parent to always be a
> RenderFrameProxy.

This is a very good observation. Let's put that in a comment to explain why a
proxy will never have a RFH as a parent.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:153:
DCHECK(parent_proxy->is_render_frame_proxy_live());
On 2015/05/14 17:01:42, alexmos wrote:
> I don't think we need my previous recursively-initialize-the-parent fix with
the
> way this is currently called.  We can revisit that if we ever hit this (which
> would also yield another repro for 476846).

Acknowledged.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:893: // frame in process A
doesn't resurrect process B just to create the new proxy.
Let's abstract this sentence to avoid the A and B, which should be explained
after the picture. Something like "Verify that proxy creation doesn't recreate a
crashed process if no frame will be created in it" or something along those
lines.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:912: TestNavigationObserver
observer(shell()->web_contents());
nit: The observer should be declared as close to the navigation it is observing
as possible. It looks like this one is only used for the last navigation in this
test.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:941: // be live, and B should
still not have a process.
nit: alive?
I don't see a test for the "B should still not have a process." part.

[alexmos, 2015-05-14 22:02:50]

Thanks!  Comments addressed, plus one small new question.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:137: // RenderFrame.  In
that case, CreateRenderView will reinitialize the
On 2015/05/14 21:26:29, nasko wrote:
> As we are trying to kill off RenderView, having CreateRenderView be in charge
of
> initializing the process is a bit fragile. Let's ensure that we init the
process
> at the time we know we will be creating a RenderFrame in it. Namely, the
callers
> of CreateProxiesForSiteInstance (or actually CreateOpenerRenderViewsIfNeeded).

Done.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:147:
frame_tree_node_->parent()->render_manager()->GetRenderFrameProxyHost(
On 2015/05/14 21:26:29, nasko wrote:
> On 2015/05/14 17:01:43, alexmos wrote:
> > Using GetRenderFrameProxyHost instead of GetRoutingIdForSiteInstance should
be
> > safe, since FrameMsg_NewFrameProxy expects the parent to always be a
> > RenderFrameProxy.
> 
> This is a very good observation. Let's put that in a comment to explain why a
> proxy will never have a RFH as a parent.

Done.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:153:
DCHECK(parent_proxy->is_render_frame_proxy_live());
On 2015/05/14 21:26:29, nasko wrote:
> On 2015/05/14 17:01:42, alexmos wrote:
> > I don't think we need my previous recursively-initialize-the-parent fix with
> the
> > way this is currently called.  We can revisit that if we ever hit this
(which
> > would also yield another repro for 476846).
> 
> Acknowledged.

I also converted this DCHECK to a CHECK.  (This should only be reachable in
--site-per-process.)

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:893: // frame in process A
doesn't resurrect process B just to create the new proxy.
On 2015/05/14 21:26:29, nasko wrote:
> Let's abstract this sentence to avoid the A and B, which should be explained
> after the picture. Something like "Verify that proxy creation doesn't recreate
a
> crashed process if no frame will be created in it" or something along those
> lines.

Done.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:912: TestNavigationObserver
observer(shell()->web_contents());
On 2015/05/14 21:26:29, nasko wrote:
> nit: The observer should be declared as close to the navigation it is
observing
> as possible. It looks like this one is only used for the last navigation in
this
> test.

Done.  Thanks for noticing; it got left behind after some code churn in the
test.

https://codereview.chromium.org/1138413002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:941: // be live, and B should
still not have a process.
On 2015/05/14 21:26:29, nasko wrote:
> nit: alive?
Done.

> I don't see a test for the "B should still not have a process." part.

This is checked by the last line of the DepictFrameTree expected output: "     
B = http://b.com/ (no process)".  I added a comment about it.

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1353:
new_instance->GetProcess()->Init();
Should I also return early here if this fails?  CreateRenderFrame/View also
check GetProcess()->Init() and will return early, but not sure if things like
CancelPending() should still happen even if process creation failed.

[nasko, 2015-05-14 22:10:53]

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1351: // process or the
old process crashed) have been initialized. Calling Init
nit: s/old/existing/

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1353:
new_instance->GetProcess()->Init();
On 2015/05/14 22:02:50, alexmos wrote:
> Should I also return early here if this fails?  CreateRenderFrame/View also
> check GetProcess()->Init() and will return early, but not sure if things like
> CancelPending() should still happen even if process creation failed.

Returning early is probably desired. Looking at the callers for that method, it
looks best if we cancel the pending RFH. I don't know if there is a specific
reason to the ordering of those calls.

[alexmos, 2015-05-14 22:54:16]

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1351: // process or the
old process crashed) have been initialized. Calling Init
On 2015/05/14 22:10:52, nasko wrote:
> nit: s/old/existing/

Done.

https://codereview.chromium.org/1138413002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1353:
new_instance->GetProcess()->Init();
On 2015/05/14 22:10:52, nasko wrote:
> On 2015/05/14 22:02:50, alexmos wrote:
> > Should I also return early here if this fails?  CreateRenderFrame/View also
> > check GetProcess()->Init() and will return early, but not sure if things
like
> > CancelPending() should still happen even if process creation failed.
> 
> Returning early is probably desired. Looking at the callers for that method,
it
> looks best if we cancel the pending RFH. I don't know if there is a specific
> reason to the ordering of those calls.

I added the early return and moved CancelPending to be done before process init.
 I double-checked with Charlie, and he didn't see any particular reason why
CancelPending should be done after CreateOpenerRenderViewsIfNeeded.  Charlie
pointed out that it was introduced by carlosk in
https://codereview.chromium.org/753173002/diff/80001/content/browser/frame_ho...,
who was probably just trying to put it above CreateRenderFrame.

[nasko, 2015-05-14 22:55:47]

LGTM

[alexmos, 2015-05-14 22:59:59]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-05-14 23:00:49]

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

[commit-bot: I haz the power, 2015-05-14 23:26:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-14 23:27:16]

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