Ensure that before creating proxy of site A, RVH of site A is initialized.

Ensure that before creating proxy of site A, RVH of site A is initialized.

Imagine we have a frame pointing to oop site A.
Now if we kill the process of that frame, the RVH of A
goes into a IsRenderViewLive = false state.
If we try to navigate another frame to site A now, the RVH in question
is not initialized and proxy creation in root() will fail because
there's no frame in that RVH.

  A
 /
B

In A-embed-B scenario, killing process B should not destroy
the host/browser counter parts, so the Swapped out RFH/RVH and
proxy of B in A's |proxy_hosts_| should stay around.
This CL makes proxy of B stay around in A's |proxy_hosts_|.

Next time we want to navigate a frame to site instance B, it
should restore the renderer process by:
1) Making sure we call RFHM::InitRenderView().
2) Making sure we call RFPH::InitRenderFrameProxy().

To achieve 2), I've added a RFPH::IsRenderFrameProxyLive()
which behind the scenes uses a boolean render_frame_proxy_created_
that indicates whether the renderer/ side of the proxy has been
created or not.

BUG=432107
Test=Load two frames in a page, one pointing to different
origin (oop) and one pointing to same origin (local).
Kill the process associated with the oop frame.
Navigate the same origin frame to the first frame's site
instance so that it goes oop.

