Track frame openers in FrameTreeNodes instead of WebContents

Track frame openers in FrameTreeNodes instead of WebContents.

This CL is the first step of refactoring frame openers to support
out-of-process iframes:

- Frame openers are now tracked by FrameTreeNodes rather than
  WebContents.  Openers can point to subframes, and this change
  ensures that a cross-process opener can be properly used for things
  like navigation or postMessage.  As part of this, the logic for
  clearing an opener when it is destroyed also moved into
  FrameTreeNode, where it uses a new Observer.

- Various renderer->browser opener plumbing is refactored to use
  RenderFrameHosts instead of RenderViewHosts, which enables a newly
  created WebContents (or rather, its main frame) to track a subframe
  opener in the browser process.

Future CLs will add support to further propagate per-frame opener
information to a new window's renderer process, so that window.opener
points to the right RenderFrameProxy in the cross-process case.

Note that a subframe does not have an opener when it is created. The
only way for it to gain an opener is if it is targeted by
window.open().  Support for this will be added in a future CL.

BUG=225940, 431769
Committed: https://crrev.com/e201c7cd1473ee1ff5b677bea1d3eb80c6680ed3
Cr-Commit-Position: refs/heads/master@{#333751}

[alexmos, 2015-06-01 17:44:54]

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

[alexmos, 2015-06-01 17:44:55]

Hey Charlie, can you please review this CL for me?  This is the first step
towards refactoring openers.  Also some questions/comments below.

This CL propagates the subframe opener info to the browser process.  It's a
prerequisite to be able to plumb subframe openers into new renderers, move
CreateOpenerRenderViews down into FTN, refactor DisownOpener into UpdateOpener,
and other good things which I hope to do in follow-up CLs.

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:504: tab->GetRenderViewHost() !=
render_frame_host->GetRenderViewHost())
I guess this prevents the blocked popup UI from being shown if the main page has
already navigated away?  I found https://codereview.chromium.org/707223004 for
context.  I was thinking what should happen if the popup is opened from a
subframe which has already navigated away (or got removed) at this point, even
though the top frame is the same.  The existing check would not pass and still
show the UI in that case, which seems fine to me, but I'm curious what you
think.

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1852: int opener_render_view_id,
I didn't just convert opener_id to be opener_render_frame_id because of a
dependency of SingleTabModeTabHelper on the RVH's ID below.  I can't refactor
SingleTabModeTabHelper without this CL, since that needs the RFH's ID, so I
thought I'd keep both IDs here, and remove opener_render_view_id once I refactor
SingleTabModeTabHelper in a follow-up CL (this avoids entangling this CL with
non-trivial Android changes).  But let me know if it's better to do it here.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:211:
params.opener->GetRenderProcessHost()->GetID(),
I didn't remove "opener" from WebContents::CreateParams because I need it here
for the RPH ID.  Alternatively, I could change opener ->
opener_render_process_id in CreateParams, or do the FTN resolution earlier (in
WebContentsImpl::CreateNewWindow) and make opener a  FrameTreeNode in the
CreateParams - any thoughts on this?

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:489: // will return nullptr,
but can we ever have a valid WebContents w/o a RFH?
I think that's impossible, but just wanted to double check.  I.e., is there any
case where getting to the opener WebContents via RFHs and FTN wouldn't work, but
the old way of just storing the link to opener contents would.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1185:
CreateWithOpener(create_params, frame_tree_.root()->opener());
Uncovered an interesting bug here which I haven't filed yet.  Create a new popup
from a subframe, then duplicate the popup tab.  The original popup's opener will
point to the subframe, but the duplicated popup's opener will point to the main
frame.  This CL isn't quite enough to fix this, as it needs to propagate the
subframe opener to the new renderer (one of my followup CLs).

https://codereview.chromium.org/1086283002/diff/160001/content/test/test_web_...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/test/test_web_...
content/test/test_web_contents.h:82: void SetOpener(TestWebContents* opener);
Several unit tests rely on this, so I left this in rather than changing all call
sites to
"contents()->GetFrameTree->root()->set_opener(opener->GetFrameTree()->root())".

[alexmos, 2015-06-01 18:02:26]

And, I forgot to take the "DRAFT" out of the title - sorry about that =)

[Charlie Reis, 2015-06-03 20:01:37]

