Swap the main frame in --site-per-process mode too.

Swap the main frame in --site-per-process mode too.

Calling swap() is required to set up a lot of required state in Blink
for the WebRemoteFrame. Without this, bad things happen.

BUG=475003
Committed: https://crrev.com/9463868c8a2d30b6a81db9fd2b61e8a5935ac347
Cr-Commit-Position: refs/heads/master@{#329012}

[dcheng, 2015-04-23 18:51:58]

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

[dcheng, 2015-04-23 18:51:58]

Note: this depends on https://codereview.chromium.org/1079973007

Before I spent time getting an automated test working, is there anything
fundamentally wrong with this approach? alexmos@ mentioned that nasko@ tried
something like this in the past, but couldn't get it to stick for various
reasons.

However, it /seems/ to be passing browser_tests and content_browsertests locally
with the required Blink patch.

[Charlie Reis, 2015-04-23 20:58:23]

On 2015/04/23 18:51:58, dcheng wrote:
> Note: this depends on https://codereview.chromium.org/1079973007
> 
> Before I spent time getting an automated test working, is there anything
> fundamentally wrong with this approach? alexmos@ mentioned that nasko@ tried
> something like this in the past, but couldn't get it to stick for various
> reasons.

I don't have the context from Nasko's previous attempt on this, so maybe he can
find it.

I'd be curious if window references still work after the swap, but I think
you've fixed that, right?  For example, window 1 might do w=window.open(); to
create window 2.  If window 2 navigates cross-process, we should still be able
to do things like w.postMessage or w.close from window 1.  That's the original
reason for keeping the main frame RenderFrameImpl around in the swapped out
state, but it seems like we should be able to support those things on the proxy
now.

> 
> However, it /seems/ to be passing browser_tests and content_browsertests
locally
> with the required Blink patch.

I'd suggest running some try jobs as well.

[dcheng, 2015-04-23 20:59:32]

On 2015/04/23 at 20:58:23, creis wrote:
> On 2015/04/23 18:51:58, dcheng wrote:
> > Note: this depends on https://codereview.chromium.org/1079973007
> > 
> > Before I spent time getting an automated test working, is there anything
> > fundamentally wrong with this approach? alexmos@ mentioned that nasko@ tried
> > something like this in the past, but couldn't get it to stick for various
> > reasons.
> 
> I don't have the context from Nasko's previous attempt on this, so maybe he
can find it.
> 
> I'd be curious if window references still work after the swap, but I think
you've fixed that, right?  For example, window 1 might do w=window.open(); to
create window 2.  If window 2 navigates cross-process, we should still be able
to do things like w.postMessage or w.close from window 1.  That's the original
reason for keeping the main frame RenderFrameImpl around in the swapped out
state, but it seems like we should be able to support those things on the proxy
now.
> 
> > 
> > However, it /seems/ to be passing browser_tests and content_browsertests
locally
> > with the required Blink patch.
> 
> I'd suggest running some try jobs as well.

I can't run tryjobs until the Blink roll with my patch makes it into ToT. We get
lots of crashes without that patch =)

Although I guess I could temporarily roll DEPS now that the Blink patch has
landed...

[nasko, 2015-04-29 17:11:28]

