Don't grant WebUI bindings during non-WebUI subframe navigations.

Don't grant WebUI bindings during non-WebUI subframe navigations.

InitRenderView should only grant the bindings if the RenderViewHost is
for an active RenderFrameHost, and not for a proxy.

BUG=536145
TEST=See bug comment 3 for repro steps.

Committed: https://crrev.com/733d4426e6a6f53bc02efba0b63a12960353ed52
Cr-Commit-Position: refs/heads/master@{#354523}

[Charlie Reis, 2015-10-15 19:25:21]

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

[Charlie Reis, 2015-10-15 19:39:07]

Alex, can you help me verify that this change is correct?  The goal is to not
grant WebUI when creating proxies for a different frame's navigation (via
CreateProxiesForNewRenderFrameHost).

I wanted to check that |render_view_host| matched the pending or speculative
RenderFrameHost's RVH (and thus SiteInstance).  Unfortunately, the pending RFH
isn't set yet when this is called from CreateRenderFrame, so we didn't grant the
bindings when we needed to.

I can't think of a case that we would want to grant WebUI bindings when creating
a RVH for a proxy, so maybe that's a reasonable check instead?

On the plus side, Carlos is moving the pending WebUI from RFHM to RFH in
https://codereview.chromium.org/1352813006/.  This bug doesn't repro with that
CL applied, so this fix should just be a stopgap until then.  (I'm CC'ing Carlos
and Nasko to make sure that this isn't otherwise a risk in the new approach.)

[Charlie Reis, 2015-10-15 21:10:18]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 21:11:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403343002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403343002/1

[commit-bot: I haz the power, 2015-10-15 21:52:34]

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

[commit-bot: I haz the power, 2015-10-15 21:52:34]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-10-15 23:11:41]

Great, I think this makes sense.  Just a couple of quick questions.  Is it
possible to ever navigate a subframe to a WebUI URL with a main frame at a
non-WebUI URL?  What would happen in that case?

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

https://codereview.chromium.org/1403343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2009: if
(proxy_routing_id == MSG_ROUTING_NONE) {
Just curious, would the proxy check be equivalent to checking is_active() on
render_view_host?

[Charlie Reis, 2015-10-16 06:06:51]

On 2015/10/15 23:11:41, alexmos wrote:
> Great, I think this makes sense.  Just a couple of quick questions.  Is it
> possible to ever navigate a subframe to a WebUI URL with a main frame at a
> non-WebUI URL?  What would happen in that case?

I don't think so.  The one open question in my mind is what happens if we start
"blessing" extension iframes inside web pages.  That might hit that case, but
that's not supported yet.  (Most chrome:// URLs would be prevented by both
FilterURL and X-Frame-Options.)

I tried hacking together a manual case that would hit it, and it looks like we
would crash in WebUIMojo because it doesn't support remote main frames yet.

I think I'm ok with that for the time being, until we go out of our way to
support it.

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

https://codereview.chromium.org/1403343002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2009: if
(proxy_routing_id == MSG_ROUTING_NONE) {
On 2015/10/15 23:11:41, alexmos wrote:
> Just curious, would the proxy check be equivalent to checking is_active() on
> render_view_host?

Good question.  I added a DCHECK that they're equivalent and it seems to hold in
all the tests, which makes sense-- we call InitRenderView when visiting the main
frame, and we only pass a proxy_routing_id if the main frame is a proxy (and
thus not active).

[Charlie Reis, 2015-10-16 06:06:59]

The CQ bit was checked by creis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 06:07:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403343002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403343002/20001

[commit-bot: I haz the power, 2015-10-16 06:11:36]

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

[commit-bot: I haz the power, 2015-10-16 06:11:36]

Dry run: This issue passed the CQ dry run.

[alexmos, 2015-10-16 16:14:37]

Great, LGTM.

[Charlie Reis, 2015-10-16 16:24:11]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-10-16 16:24:17]

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

[commit-bot: I haz the power, 2015-10-16 16:28:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-16 16:29:23]

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