Move browser-to-renderer opener plumbing to frame routing IDs

Move browser-to-renderer opener plumbing to frame routing IDs.

This CL allows a new RenderView to set its main frame's opener to any
frame, rather than a top-level frame.  I.e., it is now possible to use
window.opener to refer to a cross-process subframe.

Summary of changes:

* The ViewMsg_New IPC now passes the opener's frame ID rather than
  view ID.  RenderView::Initialize resolves this into the opener's
  WebFrame.

* The flow of opener routing IDs from RFHM::InitRenderView to
  RenderViewHostImpl::CreateRenderView (which sends the IPC) is moved
  from view to frame IDs.

* The passing of opener routing IDs from CreateOpenerProxies, etc.,
  into RFHM::InitRenderView is removed.  Instead, InitRenderView now
  looks up the proper opener frame routing ID itself.  This is because
  (1) the previous flow would complicate implementing opener updates,
  which would allow subframes to have openers and FrameTrees to have
  more than one opener to traverse, and (2) previous plumbing missed
  some cases (e.g., creating swapped-out RVs via
  CreateRenderFrameProxy or CreateProxiesForSiteInstance), and fixing
  this would add more complexity.

* There are paths where the opener chain is intentionally suppressed
  from a new RenderView, for example for <webview>: see
  WCI::CreateSwappedOutRenderView and EnsureOpenerProxiesExist.  This
  is now supported via an explicit flag which is passed into
  InitRenderView rather than passing around MSG_ROUTING_NONE for the
  opener's routing ID.

