Enable builtin Mojo JS modules in layout tests

Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002

TBR=dbeam@chromium.org for WebUI

Committed: https://crrev.com/8df5c724f1df87044980ff8b6618d367608bc4cf
Cr-Commit-Position: refs/heads/master@{#371470}

[Ken Rockot(use gerrit already), 2015-11-23 23:29:55]

Description was changed from

==========
Enable Mojo EDK JS bindings in content_shell

Second in a series of patches to expose Mojo bindings and service
overrides to layout tests
==========

to

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available along with WebUI bindings.

They're now available with WebUI bindings as well as to any
non-guest view in content_shell.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

[Ken Rockot(use gerrit already), 2015-11-23 23:35:47]

Description was changed from

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available along with WebUI bindings.

They're now available with WebUI bindings as well as to any
non-guest view in content_shell.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

to

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available in conjunction with WebUI bindings.

While still implied by WebUI bindings availability, the EDK
modules are here made available to all non-guest views in
content_shell as well.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

[Charlie Reis, 2015-11-24 17:35:59]

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

[Charlie Reis, 2015-11-24 17:36:00]

I know this isn't ready for review yet, but I just wanted to post one question
while it's in progress.  Feel free to correct me if I've misunderstood.

https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:783: // been used for
non-privileged views.
Drive-by comment: Can you add a similar check for the new BINDINGS_POLICY_MOJO
as well?  It seems like we should avoid granting privileges to processes with
arbitrary web content.  (Or is there a different meaning to these bindings?)

There's a similar check in
NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL, if it's possible to
tell whether a URL is acceptable for having Mojo bindings.

[Ken Rockot(use gerrit already), 2015-11-24 18:09:15]

On 2015/11/24 at 17:36:00, creis wrote:
> I know this isn't ready for review yet, but I just wanted to post one question
while it's in progress.  Feel free to correct me if I've misunderstood.

Actually, it is ready for review now :)  Thanks for looking in any case.

> 
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> content/browser/renderer_host/render_view_host_impl.cc:783: // been used for
non-privileged views.
> Drive-by comment: Can you add a similar check for the new BINDINGS_POLICY_MOJO
as well?  It seems like we should avoid granting privileges to processes with
arbitrary web content.  (Or is there a different meaning to these bindings?)
> 
> There's a similar check in
NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL, if it's possible to
tell whether a URL is acceptable for having Mojo bindings.

I don't think it makes sense to treat Mojo bindings as a privileged feature. The
main reason not to expose them everywhere is to avoid polluting every context
with gin globals (i.e. define) and to avoid web content ever relying on Chrome
implementation details by using Mojo directly. In practice anything which is
available through Mojo bindings is already available to render frames through
some other mechanism so AFAICT there are no real interesting security
consequences to making them available.

It is true however that we never want to expose them to arbitrary web content in
a real browser regardless of security implications. Right now this is guaranteed
in practice because they can only be enabled in WebUI contexts or in
content_shell. Can you think of a more explicit way to enforce that policy?

One thing I could do is add another ContentBrowserClient method (e.g.
"CheckBindingsPolicyForView") which allows the embedder to validate the flags
given to RenderViewHostImpl::AllowBindings. Chrome's implementation could
essentially CHECK(!(flags & BINDINGS_POLICY_MOJO) || (flags &
BINDINGS_POLICY_WEB_UI))

[Charlie Reis, 2015-11-24 22:30:50]

Description was changed from

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available in conjunction with WebUI bindings.

While still implied by WebUI bindings availability, the EDK
modules are here made available to all non-guest views in
content_shell as well.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

to

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available in conjunction with WebUI bindings.

While still implied by WebUI bindings availability, the EDK
modules are here made available to all non-guest views in
content_shell as well.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

[Charlie Reis, 2015-11-24 22:35:13]

Thanks, but I think I have more questions than answers now.  :)  CC'ing Nick and
Nasko, since they're both puzzled as well.  (And Tom, who might be more familiar
with the Mojo security plans than we are.)

Questions below.  Let me know if a VC would make it easier to clarify things, or
if it's better to take this discussion to a separate thread.


