Refactor CreateOpenerProxies to support updates to frame openers.

Refactor CreateOpenerProxies to support updates to frame openers.

Currently, CreateOpenerProxies assumes that (1) each opener chain can
be recursively traversed until hitting a node with no opener, and (2)
subframes can't have openers.  Neither (1) nor (2) will hold once we
propagate frame opener updates (that happen via window.open) to the
browser process.

To address (1), this CL changes the recursive traversal in
CreateOpenerProxies to a BFS traversal that is resilient to potential
cycles on the opener chain.  To address (2), openers are examined for
all nodes on a given FrameTree, rather than just for the root node.

If the opener graph contains cycles, some proxies created by
CreateOpenerProxies may not have their opener_routing_id available at
creation time.  In a future CL, once opener update IPCs are
implemented, these proxies will need to set their openers in a second
pass (note that this is expected to be very rare, hence an extra IPC
should be ok).  This CL prepares for this by adding logic to collect
nodes involved in cycles that will need this extra processing.

BUG=225940
Committed: https://crrev.com/ce5cab104935d3fedcb3c6af2aec0311be43a812
Cr-Commit-Position: refs/heads/master@{#342221}

[alexmos, 2015-08-05 17:23:26]

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

[alexmos, 2015-08-05 17:45:05]

Hey Charlie, what do you think about this?  This tries to prepare
CreateOpenerProxies to handle arbitrary opener chains once opener updates are
propagated to the browser process.  I broke off CollectOpenerFrameTrees into a
separate function so that I could test the cycle detection part in unit tests.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2439: // not just
current node's opener.
I noticed that this actually has a bug today: if A opens a popup B, and B has a
subframe C, then calling CreateOpenerProxiesIfNeeded on C won't do the right
thing: it should be looking at the top node's opener and not C's opener.  As a
result, window.parent.opener won't work in C.  And once subframe openers are
possible, this will need to look at all FTNs in the tree.

I wanted to add a test for this and fix it in a followup CL to keep this one
less complex, but let me know if I should do that here or before this CL.

[Charlie Reis, 2015-08-05 23:59:10]

Great explanations-- thanks.  So much complexity from such a simple thing...  :)

(I suppose we could bite the bullet and always create proxies for all frames.  I
think it makes sense to keep avoiding that for now, though, since we know
proxies are more expensive in V8 than they need to be.)

