OOPIF: Remove the FrameTreeNode when a RemoteFrame is detached.

OOPIF: Remove the FrameTreeNode when a RemoteFrame is detached.

This ensures that subframe processes aren't left around if a parent page
removes an out-of-process iframes.

TBR=jam@chromium.org
BUG=471346, 474772
TEST=See bug for repro steps.
Credit to nick@ for TaskManagerOOPIFBrowserTest.LeavePageWithCrossSiteIframes

Committed: https://crrev.com/0f6edddc2ae422fd69d9cc69c384f046b67d3e8e
Cr-Commit-Position: refs/heads/master@{#324160}

[Charlie Reis, 2015-03-27 21:33:48]

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

[Charlie Reis, 2015-03-27 21:33:49]

Nick, can you take a look?  (Thanks for the browser test code-- I've reviewed it
and it looks good to me.)

[ncarter (slow), 2015-03-27 22:03:46]

https://codereview.chromium.org/1039403002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1039403002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:161: void
RenderFrameProxyHost::OnDetach() {
Do we need to apply some filtering here?

Seems like we'd only want to allow detach operations to originate from the
process that owns the frame. So this would be a check on the site instance of
the parent, I think.

[Charlie Reis, 2015-03-28 00:09:17]

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

[Charlie Reis, 2015-03-28 00:09:18]

[+lfg, dcheng]

Thanks.  New patch up.

Lucas, I'm wondering if you can take a look as well, since I realize this may
collide with some of the work you're doing on frame detach.  In particular, it
bothers me that the parent process detaches the entire subtree bottom-up,
reporting to the browser process each time, which then has to do corresponding
work.  Maybe that's ok?  Or would it be better to just tell the browser process
and let it handle the bottom-up detach?  Are you making changes here?

(Once we settle that, I can add tests for HasAncestorFrom if we still need it.)

https://codereview.chromium.org/1039403002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1039403002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:161: void
RenderFrameProxyHost::OnDetach() {
On 2015/03/27 22:03:46, ncarter wrote:
> Do we need to apply some filtering here?
> 
> Seems like we'd only want to allow detach operations to originate from the
> process that owns the frame. So this would be a check on the site instance of
> the parent, I think.

This is an excellent point.  Unfortunately, it's a bit more complex than that. 
We need to check whether it's from the process of any of the frame's ancestors,
since Blink will recursively detach all the subframes of a frame that it removes
(even when they're RemoteFrames).

I wonder if this is changing with @lfg's work.

[nasko, 2015-03-30 22:58:16]

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

[nasko, 2015-03-30 22:58:17]

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

https://codereview.chromium.org/1039403002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:285:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CleanupCrossSiteIframe) {
Do you think it is worthwhile having a test case that removes a subframe through
JS? It is a variation of this scenario, but seems sufficiently different to have
at test for it at some point in time.

[dcheng, 2015-03-31 01:48:31]

I'll take a look at this after I get back to Seattle. Sorry for the delay.

On Tue, Mar 31, 2015, 06:58  <nasko@chromium.org> wrote:

>
> https://codereview.chromium.org/1039403002/diff/20001/
> content/browser/site_per_process_browsertest.cc
> File content/browser/site_per_process_browsertest.cc (right):
>
> https://codereview.chromium.org/1039403002/diff/20001/
> content/browser/site_per_process_browsertest.cc#newcode285
> content/browser/site_per_process_browsertest.cc:285
>
<https://codereview.chromium.org/1039403002/diff/20001/content/browser/site_pe...>
> :
> IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
> CleanupCrossSiteIframe) {
> Do you think it is worthwhile having a test case that removes a subframe
> through JS? It is a variation of this scenario, but seems sufficiently
> different to have at test for it at some point in time.
>
> https://codereview.chromium.org/1039403002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium Site Isolation Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to site-isolation-reviews+unsubscribe@chromium.org.
> For more options, visit https://groups.google.com/a/chromium.org/d/optout.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2015-04-06 18:22:44]

Patchset #4 (id:60001) has been deleted

[Charlie Reis, 2015-04-06 19:17:12]

Ok, I've rebased this to include Alex's fix for https://crbug.com/423587 and
added some tests for the restrictions that we need (plus the JS case Nasko
mentioned).

Nick, can you take another look?

Daniel and Lucas, can you verify that this doesn't interact poorly with your
frame detach plans?

Thanks!

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

https://codereview.chromium.org/1039403002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:285:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CleanupCrossSiteIframe) {
On 2015/03/30 22:58:17, nasko (out until 04-13) wrote:
> Do you think it is worthwhile having a test case that removes a subframe
through
> JS? It is a variation of this scenario, but seems sufficiently different to
have
> at test for it at some point in time.

Done.  And I'm glad you mentioned it!  By writing the test for that, I tripped
across a use case showing that my HasAncestorFrom(SiteInstance) check is
invalid.  (A frame can be detached by a process that isn't one of its parents,
if that process navigates the parent of the frame.)

[ncarter (slow), 2015-04-06 21:13:38]

lgtm

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

https://codereview.chromium.org/1039403002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:345: EXPECT_EQ(foo_url,
observer.last_navigation_url());
These tests are looking so compact and tidy :)

https://codereview.chromium.org/1039403002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:409:
IPC::IpcSecurityTestUtil::PwnMessageReceived(
:)

[dcheng, 2015-04-06 23:34:18]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2015-04-06 23:34:19]

https://codereview.chromium.org/1039403002/diff/80001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1039403002/diff/80001/content/renderer/render...
content/renderer/render_frame_proxy.cc:312: Send(new
FrameHostMsg_Detach(routing_id_));
If there are several proxies for this frame, do the proxies then send this IPC
message back up too?

[Charlie Reis, 2015-04-07 18:45:56]

https://codereview.chromium.org/1039403002/diff/80001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1039403002/diff/80001/content/renderer/render...
content/renderer/render_frame_proxy.cc:312: Send(new
FrameHostMsg_Detach(routing_id_));
On 2015/04/06 23:34:18, dcheng wrote:
> If there are several proxies for this frame, do the proxies then send this IPC
> message back up too?

Sadly yes, but the browser process won't be listening anymore.

For example, a page with a mainframe on A and two subframes on B and C might try
to remove the B subframe.  Then:
1) A's process will send up this IPC from its RFP.
2) That causes the browser to kill the FrameTreeNode, deleting the RFH for B and
the RFPH for C.
3) C's RFPH destructor will send a DeleteProxy message to its RFP, and then it
will call RemoveRoute (causing us to drop any further IPCs).
4) The RFP will hear the DeleteProxy message, which leads to this frameDetached
call again.  However, the browser process won't recognize the route anymore and
it will be dropped.

I'm guessing this frame deletion dance could be cleaned up further, but this
should work for the time being.  Better than leaving the processes around, at
any rate.

[dcheng, 2015-04-07 22:02:55]

lgtm

[Charlie Reis, 2015-04-07 22:56:33]

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

[Charlie Reis, 2015-04-07 22:56:34]

Thanks.  Alex, as I mentioned in person, I'm going to disable the
ExtensionApiTest.ActiveTab test since it starts failing in FocusController here
(https://crbug.com/474772).

TBR'ing jam@ for the chromium.fyi.json change to do so.

(Lucas, I'll leave you on as FYI since this may be relevant to you, but Daniel
didn't think it would directly conflict with your changes.)

[Charlie Reis, 2015-04-07 22:57:06]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-04-07 22:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1039403002/#ps100001
(title: "Disable ActiveTab test on FYI bot")

[commit-bot: I haz the power, 2015-04-07 22:57:24]

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

[commit-bot: I haz the power, 2015-04-08 00:21:09]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-04-08 00:22:52]

Patchset 5 (id:??) landed as
https://crrev.com/0f6edddc2ae422fd69d9cc69c384f046b67d3e8e
Cr-Commit-Position: refs/heads/master@{#324160}