Always create FrameTreeNodes and RenderFrameHosts for every frame.

Always create FrameTreeNodes and RenderFrameHosts for every frame.

This allows us to run the FrameTreeShape tests without a flag.

BUG=245126
TEST=FrameTreeBrowserTest.FrameTreeShape*

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

[Charlie Reis, 2013-11-21 23:59:14]

Albert, this is based on your draft (https://codereview.chromium.org/32343006/).
 Do you have any time to take a look?

Nasko, it would be good to get your eyes on this as well.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:228: int routing_id = MSG_ROUTING_NONE;
I added this since it didn't seem like a good idea to risk having it
uninitialized.  This follows the pattern from RenderViewImpl::createView.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3174: main_render_frame_ = NULL;
I added this as well, since I didn't like the idea of leaving main_render_frame_
as garbage when it gets detached.

(It doesn't appear that RenderViewImpl gets deleted in the same task on the
message loop, but perhaps there's some guarantee it will be the next task? 
Otherwise we'd have lots of problems when accessing webview(), etc. after this
point.)

[nasko, 2013-11-22 00:09:00]

LGTM with just one question about disappearing comment.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:265: // The |is_detaching_| flag disables
Send(). FrameHostMsg_Detach must be
Any specific reason why the existing comment about multiple calls to
frameDetached is now removed?

[Charlie Reis, 2013-11-22 00:13:20]

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:265: // The |is_detaching_| flag disables
Send(). FrameHostMsg_Detach must be
On 2013/11/22 00:09:01, nasko wrote:
> Any specific reason why the existing comment about multiple calls to
> frameDetached is now removed?

That comment no longer applies.  :)  There's always a 1:1 relationship between
blink::Frame and RenderFrameImpls now.

[nasko, 2013-11-22 00:25:04]

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:265: // The |is_detaching_| flag disables
Send(). FrameHostMsg_Detach must be
On 2013/11/22 00:13:21, creis wrote:
> On 2013/11/22 00:09:01, nasko wrote:
> > Any specific reason why the existing comment about multiple calls to
> > frameDetached is now removed?
> 
> That comment no longer applies.  :)  There's always a 1:1 relationship between
> blink::Frame and RenderFrameImpls now.

Albert, I recall you were getting multiple calls to frameDetached for the same
blink::Frame object. Is that still true? I thought the comment was related to
that.

[dcheng, 2013-11-22 00:41:26]

On 2013/11/22 00:25:04, nasko wrote:
>
https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
> content/renderer/render_frame_impl.cc:265: // The |is_detaching_| flag
disables
> Send(). FrameHostMsg_Detach must be
> On 2013/11/22 00:13:21, creis wrote:
> > On 2013/11/22 00:09:01, nasko wrote:
> > > Any specific reason why the existing comment about multiple calls to
> > > frameDetached is now removed?
> > 
> > That comment no longer applies.  :)  There's always a 1:1 relationship
between
> > blink::Frame and RenderFrameImpls now.
> 
> Albert, I recall you were getting multiple calls to frameDetached for the same
> blink::Frame object. Is that still true? I thought the comment was related to
> that.

I'm not Albert, but here's the background as I recall it.
The original comment referred to the fact that the same RenderFrame serviced
frameDetached calls for multiple blink-side Frames.

The issue with frameDetached() being called multiple times for the *same* frame
should not be a problem on the embedder. The callback is called from
FrameLoaderClientImpl which checks that WebFrame's client is non-null before
invoking the callback... and then sets the client to null.

[awong, 2013-11-22 01:07:14]

Thanks for doing this! I didn't look at the tests, but I have a question about
mainframe lifetime.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:228: int routing_id = MSG_ROUTING_NONE;
On 2013/11/21 23:59:15, creis wrote:
> I added this since it didn't seem like a good idea to risk having it
> uninitialized.  This follows the pattern from RenderViewImpl::createView.

SGTM

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_f...
content/renderer/render_frame_impl.cc:265: // The |is_detaching_| flag disables
Send(). FrameHostMsg_Detach must be
On 2013/11/22 00:25:04, nasko wrote:
> On 2013/11/22 00:13:21, creis wrote:
> > On 2013/11/22 00:09:01, nasko wrote:
> > > Any specific reason why the existing comment about multiple calls to
> > > frameDetached is now removed?
> > 
> > That comment no longer applies.  :)  There's always a 1:1 relationship
between
> > blink::Frame and RenderFrameImpls now.
> 
> Albert, I recall you were getting multiple calls to frameDetached for the same
> blink::Frame object. Is that still true? I thought the comment was related to
> that.

Daniel's reply is correct.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
content/renderer/render_view_impl.h:1385: RenderFrameImpl* main_render_frame_;
I think this still needs to be a scoped_ptr.  See comment in frameDetach()