On 2015/11/24 18:09:15, Ken Rockot (OOO slow til 12-4) wrote:
> On 2015/11/24 at 17:36:00, creis wrote:
> > I know this isn't ready for review yet, but I just wanted to post one
question
> while it's in progress.  Feel free to correct me if I've misunderstood.
> 
> Actually, it is ready for review now :)  Thanks for looking in any case.
> 
> > 
> >
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > content/browser/renderer_host/render_view_host_impl.cc:783: // been used for
> non-privileged views.
> > Drive-by comment: Can you add a similar check for the new
BINDINGS_POLICY_MOJO
> as well?  It seems like we should avoid granting privileges to processes with
> arbitrary web content.  (Or is there a different meaning to these bindings?)
> > 
> > There's a similar check in
> NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL, if it's possible to
> tell whether a URL is acceptable for having Mojo bindings.
> 
> I don't think it makes sense to treat Mojo bindings as a privileged feature.
The
> main reason not to expose them everywhere is to avoid polluting every context
> with gin globals (i.e. define) and to avoid web content ever relying on Chrome
> implementation details by using Mojo directly. In practice anything which is
> available through Mojo bindings is already available to render frames through
> some other mechanism so AFAICT there are no real interesting security
> consequences to making them available.

Mojo bindings are definitely relevant for security in some cases, right?  WebUI
is using them, DOMDistiller is using them in a way that shouldn't be exposed to
web content, etc.  Are you saying that Mojo bindings can also be used for
non-security-sensitive features, and thus we should look for some other property
when making security decisions?

> It is true however that we never want to expose them to arbitrary web content
in
> a real browser regardless of security implications. Right now this is
guaranteed
> in practice because they can only be enabled in WebUI contexts or in
> content_shell. Can you think of a more explicit way to enforce that policy?

This is where I get confused.  Why would we never want to expose them to
arbitrary web content, given the paragraph above?  If they're risky, it seems
like we should go with least privilege and avoid granting them to processes with
arbitrary web content in the first place.

Also, why does content_shell need them but not Chrome?


> One thing I could do is add another ContentBrowserClient method (e.g.
> "CheckBindingsPolicyForView") which allows the embedder to validate the flags
> given to RenderViewHostImpl::AllowBindings. Chrome's implementation could
> essentially CHECK(!(flags & BINDINGS_POLICY_MOJO) || (flags &
> BINDINGS_POLICY_WEB_UI))

Why would Chrome differ from content_shell here?  It's generally not ok for
content_shell to be insecure.

In general, I've been hoping to find a way for the process model to tell when a
process has been granted new privileges via Mojo (e.g., like DOMDistiller). 
We've got lots of CHECKs like these for WebUI, and they're not as effective now
that new APIs are being implemented without WebUI.  I could certainly believe
that BINDINGS_POLICY_MOJO isn't what I'm looking for, but do you know what would
be?  (Maybe a particular registry?)

Apologies that I don't have enough context here.  Feel free to point me to
design docs that explain these things if there are some.  Thanks!

[Ken Rockot(use gerrit already), 2015-11-25 01:28:30]

On 2015/11/24 at 22:35:13, creis wrote:
> Thanks, but I think I have more questions than answers now.  :)  CC'ing Nick
and Nasko, since they're both puzzled as well.  (And Tom, who might be more
familiar with the Mojo security plans than we are.)
> 
> Questions below.  Let me know if a VC would make it easier to clarify things,
or if it's better to take this discussion to a separate thread.
> 
> 
> On 2015/11/24 18:09:15, Ken Rockot (OOO slow til 12-4) wrote:
> > On 2015/11/24 at 17:36:00, creis wrote:
> > > I know this isn't ready for review yet, but I just wanted to post one
question
> > while it's in progress.  Feel free to correct me if I've misunderstood.
> > 
> > Actually, it is ready for review now :)  Thanks for looking in any case.
> > 
> > > 
> > >
> >
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > > content/browser/renderer_host/render_view_host_impl.cc:783: // been used
for
> > non-privileged views.
> > > Drive-by comment: Can you add a similar check for the new
BINDINGS_POLICY_MOJO
> > as well?  It seems like we should avoid granting privileges to processes
with
> > arbitrary web content.  (Or is there a different meaning to these bindings?)
> > > 
> > > There's a similar check in
> > NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL, if it's possible
to
> > tell whether a URL is acceptable for having Mojo bindings.
> > 
> > I don't think it makes sense to treat Mojo bindings as a privileged feature.
The
> > main reason not to expose them everywhere is to avoid polluting every
context
> > with gin globals (i.e. define) and to avoid web content ever relying on
Chrome
> > implementation details by using Mojo directly. In practice anything which is
> > available through Mojo bindings is already available to render frames
through
> > some other mechanism so AFAICT there are no real interesting security
> > consequences to making them available.
> 
> Mojo bindings are definitely relevant for security in some cases, right? 
WebUI is using them, DOMDistiller is using them in a way that shouldn't be
exposed to web content, etc.  Are you saying that Mojo bindings can also be used
for non-security-sensitive features, and thus we should look for some other
property when making security decisions?