On 2015/04/23 20:59:32, dcheng wrote:
> On 2015/04/23 at 20:58:23, creis wrote:
> > On 2015/04/23 18:51:58, dcheng wrote:
> > > Note: this depends on https://codereview.chromium.org/1079973007
> > > 
> > > Before I spent time getting an automated test working, is there anything
> > > fundamentally wrong with this approach? alexmos@ mentioned that nasko@
tried
> > > something like this in the past, but couldn't get it to stick for various
> > > reasons.
> > 
> > I don't have the context from Nasko's previous attempt on this, so maybe he
> can find it.
> > 
> > I'd be curious if window references still work after the swap, but I think
> you've fixed that, right?  For example, window 1 might do w=window.open(); to
> create window 2.  If window 2 navigates cross-process, we should still be able
> to do things like w.postMessage or w.close from window 1.  That's the original
> reason for keeping the main frame RenderFrameImpl around in the swapped out
> state, but it seems like we should be able to support those things on the
proxy
> now.
> > 
> > > 
> > > However, it /seems/ to be passing browser_tests and content_browsertests
> locally
> > > with the required Blink patch.
> > 
> > I'd suggest running some try jobs as well.
> 
> I can't run tryjobs until the Blink roll with my patch makes it into ToT. We
get
> lots of crashes without that patch =)
> 
> Although I guess I could temporarily roll DEPS now that the Blink patch has
> landed...

I'm doubtful this will work before we make some more changes. RenderView needs
to stay alive as we navigate across processes while the main RenderFrame should
be cleaned up. This isn't in place yet, so some preliminary refactoring needs to
happen. I'm working on that right now.

[dcheng, 2015-04-29 17:23:45]

On 2015/04/29 at 17:11:28, nasko wrote:
> On 2015/04/23 20:59:32, dcheng wrote:
> > On 2015/04/23 at 20:58:23, creis wrote:
> > > On 2015/04/23 18:51:58, dcheng wrote:
> > > > Note: this depends on https://codereview.chromium.org/1079973007
> > > > 
> > > > Before I spent time getting an automated test working, is there anything
> > > > fundamentally wrong with this approach? alexmos@ mentioned that nasko@
tried
> > > > something like this in the past, but couldn't get it to stick for
various
> > > > reasons.
> > > 
> > > I don't have the context from Nasko's previous attempt on this, so maybe
he
> > can find it.
> > > 
> > > I'd be curious if window references still work after the swap, but I think
> > you've fixed that, right?  For example, window 1 might do w=window.open();
to
> > create window 2.  If window 2 navigates cross-process, we should still be
able
> > to do things like w.postMessage or w.close from window 1.  That's the
original
> > reason for keeping the main frame RenderFrameImpl around in the swapped out
> > state, but it seems like we should be able to support those things on the
proxy
> > now.
> > > 
> > > > 
> > > > However, it /seems/ to be passing browser_tests and content_browsertests
> > locally
> > > > with the required Blink patch.
> > > 
> > > I'd suggest running some try jobs as well.
> > 
> > I can't run tryjobs until the Blink roll with my patch makes it into ToT. We
get
> > lots of crashes without that patch =)
> > 
> > Although I guess I could temporarily roll DEPS now that the Blink patch has
> > landed...
> 
> I'm doubtful this will work before we make some more changes. RenderView needs
to stay alive as we navigate across processes while the main RenderFrame should
be cleaned up. This isn't in place yet, so some preliminary refactoring needs to
happen. I'm working on that right now.

Define work.

It doesn't crash, compared to before, when it did.
And it even passes tests.

[nasko, 2015-04-29 17:31:50]

