Fix support for multiple frames per SiteInstance in CreateProxiesForSiteInstance

Fix support for multiple frames per SiteInstance in CreateProxiesForSiteInstance.

When a new cross-site frame is created, RFHM::CreateProxiesForSiteInstance walks the frame tree and creates proxies for the new frame in other nodes' SiteInstances.  This traversal currently hits this CHECK in RFHM::CreateRenderFrameProxy in cases where the new frame is added to a SiteInstance that already has other frames:

  CHECK_NE(instance, render_frame_host_->GetSiteInstance());

For example, if site A embeds two frames, B1 and B2, both pointing to site B, we hit this when we are visiting B1's frame tree node to create a proxy for B2, since B2's and B1's SiteInstance is the same.

This CL ensures that we aren't trying to create a proxy at frame tree nodes that share the new frame's SiteInstance.

BUG=None

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

[alexmos, 2014-10-21 21:12:47]

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

[alexmos, 2014-10-21 22:05:28]

Nasko, could you please take a look?

[nasko, 2014-10-21 22:25:23]

LGTM with a nit.

https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:61: if (current_instance !=
instance.get()) {
nit: one line if statements don't need {}

[Charlie Reis, 2014-10-21 22:31:27]

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

[Charlie Reis, 2014-10-21 22:31:27]

https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
content/browser/frame_host/frame_tree.cc:60:
node->render_manager()->current_frame_host()->GetSiteInstance();
What about the SiteInstances that already have RenderFrameProxyHosts on the
proxy_hosts_ list?  Those would also qualify as "existing SiteInstance."

[alexmos, 2014-10-21 22:47:09]

On 2014/10/21 22:25:23, nasko wrote:
> LGTM with a nit.
> 
>
https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
> File content/browser/frame_host/frame_tree.cc (right):
> 
>
https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
> content/browser/frame_host/frame_tree.cc:61: if (current_instance !=
> instance.get()) {
> nit: one line if statements don't need {}

Done.

On 2014/10/21 22:31:27, Charlie Reis wrote:
>
https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
> File content/browser/frame_host/frame_tree.cc (right):
> 
>
https://codereview.chromium.org/660083003/diff/1/content/browser/frame_host/f...
> content/browser/frame_host/frame_tree.cc:60:
> node->render_manager()->current_frame_host()->GetSiteInstance();
> What about the SiteInstances that already have RenderFrameProxyHosts on the
> proxy_hosts_ list?  Those would also qualify as "existing SiteInstance."

Comment fixed.  RFHM::CreateRenderFrameProxy checks for this and returns an
existing proxy if it's already created, so this only applies to the "current"
SiteInstance.

[Charlie Reis, 2014-10-21 22:55:29]

Thanks!  LGTM.

[alexmos, 2014-10-21 23:08:06]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2014-10-21 23:11:14]

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

[commit-bot: I haz the power, 2014-10-22 01:37:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-22 04:04:55]

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