These bindings essentially expose two independent things, but either is
effectively useless without the other:

 1. Mojo EDK API. Lets JS do things like create message pipes and read/write to
their endpoints. In isolation this isn't very interesting and only lets a script
talk to itself using non-standard async primitives.

 2. A ServiceRegistry interface which can be used to connect one end of a
message pipe to a real service implementation somewhere.

I would argue that #1 is not in itself very interesting from a security POV.

#2 clearly is, but I believe security decisions should be made at a layer lower
than bindings. Essentially, RFH is in control of determining which Mojo browser
services a RenderFrame's ServiceRegistry can connect to. AFAIcT it would be just
as bad if renderer C++ code could connect to a privileged service from an
unprivileged frame. i.e., do we not guard against a non-WebUI-enabled
RenderFrame connecting to WebUI Mojo services (this should be done by the
browser-side service implementation(s) using the RFH for context)?

If no, that's bad already; if yes, adding JS bindings for ServiceRegsitry is not
adding badness.

> 
> > It is true however that we never want to expose them to arbitrary web
content in
> > a real browser regardless of security implications. Right now this is
guaranteed
> > in practice because they can only be enabled in WebUI contexts or in
> > content_shell. Can you think of a more explicit way to enforce that policy?
> 
> This is where I get confused.  Why would we never want to expose them to
arbitrary web content, given the paragraph above?  If they're risky, it seems
like we should go with least privilege and avoid granting them to processes with
arbitrary web content in the first place.
> 

The main reason not to do expose this stuff to web content is the same reason we
don't expose any other arbitrary Chrome-only APIs to the open web (except of
course when we do, but I think it's generally considered an Unfortunate Thing).

> Also, why does content_shell need them but not Chrome?

The motivation is specifically to let layout tests mock out Mojo services
directly. One increasingly common pattern for web API implementation is:

Blink platform -> content/renderer -> [mojo service pipe] -> browser service

In general, all the content/renderer glue in these cases is consists of asking
RenderFrameImpl to connect to 1 or more Mojo services, and translating public
web layer interfaces into requests over those Mojo message pipes. (Geolocation,
permissions API, WebUSB are some examples of this).

In a not-so-distant future we should be able to cut out content/renderer
completely and have Blink platform code request connections to Mojo services
through an interface that would opaquely be provided by RenderFrameImpl. So:

Blink platform -> [mojo service pipe] -> browser service

In either case these service pipes are established by asking ServiceRegistry
(via IPC) to connect a pipe endpoint to a remote service impl on the RFH.

One way to support service mocks for layout tests would be to intercept these
connection requests in C++ code by wedging some kind of override support into
content_shell. Then every API has to implement their own mock framework in C++,
requiring them to deal with JS bindings details, compile/link iteration, etc.

The idea here is to reduce all that pain by exposing such an override interface
directly to the test code. So I can do something like:

// setup code
mockFrobinator = new MockFrobinatorImpl();
mojo.serviceRegistry.addServiceOverride("Frobinator", pipe => {
  mockFrobinator.bindToPipe(pipe);
});

promise_test(() => {
  // We know the implementation just connects to a "Frobinator" service on the
frame's
  // ServiceRegistry. As far as the Blink/content implementation knows it's
talking to
  // a real Frobinator service, but it's really talking to the mock above.
  navigator.frobinator.frobinate();
});

> 
> 
> > One thing I could do is add another ContentBrowserClient method (e.g.
> > "CheckBindingsPolicyForView") which allows the embedder to validate the
flags
> > given to RenderViewHostImpl::AllowBindings. Chrome's implementation could
> > essentially CHECK(!(flags & BINDINGS_POLICY_MOJO) || (flags &
> > BINDINGS_POLICY_WEB_UI))
> 
> Why would Chrome differ from content_shell here?  It's generally not ok for
content_shell to be insecure.

I was under the impression that content_shell was first and foremost the engine
which runs layout tests. Are there other uses in production?

For what it's worth, if there's a general policy to keep content_shell contexts
looking as much like a real browser as possible, I can understand this being an
undesirable path forward. Surely we do inject some test interfaces into
content_shell frames though, don't we?

> 
> In general, I've been hoping to find a way for the process model to tell when
a process has been granted new privileges via Mojo (e.g., like DOMDistiller). 
We've got lots of CHECKs like these for WebUI, and they're not as effective now
that new APIs are being implemented without WebUI.  I could certainly believe
that BINDINGS_POLICY_MOJO isn't what I'm looking for, but do you know what would
be?  (Maybe a particular registry?)
> 
> Apologies that I don't have enough context here.  Feel free to point me to
design docs that explain these things if there are some.  Thanks!

