Change allowed bindings to be per RenderFrame instead of per RenderView.

Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being created in a renderer where some global state has already been
set.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2566583002
Cr-Commit-Position: refs/heads/master@{#446950}
Committed: https://chromium.googlesource.com/chromium/src/+/7f6c6a0b64b8d4df8cb476d88bc582109c0d6bff

[Sam McNally, 2016-12-09 00:43:46]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

BUG=611838
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2016-12-09 00:43:56]

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

[commit-bot: I haz the power, 2016-12-09 00:44:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 01:03:27]

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

[commit-bot: I haz the power, 2016-12-09 01:03:28]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Sam McNally, 2016-12-09 02:59:17]

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

[commit-bot: I haz the power, 2016-12-09 02:59:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 03:18:44]

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

[commit-bot: I haz the power, 2016-12-09 03:18:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-12-09 03:37:21]

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

[commit-bot: I haz the power, 2016-12-09 03:37:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 04:23:23]

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

[commit-bot: I haz the power, 2016-12-09 04:23:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2016-12-11 23:24:36]

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

[commit-bot: I haz the power, 2016-12-11 23:24:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 00:17:43]

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

[commit-bot: I haz the power, 2016-12-12 00:17:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2016-12-12 01:51:56]

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

[commit-bot: I haz the power, 2016-12-12 01:52:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 02:39:56]

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

[commit-bot: I haz the power, 2016-12-12 02:39:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2016-12-12 04:18:32]

Patchset #4 (id:60001) has been deleted

[Sam McNally, 2016-12-12 04:18:34]

Patchset #3 (id:40001) has been deleted

[Sam McNally, 2016-12-12 04:18:37]

Patchset #2 (id:20001) has been deleted

[Sam McNally, 2016-12-12 04:18:41]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-12-12 10:21:29]

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

[Sam McNally, 2016-12-12 10:21:30]

This fixes https://bugs.chromium.org/p/chromium/issues/detail?id=611838 by
changing tracking of enabled bindings to be per-renderframe instead of
per-renderview. However, it also causes subframe webui to lose their webui
bindings unless browser side navigation is enabled and I have no idea what
should be responsible for enabling the bindings to fix that. Any suggestions?

[nasko, 2016-12-15 00:55:01]

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

[nasko, 2016-12-15 00:55:02]

I'm sorry about the huge delay. I will get to review it this week, but also
adding creis@ for extra pair of eyes.

This is a somewhat scary change, so I want to spend some quality time with the
CL to ensure it does the right thing.

Thanks for your patience!

[Charlie Reis, 2016-12-16 01:01:51]

Thanks-- in general, I do think we want to move bindings over to RenderFrame, as
long as we're careful about it.

On 2016/12/12 10:21:30, Sam McNally wrote:
> This fixes https://bugs.chromium.org/p/chromium/issues/detail?id=611838 by
> changing tracking of enabled bindings to be per-renderframe instead of
> per-renderview.

Can you explain how this fixes the bug?  I'm not sure how geolocation is related
to bindings.  Is this related to the Mojo bindings for layout tests?  Which part
wasn't working before?

> However, it also causes subframe webui to lose their webui
> bindings unless browser side navigation is enabled and I have no idea what
> should be responsible for enabling the bindings to fix that. Any suggestions?

There has kind of been an assumptions that all frames of a page will have the
same bindings, and we're not clear if there are good reasons to change that. 
That should be ok for your case, right?

If so, maybe something like CreateRenderFrame should be granting the bindings of
the parent frame to all subframes, and we can have sanity checks that frames
within a page don't ever differ?