Nice! Some comments below, but I think it's in pretty good shape.

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:504: tab->GetRenderViewHost() !=
render_frame_host->GetRenderViewHost())
On 2015/06/01 17:44:55, alexmos wrote:
> I guess this prevents the blocked popup UI from being shown if the main page
has
> already navigated away?  I found https://codereview.chromium.org/707223004 for
> context.  I was thinking what should happen if the popup is opened from a
> subframe which has already navigated away (or got removed) at this point, even
> though the top frame is the same.  The existing check would not pass and still
> show the UI in that case, which seems fine to me, but I'm curious what you
> think.

Interesting.  I don't think this new check works, though, since it compares the
RVH of the subframe to the RVH of the main frame.

Is the FromRenderFrameHost call enough?  If we've navigated away from the main
frame, then we won't find the WebContents via an old subframe anymore.  (I don't
think that returns results for RFHs that are pending deletion, right?)

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:1852: int opener_render_view_id,
On 2015/06/01 17:44:55, alexmos wrote:
> I didn't just convert opener_id to be opener_render_frame_id because of a
> dependency of SingleTabModeTabHelper on the RVH's ID below.  I can't refactor
> SingleTabModeTabHelper without this CL, since that needs the RFH's ID, so I
> thought I'd keep both IDs here, and remove opener_render_view_id once I
refactor
> SingleTabModeTabHelper in a follow-up CL (this avoids entangling this CL with
> non-trivial Android changes).  But let me know if it's better to do it here.

Yes, that approach sounds good.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:48: CHECK(owner_->opener() ==
node);
CHECK_EQ

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:176:
opener_->RemoveObserver(opener_observer_.get());
Maybe also clear opener_observer_ if |opener| is null?

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:99: void SetOpener(FrameTreeNode*
opener);
Worth mentioning a bit of what else this does.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:219: // opener is closed, or if the
frame disowns its opener by setting its
nit: this frame

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:226: // changes or when this node
is destroyed.
Should we also delete it from here if this node's opener_ is cleared?  No sense
keeping it around after that.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1226: // mirroring works
correctly, add a check here to enforce it.
If this is still main-frame only for now, we can probably DCHECK that and
replace Avi's TODO with one to support owner updates on all frames.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1236: // Notify all swapped
out RenderViewHosts for this tab.  This is important
I think this comment was stale.  It's notifying all other RenderFrameHosts and
RenderFrameProxies for the frame.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:211:
params.opener->GetRenderProcessHost()->GetID(),
On 2015/06/01 17:44:55, alexmos wrote:
> I didn't remove "opener" from WebContents::CreateParams because I need it here
> for the RPH ID.  Alternatively, I could change opener ->
> opener_render_process_id in CreateParams, or do the FTN resolution earlier (in
> WebContentsImpl::CreateNewWindow) and make opener a  FrameTreeNode in the
> CreateParams - any thoughts on this?

CreateParams is public, so we shouldn't put FrameTreeNode on it.  I think it
makes sense to replace WebContents* opener with opener_render_frame_id and
opener_render_process_id.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:213: // TODO: Is this null
check needed?  Seems like opener could navigate away.
Good question.  I *think* opener_rfh would be the one sending us the message
that causes us to arrive here, so it shouldn't be gone, but I'm not sure how
that call stack looks or if it's possible for us to get here from another path. 
The null check seems reasonable unless we can show it's safe.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:441:
new_contents->created_with_opener_ = true;
Why do we need both this and the case above?  If they're both needed, we should
probably explain in a comment.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:489: // will return nullptr,
but can we ever have a valid WebContents w/o a RFH?
On 2015/06/01 17:44:55, alexmos wrote:
> I think that's impossible, but just wanted to double check.  I.e., is there
any
> case where getting to the opener WebContents via RFHs and FTN wouldn't work,
but
> the old way of just storing the link to opener contents would.

I *think* the RFH is only null before WebContentsImpl::Init is called.  Not sure
if anyone would call this before Init.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1185:
CreateWithOpener(create_params, frame_tree_.root()->opener());
On 2015/06/01 17:44:55, alexmos wrote:
> Uncovered an interesting bug here which I haven't filed yet.  Create a new
popup
> from a subframe, then duplicate the popup tab.  The original popup's opener
will
> point to the subframe, but the duplicated popup's opener will point to the
main
> frame.  This CL isn't quite enough to fix this, as it needs to propagate the
> subframe opener to the new renderer (one of my followup CLs).