No worries at all, these are obviously good questions. I'm not familiar with a
lot of this stuff so this is only a first attempt to get it right. I hope my
explanations above are helpful (and not too verbose!)

[Charlie Reis, 2015-11-26 00:33:34]

On 2015/11/25 01:28:30, Ken Rockot (OOO slow til 12-4) wrote:
> On 2015/11/24 at 22:35:13, creis wrote:
> > Thanks, but I think I have more questions than answers now.  :)  CC'ing Nick
> and Nasko, since they're both puzzled as well.  (And Tom, who might be more
> familiar with the Mojo security plans than we are.)
> > 
> > Questions below.  Let me know if a VC would make it easier to clarify
things,
> or if it's better to take this discussion to a separate thread.
> > 
> > 
> > On 2015/11/24 18:09:15, Ken Rockot (OOO slow til 12-4) wrote:
> > > On 2015/11/24 at 17:36:00, creis wrote:
> > > > I know this isn't ready for review yet, but I just wanted to post one
> question
> > > while it's in progress.  Feel free to correct me if I've misunderstood.
> > > 
> > > Actually, it is ready for review now :)  Thanks for looking in any case.
> > > 
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > > > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1470823002/diff/80001/content/browser/rendere...
> > > > content/browser/renderer_host/render_view_host_impl.cc:783: // been used
> for
> > > non-privileged views.
> > > > Drive-by comment: Can you add a similar check for the new
> BINDINGS_POLICY_MOJO
> > > as well?  It seems like we should avoid granting privileges to processes
> with
> > > arbitrary web content.  (Or is there a different meaning to these
bindings?)
> > > > 
> > > > There's a similar check in
> > > NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL, if it's possible
> to
> > > tell whether a URL is acceptable for having Mojo bindings.
> > > 
> > > I don't think it makes sense to treat Mojo bindings as a privileged
feature.
> The
> > > main reason not to expose them everywhere is to avoid polluting every
> context
> > > with gin globals (i.e. define) and to avoid web content ever relying on
> Chrome
> > > implementation details by using Mojo directly. In practice anything which
is
> > > available through Mojo bindings is already available to render frames
> through
> > > some other mechanism so AFAICT there are no real interesting security
> > > consequences to making them available.
> > 
> > Mojo bindings are definitely relevant for security in some cases, right? 
> WebUI is using them, DOMDistiller is using them in a way that shouldn't be
> exposed to web content, etc.  Are you saying that Mojo bindings can also be
used
> for non-security-sensitive features, and thus we should look for some other
> property when making security decisions?
> 
> These bindings essentially expose two independent things, but either is
> effectively useless without the other:
> 
>  1. Mojo EDK API. Lets JS do things like create message pipes and read/write
to
> their endpoints. In isolation this isn't very interesting and only lets a
script
> talk to itself using non-standard async primitives.
> 
>  2. A ServiceRegistry interface which can be used to connect one end of a
> message pipe to a real service implementation somewhere.
> 
> I would argue that #1 is not in itself very interesting from a security POV.
> 
> #2 clearly is, but I believe security decisions should be made at a layer
lower
> than bindings. Essentially, RFH is in control of determining which Mojo
browser
> services a RenderFrame's ServiceRegistry can connect to. AFAIcT it would be
just
> as bad if renderer C++ code could connect to a privileged service from an
> unprivileged frame. i.e., do we not guard against a non-WebUI-enabled
> RenderFrame connecting to WebUI Mojo services (this should be done by the
> browser-side service implementation(s) using the RFH for context)?
> 
> If no, that's bad already; if yes, adding JS bindings for ServiceRegsitry is
not
> adding badness.

Thanks, that clarifies things.  It's the ServiceRegistry in the RenderFrameHost
we need to be careful about.  And you're asking exactly the question I'm curious
about-- how do we guard access to those?  Not only do we need to prevent
non-WebUI RenderFrames from getting to a ServiceRegistry (hopefully by using
their RFH as context), but we need to prevent non-WebUI RenderFrames from being
in the same process as WebUI RenderFrames (since they could compromise the
process and use the routing ID of the WebUI RenderFrame).  I don't know if we
have that today or not.  And importantly, similar checks for cases like
DOMDistiller, etc, which likely have ServiceRegistries but aren't WebUI.

Given that context, perhaps we should start a separate conversation off this CL,
since I agree the bindings themselves aren't the thing to check for.