On 2015/04/29 17:23:45, dcheng wrote:
> On 2015/04/29 at 17:11:28, nasko wrote:
> > On 2015/04/23 20:59:32, dcheng wrote:
> > > On 2015/04/23 at 20:58:23, creis wrote:
> > > > On 2015/04/23 18:51:58, dcheng wrote:
> > > > > Note: this depends on https://codereview.chromium.org/1079973007
> > > > > 
> > > > > Before I spent time getting an automated test working, is there
anything
> > > > > fundamentally wrong with this approach? alexmos@ mentioned that nasko@
> tried
> > > > > something like this in the past, but couldn't get it to stick for
> various
> > > > > reasons.
> > > > 
> > > > I don't have the context from Nasko's previous attempt on this, so maybe
> he
> > > can find it.
> > > > 
> > > > I'd be curious if window references still work after the swap, but I
think
> > > you've fixed that, right?  For example, window 1 might do w=window.open();
> to
> > > create window 2.  If window 2 navigates cross-process, we should still be
> able
> > > to do things like w.postMessage or w.close from window 1.  That's the
> original
> > > reason for keeping the main frame RenderFrameImpl around in the swapped
out
> > > state, but it seems like we should be able to support those things on the
> proxy
> > > now.
> > > > 
> > > > > 
> > > > > However, it /seems/ to be passing browser_tests and
content_browsertests
> > > locally
> > > > > with the required Blink patch.
> > > > 
> > > > I'd suggest running some try jobs as well.
> > > 
> > > I can't run tryjobs until the Blink roll with my patch makes it into ToT.
We
> get
> > > lots of crashes without that patch =)
> > > 
> > > Although I guess I could temporarily roll DEPS now that the Blink patch
has
> > > landed...
> > 
> > I'm doubtful this will work before we make some more changes. RenderView
needs
> to stay alive as we navigate across processes while the main RenderFrame
should
> be cleaned up. This isn't in place yet, so some preliminary refactoring needs
to
> happen. I'm working on that right now.
> 
> Define work.
> 
> It doesn't crash, compared to before, when it did.
> And it even passes tests.

Once we fix the memory leak of not deleting RenderFrame on swap(), RenderView
will have a stale pointer to its main frame. All the places
RenderView::GetMainFrame result is used, we will see crashes.

[dcheng, 2015-04-29 17:33:36]

On 2015/04/29 at 17:31:50, nasko wrote:
> On 2015/04/29 17:23:45, dcheng wrote:
> > On 2015/04/29 at 17:11:28, nasko wrote:
> > > On 2015/04/23 20:59:32, dcheng wrote:
> > > > On 2015/04/23 at 20:58:23, creis wrote:
> > > > > On 2015/04/23 18:51:58, dcheng wrote:
> > > > > > Note: this depends on https://codereview.chromium.org/1079973007
> > > > > > 
> > > > > > Before I spent time getting an automated test working, is there
anything
> > > > > > fundamentally wrong with this approach? alexmos@ mentioned that
nasko@
> > tried
> > > > > > something like this in the past, but couldn't get it to stick for
> > various
> > > > > > reasons.
> > > > > 
> > > > > I don't have the context from Nasko's previous attempt on this, so
maybe
> > he
> > > > can find it.
> > > > > 
> > > > > I'd be curious if window references still work after the swap, but I
think
> > > > you've fixed that, right?  For example, window 1 might do
w=window.open();
> > to
> > > > create window 2.  If window 2 navigates cross-process, we should still
be
> > able
> > > > to do things like w.postMessage or w.close from window 1.  That's the
> > original
> > > > reason for keeping the main frame RenderFrameImpl around in the swapped
out
> > > > state, but it seems like we should be able to support those things on
the
> > proxy
> > > > now.
> > > > > 
> > > > > > 
> > > > > > However, it /seems/ to be passing browser_tests and
content_browsertests
> > > > locally
> > > > > > with the required Blink patch.
> > > > > 
> > > > > I'd suggest running some try jobs as well.
> > > > 
> > > > I can't run tryjobs until the Blink roll with my patch makes it into
ToT. We
> > get
> > > > lots of crashes without that patch =)
> > > > 
> > > > Although I guess I could temporarily roll DEPS now that the Blink patch
has
> > > > landed...
> > > 
> > > I'm doubtful this will work before we make some more changes. RenderView
needs
> > to stay alive as we navigate across processes while the main RenderFrame
should
> > be cleaned up. This isn't in place yet, so some preliminary refactoring
needs to
> > happen. I'm working on that right now.
> > 
> > Define work.
> > 
> > It doesn't crash, compared to before, when it did.
> > And it even passes tests.
> 
> Once we fix the memory leak of not deleting RenderFrame on swap(), RenderView
will have a stale pointer to its main frame. All the places
RenderView::GetMainFrame result is used, we will see crashes.