Nice.  Worth filing it so that we make sure it gets fixed by the followup CLs.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:116: WebContentsImpl* opener()
const;
Sadly, we have to change this to GetOpener now, since it's not trivial anymore. 
And it looks like GetOpener already exists in the public API (in use by
AppModalDialogHelper), so we might need to call it GetOpenerFrame?

https://codereview.chromium.org/1086283002/diff/160001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/public/browser...
content/public/browser/web_contents.h:101: WebContents* opener;
Let's have frame ID and process ID rather than WebContents and frame ID (where
we risk grabbing the wrong process ID for the frame ID).

https://codereview.chromium.org/1086283002/diff/160001/content/test/test_web_...
File content/test/test_web_contents.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/test/test_web_...
content/test/test_web_contents.h:82: void SetOpener(TestWebContents* opener);
On 2015/06/01 17:44:55, alexmos wrote:
> Several unit tests rely on this, so I left this in rather than changing all
call
> sites to
>
"contents()->GetFrameTree->root()->set_opener(opener->GetFrameTree()->root())".

Acknowledged.

[alexmos, 2015-06-05 22:34:32]

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:504: tab->GetRenderViewHost() !=
render_frame_host->GetRenderViewHost())
On 2015/06/03 20:01:36, Charlie Reis wrote:
> On 2015/06/01 17:44:55, alexmos wrote:
> > I guess this prevents the blocked popup UI from being shown if the main page
> has
> > already navigated away?  I found https://codereview.chromium.org/707223004
for
> > context.  I was thinking what should happen if the popup is opened from a
> > subframe which has already navigated away (or got removed) at this point,
even
> > though the top frame is the same.  The existing check would not pass and
still
> > show the UI in that case, which seems fine to me, but I'm curious what you
> > think.
> 
> Interesting.  I don't think this new check works, though, since it compares
the
> RVH of the subframe to the RVH of the main frame.
> 
> Is the FromRenderFrameHost call enough?  If we've navigated away from the main
> frame, then we won't find the WebContents via an old subframe anymore.  (I
don't
> think that returns results for RFHs that are pending deletion, right?)

Ah, right, this check won't work for subframes -- IIUC, their RFHs would've been
deleted during main frame navigation and we wouldn't even find the
render_frame_host for the first check above.

But don't we still need this check for main frames?  Even if the RFH is on
pending deletion list, it seems that its delegate() will still point to the same
WebContents (delegates seem to be set at init time and never change).  I
modified the check to only apply to main frames (and changed it to use RFHs).

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:48: CHECK(owner_->opener() ==
node);
On 2015/06/03 20:01:36, Charlie Reis wrote:
> CHECK_EQ

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:176:
opener_->RemoveObserver(opener_observer_.get());
On 2015/06/03 20:01:36, Charlie Reis wrote:
> Maybe also clear opener_observer_ if |opener| is null?

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:99: void SetOpener(FrameTreeNode*
opener);
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Worth mentioning a bit of what else this does.

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:219: // opener is closed, or if the
frame disowns its opener by setting its
On 2015/06/03 20:01:37, Charlie Reis wrote:
> nit: this frame

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:226: // changes or when this node
is destroyed.
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Should we also delete it from here if this node's opener_ is cleared?  No
sense
> keeping it around after that.

Do you mean delete opener_observer_ in SetOpener, if we are clearing _opener,
i.e. the comment below?  Yeah, that's a good idea -- done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1226: // mirroring works
correctly, add a check here to enforce it.
On 2015/06/03 20:01:37, Charlie Reis wrote:
> If this is still main-frame only for now, we can probably DCHECK that and
> replace Avi's TODO with one to support owner updates on all frames.

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1236: // Notify all swapped
out RenderViewHosts for this tab.  This is important
On 2015/06/03 20:01:37, Charlie Reis wrote:
> I think this comment was stale.  It's notifying all other RenderFrameHosts and
> RenderFrameProxies for the frame.

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:211:
params.opener->GetRenderProcessHost()->GetID(),
On 2015/06/03 20:01:37, Charlie Reis wrote:
> On 2015/06/01 17:44:55, alexmos wrote:
> > I didn't remove "opener" from WebContents::CreateParams because I need it
here
> > for the RPH ID.  Alternatively, I could change opener ->
> > opener_render_process_id in CreateParams, or do the FTN resolution earlier
(in
> > WebContentsImpl::CreateNewWindow) and make opener a  FrameTreeNode in the
> > CreateParams - any thoughts on this?
> 
> CreateParams is public, so we shouldn't put FrameTreeNode on it.  I think it
> makes sense to replace WebContents* opener with opener_render_frame_id and
> opener_render_process_id.