> 
> > 
> > > It is true however that we never want to expose them to arbitrary web
> content in
> > > a real browser regardless of security implications. Right now this is
> guaranteed
> > > in practice because they can only be enabled in WebUI contexts or in
> > > content_shell. Can you think of a more explicit way to enforce that
policy?
> > 
> > This is where I get confused.  Why would we never want to expose them to
> arbitrary web content, given the paragraph above?  If they're risky, it seems
> like we should go with least privilege and avoid granting them to processes
with
> arbitrary web content in the first place.
> > 
> 
> The main reason not to do expose this stuff to web content is the same reason
we
> don't expose any other arbitrary Chrome-only APIs to the open web (except of
> course when we do, but I think it's generally considered an Unfortunate
Thing).
> 
> > Also, why does content_shell need them but not Chrome?
> 
> The motivation is specifically to let layout tests mock out Mojo services
> directly. One increasingly common pattern for web API implementation is:
> 
> Blink platform -> content/renderer -> [mojo service pipe] -> browser service
> 
> In general, all the content/renderer glue in these cases is consists of asking
> RenderFrameImpl to connect to 1 or more Mojo services, and translating public
> web layer interfaces into requests over those Mojo message pipes.
(Geolocation,
> permissions API, WebUSB are some examples of this).
> 
> In a not-so-distant future we should be able to cut out content/renderer
> completely and have Blink platform code request connections to Mojo services
> through an interface that would opaquely be provided by RenderFrameImpl. So:
> 
> Blink platform -> [mojo service pipe] -> browser service
> 
> In either case these service pipes are established by asking ServiceRegistry
> (via IPC) to connect a pipe endpoint to a remote service impl on the RFH.
> 
> One way to support service mocks for layout tests would be to intercept these
> connection requests in C++ code by wedging some kind of override support into
> content_shell. Then every API has to implement their own mock framework in
C++,
> requiring them to deal with JS bindings details, compile/link iteration, etc.
> 
> The idea here is to reduce all that pain by exposing such an override
interface
> directly to the test code. So I can do something like:
> 
> // setup code
> mockFrobinator = new MockFrobinatorImpl();
> mojo.serviceRegistry.addServiceOverride("Frobinator", pipe => {
>   mockFrobinator.bindToPipe(pipe);
> });
> 
> promise_test(() => {
>   // We know the implementation just connects to a "Frobinator" service on the
> frame's
>   // ServiceRegistry. As far as the Blink/content implementation knows it's
> talking to
>   // a real Frobinator service, but it's really talking to the mock above.
>   navigator.frobinator.frobinate();
> });
> 
> > 
> > 
> > > One thing I could do is add another ContentBrowserClient method (e.g.
> > > "CheckBindingsPolicyForView") which allows the embedder to validate the
> flags
> > > given to RenderViewHostImpl::AllowBindings. Chrome's implementation could
> > > essentially CHECK(!(flags & BINDINGS_POLICY_MOJO) || (flags &
> > > BINDINGS_POLICY_WEB_UI))
> > 
> > Why would Chrome differ from content_shell here?  It's generally not ok for
> content_shell to be insecure.
> 
> I was under the impression that content_shell was first and foremost the
engine
> which runs layout tests. Are there other uses in production?

I think I was confusing other shipping code that uses content/ but not chrome/. 
For example, Chromecast uses cast_shell, which is not the same thing as
content_shell (but which lacks chrome/).

> 
> For what it's worth, if there's a general policy to keep content_shell
contexts
> looking as much like a real browser as possible, I can understand this being
an
> undesirable path forward. Surely we do inject some test interfaces into
> content_shell frames though, don't we?
> 
> > 
> > In general, I've been hoping to find a way for the process model to tell
when
> a process has been granted new privileges via Mojo (e.g., like DOMDistiller). 
> We've got lots of CHECKs like these for WebUI, and they're not as effective
now
> that new APIs are being implemented without WebUI.  I could certainly believe
> that BINDINGS_POLICY_MOJO isn't what I'm looking for, but do you know what
would
> be?  (Maybe a particular registry?)
> > 
> > Apologies that I don't have enough context here.  Feel free to point me to
> design docs that explain these things if there are some.  Thanks!
> 
> No worries at all, these are obviously good questions. I'm not familiar with a
> lot of this stuff so this is only a first attempt to get it right. I hope my
> explanations above are helpful (and not too verbose!)

Yes, thanks!

[Ken Rockot(use gerrit already), 2016-01-14 23:02:50]

I'd like to try to resurrect this CL. If you'd like to meet to discuss Mojo
security implications, that would be fine with me. Keep in mind that in this
case the only goal (and the intended effect of this CL) is to expose these
services to the layout test runner, explicitly for the purpose of mocking
services in test JS code.

