Subframes in inner WebContents should not create proxies in process of outer WebContents.

Subframes in inner WebContents should not create proxies in process of outer WebContents.

In an inner WebContents main frame process, we create a proxy for the outer
  WebContents.  However, we do not need proxies in the outer WebContents for
  all of the subframes in the inner WebContents.

Omitting such proxies fixes issues with subframes inside <webview> when using
  OOPIFs (under --site-per-process). Previously we'd fail a
  CHECK(frame_tree_node_->IsMainFrame()) in RFHM::CreateRenderFrameProxy()
  while creating these proxies:
  RFHM::CreateRenderFrameProxy() would not find an RVH in the outer delegate's
  SiteInstance and it would try to create one for this subframe. However, RVH
  creation is only allowed on main frames.

TBR=jam@chromium.org for chromium.fyi.json
BUG=500957
Test=Open a <webview> in a chrome app with --site-per-process. Load
  an <iframe> inside the <webview>. This should work and not crash the
  browser/ process.

Committed: https://crrev.com/9ca4291019ed006cf1c3c696402aa694f3f4652c
Cr-Commit-Position: refs/heads/master@{#341455}

[lazyboy, 2015-07-28 16:57:06]

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

[Charlie Reis, 2015-07-28 22:36:02]

Thanks.  Some nits in the CL description (since "proxy to" is hard to
understand):

"Subframes in inner WebContents should not create proxies in process of outer
WebContents.

In an inner WebContents main frame process, we create a proxy for the outer
WebContents.  However, we do not need proxies in the outer WebContents for all
of the subframes in the inner WebContents.

Omitting such proxies fixes issues with subframes inside <webview> when using
OOPIFs (under --site-per-process)."

(Also, can you be more specific about what it fixes?  I can see why they're not
needed, but I don't understand why they cause problems.)

https://codereview.chromium.org/1262563003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1262563003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:266: return
frame_tree_node_->IsMainFrame() &&
I still question whether this should return false for subframes in the guest. 
That seems like a confusing API: false most of the time, true for guest main
frames, but false for guest subframes.  The name of the method doesn't indicate
that.

It's also hard to tell whether some of the call sites of this method would do
the wrong thing if this returned false for subframes (e.g., in
CrossProcessFrameConnector).

Should it be up to the caller to check for the main frame if it matters, or is
it better to rename the method to be explicit about what it's doing (e.g.,
ForInnerDelegateMainFrame)?

Note: Either way, the .h file needs updating as well, since it has a TODO about
this.

https://codereview.chromium.org/1262563003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1787: // If this proxy
lives in our outer delegate process, then subframe
Rephrase:
Do not create proxies for subframes in the outer delegate's process, since the
outer delegate does not need to interact with them.

[lazyboy, 2015-07-30 19:46:32]

Updated the CL description + added a test that loads nested iframe in <webview>.

https://chromiumcodereview.appspot.com/1262563003/diff/1/content/browser/fram...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1262563003/diff/1/content/browser/fram...
content/browser/frame_host/render_frame_host_manager.cc:266: return
frame_tree_node_->IsMainFrame() &&
On 2015/07/28 22:36:02, Charlie Reis wrote:
> I still question whether this should return false for subframes in the guest. 
> That seems like a confusing API: false most of the time, true for guest main
> frames, but false for guest subframes.  The name of the method doesn't
indicate
> that.
I've made the function only care about its inner-ness and dropped the main frame
requirement.
> 
> It's also hard to tell whether some of the call sites of this method would do
> the wrong thing if this returned false for subframes (e.g., in
> CrossProcessFrameConnector).
> 
> Should it be up to the caller to check for the main frame if it matters,
I've gone on this route.
or is
> it better to rename the method to be explicit about what it's doing (e.g.,
> ForInnerDelegateMainFrame)?
> 
> Note: Either way, the .h file needs updating as well, since it has a TODO
about
> this.
Done.

https://chromiumcodereview.appspot.com/1262563003/diff/1/content/browser/fram...
content/browser/frame_host/render_frame_host_manager.cc:1787: // If this proxy
lives in our outer delegate process, then subframe
On 2015/07/28 22:36:02, Charlie Reis wrote:
> Rephrase:
> Do not create proxies for subframes in the outer delegate's process, since the
> outer delegate does not need to interact with them.

Done.

