Fix IsRenderFrameLive and use it in RenderFrameHostManager.

Fix IsRenderFrameLive and use it in RenderFrameHostManager.

This requires updating subframe RenderFrameHosts when we know that
the RenderFrame is created, and using IsRenderViewLive for main frames.
Several test updates as well.

BUG=414894
TEST=FrameTreeBrowserTest.IsRenderFrameLive
TEST=CrossProcessFrameTreeBrowserTest.CreateCrossProcessSubframeProxies

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

[Charlie Reis, 2014-09-08 20:16:53]

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

[Charlie Reis, 2014-09-08 20:16:54]

Nasko, can you take a look?  This is a bit of cleanup that I wanted to use in
https://codereview.chromium.org/525583002/ but wasn't related to the rest.

Let me know if there's a better way to update RFHI::is_renderer_initialized_
instead.

[Charlie Reis, 2014-09-08 22:39:11]

Hmm, this isn't ready to commit.  It turns out we do the wrong thing for
same-site iframes as well, which also don't go through CreateRenderFrame.

I'm also puzzled that we never call RFHI::Init, despite that being needed to
resume requests.  I think I'll need to talk with you in person to understand how
this is supposed to work.

[Charlie Reis, 2014-09-16 20:37:34]

Nasko: Can you take another look?  This was much harder to resolve than I
expected, but now we handle all 4 cases that I'm aware of: creating main frames
normally, creating main frames in new windows, creating same-site iframes, and
creating cross-site iframes.

I treated the main frame cases differently from subframes, since main frames get
created at the same time as the RenderViewHosts.  RenderViewHostImpl can't
really tell the RenderFrameHostImpl directly, so IsRenderFrameLive just defers
to IsRenderViewLive for main frames.  (In contrast, we keep the
render_frame_created_ bit up to date for subframes, both same-site and
cross-site and after crashes.)