BUG=225940, 304341
Committed: https://crrev.com/5ac402d1ed5d323a9ea9223c2acf213b5dfc9aaa
Cr-Commit-Position: refs/heads/master@{#337995}

[alexmos, 2015-07-06 23:56:41]

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

[alexmos, 2015-07-06 23:56:42]

Nasko, can you please take a look?  This CL propagates opener frame information
to new RenderViews, which makes window.opener work properly for subframes.

In my current approach, I removed the passing of opener routing IDs from
CreateOpenerProxies(IfNeeded) and a few other places, which should make your
swapped-out-removal changes to CreateOpenerProxies straightforward.  I went back
and forth on whether to do that vs move the existing plumbing to frame routing
ID.  I tried both and ended up removing it -- see my explanation in the
description.  I did have to preserve an ability for <webview> to not expose
opener chains (see comment inside EnsureOpenerProxiesExist), which you'll see as
the suppress_opener plumbing.  Let me know if this makes sense.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1828: return
pending_render_frame_host_->GetRoutingID();
See WebContentsImplTest.FindOpenerRVHWhenPending for context of why this is
needed.  Previously, CreateOpenerProxies took care of this by looking up and
returning the pending RVH's routing ID.

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:666: opener_id_ = params.opener_route_id;
Unfortunately, I had to keep this around as a view routing ID, as this is
entangled with a bunch of non-opener stuff (such as ViewHostMsg_Show* IPCs).  I
didn't want to pass in two IDs (for view and frame) around though, so I now look
up the view ID from the opener frame (I ran tryjobs with DCHECKs to verify that
this is equivalent).

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:863:
webview()->mainFrame()->setOpener(opener_frame);
Note that previously, if this RenderView was opened from a subframe on a
cross-process RenderView, its opener was set to the main frame of the opener
RenderView rather than the subframe.  This is incorrect and inconsistent with
same-process behavior.  Now, if the subframe's proxy doesn't exist in the new
RenderView (as would be the case without --site-per-process), the opener won't
be set and is left as null.  This is consistent with how we handled a similar
case for postMessage (issue 485520).  Trybots seem happy with this as well.

[nasko, 2015-07-07 16:26:21]

I'm actually super excited that we don't return routing ids from those creation
functions anymore! Yay!

It is very close, but I do have one concern with returning the pending RFH
routing id.

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:89: // The RenderFrame will
not have an opener frame assigned, even if the RFH's
nit: I think dropping "RFH's" still preserves the meaning without making it hard
to read/understand.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:867:
frame_tree_node_->IsMainFrame(), false)) {
Sprinkling a ton of extra "false" params is bugging me a bit. Can we do it some
other way, such that the majority of the call sites (using false) will be
simpler?

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1828: return
pending_render_frame_host_->GetRoutingID();
On 2015/07/06 23:56:42, alexmos wrote:
> See WebContentsImplTest.FindOpenerRVHWhenPending for context of why this is
> needed.  Previously, CreateOpenerProxies took care of this by looking up and
> returning the pending RVH's routing ID.

This has me slightly concerned. If we have already a proxy for SiteInstance A
and the current RFH is for SiteInstance B, then navigating cross-process to a
URL in SiteInstance A will create a pending RFH for it, which now will be
returned. Previously, we would've returned the proxy.

This method is called in a bunch of contexts for currently running documents, so
I think it is not quite correct to return a pending RFH. Maybe we can gate this
on "IsSwappedOutForbidden", in which case we will never store RFHs and expect to
find them again. I assume that if we reorder the pending RFH and the proxy, we
will return the proxy and that will still be breaking the test case, right?

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2328: return
frame_tree_node_->opener()
nit: Empty line between the two returns makes it slightly more readable.

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:418: // Returns the
routing id for a RenderFrameHost or RenderFrameProxyHost
Nice catch!

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:470: // given
SiteInstance.  May return a routing ID of either a RenderFrame (if
s/RenderFrame/RenderFrameHost/

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:472: // RenderFrameProxy.
 Returns MSG_ROUTING_NONE if there is no opener, or if
s/RenderFrameProxy/RenderFrameProxyHost/

https://codereview.chromium.org/1202593002/diff/200001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:2546: // propagated to the
popup's RenderViews.  Wait for subframe1 to send a reply
RenderViews? I thought we are tracking openers per frame now, no?

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:925: // We should
find the pending RFH and not create a new one.
This is only valid in regular chrome, not with --site-per-process. This tests
passes most likely in --site-per-process, because you explicitly made it so :).
I wonder if this should be restricted to cases of IsSwappedOutForbidden being
false.

https://codereview.chromium.org/1202593002/diff/200001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/1202593002/diff/200001/content/public/test/te...
content/public/test/test_renderer_host.h:135: virtual bool
CreateTestRenderView(const base::string16& frame_name,
Yuck, I've missed this in my patch to move frame name to be std::string!

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:632: if
(opener_proxy->IsMainFrameDetachedFromTree())
Can we DCHECK the IsSwappedOutForbidden() value? This way we ensure we don't
regress somehow.

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:863:
webview()->mainFrame()->setOpener(opener_frame);
On 2015/07/06 23:56:42, alexmos wrote:
> Note that previously, if this RenderView was opened from a subframe on a
> cross-process RenderView, its opener was set to the main frame of the opener
> RenderView rather than the subframe.  This is incorrect and inconsistent with
> same-process behavior.  Now, if the subframe's proxy doesn't exist in the new
> RenderView (as would be the case without --site-per-process), the opener won't
> be set and is left as null.  This is consistent with how we handled a similar
> case for postMessage (issue 485520).  Trybots seem happy with this as well.

Acknowledged.

[alexmos, 2015-07-08 04:42:18]

Thanks!  Most comments addressed, plus some discussion on the pending RFH and
the "false" parameters below.

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:89: // The RenderFrame will
not have an opener frame assigned, even if the RFH's
On 2015/07/07 16:26:20, nasko (paris) wrote:
> nit: I think dropping "RFH's" still preserves the meaning without making it
hard
> to read/understand.

Done.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:867:
frame_tree_node_->IsMainFrame(), false)) {
On 2015/07/07 16:26:21, nasko (paris) wrote:
> Sprinkling a ton of extra "false" params is bugging me a bit. Can we do it
some
> other way, such that the majority of the call sites (using false) will be
> simpler?

Yeah, I agree it's pretty annoying.  One alternative is to switch InitRenderView
to take a flags enum like CreateRenderFrameFlags.  That would be more readable,
but I don't know if it's worth doing for just two bools (plus, it'd need to
convert things like IsMainFrame() here into flag values).  Another option could
be different versions of InitRenderView (e.g., InitRenderView and
InitRenderViewWithoutOpener calling a private third method that takes the bool) 
-- that may be a bit much for this though?  Or maybe just add a
/*suppress_opener=*/ before the "false" to improve call site readability, which
I see is sometimes used elsewhere in the code base.  Any thoughts on these?

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1828: return
pending_render_frame_host_->GetRoutingID();
On 2015/07/07 16:26:21, nasko (paris) wrote:
> On 2015/07/06 23:56:42, alexmos wrote:
> > See WebContentsImplTest.FindOpenerRVHWhenPending for context of why this is
> > needed.  Previously, CreateOpenerProxies took care of this by looking up and
> > returning the pending RVH's routing ID.
> 
> This has me slightly concerned. If we have already a proxy for SiteInstance A
> and the current RFH is for SiteInstance B, then navigating cross-process to a
> URL in SiteInstance A will create a pending RFH for it, which now will be
> returned. Previously, we would've returned the proxy.
> 
> This method is called in a bunch of contexts for currently running documents,
so
> I think it is not quite correct to return a pending RFH. Maybe we can gate
this
> on "IsSwappedOutForbidden", in which case we will never store RFHs and expect
to
> find them again. I assume that if we reorder the pending RFH and the proxy, we
> will return the proxy and that will still be breaking the test case, right?

Interestingly, if I reorder the pending RFH and the proxy checks, the
FindOpenerRVHWhenPending test still passes: it still returns the pending RFH,
both with --site-per-process and without.  That's because for that scenario
there's no proxy today even if IsSwappedOutForbidden is true. Charlie and I
discussed this and thought that this is probably wrong - there should be a proxy
if IsSwappedOutForbidden is true.

So just to walk through what's happening with this test (which is a bit
different from your example).  There's a window currently at A that navigates to
B and creates a pending RFH for B.  With that pending, A opens a popup and also
navigates it to B.  The question is what should happen when the popup's pending
RFH creation runs CreateOpenerProxies(B) on the popup's opener.  At that point
the opener doesn't have a proxy for B, and the question is - should it create
one, or should it reuse the pending RFH's routing ID?  Currently, it doesn't
create the proxy, not even in --site-per-process (CreateOpenerProxies returns
early if there's a pending RVH).  This seems wrong, since if the opener's
navigation never commits, we still want to deliver window.opener.postMessages to
A.  It seems like think we should fix this to create a proxy for the opener in B
(if IsSwappedOutForbidden is true), and the proxy should go away if/when the
opener's navigation commits.  Does that make sense?

I haven't tried to fix this here yet.  Actually, I'm wondering whether it's
better to split that off into a follow-up CL, since we're already broken here
and this CL is about something a bit different.  In the meantime, I've followed
your suggestion to only return a pending RFH if swapped out exists -- I agree we
need that regardless of this proxy creation fix.  Same for the corresponding
check in the FindOpenerRVHWhenPending test.

Re: "this method is called in a bunch of contexts for currently running
documents" - I saw only two call sites, both in InitRenderFrame, for looking up
the parent and previous sibling's routing IDs.  Were you thinking of some other
uses?  Both of these ones assume the result is always a RenderFrameProxy on the
renderer side, so yeah, it'd be problematic if they ever receive a pending RFH's
routing ID instead (though I'm not sure if either call site is reachable at a
time when there's a pending RFH).

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2328: return
frame_tree_node_->opener()
On 2015/07/07 16:26:21, nasko (paris) wrote:
> nit: Empty line between the two returns makes it slightly more readable.

Done.

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:470: // given
SiteInstance.  May return a routing ID of either a RenderFrame (if
On 2015/07/07 16:26:21, nasko (paris) wrote:
> s/RenderFrame/RenderFrameHost/

Done.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:472: // RenderFrameProxy.
 Returns MSG_ROUTING_NONE if there is no opener, or if
On 2015/07/07 16:26:21, nasko (paris) wrote:
> s/RenderFrameProxy/RenderFrameProxyHost/

Done.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:2546: // propagated to the
popup's RenderViews.  Wait for subframe1 to send a reply
On 2015/07/07 16:26:21, nasko (paris) wrote:
> RenderViews? I thought we are tracking openers per frame now, no?

Oops, fixed.

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:925: // We should
find the pending RFH and not create a new one.
On 2015/07/07 16:26:21, nasko (paris) wrote:
> This is only valid in regular chrome, not with --site-per-process. This tests
> passes most likely in --site-per-process, because you explicitly made it so
:).
> I wonder if this should be restricted to cases of IsSwappedOutForbidden being
> false.

Done - see my earlier discussion.

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/renderer/rende...
content/renderer/render_view_impl.cc:632: if
(opener_proxy->IsMainFrameDetachedFromTree())
On 2015/07/07 16:26:21, nasko (paris) wrote:
> Can we DCHECK the IsSwappedOutForbidden() value? This way we ensure we don't
> regress somehow.

Done.  Added a check for IsSwappedOutForbidden() being false if
IsMainFrameDetachedFromTree() is true.

[nasko, 2015-07-08 09:35:19]

Another round of feedback. Once we resolve the <webview>/suppress opener
question, I think it is good to go.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:867:
frame_tree_node_->IsMainFrame(), false)) {
On 2015/07/08 04:42:18, alexmos wrote:
> On 2015/07/07 16:26:21, nasko (paris) wrote:
> > Sprinkling a ton of extra "false" params is bugging me a bit. Can we do it
> some
> > other way, such that the majority of the call sites (using false) will be
> > simpler?
> 
> Yeah, I agree it's pretty annoying.  One alternative is to switch
InitRenderView
> to take a flags enum like CreateRenderFrameFlags.  That would be more
readable,
> but I don't know if it's worth doing for just two bools (plus, it'd need to
> convert things like IsMainFrame() here into flag values).  

Yeah, I think the flag version is probably not worthwhile at this time.

> Another option could
> be different versions of InitRenderView (e.g., InitRenderView and
> InitRenderViewWithoutOpener calling a private third method that takes the
bool) 
> -- that may be a bit much for this though?  

It isn't just InitRenderView that is a problem. CreateRenderFrameProxies also
has
the bool, which it passes over to InitRenderView. From all the callers of 
CreateRenderFrameProxies, only <webview> cares to suppress the opener.
Do we really have an opener in this case? Where does it come from? Can we neuter
the opener relationship earlier so we don't need this boolean?
An alternative (which I don't quite like) is to create it still unconditionally,
but have the <webview> code send an IPC to disown the opener. The problem with 
that is the extra IPC and async nature of creation and clearing the opener.

> Or maybe just add a
> /*suppress_opener=*/ before the "false" to improve call site readability,
which
> I see is sometimes used elsewhere in the code base.  Any thoughts on these?

Nah, I think adding comment isn't really worth it.

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1828: return
pending_render_frame_host_->GetRoutingID();
On 2015/07/08 04:42:18, alexmos wrote:
> On 2015/07/07 16:26:21, nasko (paris) wrote:
> > On 2015/07/06 23:56:42, alexmos wrote:
> > > See WebContentsImplTest.FindOpenerRVHWhenPending for context of why this
is
> > > needed.  Previously, CreateOpenerProxies took care of this by looking up
and
> > > returning the pending RVH's routing ID.
> > 
> > This has me slightly concerned. If we have already a proxy for SiteInstance
A
> > and the current RFH is for SiteInstance B, then navigating cross-process to
a
> > URL in SiteInstance A will create a pending RFH for it, which now will be
> > returned. Previously, we would've returned the proxy.
> > 
> > This method is called in a bunch of contexts for currently running
documents,
> so
> > I think it is not quite correct to return a pending RFH. Maybe we can gate
> this
> > on "IsSwappedOutForbidden", in which case we will never store RFHs and
expect
> to
> > find them again. I assume that if we reorder the pending RFH and the proxy,
we
> > will return the proxy and that will still be breaking the test case, right?
> 
> Interestingly, if I reorder the pending RFH and the proxy checks, the
> FindOpenerRVHWhenPending test still passes: it still returns the pending RFH,
> both with --site-per-process and without.  That's because for that scenario
> there's no proxy today even if IsSwappedOutForbidden is true. Charlie and I
> discussed this and thought that this is probably wrong - there should be a
proxy
> if IsSwappedOutForbidden is true.
> 
> So just to walk through what's happening with this test (which is a bit
> different from your example).  There's a window currently at A that navigates
to
> B and creates a pending RFH for B.  With that pending, A opens a popup and
also
> navigates it to B.  The question is what should happen when the popup's
pending
> RFH creation runs CreateOpenerProxies(B) on the popup's opener.  At that point
> the opener doesn't have a proxy for B, and the question is - should it create
> one, or should it reuse the pending RFH's routing ID?  Currently, it doesn't
> create the proxy, not even in --site-per-process (CreateOpenerProxies returns
> early if there's a pending RVH).  This seems wrong, since if the opener's
> navigation never commits, we still want to deliver window.opener.postMessages
to
> A.  It seems like think we should fix this to create a proxy for the opener in
B
> (if IsSwappedOutForbidden is true), and the proxy should go away if/when the
> opener's navigation commits.  Does that make sense?

Thanks for the thorough explanation. Yes, indeed this makes sense. I don't think

we should reuse the pending RFH for B in the opener as the object/routing id. We
should be creating a proxy, which will then be deleted if/when the opener
commits.
This way we don't have to have special casing code dealing with the case of 
opener cancelling the navigation, destroying the pending RFH and having to
create
a proxy. It seems way more convoluted than using our existing primitives -
create
a proxy, swap it with a frame on commit.

> I haven't tried to fix this here yet.  Actually, I'm wondering whether it's
> better to split that off into a follow-up CL, since we're already broken here
> and this CL is about something a bit different.  In the meantime, I've
followed
> your suggestion to only return a pending RFH if swapped out exists -- I agree
we
> need that regardless of this proxy creation fix.  Same for the corresponding
> check in the FindOpenerRVHWhenPending test.

I agree it should be a separate CL. Big/complex CLs are hard to review, are hard
to reason about, and take a ton more time. Also, distinct CLs for distinct
problems
are better approach from code history perspective too.
 
> Re: "this method is called in a bunch of contexts for currently running
> documents" - I saw only two call sites, both in InitRenderFrame, for looking
up
> the parent and previous sibling's routing IDs.  Were you thinking of some
other
> uses?  

Hmmm, I thought it was more than that. My bad.

> Both of these ones assume the result is always a RenderFrameProxy on the
> renderer side, so yeah, it'd be problematic if they ever receive a pending
RFH's
> routing ID instead (though I'm not sure if either call site is reachable at a
> time when there's a pending RFH).

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

https://codereview.chromium.org/1202593002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:925: // We should
find the pending RFH and not create a new one.
On 2015/07/08 04:42:18, alexmos wrote:
> On 2015/07/07 16:26:21, nasko (paris) wrote:
> > This is only valid in regular chrome, not with --site-per-process. This
tests
> > passes most likely in --site-per-process, because you explicitly made it so
> :).
> > I wonder if this should be restricted to cases of IsSwappedOutForbidden
being
> > false.
> 
> Done - see my earlier discussion.

Acknowledged.

[alexmos, 2015-07-08 15:09:27]

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:867:
frame_tree_node_->IsMainFrame(), false)) {
> It isn't just InitRenderView that is a problem. CreateRenderFrameProxies also
> has
> the bool, which it passes over to InitRenderView. From all the callers of 
> CreateRenderFrameProxies, only <webview> cares to suppress the opener.
> Do we really have an opener in this case? Where does it come from? Can we
neuter
> the opener relationship earlier so we don't need this boolean?
> An alternative (which I don't quite like) is to create it still
unconditionally,
> but have the <webview> code send an IPC to disown the opener. The problem with

> that is the extra IPC and async nature of creation and clearing the opener.

I suppose the scenario here is some page A opening a new window with a
<webview>.  In this case,
we want the embedder to still be able to talk to A as its opener, and we want to
allow the guest to talk to its embedder, but we want to disallow the guest from
talking to embedder's opener.  Doesn't seem like we can sever the opener
relationship earlier, as then the embedder won't be able to talk to its opener. 
Disowning the opener may work if we send the IPC it to the guest's process for
the embedder's swapped-out RV, without actually setting the embedder's opener to
null in the browser process, but that seems like it introduces more complexity
and will require exposing the disown-opener mechanism at the WebContents layer
(which I just took away in my last CL).

Doing some code archealogy, this was introduced with
https://codereview.chromium.org/10829225, and this comment by Charlie for a
related piece of code kind of covers the rationale:
https://chromiumcodereview.appspot.com/10829225/diff/2001/content/browser/bro...

Looking at this, CreateSwappedOutRenderView is really intended as a version of
CreateOpenerProxies that only walks one level of the opener chain.  I was
thinking about this alternative: have CreateOpenerProxies take a suppress_opener
bool and not recurse further if it's true.  Then, the call site in
CreateOpenerProxiesIfNeeded (which redirects most other calls to this) will pass
false, and the two calls to CreateSwappedOutRenderView from <webview> can become
CreateOpenerProxies which will pass true.  So the bool, while still there, will
affect only one function, CreateOpenerProxies, with only three call sites (plus
one in a test).  Plus, CreateSwappedOutRenderView can go away.  However, that
requires us to still pass back the RenderView ID from CreateOpenerProxies (since
<webview> needs it in
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...)
-- yuck.  That won't play well if your swapped-out removal patch for
CreateOpenerProxies, though it seems like you're already breaking that, judging
by how that view ID is left as NONE in CreateSwappedOutRenderView in case of
IsSwappedOutForbidden. 

Another alternative is to maintain a flag like
suppress_opener_for_swapped_out_views in RFHM, which InitRenderView would
consult, and set the flag on the on the embedder's RFHM as first thing in
CreateSwappedOutRenderView.  Thoughts?

[alexmos, 2015-07-09 02:43:23]

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:867:
frame_tree_node_->IsMainFrame(), false)) {
Thinking more about this, I think we may actually be ok if we just always look
up the opener in InitRenderView and remove suppress_opener altogether.  The
reason is that <webview> already calls CreateSwappedOutRenderView instead of
CreateOpenerProxies for the embedder, so we won't create any new RVs for the
embedder's opener(s) in the guest SiteInstance.  Thus, when we later go and look
up the embedder's opener for the guest's SiteInstance in InitRenderView, we
won't find it, *unless* it has existed before.  The only scenario in which the
latter can happen is if the guest is in the same SiteInstance as the embedder's
opener.  Neither I nor Charlie could come up with an example of how this could
happen today, though it's plausible that it may happen in the future with
extensionview/appview -- but even then, what I think this means is that if the
embedder sends a postMessage to guest, the guest will be able to reach the
embedder's opener via event.source.opener, but since the two are in the same
SiteInstance, I think this is ok since we aren't really increasing the attack
surface.

So, I've rolled back the suppress_opener plumbing - please take a look at PS13
and let me know what you think.  All tests still pass. :)  Note that there will
be another slight change of behavior (which I discovered with some experimental
DCHECKs back in PS6).  If the guest opens a new window, <webview> code will call
CreateSwappedOutRenderView to create an RV for the new window in embedder's
process, here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br....
 With my new approach, the new window will have a valid window.opener pointer to