(Why does it work in PlzNavigate, by the way?  How do the subframes get bindings
in that case?)

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1709: void
RenderFrameHostImpl::AllowBindings(int bindings_flags) {
Do we ever need frames to have different bindings than their parents?  That's
not possible today, so I'm curious if we can enforce that here.

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1834: //
EXPECT_EQ(0, initial_rvh->GetMainFrame()->GetEnabledBindings());
Shouldn't comment this line out.

https://codereview.chromium.org/2566583002/diff/80001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2566583002/diff/80001/content/common/frame.mo...
content/common/frame.mojom:17: // Used to tell a render view whether it should
expose various bindings
s/view/frame/?

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:225: // Tell the render view to
enable a set of javascript bindings. The argument
s/view/frame/

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:230: // RenderView. See
BindingsPolicy for details.
RenderFrame

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:6204: int enabled_bindings =
RenderProcess::current()->GetEnabledBindings();
This might be wrong.  Before, it was checking against the process's bindings,
which include the bindings granted to all views/frames in the process.  Now it's
only checking the current frame's bindings.

Maybe we should put this back?  Or should we be asserting that they're
equivalent?

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2681: if (IsMainFrame() &&
(enabled_bindings_flags & BINDINGS_POLICY_WEB_UI) &&
Should we have a TODO to move WebUIExtensionData to be a RenderFrameObserver? 
It doesn't seem right to leave it associated with the view, though I'm not sure
if there are any concrete bugs due to it.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
content/renderer/web_ui_extension.cc:48: RenderFrame* render_frame =
render_view->GetMainRenderFrame();
Is this wrong?  The frame_ptr passed in might not be the main frame.  I would
guess that we should be using RenderFrame::FromWebFrame(frame) instead.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
content/renderer/web_ui_extension.cc:132: render_view->Send(new
ViewHostMsg_WebUISend(render_view->GetRoutingID(),
I suppose this class will need conversion soon as well.  Will it be safe in the
meantime?

[Sam McNally, 2016-12-19 02:24:59]

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

[commit-bot: I haz the power, 2016-12-19 02:25:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2016-12-19 02:58:08]

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

[commit-bot: I haz the power, 2016-12-19 02:58:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 03:49:19]

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

[commit-bot: I haz the power, 2016-12-19 03:49:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2017-01-09 01:55:12]

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

[commit-bot: I haz the power, 2017-01-09 01:55:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 02:37:42]

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

[commit-bot: I haz the power, 2017-01-09 02:37:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Sam McNally, 2017-01-09 22:41:58]

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

[commit-bot: I haz the power, 2017-01-09 22:42:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:53:38]

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

[commit-bot: I haz the power, 2017-01-09 23:53:39]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Sam McNally, 2017-01-10 04:58:27]

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

[commit-bot: I haz the power, 2017-01-10 04:58:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 06:50:09]

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

[commit-bot: I haz the power, 2017-01-10 06:50:10]

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

[Sam McNally, 2017-01-10 22:08:04]

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

[commit-bot: I haz the power, 2017-01-10 22:08:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:13:27]

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

[commit-bot: I haz the power, 2017-01-10 22:13:28]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sam McNally, 2017-01-10 22:42:09]

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

[commit-bot: I haz the power, 2017-01-10 22:42:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 23:55:49]

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

[commit-bot: I haz the power, 2017-01-10 23:55:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2017-01-11 04:31:54]

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

[commit-bot: I haz the power, 2017-01-11 04:32:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 06:05:14]

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

[commit-bot: I haz the power, 2017-01-11 06:05:16]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-12 09:06:43]

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

[commit-bot: I haz the power, 2017-01-12 09:06:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2017-01-12 09:27:09]

On 2016/12/16 01:01:51, Charlie Reis wrote:
> Thanks-- in general, I do think we want to move bindings over to RenderFrame,
as
> long as we're careful about it.
> 
> On 2016/12/12 10:21:30, Sam McNally wrote:
> > This fixes https://bugs.chromium.org/p/chromium/issues/detail?id=611838 by
> > changing tracking of enabled bindings to be per-renderframe instead of
> > per-renderview.
> 
> Can you explain how this fixes the bug?  I'm not sure how geolocation is
related
> to bindings.  Is this related to the Mojo bindings for layout tests?  Which
part
> wasn't working before?

Yes. Originally, the layout test mojo bindings were only available from main
frames. To make these subframe geolocation layout tests work I made frames pick
up whatever bindings had been enabled for a process; this works fine as long as
all frames live in a renderer process that contains a RenderView. With
site-per-process enabled, the subframe used in this test ends up in a different
process so that didn't work.
> 
> > However, it also causes subframe webui to lose their webui
> > bindings unless browser side navigation is enabled and I have no idea what
> > should be responsible for enabling the bindings to fix that. Any
suggestions?
> 
> There has kind of been an assumptions that all frames of a page will have the
> same bindings, and we're not clear if there are good reasons to change that. 
> That should be ok for your case, right?

Yes.
> 
> If so, maybe something like CreateRenderFrame should be granting the bindings
of
> the parent frame to all subframes, and we can have sanity checks that frames
> within a page don't ever differ?

Done.
> 
> (Why does it work in PlzNavigate, by the way?  How do the subframes get
bindings
> in that case?)

RenderFrameHostManager::GetFrameHostForNavigation() is called for all frames,
and that calls UpdatePendingWebUIOnCurrentFrameHost() which calls
RenderFrameHostImpl::UpdatePendingWebUI(), which enables the bindings.

With browser-side navigation disabled,
RenderFrameHostManager::UpdateStateForNavigate(), which calls
RenderFrameHostImpl::UpdatePendingWebUI() for main frames, returns early for
WebUI subframes.

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1709: void
RenderFrameHostImpl::AllowBindings(int bindings_flags) {
On 2016/12/16 01:01:52, Charlie Reis wrote:
> Do we ever need frames to have different bindings than their parents?  That's
> not possible today, so I'm curious if we can enforce that here.

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2566583002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1834: //
EXPECT_EQ(0, initial_rvh->GetMainFrame()->GetEnabledBindings());
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> Shouldn't comment this line out.

So it turns out that this RVH doesn't have a RenderFrameHost. As far as I can
tell, there's only a RenderFrameProxyHost for |site_instance1| at this point.
I'm not sure if there's anything that can be checked instead.

https://codereview.chromium.org/2566583002/diff/80001/content/common/frame.mojom
File content/common/frame.mojom (right):

https://codereview.chromium.org/2566583002/diff/80001/content/common/frame.mo...
content/common/frame.mojom:17: // Used to tell a render view whether it should
expose various bindings
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> s/view/frame/?

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:225: // Tell the render view to
enable a set of javascript bindings. The argument
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> s/view/frame/

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/public/browser/...
content/public/browser/render_frame_host.h:230: // RenderView. See
BindingsPolicy for details.
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> RenderFrame

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:6204: int enabled_bindings =
RenderProcess::current()->GetEnabledBindings();
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> This might be wrong.  Before, it was checking against the process's bindings,
> which include the bindings granted to all views/frames in the process.  Now
it's
> only checking the current frame's bindings.
> 
> Maybe we should put this back?  Or should we be asserting that they're
> equivalent?

This was the papering-over that made layout test mojo bindings work most of the
time. Added a DCHECK.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2681: if (IsMainFrame() &&
(enabled_bindings_flags & BINDINGS_POLICY_WEB_UI) &&
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> Should we have a TODO to move WebUIExtensionData to be a RenderFrameObserver? 
> It doesn't seem right to leave it associated with the view, though I'm not
sure
> if there are any concrete bugs due to it.

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
content/renderer/web_ui_extension.cc:48: RenderFrame* render_frame =
render_view->GetMainRenderFrame();
On 2016/12/16 01:01:52, Charlie Reis (OOO until 2017) wrote:
> Is this wrong?  The frame_ptr passed in might not be the main frame.  I would
> guess that we should be using RenderFrame::FromWebFrame(frame) instead.

Done.

https://codereview.chromium.org/2566583002/diff/80001/content/renderer/web_ui...
content/renderer/web_ui_extension.cc:132: render_view->Send(new
ViewHostMsg_WebUISend(render_view->GetRoutingID(),
On 2016/12/16 01:01:52, Charlie Reis wrote:
> I suppose this class will need conversion soon as well.  Will it be safe in
the
> meantime?

I think so.

[commit-bot: I haz the power, 2017-01-12 10:55:48]

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

[commit-bot: I haz the power, 2017-01-12 10:55:49]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-18 22:18:43]

Thanks, and apologies for the long delay!  This looks good apart from a few
small issues noted below.  The extra checks to ensure all frames in a page have
the same bindings are much appreciated.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:958: if (enabled_bindings_)
{
Should this also check |created|?  I'm guessing we don't want to call
AllowBindings if this is about a crash / RenderFrameDeleted case.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1028: bool added =
frame_tree_->AddFrame(
Note that AddFrame already calls SetRenderFrameCreated(true) on the new child,
which will hit your AllowBindings call above.  I think it would be better to
avoid the need for the second AllowBindings call below.

Can we inherit the parent's bindings in the RenderFrameHostImpl constructor, on
line 358 instead?  Let's include a comment with it saying that all frames in a
page are expected to have the same bindings.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2833: // process, then
enable missing bindings with the RenderViewHost.
nit: Drop "with the RenderViewHost."

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1834: //
EXPECT_EQ(0, initial_rvh->GetMainFrame()->GetEnabledBindings());
Ok.  Can we replace it with an EXPECT_FALSE version of the HasWebUIBindings
check on line 1812, so that we're checking the process instead?

If so, let's do that.  If not, just remove these two lines rather than have them
commented out (or maybe leave a comment saying what the bug used to be).

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:506:
EXPECT_TRUE(contents()->GetMainFrame()->GetEnabledBindings() &
nit: main_rfh() might be more concise?

https://codereview.chromium.org/2566583002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/render...
content/browser/renderer_host/render_view_host_unittest.cc:91:
rvh()->GetMainFrame()->AllowBindings(BINDINGS_POLICY_WEB_UI);
main_rfh() here as well.

[Sam McNally, 2017-01-19 02:37:21]

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

[commit-bot: I haz the power, 2017-01-19 02:37:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 02:40:21]

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

[commit-bot: I haz the power, 2017-01-19 02:40:22]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Sam McNally, 2017-01-19 03:27:18]

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

[commit-bot: I haz the power, 2017-01-19 03:27:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 05:07:19]

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

[commit-bot: I haz the power, 2017-01-19 05:07:21]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-19 05:29:37]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2017-01-19 05:30:37]

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:958: if (enabled_bindings_)
{
On 2017/01/18 22:18:43, Charlie Reis wrote:
> Should this also check |created|?  I'm guessing we don't want to call
> AllowBindings if this is about a crash / RenderFrameDeleted case.

Done.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1028: bool added =
frame_tree_->AddFrame(
On 2017/01/18 22:18:43, Charlie Reis wrote:
> Note that AddFrame already calls SetRenderFrameCreated(true) on the new child,
> which will hit your AllowBindings call above.  I think it would be better to
> avoid the need for the second AllowBindings call below.
> 
> Can we inherit the parent's bindings in the RenderFrameHostImpl constructor,
on
> line 358 instead?  Let's include a comment with it saying that all frames in a
> page are expected to have the same bindings.

Done.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2833: // process, then
enable missing bindings with the RenderViewHost.
On 2017/01/18 22:18:43, Charlie Reis wrote:
> nit: Drop "with the RenderViewHost."

Done.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1834: //
EXPECT_EQ(0, initial_rvh->GetMainFrame()->GetEnabledBindings());
On 2017/01/18 22:18:43, Charlie Reis wrote:
> Ok.  Can we replace it with an EXPECT_FALSE version of the HasWebUIBindings
> check on line 1812, so that we're checking the process instead?
> 
> If so, let's do that.  If not, just remove these two lines rather than have
them
> commented out (or maybe leave a comment saying what the bug used to be).

No, that's true since both webui WebContents end up in the same renderer
process, and process was granted the webui bindings for the first WebContents.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:506:
EXPECT_TRUE(contents()->GetMainFrame()->GetEnabledBindings() &
On 2017/01/18 22:18:43, Charlie Reis wrote:
> nit: main_rfh() might be more concise?

Done.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/render...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/render...
content/browser/renderer_host/render_view_host_unittest.cc:91:
rvh()->GetMainFrame()->AllowBindings(BINDINGS_POLICY_WEB_UI);
On 2017/01/18 22:18:43, Charlie Reis wrote:
> main_rfh() here as well.

Done.

[Charlie Reis, 2017-01-19 17:58:54]

Thanks!  LGTM.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1028: bool added =
frame_tree_->AddFrame(
On 2017/01/19 05:30:37, Sam McNally wrote:
> On 2017/01/18 22:18:43, Charlie Reis wrote:
> > Note that AddFrame already calls SetRenderFrameCreated(true) on the new
child,
> > which will hit your AllowBindings call above.  I think it would be better to
> > avoid the need for the second AllowBindings call below.
> > 
> > Can we inherit the parent's bindings in the RenderFrameHostImpl constructor,
> on
> > line 358 instead?  Let's include a comment with it saying that all frames in
a
> > page are expected to have the same bindings.
> 
> Done.

Thanks!  That looks better.

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/2566583002/diff/300001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1834: //
EXPECT_EQ(0, initial_rvh->GetMainFrame()->GetEnabledBindings());
On 2017/01/19 05:30:37, Sam McNally wrote:
> On 2017/01/18 22:18:43, Charlie Reis wrote:
> > Ok.  Can we replace it with an EXPECT_FALSE version of the HasWebUIBindings
> > check on line 1812, so that we're checking the process instead?
> > 
> > If so, let's do that.  If not, just remove these two lines rather than have
> them
> > commented out (or maybe leave a comment saying what the bug used to be).
> 
> No, that's true since both webui WebContents end up in the same renderer
> process, and process was granted the webui bindings for the first WebContents.

Acknowledged.

https://codereview.chromium.org/2566583002/diff/360001/content/browser/render...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/360001/content/browser/render...
content/browser/renderer_host/render_view_host_unittest.cc:92:
EXPECT_FALSE(rvh()->GetMainFrame()->GetEnabledBindings() &
nit: This one should be main_rfh(), too.  :)

[Sam McNally, 2017-01-19 22:38:52]

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

[commit-bot: I haz the power, 2017-01-19 22:39:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2017-01-19 23:18:02]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since, all RenderViews are granted these bindings in layout tests, this
was sufficient as long as all frames share a renderer process with at
least one RenderView. With site isolation, there is one layout test
where a RenderFrame ends up in a process without a RenderView, and as a
result it cannot access the mojo bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being in the same renderer process as their parent frames.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2017-01-19 23:34:08]

sammc@chromium.org changed reviewers:
+ alexclarke@chromium.org, dcheng@chromium.org, nyquist@chromium.org,
sky@chromium.org

[Sam McNally, 2017-01-19 23:34:09]

+alexclarke for //headless

+nyquist for //components/dom_distiller

+sky for //chrome

+dcheng for the mojom

https://codereview.chromium.org/2566583002/diff/360001/content/browser/render...
File content/browser/renderer_host/render_view_host_unittest.cc (right):

https://codereview.chromium.org/2566583002/diff/360001/content/browser/render...
content/browser/renderer_host/render_view_host_unittest.cc:92:
EXPECT_FALSE(rvh()->GetMainFrame()->GetEnabledBindings() &
On 2017/01/19 17:58:55, Charlie Reis wrote:
> nit: This one should be main_rfh(), too.  :)

Done.

[commit-bot: I haz the power, 2017-01-20 00:16:41]

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

[commit-bot: I haz the power, 2017-01-20 00:16:42]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-20 00:45:33]

LGTM

[dcheng, 2017-01-20 08:29:27]

https://codereview.chromium.org/2566583002/diff/380001/content/common/frame.m...
File content/common/frame.mojom (right):

https://codereview.chromium.org/2566583002/diff/380001/content/common/frame.m...
content/common/frame.mojom:29: interface FrameBindingsControl {
Nit: it might be nice to group the browser and renderer interfaces together...
I'm not sure.

Also, I'm curious: why a separate interface? Is it just to make the Frame
interface itself more modular? It makes it a bit harder to tell how the
different interfaces here are related: for example, I think that this is
actually associated with Frame, but I wouldn't be able to tell from just reading
the mojom.

[alex clarke (OOO till 29th), 2017-01-20 08:33:10]

headless/ LGTM

[nyquist, 2017-01-20 18:22:00]

//components/dom_distiller lgtm

[Sam McNally, 2017-01-23 00:50:04]

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

[commit-bot: I haz the power, 2017-01-23 00:50:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sam McNally, 2017-01-23 00:54:38]

Patchset #18 (id:420001) has been deleted

[commit-bot: I haz the power, 2017-01-23 03:08:11]

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

[commit-bot: I haz the power, 2017-01-23 03:08:12]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-23 03:20:28]

https://codereview.chromium.org/2566583002/diff/380001/content/common/frame.m...
File content/common/frame.mojom (right):

https://codereview.chromium.org/2566583002/diff/380001/content/common/frame.m...
content/common/frame.mojom:29: interface FrameBindingsControl {
On 2017/01/20 08:29:27, dcheng wrote:
> Nit: it might be nice to group the browser and renderer interfaces together...
> I'm not sure.

Done.
> 
> Also, I'm curious: why a separate interface? Is it just to make the Frame
> interface itself more modular? It makes it a bit harder to tell how the
> different interfaces here are related: for example, I think that this is
> actually associated with Frame, but I wouldn't be able to tell from just
reading
> the mojom.

It's because AllowBindings() needs to be associated with the legacy IPC channel,
but GetInterfaceProvider() needs to support queuing (which associated interfaces
don't allow).

[dcheng, 2017-01-23 23:36:40]

Sorry, one more question. In the CL description, it says:

 With site isolation, there is one layout test
where a RenderFrame ends up in a process without a RenderView, and as a
result it cannot access the mojo bindings necessary to run that test.


But I'm not sure how we can have a RenderView without a RenderFrame.

[Sam McNally, 2017-01-23 23:47:39]

On 2017/01/23 23:36:40, dcheng wrote:
> Sorry, one more question. In the CL description, it says:
> 
>  With site isolation, there is one layout test
> where a RenderFrame ends up in a process without a RenderView, and as a
> result it cannot access the mojo bindings necessary to run that test.
> 
> 
> But I'm not sure how we can have a RenderView without a RenderFrame.

Renderer process 1 contains the RenderView and the main RenderFrame. Renderer
process 2 contains just a subframe RenderFrame.

[Charlie Reis, 2017-01-23 23:50:05]

On 2017/01/23 23:47:39, Sam McNally wrote:
> On 2017/01/23 23:36:40, dcheng wrote:
> > Sorry, one more question. In the CL description, it says:
> > 
> >  With site isolation, there is one layout test
> > where a RenderFrame ends up in a process without a RenderView, and as a
> > result it cannot access the mojo bindings necessary to run that test.
> > 
> > 
> > But I'm not sure how we can have a RenderView without a RenderFrame.
> 
> Renderer process 1 contains the RenderView and the main RenderFrame. Renderer
> process 2 contains just a subframe RenderFrame.

Daniel and I are confused how that's happening.  Process 2 should still have a
RenderView for the OOPIF RenderFrame, accessible via render_view().  Is that not
the case?

[Sam McNally, 2017-01-24 00:24:36]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since, all RenderViews are granted these bindings in layout tests, this
was sufficient as long as all frames share a renderer process with at
least one RenderView. With site isolation, there is one layout test
where a RenderFrame ends up in a process without a RenderView, and as a
result it cannot access the mojo bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being in the same renderer process as their parent frames.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being in the same renderer process as their parent frames.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2017-01-24 00:32:21]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being in the same renderer process as their parent frames.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being created in a renderer where some global state has already been
set.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Sam McNally, 2017-01-24 00:33:39]

On 2017/01/23 23:50:05, Charlie Reis wrote:
> On 2017/01/23 23:47:39, Sam McNally wrote:
> > On 2017/01/23 23:36:40, dcheng wrote:
> > > Sorry, one more question. In the CL description, it says:
> > > 
> > >  With site isolation, there is one layout test
> > > where a RenderFrame ends up in a process without a RenderView, and as a
> > > result it cannot access the mojo bindings necessary to run that test.
> > > 
> > > 
> > > But I'm not sure how we can have a RenderView without a RenderFrame.
> > 
> > Renderer process 1 contains the RenderView and the main RenderFrame.
Renderer
> > process 2 contains just a subframe RenderFrame.
> 
> Daniel and I are confused how that's happening.  Process 2 should still have a
> RenderView for the OOPIF RenderFrame, accessible via render_view().  Is that
not
> the case?

OK, it turns out that was inaccurate, due to insufficient observations. Digging
a little further, the actual problem seems to be that a RenderFrame checks the
allowed bindings before the AllowBindings IPC reaches the RenderView. The
RenderView notifies the main frame about new bindings so that works correctly.
In-process iframes seem to be created late enough that they don't experience
this problem. Updated the CL description.

[dcheng, 2017-01-24 19:53:55]

Talked with creis@. Some high-level notes:

Logically, it feels a bit backwards to have "state shared by everything on the
page" replicated to all the frames. It seemed much more logically
straightforward when the frames would just delegate to the page-level state.

creis@ pointed out that any page-level state would still have to be managed by
the render frame host (of the main frame) though, as binding grants can be
changed by navigations. So in some sense, replicating the state downwards to
children makes sense as well.

Perhaps we can do something better in the future, but LGTM.

[Sam McNally, 2017-01-24 23:12:01]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2017-01-24 23:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
sky@chromium.org, nyquist@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2566583002/#ps440001
(title: " ")

[commit-bot: I haz the power, 2017-01-24 23:15:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 01:53:38]

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

[commit-bot: I haz the power, 2017-01-25 01:53:40]

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

[Sam McNally, 2017-01-25 04:35:30]

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

[commit-bot: I haz the power, 2017-01-25 04:36:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:43:46]

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

[commit-bot: I haz the power, 2017-01-25 04:43:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2017-01-25 04:45:33]

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

[commit-bot: I haz the power, 2017-01-25 04:45:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 05:46:37]

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

[commit-bot: I haz the power, 2017-01-25 05:46:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sam McNally, 2017-01-29 22:48:41]

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

[commit-bot: I haz the power, 2017-01-29 22:48:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 00:44:17]

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

[commit-bot: I haz the power, 2017-01-30 00:44:19]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-01-30 00:49:24]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2017-01-30 00:49:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org, nyquist@chromium.org, creis@chromium.org,
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/2566583002/#ps480001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-30 00:49:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 00:53:55]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1485737364657030,
"parent_rev": "b84d9d8be2e7f07b9e53e8d6243b7d41557fd77c", "commit_rev":
"7f6c6a0b64b8d4df8cb476d88bc582109c0d6bff"}

[commit-bot: I haz the power, 2017-01-30 00:54:32]

Description was changed from

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being created in a renderer where some global state has already been
set.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Change allowed bindings to be per RenderFrame instead of per RenderView.

Previously, extra JS bindings were granted on a per-RenderView basis.
One of the binding types is access to mojo JS bindings for layout tests.
To allow access to these bindings in subframes, RenderFrame was changed
to inherit whatever bindings were allowed at a process-wide level.
Since all RenderViews are granted these bindings in layout tests, this
was sufficient as long as the frame checks for allowed bindings after
a RenderView has received the IPC granting it bindings for the test or
is a main frame, since main frames are notified by the RenderView when
bindings are granted. With site isolation, there is one layout test
where a RenderFrame checks for bindings before any RenderView receives
the binding-granting IPC, and as a result it cannot access the mojo
bindings necessary to run that test.

This CL fixes this problem by moving management of these bindings to be
per-RenderFrame. To maintain the existing behavior, subframes are
granted the same bindings as their parent frames. The difference is that
now the browser is responsible, so it no longer relies on subframes
being created in a renderer where some global state has already been
set.

BUG=611838
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2566583002
Cr-Commit-Position: refs/heads/master@{#446950}
Committed:
https://chromium.googlesource.com/chromium/src/+/7f6c6a0b64b8d4df8cb476d88bc5...
==========

[commit-bot: I haz the power, 2017-01-30 00:54:35]

Committed patchset #20 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/7f6c6a0b64b8d4df8cb476d88bc5...