If there are better steps we can take here to more convincingly ensure this
exposure doesn't creep beyond that boundary, I think the broader question of the
security model for Mojo services in Chrome is a bit out of scope. WDYT?

[Charlie Reis, 2016-01-15 01:26:51]

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

[Charlie Reis, 2016-01-15 01:26:52]

Yes, we can postpone the larger security discussion.  Let's try to find time for
that, but it doesn't have to be right now.

One question for John on the public API below.  It might also make sense to have
John review from a Mojo perspective.

I'm slightly concerned about other places that might have assumed
enabled_bindings_ would be 0 by default.  I haven't audited them yet, so maybe I
can find some time.  (Did you audit them yourself, or were you just fixing the
tests that failed?)

Otherwise it seems fine.

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

https://codereview.chromium.org/1470823002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:725: virtual int
GetDefaultEnabledBindingsForView(RenderViewHost* view);
Hmm, no one outside content/ is using this.  John, does content shell count as
"outside content" in terms of adding public APIs like this?

https://codereview.chromium.org/1470823002/diff/80001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/1470823002/diff/80001/content/public/common/b...
content/public/common/bindings_policy.h:25: // Bindings that allows the JS
context to access Mojo EDK modules and use a
Please elaborate what EDK means in the comment.  I don't think most content/
folks (myself included) know what it is.

https://codereview.chromium.org/1470823002/diff/80001/content/shell/browser/s...
File content/shell/browser/shell_content_browser_client.cc (right):

https://codereview.chromium.org/1470823002/diff/80001/content/shell/browser/s...
content/shell/browser/shell_content_browser_client.cc:349: 
nit: Remove extra line.

[jam, 2016-01-15 02:25:15]

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

https://codereview.chromium.org/1470823002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:725: virtual int
GetDefaultEnabledBindingsForView(RenderViewHost* view);
On 2016/01/15 01:26:51, Charlie Reis wrote:
> Hmm, no one outside content/ is using this.  John, does content shell count as
> "outside content" in terms of adding public APIs like this?

correct. content/shell can only use content/public (so that it can be a good
example of embedding content)

https://codereview.chromium.org/1470823002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:725: virtual int
GetDefaultEnabledBindingsForView(RenderViewHost* view);
On 2016/01/15 01:26:51, Charlie Reis wrote:
> Hmm, no one outside content/ is using this.  John, does content shell count as
> "outside content" in terms of adding public APIs like this?

Why isn't this on WebContentsDelegate? ContentBrowserClient is a last-resort
place to put methods, to avoid making it even larger, where a more focused
delegate interface exists.

[Ken Rockot(use gerrit already), 2016-01-24 22:34:04]

Description was changed from

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling core
Mojo EDK modules to a render view. Previously these were only
made available in conjunction with WebUI bindings.

While still implied by WebUI bindings availability, the EDK
modules are here made available to all non-guest views in
content_shell as well.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

to

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are here made available to all views created
in layout tests.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

[Ken Rockot(use gerrit already), 2016-01-24 22:34:42]

Description was changed from

==========
Enable Mojo EDK JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are here made available to all views created
in layout tests.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

to

==========
Enable builtin Mojo JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are here made available to all views created
in layout tests.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

[Ken Rockot(use gerrit already), 2016-01-24 22:55:46]

Description was changed from

==========
Enable builtin Mojo JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are here made available to all views created
in layout tests.

Part of a CL series to enable Mojo service mocks in layout tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1464373002
 4. https://codereview.chromium.org/1470153002
==========

to

==========
Enable builtin Mojo JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002
==========

[Ken Rockot(use gerrit already), 2016-01-24 23:03:51]

Thanks for looking. Please take another look when you get a chance. I simplified
quite a bit and so decided to merge in another CL to move builtin Mojo module
resolution out of WebUI-specific code. Also added a layout test to verify Mojo
bindings availability.

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

https://codereview.chromium.org/1470823002/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:725: virtual int
GetDefaultEnabledBindingsForView(RenderViewHost* view);
On 2016/01/15 at 02:25:15, jam wrote:
> On 2016/01/15 01:26:51, Charlie Reis wrote:
> > Hmm, no one outside content/ is using this.  John, does content shell count
as
> > "outside content" in terms of adding public APIs like this?
> 
> Why isn't this on WebContentsDelegate? ContentBrowserClient is a last-resort
place to put methods, to avoid making it even larger, where a more focused
delegate interface exists.

Actually that's not even necessary either. I've gone an even simpler (and IMHO
safer) route and explicitly add the bindings from content_shell's
WebContentsDelegate if and only if --run-layout-test is set.

https://codereview.chromium.org/1470823002/diff/80001/content/public/common/b...
File content/public/common/bindings_policy.h (right):