I agree that this patch will expose other problems. But it's only in
--site-per-process, and it moves us down the road a bit more?

[nasko, 2015-04-29 17:50:10]

On 2015/04/29 17:33:36, dcheng wrote:
> On 2015/04/29 at 17:31:50, nasko wrote:
> > On 2015/04/29 17:23:45, dcheng wrote:
> > > On 2015/04/29 at 17:11:28, nasko wrote:
> > > > On 2015/04/23 20:59:32, dcheng wrote:
> > > > > On 2015/04/23 at 20:58:23, creis wrote:
> > > > > > On 2015/04/23 18:51:58, dcheng wrote:
> > > > > > > Note: this depends on https://codereview.chromium.org/1079973007
> > > > > > > 
> > > > > > > Before I spent time getting an automated test working, is there
> anything
> > > > > > > fundamentally wrong with this approach? alexmos@ mentioned that
> nasko@
> > > tried
> > > > > > > something like this in the past, but couldn't get it to stick for
> > > various
> > > > > > > reasons.
> > > > > > 
> > > > > > I don't have the context from Nasko's previous attempt on this, so
> maybe
> > > he
> > > > > can find it.
> > > > > > 
> > > > > > I'd be curious if window references still work after the swap, but I
> think
> > > > > you've fixed that, right?  For example, window 1 might do
> w=window.open();
> > > to
> > > > > create window 2.  If window 2 navigates cross-process, we should still
> be
> > > able
> > > > > to do things like w.postMessage or w.close from window 1.  That's the
> > > original
> > > > > reason for keeping the main frame RenderFrameImpl around in the
swapped
> out
> > > > > state, but it seems like we should be able to support those things on
> the
> > > proxy
> > > > > now.
> > > > > > 
> > > > > > > 
> > > > > > > However, it /seems/ to be passing browser_tests and
> content_browsertests
> > > > > locally
> > > > > > > with the required Blink patch.
> > > > > > 
> > > > > > I'd suggest running some try jobs as well.
> > > > > 
> > > > > I can't run tryjobs until the Blink roll with my patch makes it into
> ToT. We
> > > get
> > > > > lots of crashes without that patch =)
> > > > > 
> > > > > Although I guess I could temporarily roll DEPS now that the Blink
patch
> has
> > > > > landed...
> > > > 
> > > > I'm doubtful this will work before we make some more changes. RenderView
> needs
> > > to stay alive as we navigate across processes while the main RenderFrame
> should
> > > be cleaned up. This isn't in place yet, so some preliminary refactoring
> needs to
> > > happen. I'm working on that right now.
> > > 
> > > Define work.
> > > 
> > > It doesn't crash, compared to before, when it did.
> > > And it even passes tests.
> > 
> > Once we fix the memory leak of not deleting RenderFrame on swap(),
RenderView
> will have a stale pointer to its main frame. All the places
> RenderView::GetMainFrame result is used, we will see crashes.
> 
> I agree that this patch will expose other problems. But it's only in
> --site-per-process, and it moves us down the road a bit more?

My only worry is whether we will be able to fix the leaks easily after this
lands or is it going to force us to go down a longer path to get that fixed. I'm
fine going with this CL. It is trivial enough that we can revert if needed.

LGTM

[dcheng, 2015-05-05 21:58:08]

PTAL: I've updated and incorporated lfg@'s test from the original bug.

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

https://codereview.chromium.org/1106673002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2337: // the frame tree, execute
a simple bit of JS first.
It seems like there should be a better way to do this... but meh.

https://codereview.chromium.org/1106673002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2343:
EXPECT_TRUE(root->current_frame_host()->IsRenderFrameLive());
Should I do this for the cross-site window too?

[nasko, 2015-05-06 21:55:24]

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