Committed: https://crrev.com/bb1af56567bd6f8724f4415c5151e3f4b0dd63d5
Cr-Commit-Position: refs/heads/master@{#314482}

[lazyboy, 2014-12-09 20:04:19]

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

[Charlie Reis, 2014-12-10 21:30:24]

Great.  Fix looks reasonable to me.  Can you add a test to
site_per_process_browsertest.cc based on your description of the repro steps?

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

[Also +site-isolation-reviews@chromium.org]

[lazyboy, 2014-12-10 21:35:10]

On 2014/12/10 21:30:24, Charlie Reis wrote:
> Great.  Fix looks reasonable to me.  Can you add a test to
> site_per_process_browsertest.cc based on your description of the repro steps?

OK, will do and ping back on this thread. Ty.

[lazyboy, 2014-12-11 01:29:26]

Patchset #3 (id:40001) has been deleted

[lazyboy, 2014-12-11 01:29:48]

New attempt is on patchset #3.

https://chromiumcodereview.appspot.com/782093002/diff/20001/content/browser/f...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/20001/content/browser/f...
content/browser/frame_host/frame_tree.cc:277: return iter->second;
Trying to reuse the RVH fails after the frame tree replication CL landed,
because once we try to Initialize() this RVH, we want to access the main_frame's
replication_state():
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
Also reusing would make RVH->main_frame_routing_id() point to incorrect id.

So I've changed the CL to create a new RVH in this case as well.

Now, this will insert the old RVH to render_view_host_pending_shutdown_map_, is
there any side-effect to that?

[Charlie Reis, 2014-12-12 19:18:31]

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

[Charlie Reis, 2014-12-12 19:18:32]

[+nasko to sanity check]

Thanks for the test!  I had a second thought about the fix after looking more
closely.

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:208:
root()->render_manager()->CreateRenderFrame(
I took a closer look, and now I'm not sure this is the right fix.  We don't want
to create a new RVH+RFH if the previous ones are around but the process has
crashed.

Instead, we want to call RFHM::InitRenderView on the existing RVH if
!IsRenderViewLive.  (I'm guessing we don't want to call InitRenderFrame since
we'll be creating a proxy for it below.)

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

https://codereview.chromium.org/782093002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:399:
NavigateRemoteFrameToAKilledSiteInstance) {
nit: ToAKilledSiteInstance -> ToKilledProcess

https://codereview.chromium.org/782093002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:410: FrameTreeNode* first_child
= root->child_at(0);
Before this, we should ASSERT_EQ(2U, root->child_count()), since we depend on
that below.

https://codereview.chromium.org/782093002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:420: {
nit: No need for brace here if we're only doing one crash.

[nasko, 2014-12-12 22:25:01]

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:208:
root()->render_manager()->CreateRenderFrame(
On 2014/12/12 19:18:31, Charlie Reis wrote:
> I took a closer look, and now I'm not sure this is the right fix.  We don't
want
> to create a new RVH+RFH if the previous ones are around but the process has
> crashed.
> 
> Instead, we want to call RFHM::InitRenderView on the existing RVH if
> !IsRenderViewLive.  (I'm guessing we don't want to call InitRenderFrame since
> we'll be creating a proxy for it below.)

I agree we shouldn't be calling CreateRenderFrame. We already have the
browser-site host objects, we just need to create the renderer side ones.
InitRenderView is supposed to do this.

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:268: // |main_frame| is NULL when the
RVH is uninitialized.
What sequence of events causes uninitialized RVH to be present in this map and
for this code to be reached?

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

https://codereview.chromium.org/782093002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:398:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
nit: Add a comment about what this case is testing and a link to the bug.

https://codereview.chromium.org/782093002/diff/60001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:430: url =
embedded_test_server()->GetURL("foo.com", "/title1.html");
nit: It seems strange to navigate to title2 first and 1 second :), very minor,
but let's flip it around as debugging is easier if they are consecutive.

[lazyboy, 2014-12-16 07:10:04]

I'm answering one major question and uploaded one patchset #4 for you guys to
look at, will address other comments once we figure out a solution to the main
issue, thanks.

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:208:
root()->render_manager()->CreateRenderFrame(
On 2014/12/12 22:25:01, nasko wrote:
> On 2014/12/12 19:18:31, Charlie Reis wrote:
> > I took a closer look, and now I'm not sure this is the right fix.  We don't
> want
> > to create a new RVH+RFH if the previous ones are around but the process has
> > crashed.
> > 
> > Instead, we want to call RFHM::InitRenderView on the existing RVH if
> > !IsRenderViewLive.  (I'm guessing we don't want to call InitRenderFrame
since
> > we'll be creating a proxy for it below.)
> 
> I agree we shouldn't be calling CreateRenderFrame. We already have the
> browser-site host objects, we just need to create the renderer side ones.
> InitRenderView is supposed to do this.

For context about uninitialized RVH:
  1
 / \
2   3
1 and 3 point to site A, 2 points to site B.
In process B, other than the proxies, we have
a) swapped out RVH_B,
b) swapped out sRFH_B, this is the "main_frame" for RVH_B. and
c) the non-swapped out frame for 2 say, RFH2.

[1] Now we kill the process of site B, this destroys sRFH_B along with
all the proxy hosts in process B,
but we still have RVH_B whose IsRenderViewLive==false, RFH2 pointing to 2.

Now if we try to navigate 3 to site B,
we see that we have the (swapped_out) RVH_B already, but the problem is
that its main_frame_routing_id is pointing to previously created
swapped_out sRFH_B's routing id which is no longer valid [2].

Does that sound right?
We do need to create this swapped out main render frame again [2]


(btw, trying to re-navigate 2 to site B again after killing process B in [1] is
a different story. We need to fix that separately).

[2] To fix this, one thing I tried is
FrameTree::CreateRenderViewHost() can return (existing)
RVH_B, but its main_frame needs to be updated to the point to the
render frame with |main_frame_routing_id| param of FT::CreateRenderViewHost().
This is shown in patchset #4

[lazyboy, 2014-12-16 20:53:01]

On 2014/12/16 07:10:04, lazyboy wrote:
> I'm answering one major question and uploaded one patchset #4 for you guys to
> look at, will address other comments once we figure out a solution to the main
> issue, thanks.
> 
>
https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
> File content/browser/frame_host/frame_tree.cc (right):
> 
>
https://codereview.chromium.org/782093002/diff/60001/content/browser/frame_ho...
> content/browser/frame_host/frame_tree.cc:208:
> root()->render_manager()->CreateRenderFrame(
> On 2014/12/12 22:25:01, nasko wrote:
> > On 2014/12/12 19:18:31, Charlie Reis wrote:
> > > I took a closer look, and now I'm not sure this is the right fix.  We
don't
> > want
> > > to create a new RVH+RFH if the previous ones are around but the process
has
> > > crashed.
> > > 
> > > Instead, we want to call RFHM::InitRenderView on the existing RVH if
> > > !IsRenderViewLive.  (I'm guessing we don't want to call InitRenderFrame
> since
> > > we'll be creating a proxy for it below.)
> > 
> > I agree we shouldn't be calling CreateRenderFrame. We already have the
> > browser-site host objects, we just need to create the renderer side ones.
> > InitRenderView is supposed to do this.
> 
> For context about uninitialized RVH:
>   1
>  / \
> 2   3
> 1 and 3 point to site A, 2 points to site B.
> In process B, other than the proxies, we have
> a) swapped out RVH_B,
> b) swapped out sRFH_B, this is the "main_frame" for RVH_B. and
> c) the non-swapped out frame for 2 say, RFH2.
> 
> [1] Now we kill the process of site B, this destroys sRFH_B along with
> all the proxy hosts in process B,
> but we still have RVH_B whose IsRenderViewLive==false, RFH2 pointing to 2.
> 
> Now if we try to navigate 3 to site B,
> we see that we have the (swapped_out) RVH_B already, but the problem is
> that its main_frame_routing_id is pointing to previously created
> swapped_out sRFH_B's routing id which is no longer valid [2].
> 
> Does that sound right?
> We do need to create this swapped out main render frame again [2]
> 
> 
> (btw, trying to re-navigate 2 to site B again after killing process B in [1]
is
> a different story. We need to fix that separately).
> 
> [2] To fix this, one thing I tried is
> FrameTree::CreateRenderViewHost() can return (existing)
> RVH_B, but its main_frame needs to be updated to the point to the
> render frame with |main_frame_routing_id| param of FT::CreateRenderViewHost().
> This is shown in patchset #4

I'll re-up with the new approach we discussed offline and ping back in the
thread.

[lazyboy, 2014-12-23 22:03:11]

I've got the new approach to work.

See updated CL description for details.

At high level, the changes are to make sure we keep browser/ side stuffs around
when we kill a process, including the RFPH. And when navigating a frame to that
process, we make sure we initialize the RVH and the proxy. For the latter, I've
added IsRenderFrameProxyLive() to RFPH.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/f...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/f...
content/browser/frame_host/frame_tree.cc:268: // |main_frame| is NULL when the
RVH is uninitialized.
On 2014/12/12 22:25:01, nasko wrote:
> What sequence of events causes uninitialized RVH to be present in this map and
> for this code to be reached?

Since we keep around the proxy (with swapped out rfh), this won't happen
anymore.
Before it was happening because the swapped out rfh wasn't there and
iter->second->main_frame_routing_id_ was pointing to that (deleted) swapped out
rfh's routing id.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
content/browser/site_per_process_browsertest.cc:398:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
On 2014/12/12 22:25:01, nasko wrote:
> nit: Add a comment about what this case is testing and a link to the bug.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
content/browser/site_per_process_browsertest.cc:399:
NavigateRemoteFrameToAKilledSiteInstance) {
On 2014/12/12 19:18:31, Charlie Reis (OOO until Jan 5) wrote:
> nit: ToAKilledSiteInstance -> ToKilledProcess

Done.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
content/browser/site_per_process_browsertest.cc:410: FrameTreeNode* first_child
= root->child_at(0);
On 2014/12/12 19:18:31, Charlie Reis (OOO until Jan 5) wrote:
> Before this, we should ASSERT_EQ(2U, root->child_count()), since we depend on
> that below.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
content/browser/site_per_process_browsertest.cc:420: {
On 2014/12/12 19:18:31, Charlie Reis (OOO until Jan 5) wrote:
> nit: No need for brace here if we're only doing one crash.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/60001/content/browser/s...
content/browser/site_per_process_browsertest.cc:430: url =
embedded_test_server()->GetURL("foo.com", "/title1.html");
On 2014/12/12 22:25:01, nasko (Out till 2015) wrote:
> nit: It seems strange to navigate to title2 first and 1 second :), very minor,
> but let's flip it around as debugging is easier if they are consecutive.

Done.

[lazyboy, 2015-01-20 18:41:41]

@Nasko, can you take a look at the CL? Thanks.

[nasko, 2015-01-20 23:51:23]

Overall it looks nice. Mostly nits and one suggestion for improving the test.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/frame_tree.cc:211: } else if
(!render_view_host->IsRenderViewLive()) {
Why not move this check inside the Ensure* method? It can return early if the RV
is already initialized. It will be unconditional call here and make code a bit
more readable.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:523: DCHECK(proxy);
nit: no need for this DCHECK, it will crash on the following line.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1429: DCHECK(proxy);
nit: no need for DCHECK, it will crash on the next line.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:417: static bool
ResetProxiesInSiteInstanceForNewProcess(int32 site_instance_id,
nit: The "ForNewProcess" doesn't add much to the name of the method, so it might
be better to drop it for readability.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:124: bool
RenderFrameProxyHost::IsRenderFrameProxyLive() {
If this is just an accessor, it needs to be hacker_case and can be inlined in
the header file.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:393: // After we kill B, we make
sure B stays in node1.proxy_hosts_, then we navigate
"navigate B" is unclear. Does it mean "navigate 3 to B"?

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:397: // Note that due to another
bug node2 does not re-navigate to site B and stays
Do we have a bug filed for this? If not, let's file one and reference it here.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:405: FrameTreeNode* node1 =
nit: s/node1/root/

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:443: // 4          C
Awesome ASCII art! It is very useful to understand the test! Thanks!

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:465: GURL
site_a_url(embedded_test_server()->GetURL("/title1.html"));
It will be useful to just create a full structure of the frame tree in HTML and
avoid lots of the boilerplate code.

[lazyboy, 2015-01-21 18:52:14]

Comments addressed in patchset #10.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/frame_tree.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/frame_tree.cc:211: } else if
(!render_view_host->IsRenderViewLive()) {
On 2015/01/20 23:51:22, nasko wrote:
> Why not move this check inside the Ensure* method? It can return early if the
RV
> is already initialized. It will be unconditional call here and make code a bit
> more readable.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:523: DCHECK(proxy);
On 2015/01/20 23:51:23, nasko wrote:
> nit: no need for this DCHECK, it will crash on the following line.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1429: DCHECK(proxy);
On 2015/01/20 23:51:23, nasko wrote:
> nit: no need for DCHECK, it will crash on the next line.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:417: static bool
ResetProxiesInSiteInstanceForNewProcess(int32 site_instance_id,
On 2015/01/20 23:51:23, nasko wrote:
> nit: The "ForNewProcess" doesn't add much to the name of the method, so it
might
> be better to drop it for readability.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.cc:124: bool
RenderFrameProxyHost::IsRenderFrameProxyLive() {
On 2015/01/20 23:51:23, nasko wrote:
> If this is just an accessor, it needs to be hacker_case and can be inlined in
> the header file.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:393: // After we kill B, we make
sure B stays in node1.proxy_hosts_, then we navigate
On 2015/01/20 23:51:23, nasko wrote:
> "navigate B" is unclear. Does it mean "navigate 3 to B"?

Yes, Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:397: // Note that due to another
bug node2 does not re-navigate to site B and stays
On 2015/01/20 23:51:23, nasko wrote:
> Do we have a bug filed for this? If not, let's file one and reference it here.

Filed bug and added reference to it.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:405: FrameTreeNode* node1 =
On 2015/01/20 23:51:23, nasko wrote:
> nit: s/node1/root/

Done.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:443: // 4          C
On 2015/01/20 23:51:23, nasko wrote:
> Awesome ASCII art! It is very useful to understand the test! Thanks!

Acknowledged.

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:465: GURL
site_a_url(embedded_test_server()->GetURL("/title1.html"));
On 2015/01/20 23:51:23, nasko wrote:
> It will be useful to just create a full structure of the frame tree in HTML
and
> avoid lots of the boilerplate code.

This could only be done for the second subframe (node3) to point to a
local frame directly from HTML (to "title1.html"), I've done that in
the updated patch.
The other steps/navigations, e.g. navigating node2 to a remote frame,
need to happen afterwards and hence that code is still in this file.
Is that OK or you had something else in mind?

[lazyboy, 2015-01-22 23:27:53]

Patchset #12 (id:240001) has been deleted

[lazyboy, 2015-01-22 23:28:10]

Patchset #11 (id:220001) has been deleted

[lazyboy, 2015-01-22 23:32:35]

I've update the CL to specify the tree structure of the test directly in the
html file (Patchset #11).

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
content/browser/site_per_process_browsertest.cc:465: GURL
site_a_url(embedded_test_server()->GetURL("/title1.html"));
On 2015/01/21 18:52:14, lazyboy wrote:
> On 2015/01/20 23:51:23, nasko wrote:
> > It will be useful to just create a full structure of the frame tree in HTML
> and
> > avoid lots of the boilerplate code.
> 
> This could only be done for the second subframe (node3) to point to a
> local frame directly from HTML (to "title1.html"), I've done that in
> the updated patch.
> The other steps/navigations, e.g. navigating node2 to a remote frame,
> need to happen afterwards and hence that code is still in this file.
> Is that OK or you had something else in mind?

After discussion, I've update this further. Now the initial tree structure is
coming directly from html.

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/site_per_process_browsertest.cc:478: ASSERT_EQ(1U,
root->child_at(0)->child_count());
This is the part I was worried about: whether the first child's
descendants are safe to access right after NavigateToURL() call,
in practice seems to work. Let me know if there's a chance of
race here.

[lazyboy, 2015-02-02 20:39:19]

On 2015/01/22 23:32:35, lazyboy wrote:
> I've update the CL to specify the tree structure of the test directly in the
> html file (Patchset #11).
> 
>
https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/782093002/diff/180001/content/browser/...
> content/browser/site_per_process_browsertest.cc:465: GURL
> site_a_url(embedded_test_server()->GetURL("/title1.html"));
> On 2015/01/21 18:52:14, lazyboy wrote:
> > On 2015/01/20 23:51:23, nasko wrote:
> > > It will be useful to just create a full structure of the frame tree in
HTML
> > and
> > > avoid lots of the boilerplate code.
> > 
> > This could only be done for the second subframe (node3) to point to a
> > local frame directly from HTML (to "title1.html"), I've done that in
> > the updated patch.
> > The other steps/navigations, e.g. navigating node2 to a remote frame,
> > need to happen afterwards and hence that code is still in this file.
> > Is that OK or you had something else in mind?
> 
> After discussion, I've update this further. Now the initial tree structure is
> coming directly from html.
> 
>
https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
> File content/browser/site_per_process_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
> content/browser/site_per_process_browsertest.cc:478: ASSERT_EQ(1U,
> root->child_at(0)->child_count());
> This is the part I was worried about: whether the first child's
> descendants are safe to access right after NavigateToURL() call,
> in practice seems to work. Let me know if there's a chance of
> race here.

Ping

[nasko, 2015-02-02 22:47:09]

LGTM with a few nits.

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:592: // SwapOut creates
a RenderFrameProxy, so set the proxy to be initialized.
nit: empty line before comment

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/site_per_process_browsertest.cc:447: // Note that due to another
bug node2 does not re-navigate to site B and stays
nit: cite the bug number here, as you've done in the description of the other
test.

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/site_per_process_browsertest.cc:478: ASSERT_EQ(1U,
root->child_at(0)->child_count());
On 2015/01/22 23:32:35, lazyboy wrote:
> This is the part I was worried about: whether the first child's
> descendants are safe to access right after NavigateToURL() call,
> in practice seems to work. Let me know if there's a chance of
> race here.

This should work. It waits for DidStopLoading, which shouldn't fire until all
documents in the frame tree have completed loading. If you don't do JavaScript
work that you expect to alter the DOM and this fails, it is a bug : ).

[lazyboy, 2015-02-03 00:22:43]

Comments addressed in patchset #13

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:592: // SwapOut creates
a RenderFrameProxy, so set the proxy to be initialized.
On 2015/02/02 22:47:09, nasko wrote:
> nit: empty line before comment

Done.

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
File content/browser/site_per_process_browsertest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/site_per_process_browsertest.cc:447: // Note that due to another
bug node2 does not re-navigate to site B and stays
On 2015/02/02 22:47:09, nasko wrote:
> nit: cite the bug number here, as you've done in the description of the other
> test.

Done.

https://chromiumcodereview.appspot.com/782093002/diff/260001/content/browser/...
content/browser/site_per_process_browsertest.cc:478: ASSERT_EQ(1U,
root->child_at(0)->child_count());
On 2015/02/02 22:47:09, nasko wrote:
> On 2015/01/22 23:32:35, lazyboy wrote:
> > This is the part I was worried about: whether the first child's
> > descendants are safe to access right after NavigateToURL() call,
> > in practice seems to work. Let me know if there's a chance of
> > race here.
> 
> This should work. It waits for DidStopLoading, which shouldn't fire until all
> documents in the frame tree have completed loading. If you don't do JavaScript
> work that you expect to alter the DOM and this fails, it is a bug : ).

Acknowledged.

[lazyboy, 2015-02-03 21:02:01]

There are couple of tests failing:
1) TaskManagerOOPIFBrowserTest.KillSubframe/1
2) RenderFrameHostManagerTest.CleanUpSwappedOutRVHOnProcessCrash

#1 is a result of another bug, I'm in the process of
fixing that in http://crrev.com/900463004

#2 is fixed in patchset #14 by changing the check. We
can't look for swapped out RVH to be gone anymore because
we keep them around.

PTAL for #2

[nasko, 2015-02-03 21:29:36]

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1422:
rvh1->GetSiteInstance())->is_render_frame_proxy_live());
Let's split this into two checks: EXPECT_TRUE for RFH, so we don't just crash
and then the liveness check.

Also, we need to keep the previous check, but update it to reflect reality: the
RVH should exist, but we should also confirm it is *not* live.

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.h:113: // Returns if the
renderer/ side counterpart for this proxy is alive.
nit: "if the RenderFrameProxy for this host"

[lazyboy, 2015-02-03 23:15:14]

Comments addressed in patchset #15.

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1422:
rvh1->GetSiteInstance())->is_render_frame_proxy_live());
On 2015/02/03 21:29:36, nasko wrote:
> Let's split this into two checks: EXPECT_TRUE for RFH, so we don't just crash
> and then the liveness check.
>
Done.
 
> Also, we need to keep the previous check, but update it to reflect reality:
the
> RVH should exist, but we should also confirm it is *not* live. 
I've added the check for the existence of RVH, but cannot check the live-ness as
this is a TestRenderViewHost and the process crash is faked by sending a
notification. TestRenderViewHost manually manages IsRenderViewLive, the rvh
can be made not live by calling set_render_view_created(false). However, adding
that call and checking !IsRenderViewLive() seems pointless.
The |is_render_frame_proxy_live()| call above however, should cover the case.

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_proxy_host.h:113: // Returns if the
renderer/ side counterpart for this proxy is alive.
On 2015/02/03 21:29:36, nasko wrote:
> nit: "if the RenderFrameProxy for this host"

Done.

[nasko, 2015-02-03 23:39:51]

LGTM again. Just one favor to ask - adding a TODO in the test code.

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1422:
rvh1->GetSiteInstance())->is_render_frame_proxy_live());
On 2015/02/03 23:15:14, lazyboy wrote:
> On 2015/02/03 21:29:36, nasko wrote:
> > Let's split this into two checks: EXPECT_TRUE for RFH, so we don't just
crash
> > and then the liveness check.
> >
> Done.
>  
> > Also, we need to keep the previous check, but update it to reflect reality:
> the
> > RVH should exist, but we should also confirm it is *not* live. 
> I've added the check for the existence of RVH, but cannot check the live-ness
as
> this is a TestRenderViewHost and the process crash is faked by sending a
> notification. TestRenderViewHost manually manages IsRenderViewLive, the rvh
> can be made not live by calling set_render_view_created(false). However,
adding
> that call and checking !IsRenderViewLive() seems pointless.
> The |is_render_frame_proxy_live()| call above however, should cover the case.

