With --site-per-process, avoid a crash when a subframe process goes away.

With --site-per-process, avoid a crash when a subframe process goes away.

We need to clear out the children of any nodes that are affected by the
crash, not the entire FrameTree.

BUG=338508
TEST=Killing an iframe process with --site-per-process shows a green rectangle.
R=ajwong@chromium.org, nasko@chromium.org

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

[Charlie Reis, 2014-02-08 02:13:35]

Albert, this still needs a test, but I wanted to get your thoughts on the
changes to ResetForMainFrameSwap, and how I'm using ForEach.  The goal is to
just reset the affected nodes when a process crashes (and clear out their
children), rather than clearing the whole tree.

I'm chasing down some issues with getting the browser test to work.

[Charlie Reis, 2014-02-11 02:03:39]

Ok, test added.  I've disabled these tests on GTK since RWHVChildFrame crashes
there, and since GTK support is going away.

Nasko, maybe you can take a look?

[nasko, 2014-02-11 04:49:16]

Just a few comments.

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

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:48: bool
ResetNodesForNewProcess(RenderViewHost* render_view_host,
Shouldn't we be passing the process that is gone? Adding new code that uses RVH
seems wrong to me. We can pass the RPH that has crashed and check each frame's
process for a match, can't we?

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:169: void
FrameTree::RenderProcessGone(RenderViewHost* render_view_host) {
Again, it will be better with RPH parameter.

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:172: // Note that the helper function
may call ResetForNewProcess on a node, which
If we are now putting explicit dependency on this order of operation, it will be
useful to put a comment in ForEach, to ensure we don't change it and break
things.

https://codereview.chromium.org/156303004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/156303004/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1435: // TODO(creis):
Once subframes can be in different processes, we'll need to
nit: This comment is now stale, right?

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

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:217: //
RenderWidgetHostViewChildFrame::AllocBackingStore.  GTK support will be
This reminded me that I worked around this in my builds by specifying
--force-compositing-mode. I'm not sure if it will help here or if it worth
trying, since GTK is going away, so just mentioning it.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:285:
EXPECT_TRUE(NavigateIframeToURL(shell(), https_url, "test"));
This reminds me that it soon might be the time to look into providing generic
functions for navigating frames in the content test utils. I just need to move
ViewMsg_Navigate and it should be doable.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:296:
shell()->web_contents()->GetRenderProcessHost();
nit: Why not keep it consistent with the next line and use
root->current_frame_host()->GetProcess()?

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:304: // Ensure that the child
frame still exists but has been cleared.
nit: How are we testing that it is cleared?

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:308: RenderProcessHostWatcher
crash_observer2(
nit: Why not add local scope to avoid having variable(1|2)?

[awong, 2014-02-11 18:24:16]

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

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:49: FrameTreeNode** out_node,
Why bother with out_node at all?

[Charlie Reis, 2014-02-12 00:27:56]

Thanks!  I've addressed your comments and also removed the use of
SpawnedTestServer, since that's broken on Android (http://crbug.com/187570).  We
should remove that from RenderFrameHostManagerTest as well in another CL, since
those tests are disabled today (see content_browsertests_disabled).

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

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:48: bool
ResetNodesForNewProcess(RenderViewHost* render_view_host,
On 2014/02/11 04:49:16, nasko wrote:
> Shouldn't we be passing the process that is gone? Adding new code that uses
RVH
> seems wrong to me. We can pass the RPH that has crashed and check each frame's
> process for a match, can't we?

I've added a TODO for that.  For now, that just adds unnecessary work for both
the caller and each step of the tree traversal, since the only way to get to the
RPH is by asking the RVH.  I agree that we can cut out RVH entirely once RFH
knows its process directly.

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:49: FrameTreeNode** out_node,
On 2014/02/11 18:24:16, awong wrote:
> Why bother with out_node at all?

base::Bind is magic.  Done.

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:169: void
FrameTree::RenderProcessGone(RenderViewHost* render_view_host) {
On 2014/02/11 04:49:16, nasko wrote:
> Again, it will be better with RPH parameter.

Then we're just calling RenderViewHostImpl::GetProcess via
RenderFrameHostImpl::GetProcess at each node, as well as the call site of this
function.  I think it makes sense when RFH starts to know its own process.

https://codereview.chromium.org/156303004/diff/110001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:172: // Note that the helper function
may call ResetForNewProcess on a node, which
On 2014/02/11 04:49:16, nasko wrote:
> If we are now putting explicit dependency on this order of operation, it will
be
> useful to put a comment in ForEach, to ensure we don't change it and break
> things.

Done.

https://codereview.chromium.org/156303004/diff/110001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/156303004/diff/110001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1435: // TODO(creis):
Once subframes can be in different processes, we'll need to
On 2014/02/11 04:49:16, nasko wrote:
> nit: This comment is now stale, right?

Ah, of course.  Done.

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

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:217: //
RenderWidgetHostViewChildFrame::AllocBackingStore.  GTK support will be
On 2014/02/11 04:49:16, nasko wrote:
> This reminded me that I worked around this in my builds by specifying
> --force-compositing-mode. I'm not sure if it will help here or if it worth
> trying, since GTK is going away, so just mentioning it.

I'll give it a try in SetUpCommandLine.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:285:
EXPECT_TRUE(NavigateIframeToURL(shell(), https_url, "test"));
On 2014/02/11 04:49:16, nasko wrote:
> This reminds me that it soon might be the time to look into providing generic
> functions for navigating frames in the content test utils. I just need to move
> ViewMsg_Navigate and it should be doable.

That will be great.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:296:
shell()->web_contents()->GetRenderProcessHost();
On 2014/02/11 04:49:16, nasko wrote:
> nit: Why not keep it consistent with the next line and use
> root->current_frame_host()->GetProcess()?

Done.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:304: // Ensure that the child
frame still exists but has been cleared.
On 2014/02/11 04:49:16, nasko wrote:
> nit: How are we testing that it is cleared?

Heh, I started with some lines to check that the URL was gone, but I discovered
we have a separate bug in putting the URL on the wrong node (thanks to frame
ID).  I figured I'd fix that bug later.

I've put in a check for the frame_id getting cleared for now.  This will all get
updated in my frame_id CL.

https://codereview.chromium.org/156303004/diff/110001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:308: RenderProcessHostWatcher
crash_observer2(
On 2014/02/11 04:49:16, nasko wrote:
> nit: Why not add local scope to avoid having variable(1|2)?

Done.

[nasko, 2014-02-12 00:41:41]

LGTM

[awong, 2014-02-12 00:42:43]

LGTM

[Charlie Reis, 2014-02-12 21:10:05]

Ok, I carved out the CrossSiteIframe test into its own CL
(https://codereview.chromium.org/147363007/) and landed it in r250787.

The rest of this is the same, except that I've disabled the CrashSubframe test
on Android because it fails there.  I suspect this line:
[ERROR:kill_posix.cc(182)] Unable to terminate process 10421: Operation not
permitted
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

I think we can get it working there separately.  I'll plan to land this after
some try jobs look ok.

[Charlie Reis, 2014-02-12 23:06:11]

Committed patchset #8 manually as r250823 (presubmit successful).