Done.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:213: // TODO: Is this null
check needed?  Seems like opener could navigate away.
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Good question.  I *think* opener_rfh would be the one sending us the message
> that causes us to arrive here, so it shouldn't be gone, but I'm not sure how
> that call stack looks or if it's possible for us to get here from another
path. 
> The null check seems reasonable unless we can show it's safe.

Acknowledged.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:441:
new_contents->created_with_opener_ = true;
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Why do we need both this and the case above?  If they're both needed, we
should
> probably explain in a comment.

I added a comment.  It's needed in the case of allowing blocked popups (which
now have a null opener, but still need the created_with_opener_ bit set).

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:489: // will return nullptr,
but can we ever have a valid WebContents w/o a RFH?
On 2015/06/03 20:01:37, Charlie Reis wrote:
> On 2015/06/01 17:44:55, alexmos wrote:
> > I think that's impossible, but just wanted to double check.  I.e., is there
> any
> > case where getting to the opener WebContents via RFHs and FTN wouldn't work,
> but
> > the old way of just storing the link to opener contents would.
> 
> I *think* the RFH is only null before WebContentsImpl::Init is called.  Not
sure
> if anyone would call this before Init.

Yes, CreateWithOpener only has a few lines between creating the WebContents and
calling Init, which don't call this, so I think this is safe.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1185:
CreateWithOpener(create_params, frame_tree_.root()->opener());
On 2015/06/03 20:01:37, Charlie Reis wrote:
> On 2015/06/01 17:44:55, alexmos wrote:
> > Uncovered an interesting bug here which I haven't filed yet.  Create a new
> popup
> > from a subframe, then duplicate the popup tab.  The original popup's opener
> will
> > point to the subframe, but the duplicated popup's opener will point to the
> main
> > frame.  This CL isn't quite enough to fix this, as it needs to propagate the
> > subframe opener to the new renderer (one of my followup CLs).
> 
> Nice.  Worth filing it so that we make sure it gets fixed by the followup CLs.

Done.  Filed issue 497382.

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:116: WebContentsImpl* opener()
const;
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Sadly, we have to change this to GetOpener now, since it's not trivial
anymore. 
> And it looks like GetOpener already exists in the public API (in use by
> AppModalDialogHelper), so we might need to call it GetOpenerFrame?

Done.  I renamed it to GetOpenerWebContents, thinking that this may be better
since it's returning a WebContentsImpl* and not a frame.  

Actually, there was just one use of opener() outside of WebContentsImpl, which
was only checking whether it's not null.  I changed that to use HasOpener() and
made GetOpenerWebContents() private.

https://codereview.chromium.org/1086283002/diff/160001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1086283002/diff/160001/content/public/browser...
content/public/browser/web_contents.h:101: WebContents* opener;
On 2015/06/03 20:01:37, Charlie Reis wrote:
> Let's have frame ID and process ID rather than WebContents and frame ID (where
> we risk grabbing the wrong process ID for the frame ID).

Done.

[Charlie Reis, 2015-06-09 04:38:38]