https://codereview.chromium.org/1106673002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2343:
EXPECT_TRUE(root->current_frame_host()->IsRenderFrameLive());
On 2015/05/05 21:58:08, dcheng wrote:
> Should I do this for the cross-site window too?

I don't think it will hurt. Also, why not use WebConetntsImpl::GetMainFrame()?
It is equivalent to doing frame_tree.root()->current_frame_host().

[dcheng, 2015-05-06 22:52:28]

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

https://codereview.chromium.org/1106673002/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2343:
EXPECT_TRUE(root->current_frame_host()->IsRenderFrameLive());
On 2015/05/06 21:55:24, nasko wrote:
> On 2015/05/05 21:58:08, dcheng wrote:
> > Should I do this for the cross-site window too?
> 
> I don't think it will hurt. Also, why not use WebConetntsImpl::GetMainFrame()?
> It is equivalent to doing frame_tree.root()->current_frame_host().

Done.

No real reason other than I didn't know about it =)

[nasko, 2015-05-06 22:57:44]

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

https://codereview.chromium.org/1106673002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2315:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
nit: It will be useful to add a description of what this test case is trying to
verify. As it is, I can see what it does, but don't know much of why it does it
and what it ensures.

[alexmos, 2015-05-06 23:03:14]

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

[alexmos, 2015-05-06 23:03:14]

Just a drive-by comment: presumably we should now be able to clean up all the
uses of IsMainFrameDetachedFromTree(), right?

[dcheng, 2015-05-06 23:14:08]

On 2015/05/06 at 23:03:14, alexmos wrote:
> Just a drive-by comment: presumably we should now be able to clean up all the
uses of IsMainFrameDetachedFromTree(), right?

I think I might be missing the context of this cleanup.

[alexmos, 2015-05-06 23:24:50]

On 2015/05/06 23:14:08, dcheng wrote:
> On 2015/05/06 at 23:03:14, alexmos wrote:
> > Just a drive-by comment: presumably we should now be able to clean up all
the
> uses of IsMainFrameDetachedFromTree(), right?
> 
> I think I might be missing the context of this cleanup.

So, for example, this workaround shouldn't be necessary anymore:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

That can happen in a separate CL though (and I'm happy to do it).  Basically, if
I understand it correctly, IsMainFrameDetachedFromTree() shouldn't ever return
true after this CL and can be removed.

[alexmos, 2015-05-06 23:51:25]

On 2015/05/06 23:24:50, alexmos wrote:
> On 2015/05/06 23:14:08, dcheng wrote:
> > On 2015/05/06 at 23:03:14, alexmos wrote:
> > > Just a drive-by comment: presumably we should now be able to clean up all
> the
> > uses of IsMainFrameDetachedFromTree(), right?
> > 
> > I think I might be missing the context of this cleanup.
> 
> So, for example, this workaround shouldn't be necessary anymore:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> That can happen in a separate CL though (and I'm happy to do it).  Basically,
if
> I understand it correctly, IsMainFrameDetachedFromTree() shouldn't ever return
> true after this CL and can be removed.

Ah, never mind, I guess we still need this until we can swap the proxies into
the tree even without --site-per-process, which is hopefully soon. :)

[dcheng, 2015-05-07 22:30:13]

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

https://codereview.chromium.org/1106673002/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:2315:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
On 2015/05/06 22:57:44, nasko wrote:
> nit: It will be useful to add a description of what this test case is trying
to
> verify. As it is, I can see what it does, but don't know much of why it does
it
> and what it ensures.

Done.

[nasko, 2015-05-08 18:00:01]

Still LGTM!

[dcheng, 2015-05-08 18:00:11]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2015-05-08 18:01:12]

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

[commit-bot: I haz the power, 2015-05-08 19:30:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-08 19:32:17]

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

[dcheng, 2015-05-08 22:21:29]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1137833002/ by dcheng@chromium.org.

The reason for reverting is: This breaks some tests on the site isolation bots..