Mainly nits below.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:172: namespace {
Nit: This should go above the RFHM:: stuff above.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:174: // Helper function
to add the FrameTree of the current node's opener
s/current/given/

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:177: // been visited
(those at indexes less than |visited_index|).
nit: (i.e., those at indices...

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:178: //
|nodes_with_back_links|, if not null, collects FrameTreeNodes with openers
Why would it be null?  I don't see a case where that happens.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:179: // that point to
FrameTrees that have already been visited (such as those with
nit: with openers in FrameTrees that...

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:181: bool
OpenerForFrameTreeNode(
Does this need to return a bool?  Looks like it always returns true.  (If that's
just for the ForEach contract, let's mention that in the comment.)

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:185: FrameTreeNode*
node) {
nit: Let's list |node| first, since it's the main subject.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2439: // not just
current node's opener.
On 2015/08/05 17:45:05, alexmos wrote:
> I noticed that this actually has a bug today: if A opens a popup B, and B has
a
> subframe C, then calling CreateOpenerProxiesIfNeeded on C won't do the right
> thing: it should be looking at the top node's opener and not C's opener.  As a
> result, window.parent.opener won't work in C.  And once subframe openers are
> possible, this will need to look at all FTNs in the tree.
> 
> I wanted to add a test for this and fix it in a followup CL to keep this one
> less complex, but let me know if I should do that here or before this CL.

Nice catch.  Yes, a separate CL is fine, either before or after.

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

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:486:
TraverseComplexOpenerChain);
Maybe we can expose CollectOpenerFrameTrees via RenderFrameHostManagerTest
instead, since that's already a friend class?

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

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2307: // Create
an opener tab, and simulate that its opener points to itself.
Sigh, the things the web platform can do...

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2326: // Verify
that the proxy' opener points to itself.
nit: proxy's

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2349: //
available during the first pass of CreateOpenerProxies.
Wow, this is a well-constructed test case, and it's really useful to have that
diagram.  Nicely done.

[alexmos, 2015-08-06 17:05:40]

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:172: namespace {
On 2015/08/05 23:59:09, Charlie Reis wrote:
> Nit: This should go above the RFHM:: stuff above.

Done.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:174: // Helper function
to add the FrameTree of the current node's opener
On 2015/08/05 23:59:09, Charlie Reis wrote:
> s/current/given/

Done.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:177: // been visited
(those at indexes less than |visited_index|).
On 2015/08/05 23:59:09, Charlie Reis wrote:
> nit: (i.e., those at indices...

Done.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:178: //
|nodes_with_back_links|, if not null, collects FrameTreeNodes with openers
On 2015/08/05 23:59:09, Charlie Reis wrote:
> Why would it be null?  I don't see a case where that happens.

Removed from description and the if statement below.  Yes, there's no case right
now when that happens.  I was thinking that down the road something might want
to call this and not care about nodes with cycles, but I suppose we can always
add that case if/when it actually becomes needed.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:179: // that point to
FrameTrees that have already been visited (such as those with
On 2015/08/05 23:59:09, Charlie Reis wrote:
> nit: with openers in FrameTrees that...

Done.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:181: bool
OpenerForFrameTreeNode(
On 2015/08/05 23:59:09, Charlie Reis wrote:
> Does this need to return a bool?  Looks like it always returns true.  (If
that's
> just for the ForEach contract, let's mention that in the comment.)

Done.  Yes, it's returning true just so it visits all nodes as part of ForEach,
so added a sentence about it in the comment.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:185: FrameTreeNode*
node) {
On 2015/08/05 23:59:09, Charlie Reis wrote:
> nit: Let's list |node| first, since it's the main subject.

Hmm, I can't do that given how FrameTree::ForEach always puts the |node| last in
the arg list, can I?

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2439: // not just
current node's opener.
On 2015/08/05 23:59:09, Charlie Reis wrote:
> On 2015/08/05 17:45:05, alexmos wrote:
> > I noticed that this actually has a bug today: if A opens a popup B, and B
has
> a
> > subframe C, then calling CreateOpenerProxiesIfNeeded on C won't do the right
> > thing: it should be looking at the top node's opener and not C's opener.  As
a
> > result, window.parent.opener won't work in C.  And once subframe openers are
> > possible, this will need to look at all FTNs in the tree.
> > 
> > I wanted to add a test for this and fix it in a followup CL to keep this one
> > less complex, but let me know if I should do that here or before this CL.
> 
> Nice catch.  Yes, a separate CL is fine, either before or after.

OK, will do in a later CL.

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

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:486:
TraverseComplexOpenerChain);
On 2015/08/05 23:59:09, Charlie Reis wrote:
> Maybe we can expose CollectOpenerFrameTrees via RenderFrameHostManagerTest
> instead, since that's already a friend class?

Done.

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

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2307: // Create
an opener tab, and simulate that its opener points to itself.
On 2015/08/05 23:59:10, Charlie Reis wrote:
> Sigh, the things the web platform can do...

Acknowledged. :)

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:2326: // Verify
that the proxy' opener points to itself.
On 2015/08/05 23:59:10, Charlie Reis wrote:
> nit: proxy's

Done.

[Charlie Reis, 2015-08-06 17:35:06]

Great.  LGTM with nits.

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:185: FrameTreeNode*
node) {
On 2015/08/06 17:05:39, alexmos wrote:
> On 2015/08/05 23:59:09, Charlie Reis wrote:
> > nit: Let's list |node| first, since it's the main subject.
> 
> Hmm, I can't do that given how FrameTree::ForEach always puts the |node| last
in
> the arg list, can I?

Acknowledged.

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:52: // Helper function
to add the FrameTree of the current node's opener to the
nit: s/current/given/

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

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:429: //
|nodes_with_back_links|.
It'd be ok to just say "Exposes CollectOpenerFrameTrees for testing," since this
implementation doesn't do anything special beyond the original one.

[alexmos, 2015-08-06 17:42:57]

Thanks!

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:52: // Helper function
to add the FrameTree of the current node's opener to the
On 2015/08/06 17:35:06, Charlie Reis (slow till 8-14) wrote:
> nit: s/current/given/

Done.

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

https://codereview.chromium.org/1268153002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:429: //
|nodes_with_back_links|.
On 2015/08/06 17:35:06, Charlie Reis (slow till 8-14) wrote:
> It'd be ok to just say "Exposes CollectOpenerFrameTrees for testing," since
this
> implementation doesn't do anything special beyond the original one.

Done.

[alexmos, 2015-08-06 20:12:28]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-08-06 20:12:29]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1268153002/#ps80001
(title: "Charlie's nits")

[commit-bot: I haz the power, 2015-08-06 20:13:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1268153002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1268153002/80001

[commit-bot: I haz the power, 2015-08-06 21:40:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 21:40:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alexmos, 2015-08-06 21:59:12]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-08-06 22:00:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1268153002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1268153002/80001

[commit-bot: I haz the power, 2015-08-06 22:57:32]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-06 22:58:08]

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