https://codereview.chromium.org/1470823002/diff/80001/content/public/common/b...
content/public/common/bindings_policy.h:25: // Bindings that allows the JS
context to access Mojo EDK modules and use a
On 2016/01/15 at 01:26:51, Charlie Reis wrote:
> Please elaborate what EDK means in the comment.  I don't think most content/
folks (myself included) know what it is.

Done.

[Ken Rockot(use gerrit already), 2016-01-24 23:10:03]

Description was changed from

==========
Enable builtin Mojo JS modules in content_shell

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002
==========

to

==========
Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002
==========

[jam, 2016-01-25 22:00:54]

On 2016/01/24 23:03:51, Ken Rockot wrote:
> Thanks for looking. Please take another look when you get a chance. I
simplified
> quite a bit and so decided to merge in another CL to move builtin Mojo module
> resolution out of WebUI-specific code. Also added a layout test to verify Mojo
> bindings availability.

looks like there's no public API changes to content (i.e. the reason that
Charlie added me originally), so nothing for me to add here. Charlie is a better
reviewer as is for the bindings policy addition.

[Ken Rockot(use gerrit already), 2016-01-25 23:47:34]

OK. Charlie, WDYT? The new bindings policy is now only added when content_shell
is run with --run-layout-test.

[Charlie Reis, 2016-01-26 01:01:02]

content/ LGTM, with a few small questions below.

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/mojo_...
File content/renderer/mojo_context_state.cc (right):

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/mojo_...
content/renderer/mojo_context_state.cc:57: scoped_refptr<base::RefCountedMemory>
GetBuildinModuleData(
nit: Builtin?

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:2367: (BINDINGS_POLICY_WEB_UI |
BINDINGS_POLICY_MOJO))) {
Looks right, but would one of your tests catch it if there were a typo in here? 
(Seems like an easy place for a typo to slip by.)  I'm guessing the layout test
would?

https://codereview.chromium.org/1470823002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/1470823002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:5984:
method define
You'll need another reviewer for this part.  (The "webexposed" part of this file
name worries me, but I'm guessing your CL isn't introducing a new global.)

[Ken Rockot(use gerrit already), 2016-01-26 01:32:55]

rockot@chromium.org changed reviewers:
+ tkent@chromium.org
- jam@chromium.org

[Ken Rockot(use gerrit already), 2016-01-26 01:32:56]

Thanks! tkent@ Could you please take a look at blink changes? I do see other
test-only interfaces global interface expectations, but I'm probably missing
something.

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/mojo_...
File content/renderer/mojo_context_state.cc (right):

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/mojo_...
content/renderer/mojo_context_state.cc:57: scoped_refptr<base::RefCountedMemory>
GetBuildinModuleData(
On 2016/01/26 at 01:01:02, Charlie Reis wrote:
> nit: Builtin?

Woops, fixed

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1470823002/diff/160001/content/renderer/rende...
content/renderer/render_view_impl.cc:2367: (BINDINGS_POLICY_WEB_UI |
BINDINGS_POLICY_MOJO))) {
On 2016/01/26 at 01:01:02, Charlie Reis wrote:
> Looks right, but would one of your tests catch it if there were a typo in
here?  (Seems like an easy place for a typo to slip by.)  I'm guessing the
layout test would?

Yeah - the layout tests cover the affirmative case (i.e. it's set when it should
be). There are already existing tests for WebUI which test the negative case,
i.e. that no extra bindings flags are set when WebUI is not enabled.

I think we'd have to add a separate test here if we ever wanted to expose Mojo
bindings beyond WebUI and layout tests, but I seriously doubt we'll ever do
that.

[tkent, 2016-01-26 02:15:05]

tkent@chromium.org changed reviewers:
+ creis@chromium.org>
- creis@chromium.org

[tkent, 2016-01-26 02:15:07]

> Enable builtin Mojo JS modules in layout tests

Why Mojo should be available in layout tests?

https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:5984:
method define
What's this?

[Ken Rockot(use gerrit already), 2016-01-26 02:19:04]

On 2016/01/26 at 02:15:07, tkent wrote:
> > Enable builtin Mojo JS modules in layout tests
> 
> Why Mojo should be available in layout tests?

Because we've started building API implementations on top of Mojo services. This
lets us mock those services from within layout test JS instead of having to
write different bindings to support mocks for each API.

> 
>
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):
> 
>
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:5984:
method define
> What's this?

Mojo modules are built on the gin module system which exposes 'define' as the
entry point for loading registered modules async.

[Ken Rockot(use gerrit already), 2016-01-26 02:21:15]