LGTM with some small changes, and possibly one preliminary change to
WebContents::FromRenderFrameHost.

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1086283002/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:504: tab->GetRenderViewHost() !=
render_frame_host->GetRenderViewHost())
On 2015/06/05 22:34:30, alexmos wrote:
> On 2015/06/03 20:01:36, Charlie Reis wrote:
> > On 2015/06/01 17:44:55, alexmos wrote:
> > > I guess this prevents the blocked popup UI from being shown if the main
page
> > has
> > > already navigated away?  I found https://codereview.chromium.org/707223004
> for
> > > context.  I was thinking what should happen if the popup is opened from a
> > > subframe which has already navigated away (or got removed) at this point,
> even
> > > though the top frame is the same.  The existing check would not pass and
> still
> > > show the UI in that case, which seems fine to me, but I'm curious what you
> > > think.
> > 
> > Interesting.  I don't think this new check works, though, since it compares
> the
> > RVH of the subframe to the RVH of the main frame.
> > 
> > Is the FromRenderFrameHost call enough?  If we've navigated away from the
main
> > frame, then we won't find the WebContents via an old subframe anymore.  (I
> don't
> > think that returns results for RFHs that are pending deletion, right?)
> 
> Ah, right, this check won't work for subframes -- IIUC, their RFHs would've
been
> deleted during main frame navigation and we wouldn't even find the
> render_frame_host for the first check above.
> 
> But don't we still need this check for main frames?  Even if the RFH is on
> pending deletion list, it seems that its delegate() will still point to the
same
> WebContents (delegates seem to be set at init time and never change).  I
> modified the check to only apply to main frames (and changed it to use RFHs).

Hmm, you're right, but I wonder if we should change that.  It seems really
unfortunate to require all callers of WebContents::FromRenderFrameHost to check
whether it's still really in use or not.  That's especially true now that
Nasko's CL has landed and we don't have swapped out RFHs anymore (at least in
--site-per-process), giving us a chance to clean up uses of RFHs.

I wonder if we should do a preliminary CL to make FromRenderFrameHost somehow
check whether the frame is on the pending deletion list, and then not return its
delegate WebContents.  (I guess we could clear its delegate, but I think that's
likely to cause problems.)  Let's chat with Nasko about this in person.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:229: // changes or when this node
is destroyed.
Can you add something like this?
It is also cleared if |opener_| is disowned.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2493: return
static_cast<WebContents*>(GetOpenerWebContents());
Yeah, just put the new GetOpenerWebContents logic here and return a
WebContentsImpl.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:861: WebContentsImpl*
GetOpenerWebContents() const;
Oh, for some reason I thought we were returning a FrameTreeNode.  We can just
use GetOpener (and give it a covariant return type of WebContentsImpl).

[alexmos, 2015-06-09 20:13:05]

Thanks!  Comments addressed, apart from the FromRenderFrameHost change, which we
decided to keep here.  We'll separately look at its uses and how they would be
affected if we decided to stop returning the delegate for RFHs pending deletion.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:229: // changes or when this node
is destroyed.
On 2015/06/09 04:38:38, Charlie Reis wrote:
> Can you add something like this?
> It is also cleared if |opener_| is disowned.

Done.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2493: return
static_cast<WebContents*>(GetOpenerWebContents());
On 2015/06/09 04:38:38, Charlie Reis wrote:
> Yeah, just put the new GetOpenerWebContents logic here and return a
> WebContentsImpl.

Done.

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1086283002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.h:861: WebContentsImpl*
GetOpenerWebContents() const;
On 2015/06/09 04:38:38, Charlie Reis wrote:
> Oh, for some reason I thought we were returning a FrameTreeNode.  We can just
> use GetOpener (and give it a covariant return type of WebContentsImpl).

Done.

[Charlie Reis, 2015-06-09 20:15:08]

Yeah, let's investigate the FromRFH thing separately.  LGTM.

[alexmos, 2015-06-09 20:20:09]

alexmos@chromium.org changed reviewers:
+ gunsch@chromium.org, jochen@chromium.org, sgurun@chromium.org

[alexmos, 2015-06-09 20:20:09]

Adding OWNERS:

sgurun@: please review android_webview/
jochen@: please review chrome/
gunsch@: please review chromecast/

[gunsch, 2015-06-09 20:27:27]

chromecast rs lgtm

[gunsch, 2015-06-09 20:27:27]

chromecast rs lgtm

[sgurun-gerrit only, 2015-06-09 21:10:51]

On 2015/06/09 20:27:27, gunsch wrote:
> chromecast rs lgtm

aw lgtm

[jochen (gone - plz use gerrit), 2015-06-10 08:34:38]

lgtm

[alexmos, 2015-06-10 16:38:09]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-06-10 16:38:30]

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

[commit-bot: I haz the power, 2015-06-10 17:14:27]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-06-10 17:15:13]

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