https://chromiumcodereview.appspot.com/1262563003/diff/20001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/1262563003/diff/20001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1788: if
(ForInnerDelegate() && GetProxyToOuterDelegate() == pair.second)
FYI, for
    A
   /
  B
 /
C

When C is added we would end up here (with this~=B), but B won't have a proxy to
outer delegate.
So checking for IsMainFrame() is not necessary.

[Charlie Reis, 2015-07-31 18:44:08]

Thanks.  I can see it makes some of the call sites a bit more complex, but I
think it's an easier API to understand (especially if we later decide to create
proxies for guest subframes in the embedder's process).

From the CL description, drop the "In inner delegate's root of FrameTree"
paragraph.  Looks like you accidentally left it around.

Otherwise LGTM with nits.

https://codereview.chromium.org/1262563003/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/child_frame.html
(right):

https://codereview.chromium.org/1262563003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/child_frame.html:7: <!--
Our parent frame will navigate this frame -->
nit: End with period.

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

https://codereview.chromium.org/1262563003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1788: if
(ForInnerDelegate() && GetProxyToOuterDelegate() == pair.second)
On 2015/07/30 19:46:32, lazyboy wrote:
> FYI, for
>     A
>    /
>   B
>  /
> C
> 
> When C is added we would end up here (with this~=B), but B won't have a proxy
to
> outer delegate.
> So checking for IsMainFrame() is not necessary.

Thanks.

nit: pair.second == GetProxyToOuterDelegate() seems more natural to read for the
second condition.

[Charlie Reis, 2015-07-31 18:45:03]

Note: Can you get a webview person to review the new test?  I'm not as familiar
with the test setup you have there.

[lazyboy, 2015-07-31 19:21:41]

lazyboy@chromium.org changed reviewers:
+ lfg@chromium.org

[lazyboy, 2015-07-31 19:21:41]

+lfg for reviewing the newly added <webview> test.

https://codereview.chromium.org/1262563003/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/child_frame.html
(right):

https://codereview.chromium.org/1262563003/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/child_frame.html:7: <!--
Our parent frame will navigate this frame -->
On 2015/07/31 18:44:08, Charlie Reis wrote:
> nit: End with period.

Done.

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

https://codereview.chromium.org/1262563003/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1788: if
(ForInnerDelegate() && GetProxyToOuterDelegate() == pair.second)
On 2015/07/31 18:44:08, Charlie Reis wrote:
> On 2015/07/30 19:46:32, lazyboy wrote:
> > FYI, for
> >     A
> >    /
> >   B
> >  /
> > C
> > 
> > When C is added we would end up here (with this~=B), but B won't have a
proxy
> to
> > outer delegate.
> > So checking for IsMainFrame() is not necessary.
> 
> Thanks.
> 
> nit: pair.second == GetProxyToOuterDelegate() seems more natural to read for
the
> second condition.

Done.

[lfg, 2015-07-31 19:35:29]

lgtm with nit.

The test only loads a same-origin iframe, it would be nice to have another test
that also loads a cross-origin iframe.

https://codereview.chromium.org/1262563003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/1262563003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1340:
window.console.log('G: ' + e.message);
Is this needed? If yes, then G wasn't very obvious when I saw it, I assume it's
Guest? Can you make it more explicit?

[lazyboy, 2015-07-31 20:36:47]

I've added another test that loads subframes from cross-origin.

https://codereview.chromium.org/1262563003/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right):

https://codereview.chromium.org/1262563003/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/shim/main.js:1340:
window.console.log('G: ' + e.message);
On 2015/07/31 19:35:29, lfg wrote:
> Is this needed? If yes, then G wasn't very obvious when I saw it, I assume
it's
> Guest? Can you make it more explicit?

Not a requirement, this is just log, I've made it explicit.
Done.

[lazyboy, 2015-08-01 01:04:56]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2015-08-01 01:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
lfg@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1262563003/#ps60001
(title: "address comments from lfg@")

[commit-bot: I haz the power, 2015-08-01 01:05:03]

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

[commit-bot: I haz the power, 2015-08-01 01:13:04]

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

[commit-bot: I haz the power, 2015-08-01 01:13:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lazyboy, 2015-08-01 02:01:25]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2015-08-01 02:01:37]

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

[commit-bot: I haz the power, 2015-08-01 02:06:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-01 02:06:45]

Patchset 4 (id:??) landed as
https://crrev.com/9ca4291019ed006cf1c3c696402aa694f3f4652c
Cr-Commit-Position: refs/heads/master@{#341455}