I wonder if the test can be made more realistic by not faking the notification
and just doing the RenderProcessGone. Though this seems like a bigger change and
a bit more out of scope for this CL. Can you put a TODO(nasko) above the
simulation to investigate that and not faking the created boolean?

[lazyboy, 2015-02-03 23:54:09]

New patchsets have been uploaded after l-g-t-m from nasko@chromium.org

[lazyboy, 2015-02-03 23:55:42]

TODO added in patchset #16

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/782093002/diff/320001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1422:
rvh1->GetSiteInstance())->is_render_frame_proxy_live());
On 2015/02/03 23:39:50, nasko wrote:
> On 2015/02/03 23:15:14, lazyboy wrote:
> > On 2015/02/03 21:29:36, nasko wrote:
> > > Let's split this into two checks: EXPECT_TRUE for RFH, so we don't just
> crash
> > > and then the liveness check.
> > >
> > Done.
> >  
> > > Also, we need to keep the previous check, but update it to reflect
reality:
> > the
> > > RVH should exist, but we should also confirm it is *not* live. 
> > I've added the check for the existence of RVH, but cannot check the
live-ness
> as
> > this is a TestRenderViewHost and the process crash is faked by sending a
> > notification. TestRenderViewHost manually manages IsRenderViewLive, the rvh
> > can be made not live by calling set_render_view_created(false). However,
> adding
> > that call and checking !IsRenderViewLive() seems pointless.
> > The |is_render_frame_proxy_live()| call above however, should cover the
case.
> 
> I wonder if the test can be made more realistic by not faking the notification
> and just doing the RenderProcessGone. Though this seems like a bigger change
and
> a bit more out of scope for this CL. Can you put a TODO(nasko) above the
> simulation to investigate that and not faking the created boolean?

Done.

[lazyboy, 2015-02-04 01:59:47]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2015-02-04 02:00:45]

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

[commit-bot: I haz the power, 2015-02-04 02:36:40]

Committed patchset #17 (id:380001)

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

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