On 2016/01/26 at 02:19:04, Ken Rockot wrote:
> On 2016/01/26 at 02:15:07, tkent wrote:
> > > Enable builtin Mojo JS modules in layout tests
> > 
> > Why Mojo should be available in layout tests?
> 
> Because we've started building API implementations on top of Mojo services.
This lets us mock those services from within layout test JS instead of having to
write different bindings to support mocks for each API.
> 
> > 
> >
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):
> > 
> >
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:5984:
method define
> > What's this?
> 
> Mojo modules are built on the gin module system which exposes 'define' as the
entry point for loading registered modules async.


For a concrete example, https://codereview.chromium.org/1472693004 is a sample
CL which partially mocks the browser's USB device service. The WebUSB team would
like to use these bindings to support their layout tests.

[tkent, 2016-01-26 03:59:11]

ok, I understand the motivation.

> >
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> > File
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):
> > 
> >
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-expected.txt:5984:
method define
> > What's this?
> 
> Mojo modules are built on the gin module system which exposes 'define' as the
entry point for loading registered modules async.

Why is this the global 'define'?  Adding such generic word to the global scope
will be confusing.  ECMAScript module will use 'define', and will they conflict?
 Can you rename it?

[tkent, 2016-01-26 04:01:03]

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

[tkent, 2016-01-26 04:01:04]

https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/mojo-api.html (right):

https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/mojo-api.html:1: <!DOCTYPE html>
mojo is not a web-platform feature.  So, please don't make LayoutTests/mojo/
directory.

IMO, we don't need a test for this.  If you want to add a test, please add to
LayoutTests/harness-tests/.

[Ken Rockot(use gerrit already), 2016-01-26 04:28:38]

OK - I'll move it to a global 'mojo' or 'mojoDefine' or somesuch.

On 2016/01/26 at 04:01:04, tkent wrote:
>
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/mojo/mojo-api.html (right):
> 
>
https://codereview.chromium.org/1470823002/diff/180001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/mojo/mojo-api.html:1: <!DOCTYPE html>
> mojo is not a web-platform feature.  So, please don't make LayoutTests/mojo/
directory.
> 
> IMO, we don't need a test for this.  If you want to add a test, please add to
LayoutTests/harness-tests/.

That's fair. I would still like to establish mojo-helpers.js as a general
purpose support library for Mojo service layout tests. Where should that live?

[Ken Rockot(use gerrit already), 2016-01-26 04:57:20]

I've moved the global to 'mojo' instead; test helpers moved to
LayoutTests/resources and the small test moved to LayoutTests/harness-tests.
Please take another look. Thanks!

[tkent, 2016-01-26 05:13:04]

lgtm

[Ken Rockot(use gerrit already), 2016-01-26 05:34:24]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-01-26 05:34:24]

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/1470823002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2016-01-26 05:35:20]

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

[commit-bot: I haz the power, 2016-01-26 05:45:05]

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

[commit-bot: I haz the power, 2016-01-26 05:45:06]

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...)

[Ken Rockot(use gerrit already), 2016-01-26 05:53:32]

Description was changed from

==========
Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002
==========

to

==========
Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002

TBR=dbeam@chromium.org for WebUI
==========

[Ken Rockot(use gerrit already), 2016-01-26 05:53:35]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-01-26 05:53:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1470823002/#ps240001
(title: "oops, clumsy")

[commit-bot: I haz the power, 2016-01-26 05:53:52]

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

[commit-bot: I haz the power, 2016-01-26 07:15:24]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-01-26 07:16:44]

Description was changed from

==========
Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling 
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002

TBR=dbeam@chromium.org for WebUI
==========

to

==========
Enable builtin Mojo JS modules in layout tests

This adds a new BindingsPolicy exclusively for enabling
Mojo system API and ServiceRegistry modules to a render
view. Previously these were only made available in
conjunction with WebUI bindings.

While still implied by WebUI bindings availability, Mojo
modules are now also available to any view created during
a layout test run.

Part of a CL series to enable Mojo service mocks in layout
tests:

 1. https://codereview.chromium.org/1468903002
 2. this CL
 3. https://codereview.chromium.org/1470153002

TBR=dbeam@chromium.org for WebUI

Committed: https://crrev.com/8df5c724f1df87044980ff8b6618d367608bc4cf
Cr-Commit-Position: refs/heads/master@{#371470}
==========

[commit-bot: I haz the power, 2016-01-26 07:16:45]

Patchset 13 (id:??) landed as
https://crrev.com/8df5c724f1df87044980ff8b6618d367608bc4cf
Cr-Commit-Position: refs/heads/master@{#371470}