[Charlie Reis, 2013-11-22 02:07:23]

It's possible I'm misunderstanding something, but that doesn't match what I'm
seeing.

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/82033002/diff/30001/content/renderer/render_v...
content/renderer/render_view_impl.h:1385: RenderFrameImpl* main_render_frame_;
On 2013/11/22 01:07:15, awong wrote:
> I think this still needs to be a scoped_ptr.  See comment in frameDetach()

I'm confused.  Are you referring to this comment?

  // If the frame does not have a parent, it is the main frame. The main
  // frame is owned by the containing RenderViewHost so it does not require
  // any cleanup here.

That comment isn't consistent with what I see in the debugger.  When closing a
tab with an iframe that is sharing a process with another tab, I see calls to
RenderFrameImpl::frameDetach for both the iframe and the main frame.

Indeed, the comments in RenderFrameImpl::frameDetach seem wrong, since
detachFromParent seems to be called for every frame.

[Charlie Reis, 2013-11-22 03:10:34]

Ok, thanks for your comments offline.  Here's another patch that tries to get
the lifetime issues right.  I had to add an IPC handler to MockRenderThread so
that the new child frame got a valid routing ID.

[awong, 2013-11-22 03:19:28]

LGTM
 Nice comments.

https://codereview.chromium.org/82033002/diff/30001/content/browser/frame_hos...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/browser/frame_hos...
content/browser/frame_host/frame_tree_browsertest.cc:21: FrameTreeBrowserTest()
{}
nit: Unless there's precedent, I'd generally just not bother with
DISALLOW_COPY_AND_ASSIGN on test harnesses. That avoids all the boilerplate of
declaring a default constructor, etc.

[awong, 2013-11-22 03:19:29]

LGTM
 Nice comments.

[Charlie Reis, 2013-11-23 00:56:37]

[+site-isolation-reviews@chromium.org, since I forgot to earlier]

Nasko, can you take a look at the diff between patches 2 and 3?  (Albert, you
can as well if you have time.)

The earlier try jobs revealed that we could hit the DCHECK in
FrameTreeNode::ResetForMainFrame if the old renderer process didn't clean up
after itself.

I wanted to make sure we had a test for that in the content module (rather than
seeing it in some random extension tests), so I added
FrameTreeBrowserTest.NavigateWithLeftoverFrames.  That led me to discover
another bug: we were leaving the frame tree around after a crash.  That's fixed
and tested with FrameTreeBrowserTest.FrameTreeAfterCrash.

We'll see how these try jobs go, and I can split these fixes out into a separate
CL if you think that's worthwhile.

[Charlie Reis, 2013-11-23 01:04:16]

https://codereview.chromium.org/82033002/diff/30001/content/browser/frame_hos...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/82033002/diff/30001/content/browser/frame_hos...
content/browser/frame_host/frame_tree_browsertest.cc:21: FrameTreeBrowserTest()
{}
On 2013/11/22 03:19:29, awong wrote:
> nit: Unless there's precedent, I'd generally just not bother with
> DISALLOW_COPY_AND_ASSIGN on test harnesses. That avoids all the boilerplate of
> declaring a default constructor, etc.

Oops, forgot this one.  There's a precedent in WebContentsImplBrowserTest, but
there's also a precedent for simpler:

class RenderProcessHostTest : public ContentBrowserTest {};

I'll fix it before committing.

[nasko, 2013-11-23 01:05:21]

Great job on catching these!

Still LGTM with one comment.

https://codereview.chromium.org/82033002/diff/390001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/82033002/diff/390001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1387:
delegate_->GetFrameTree()->SwapMainFrame(main_render_frame_host_.get());
This seems to do the right thing now, but I wonder if this is what we want
things to look long term. Maybe a TODO to revisit this once we have RFHM and
have a cleaner FrameTree Reset().

[Charlie Reis, 2013-11-23 01:20:13]

kenrb: Can you review frame_message.h for IPC OWNERS?  I just had to add an
include for GURL.

https://codereview.chromium.org/82033002/diff/390001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/82033002/diff/390001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1387:
delegate_->GetFrameTree()->SwapMainFrame(main_render_frame_host_.get());
On 2013/11/23 01:05:21, nasko wrote:
> This seems to do the right thing now, but I wonder if this is what we want
> things to look long term. Maybe a TODO to revisit this once we have RFHM and
> have a cleaner FrameTree Reset().

Good point.  SwapMainFrame is definitely going to be changing when
main_render_frame_host_ moves to RFHM.  I'll add a TODO.

[kenrb, 2013-11-25 17:04:25]

lgtm

[commit-bot: I haz the power, 2013-11-25 17:09:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/82033002/390001

[commit-bot: I haz the power, 2013-11-25 19:28:55]

Change committed as 237119