Camille: Just an FYI for the change to RFHM::RequestNavigation, resolving your
TODO.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:129:
EXPECT_FALSE(rfh->IsRenderFrameLive());
Sanity checking that these are false after a main frame crash.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:177:
IN_PROC_BROWSER_TEST_F(FrameTreeBrowserTest, IsRenderFrameLive) {
We can test main frames and same-process iframes without using
--site-per-process.  There's a test for cross-process iframes below.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:322:
FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest, CrashSubframe);
I needed this to test the value of render_frame_created_ after a crash.  (It's
important for that to be false, and not just rely on
GetProcess()->HasConnection().)

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:424: bool
InitRenderView(RenderViewHostImpl* render_view_host,
This is just to avoid some unnecessary static_casts.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:945:
EXPECT_TRUE(manager1->current_frame_host()->IsRenderFrameLive());
I added these because my earlier attempt to track main frame creation broke when
we called CreateRenderView here instead of InitRenderView.  It's no longer an
issue, but it makes sense to check these just to be safe.

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

https://codereview.chromium.org/552683004/diff/120001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:306:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrashSubframe) {
I'm leaving this disabled because it's still flaky, but I verified locally that
the checks below used to fail and now pass.  We'll want them to be present when
we re-enable the test.

https://codereview.chromium.org/552683004/diff/120001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:356:
EXPECT_FALSE(child->current_frame_host()->render_frame_created_);
Here's where it matters that not just IsRenderFrameLive() is false, but also
render_frame_created_.

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1883: if
(pending_entry->frame_tree_node_id() != -1) {
This is safe to use in non-site-per-process tests (in NavigateFrameToURL), so
I'm removing the guard.

[nasko, 2014-09-17 20:40:13]

I have just a couple of clarification questions. LGTM.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:456: // the renderer
process.  Only used for subframes.
Shouldn't we use that for all frames? For main frame, we know that we create it
along with the RVH, so as long as the RVH is successfully created, we can set
this one to true. This way it is a consistent value across the types of frames.
Also, once RVH disappears, we will need to have this work for main frames,
right?

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:945:
EXPECT_TRUE(manager1->current_frame_host()->IsRenderFrameLive());
On 2014/09/16 20:37:33, Charlie Reis wrote:
> I added these because my earlier attempt to track main frame creation broke
when
> we called CreateRenderView here instead of InitRenderView.  It's no longer an
> issue, but it makes sense to check these just to be safe.

Acknowledged.

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1883: if
(pending_entry->frame_tree_node_id() != -1) {
On 2014/09/16 20:37:33, Charlie Reis wrote:
> This is safe to use in non-site-per-process tests (in NavigateFrameToURL), so
> I'm removing the guard.

Acknowledged.

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1885:
frame_tree_.FindByID(pending_entry->frame_tree_node_id());
Should we DCHECK here? I don't think we expect to navigate a frame that we don't
know about, right?

[Charlie Reis, 2014-09-18 00:04:08]

Thanks!  One question below.

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:456: // the renderer
process.  Only used for subframes.
On 2014/09/17 20:40:12, nasko wrote:
> Shouldn't we use that for all frames? For main frame, we know that we create
it
> along with the RVH, so as long as the RVH is successfully created, we can set
> this one to true. This way it is a consistent value across the types of
frames.
> Also, once RVH disappears, we will need to have this work for main frames,
> right?

Yes, I was trying to get that to work in earlier patchsets, leading to things
like this addition to RFHM::InitRenderView on line 1238:
https://codereview.chromium.org/552683004/diff/100001/content/browser/frame_h...

The problem was that there's a ton of places that call RVHI::CreateRenderView
that we would have to update (normal main frame, main frame in new window,
interstitial, many tests), since we can't access RenderFrameHost from within
RVHI::CreateRenderView (per DEPS).  It felt very fragile to require a
set_render_frame_created call at every callsite of CreateRenderView.

I agree that we'll need to handle those cases when RVH goes away, but I imagine
some other refactoring will be needed there as well.  Is this new TODO
sufficient for now?

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1885:
frame_tree_.FindByID(pending_entry->frame_tree_node_id());
On 2014/09/17 20:40:12, nasko wrote:
> Should we DCHECK here? I don't think we expect to navigate a frame that we
don't
> know about, right?

Yeah, a DCHECK is a good idea.

[Charlie Reis, 2014-09-18 17:15:48]

Heh, the new DCHECK uncovered a test that violated it by passing in an arbitrary
frame ID (OpenURLSubframe).  I've updated the test.

[nasko, 2014-09-18 17:44:21]

LGTM

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/552683004/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:456: // the renderer
process.  Only used for subframes.
On 2014/09/18 00:04:07, Charlie Reis wrote:
> On 2014/09/17 20:40:12, nasko wrote:
> > Shouldn't we use that for all frames? For main frame, we know that we create
> it
> > along with the RVH, so as long as the RVH is successfully created, we can
set
> > this one to true. This way it is a consistent value across the types of
> frames.
> > Also, once RVH disappears, we will need to have this work for main frames,
> > right?
> 
> Yes, I was trying to get that to work in earlier patchsets, leading to things
> like this addition to RFHM::InitRenderView on line 1238:
>
https://codereview.chromium.org/552683004/diff/100001/content/browser/frame_h...
> 
> The problem was that there's a ton of places that call RVHI::CreateRenderView
> that we would have to update (normal main frame, main frame in new window,
> interstitial, many tests), since we can't access RenderFrameHost from within
> RVHI::CreateRenderView (per DEPS).  It felt very fragile to require a
> set_render_frame_created call at every callsite of CreateRenderView.
> 
> I agree that we'll need to handle those cases when RVH goes away, but I
imagine
> some other refactoring will be needed there as well.  Is this new TODO
> sufficient for now?

TODO is fine by me.

[Charlie Reis, 2014-09-18 17:45:21]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2014-09-18 17:46:58]

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

[commit-bot: I haz the power, 2014-09-18 18:15:17]

Committed patchset #10 (id:180001) as 358004a7faf5f5bcf673f4fa3ca603d6498e5488

[commit-bot: I haz the power, 2014-09-18 18:15:53]

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