its opener in the embedder process, whereas before, it didn't (since
CreateSwappedOutRenderView always passed NONE for opener ID).  I think the new
behavior is actually more correct than old behavior (e.g., I think it allows the
embedder to listen for a newwindow event and then use event.window.opener, which
wouldn't have worked previously - see WebViewAPITest.TestNewWindow for an
example of how this can be set up), though I don't know if anyone cares about
this in practice.

[nasko, 2015-07-09 07:46:23]

On 2015/07/09 02:43:23, alexmos wrote:
>
https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_manager.cc (right):
> 
>
https://codereview.chromium.org/1202593002/diff/200001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_manager.cc:867:
> frame_tree_node_->IsMainFrame(), false)) {
> Thinking more about this, I think we may actually be ok if we just always look
> up the opener in InitRenderView and remove suppress_opener altogether.  The
> reason is that <webview> already calls CreateSwappedOutRenderView instead of
> CreateOpenerProxies for the embedder, so we won't create any new RVs for the
> embedder's opener(s) in the guest SiteInstance.  Thus, when we later go and
look
> up the embedder's opener for the guest's SiteInstance in InitRenderView, we
> won't find it, *unless* it has existed before.  The only scenario in which the
> latter can happen is if the guest is in the same SiteInstance as the
embedder's
> opener.  Neither I nor Charlie could come up with an example of how this could
> happen today, though it's plausible that it may happen in the future with
> extensionview/appview -- but even then, what I think this means is that if the
> embedder sends a postMessage to guest, the guest will be able to reach the
> embedder's opener via event.source.opener, but since the two are in the same
> SiteInstance, I think this is ok since we aren't really increasing the attack
> surface.

This makes sense. It also brings it closer to how the open web behaves and I
think this is a good thing.

> So, I've rolled back the suppress_opener plumbing - please take a look at PS13
> and let me know what you think.  All tests still pass. :)  

Thanks! This looks better!

> Note that there will
> be another slight change of behavior (which I discovered with some
experimental
> DCHECKs back in PS6).  If the guest opens a new window, <webview> code will
call
> CreateSwappedOutRenderView to create an RV for the new window in embedder's
> process, here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br....
> With my new approach, the new window will have a valid window.opener pointer
to
> its opener in the embedder process, whereas before, it didn't (since
> CreateSwappedOutRenderView always passed NONE for opener ID).  I think the new
> behavior is actually more correct than old behavior (e.g., I think it allows
the
> embedder to listen for a newwindow event and then use event.window.opener,
which
> wouldn't have worked previously - see WebViewAPITest.TestNewWindow for an
> example of how this can be set up), though I don't know if anyone cares about
> this in practice.

[nasko, 2015-07-09 07:46:33]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-07-09 07:46:34]

lgtm

[commit-bot: I haz the power, 2015-07-09 07:46:44]

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

[commit-bot: I haz the power, 2015-07-09 07:51:20]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-09 07:52:19]

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