[Experimental] Fool around with exposing extension bindings via blink

[Experimental] Fool around with exposing extension bindings via blink

NOT FOR COMMIT.  Rough, experimental, partial implementation

BUG=

[Devlin, 2016-06-18 01:24:58]

rdevlin.cronin@chromium.org changed reviewers:
+ esprehn@chromium.org, rockot@chromium.org

[Devlin, 2016-06-18 01:24:59]

Hey folks - I'm curious for some early thoughts here.  This is me hacking some
stuff together to see what happens.  In particular:
esprehn@ - I'd like to know what you think of the general approach and the
feasibility of having much of this code be generated, rather than hand-written.
rockot@ - Does this seem generally mojo-friendly and sane?
lazyboy@, asargent@ - just FYI to keep you in the loop.

There's also a super-barebones design doc at
https://docs.google.com/document/d/1iVp0M8WtnjT1xF3F2d1j3G_WYaUwt9EcaeT_iioQV....

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
File extensions/common/api/mojo/developer_private.mojom (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/developer_private.mojom:21: interface
DeveloperPrivate {
Ideally, this interface (and corresponding structs) would be generated.

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/extensions/DEPS (right):

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/extensions/DEPS:3: "+extensions/renderer"
Adding this to the upper DEPS level freaks things out because of include rules. 
We could probably make this a separate build target, but I'm curious what you
would think of having the modules live in //extensions (since that's where all
related code is and would make much of this a lot easier).

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/extensions/DeveloperPrivate.cpp (right):

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/extensions/DeveloperPrivate.cpp:19:
DeveloperPrivate::DeveloperPrivate(LocalFrame* frame)
Ideally, since all this file does is forward to a mojo pipe, it could be
generated.

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/extensions/UpdateExtensionConfigurationParams.idl
(right):

https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/extensions/UpdateExtensionConfigurationParams.idl:5:
dictionary UpdateExtensionConfigurationParams {
It would be helpful if we could put this definition in the DeveloperPrivate.idl
file.

[Devlin, 2016-06-18 01:25:21]

Description was changed from

==========
[Experimental] Fool around with exposing extension bindings via blink

BUG=
==========

to

==========
[Experimental] Fool around with exposing extension bindings via blink

NOT FOR COMMIT.  Rough, experimental, partial implementation

BUG=
==========

[Ken Rockot(use gerrit already), 2016-06-20 17:19:25]

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
Why does this even still need to exist?

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_mojo.cc
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_mojo.cc:21:
const UpdateExtensionConfigurationCallback& callback) {
Why not just implement the function here and run the callback here?

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_mojo.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_mojo.h:18:
class DeveloperPrivateMojo : public mojom::DeveloperPrivate {
Sticking "Mojo" suffixes onto things is an unfortunate pattern that has been
adopted in some places. I'd really like to stop it from spreading. Impl would be
reasonable, or just no suffix at all.

Though also see comments about the interface name in the mojom file.

https://codereview.chromium.org/2074263002/diff/1/extensions/browser/mojo_ext...
File extensions/browser/mojo_extension_function.h (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/browser/mojo_ext...
extensions/browser/mojo_extension_function.h:18: class MojoExtensionFunctionBase
I'm not sure you really want to do this. We should not need an
ExtensionFunction-like thing at all when implementing APIs in terms of mojo
interfaces.

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
File extensions/common/api/mojo/BUILD.gn (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/BUILD.gn:4: 
I would strongly prefer that we not keep introducing "mojo" directories. How
about extensions/public/interfaces?

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/BUILD.gn:7: mojom_files = [ "developer_private.mojom"
]
This variable seems unnecessary. Just inline the sources list?

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
File extensions/common/api/mojo/developer_private.mojom (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/developer_private.mojom:9: bool has_file_access;
Another possible pattern here would be to define constants for flags like:

struct UpdateExtensionConfigurationParams {
  const uint16 kFlagFileAccess = 1;
  const uint16 kFlagIncognitoAccess = 2;
  const uint16 kFlagErrorCollection = 4;
  // ...

  string extension_id;
  uint16 flags_update_mask;
  uint16 flags;
};

Your call on what you'd prefer. In this case my main hesitation about
"foo"/"has_foo" is that the flags themselves deal with capabilities, so
"has_foo" sounds like it could be referring to the capability "foo" rather than
the presence of the capability flag "foo".

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/developer_private.mojom:21: interface
DeveloperPrivate {
On 2016/06/18 at 01:24:59, Devlin wrote:
> Ideally, this interface (and corresponding structs) would be generated.

Generated from what, the IDL? I don't think we really want there to be a 1:1
correspondence between component mojo interfaces and extension APIs which
consume them. We started down that road when first looking at extensions-on-mojo
and it didn't end up making much sense in practice.

In general, interfaces needed to implement extension APIs should be useful
beyond the scope of only the extensions platform. There may be exceptions to
that rule but I'd expect them to be few in number.

This API of course seems like an exception since it's explicitly dealing with
extensions features, but it's unclear to me that it'd be worth generating mojom
from IDL just for the few cases like this.


Also, the name seems pretty confusing for a mojo interface. I don't think it
always needs to match an extension API name, and in this case I don't think it
should. What about something like:

interface Configuration {
  Update(bool from_user_gesture, ConfigurationUpdateParams params) => ();
};

Note, I dropped Extension from names because this already lives in
extensions::mojom. WDYT?

[Devlin, 2016-06-20 17:49:24]

just to reiterate, I have no intention of landing this in its current
incarnation, so we shouldn't worry about naming type things just yet. :)

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 17:19:24, Ken Rockot wrote:
> Why does this even still need to exist?

A few reasons.
- If you look at the delta in the code, it's pretty small.  That means that the
ability to convert these en masse is much higher than if we re-implement them
entirely.  I like that.
- There's a *bunch* of bookkeeping we need to do for each extension function
call - check permissions, log call and performance histograms, increment keep
alive as necessary, etc.  Right now, this is almost all done in
ExtensionFunctionDispatcher, but I think it'd probably just move to
MojoExtensionFunction::Run(), which can call PreRun() or Bookkeeping() or
whatever we call it.  That way, we don't risk people not calling it in a new API
(which I'm pretty sure would happen).
- I'm hopeful that we could even generate the DeveloperPrivateMojo file, since
all it needs to do is do a call.  This reduces the amount of code necessary
since more of the boilerplate is taken care of.
- A lot of async functions need to store some state, so having an OO approach
really does make things cleaner, rather than needing to store everything on the
API.

Is there a reason to *not* have this pattern?

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_mojo.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_mojo.h:18:
class DeveloperPrivateMojo : public mojom::DeveloperPrivate {
On 2016/06/20 17:19:24, Ken Rockot wrote:
> Sticking "Mojo" suffixes onto things is an unfortunate pattern that has been
> adopted in some places. I'd really like to stop it from spreading. Impl would
be
> reasonable, or just no suffix at all.
> 
> Though also see comments about the interface name in the mojom file.

Names are TBD (read: hacky things that I threw together and figured sounded good
enough for the rough patch :)).  I agree that we shouldn't have "Mojo" stuff,
because this should be the API itself (so it should probably just be called
DeveloperPrivateAPI).

https://codereview.chromium.org/2074263002/diff/1/extensions/browser/mojo_ext...
File extensions/browser/mojo_extension_function.h (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/browser/mojo_ext...
extensions/browser/mojo_extension_function.h:18: class MojoExtensionFunctionBase
On 2016/06/20 17:19:24, Ken Rockot wrote:
> I'm not sure you really want to do this. We should not need an
> ExtensionFunction-like thing at all when implementing APIs in terms of mojo
> interfaces.

See other comment.

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
File extensions/common/api/mojo/developer_private.mojom (right):

https://codereview.chromium.org/2074263002/diff/1/extensions/common/api/mojo/...
extensions/common/api/mojo/developer_private.mojom:21: interface
DeveloperPrivate {
On 2016/06/20 17:19:25, Ken Rockot wrote:
> On 2016/06/18 at 01:24:59, Devlin wrote:
> > Ideally, this interface (and corresponding structs) would be generated.
> 
> Generated from what, the IDL? I don't think we really want there to be a 1:1
> correspondence between component mojo interfaces and extension APIs which
> consume them. We started down that road when first looking at
extensions-on-mojo
> and it didn't end up making much sense in practice.
> 
> In general, interfaces needed to implement extension APIs should be useful
> beyond the scope of only the extensions platform. There may be exceptions to
> that rule but I'd expect them to be few in number.
> 
> This API of course seems like an exception since it's explicitly dealing with
> extensions features, but it's unclear to me that it'd be worth generating
mojom
> from IDL just for the few cases like this.
> 
> 
> Also, the name seems pretty confusing for a mojo interface. I don't think it
> always needs to match an extension API name, and in this case I don't think it
> should. What about something like:
> 
> interface Configuration {
>   Update(bool from_user_gesture, ConfigurationUpdateParams params) => ();
> };
> 
> Note, I dropped Extension from names because this already lives in
> extensions::mojom. WDYT?

I think there's multiple scopes here.  What I'm mostly worried about in the
short-term is a "simple" conversion to mojo and blink bindings, which should
still be a *huge* win in terms of security and performance.  I agree that, in
the long run, we should rearchitect the whole system and make interfaces that
are consumable for multiple uses, at which point maybe this pattern becomes less
useful (or we just implement one in terms of the other).  But that's a project
that a) is being worked on by other teams and b) is related but not entirely
parallel to this one.

Note also that no matter what we do, it may well be that we always need an
endpoint for extensions because we need to do all the permissions
checks/bookkeeping/etc (which can't be done in the renderer for security
reasons).  So we could never just say in the renderer:
BlinkTabsCreate() {
  TabsService::Create();
}
There would always have to be an intermediary step there.  So I think more
likely (and compatible with this step) would be:
BlinkTabsCreate() {
  TabsApi::Create();
}

TabsApi::Create() {
  <check stuff>
  TabService::Create();
}

[Ken Rockot(use gerrit already), 2016-06-20 18:34:54]

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 at 17:49:23, Devlin wrote:
> On 2016/06/20 17:19:24, Ken Rockot wrote:
> > Why does this even still need to exist?
> 
> A few reasons.
> - If you look at the delta in the code, it's pretty small.  That means that
the ability to convert these en masse is much higher than if we re-implement
them entirely.  I like that.
> - There's a *bunch* of bookkeeping we need to do for each extension function
call 
- check permissions, log call and performance histograms, increment keep alive
as necessary, etc.  Right now, this is almost all done in
ExtensionFunctionDispatcher, but I think it'd probably just move to
MojoExtensionFunction::Run(), which can call PreRun() or Bookkeeping() or
whatever we call it.  That way, we don't risk people not calling it in a new API
(which I'm pretty sure would happen).

I think we should prefer to do histogram logging and keepalive from render-side
code attached to the bindings, if at all possible. There can be a generic
extensions::mojom::KeepAlive interface which renderer-side bindings code can use
to control keepalive behavior (in fact IIRC this was already implemented by
sammc a while ago), and it doesn't seem like it should be a problem to log
histogram data for calls in the renderer rather than in the browser, does it?

I see one of the major advantages of using Mojo interfaces here being that you
can get rid of tons of extensions code in the browser. While I sympathize with
the desire to do this conversion incrementally and not boil the ocean, it seems
like going this route would create significantly more work in the long run.

It might not be obvious or trivial to accomplish in some cases but I do believe
it's possible that we can have extension APIs defined entirely in Blink which
consume interfaces from the browser whose implementations know nothing about
extensions or extension APIs. The vast majority of these interfaces will expose
features that could be consumed by other clients, and they're only relevant to
extensions insofar as extensions expose one public API surface (and a generic
permissions layer) to get at them.

I'd like us to try very hard to get to that place, and that would mean not
requiring explicit browser-side support for every extension API and function.
Does this not seem feasible?

> - I'm hopeful that we could even generate the DeveloperPrivateMojo file, since
all it needs to do is do a call.  This reduces the amount of code necessary
since more of the boilerplate is taken care of.
> - A lot of async functions need to store some state, so having an OO approach
really does make things cleaner, rather than needing to store everything on the
API.
> 
> Is there a reason to *not* have this pattern?

It seems like unnecessary complexity. You're receiving an IPC, creating a
ref-counted impl object, calling it synchronously to do work for you, and then
deleting it. Maybe it will make sense to establish a common pattern in cases
where you have to persist state during an async callback, but this isn't one of
those cases.

As far as permission checking goes, I'd expect the renderer to be unable to
acquire an interface connection in the first place if it didn't have permission
to use a given interface. That's an interesting problem in itself but it seems
like we could avoid having boilerplate impl-side support if permissions were
handled at a higher layer when possible.

[Devlin, 2016-06-20 18:58:41]

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 18:34:54, Ken Rockot wrote:
> I think we should prefer to do histogram logging and keepalive from
render-side
> code attached to the bindings, if at all possible. There can be a generic
> extensions::mojom::KeepAlive interface which renderer-side bindings code can
use
> to control keepalive behavior (in fact IIRC this was already implemented by
> sammc a while ago), and it doesn't seem like it should be a problem to log
> histogram data for calls in the renderer rather than in the browser, does it?
Renderer-side lifetime management has caused a ton of issues for us, and I'd
like to kill it. :)  Generally, relying on renderers for this type of
information is bad, and we're already moving a bunch of logic to the browser
both in extensions and elsewhere (e.g., PlzNavigate) to avoid it.

> I see one of the major advantages of using Mojo interfaces here being that you
> can get rid of tons of extensions code in the browser. While I sympathize with
> the desire to do this conversion incrementally and not boil the ocean, it
seems
> like going this route would create significantly more work in the long run.
Is there an advantage to moving extensions code from the browser to the
renderer?  I can see advantages in untangling it from chrome and having it in a
separate module, but unless it's only doing renderer work, I can't really see
the advantage in not having it in the browser (or, more generically, a trusted
process).

> It might not be obvious or trivial to accomplish in some cases but I do
believe
> it's possible that we can have extension APIs defined entirely in Blink which
> consume interfaces from the browser whose implementations know nothing about
> extensions or extension APIs. The vast majority of these interfaces will
expose
> features that could be consumed by other clients, and they're only relevant to
> extensions insofar as extensions expose one public API surface (and a generic
> permissions layer) to get at them.
> I'd like us to try very hard to get to that place, and that would mean not
> requiring explicit browser-side support for every extension API and function.
> Does this not seem feasible?
Permissions scare me (more below), as does the scope of the project.

> It seems like unnecessary complexity. You're receiving an IPC, creating a
> ref-counted impl object, calling it synchronously to do work for you, and then
> deleting it. Maybe it will make sense to establish a common pattern in cases
> where you have to persist state during an async callback, but this isn't one
of
> those cases.
Receiving an IPC will almost certainly be necessary in all cases, no?  And
creating/destroying an object really isn't a con (we have plenty of
stack-allocated objects to do work that are created/destroyed).  The refcounting
is a bit of a shame, but really only matters when we need it.

> As far as permission checking goes, I'd expect the renderer to be unable to
> acquire an interface connection in the first place if it didn't have
permission
> to use a given interface. That's an interesting problem in itself but it seems
> like we could avoid having boilerplate impl-side support if permissions were
> handled at a higher layer when possible.
I can see how this would be possible with a 1:1 mapping between services and
apis, but if you have a general service which is used for multiple generic
things and an extension has permission to do only some of them, how do you
control whether or not the extension gets a port?  And renderer-side permission
checking is definitely not sufficient.  So it seems like we'll need some
permissions component somewhere anyway, right?  In the eventual world, it could
be that the extensions api services become permission check -> send to the right
mojo service, but it seems like we'll likely still need some piece of that
somewhere.

Finally, there really is the practicality aspect to this.  I don't want
converting to blink bindings to be blocked on rewriting 100+ APIs to use generic
mojo services (which, in no exaggeration, largely boils down to rewriting the
whole browser if we want to do it The Right Way from the start).  If you feel
very strongly that we shouldn't have an incremental step that converts to mojo
here, then we could hypothetically do the blink binding conversion and have it
send over the single monolithic IPC (ExtensionMsg_Request), but that really does
seem like a shame to me.

[Ken Rockot(use gerrit already), 2016-06-20 19:54:20]

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 at 18:58:41, Devlin wrote:
> On 2016/06/20 18:34:54, Ken Rockot wrote:
> > I think we should prefer to do histogram logging and keepalive from
render-side
> > code attached to the bindings, if at all possible. There can be a generic
> > extensions::mojom::KeepAlive interface which renderer-side bindings code can
use
> > to control keepalive behavior (in fact IIRC this was already implemented by
> > sammc a while ago), and it doesn't seem like it should be a problem to log
> > histogram data for calls in the renderer rather than in the browser, does
it?
> Renderer-side lifetime management has caused a ton of issues for us, and I'd
like to kill it. :)  Generally, relying on renderers for this type of
information is bad, and we're already moving a bunch of logic to the browser
both in extensions and elsewhere (e.g., PlzNavigate) to avoid it.

Hmm. I'm not sure I understand the problem. Is it really more reliable to do:

R            B
Call ->
          -> Call
             +keepalive
             CallImpl()
             -keepalive
          <- reply
reply <-



than it is to do



R               B
+keepalive ->
Call ->
             -> +keepalive
             -> Call
                CallImpl()
             <- reply
reply <-
-keepalive ->
             -> -keepalive


The latter does rely on the renderer sending extra messages, but it seems no
more difficult to bake this into renderer-side helpers than it would be to bake
it into browser-side helpers.

> 
> > I see one of the major advantages of using Mojo interfaces here being that
you
> > can get rid of tons of extensions code in the browser. While I sympathize
with
> > the desire to do this conversion incrementally and not boil the ocean, it
seems
> > like going this route would create significantly more work in the long run.
> Is there an advantage to moving extensions code from the browser to the
renderer?  I can see advantages in untangling it from chrome and having it in a
separate module, but unless it's only doing renderer work, I can't really see
the advantage in not having it in the browser (or, more generically, a trusted
process).

Not so much an advantage to that move (browser-to-renderer) in itself, but more
generally in reducing the total volume of code that exists. If you have an
interface impl in some browser component which can be consumed by various
clients unprivileged or otherwise, ideally it wouldn't need to know anything
about extensions or extensions functions in order to lend its functionality to
the extensions system. At worst the extensions layer in the browser might need
to provide a lightweight facade interface to acquire the component's
interface(s) subject to some additional extensions-specific capability
constraints. But the core interface implementation itself could remain unaware
of the extensions system, and in many cases that facade wouldn't be necessary at
all.

> 
> > It might not be obvious or trivial to accomplish in some cases but I do
believe
> > it's possible that we can have extension APIs defined entirely in Blink
which
> > consume interfaces from the browser whose implementations know nothing about
> > extensions or extension APIs. The vast majority of these interfaces will
expose
> > features that could be consumed by other clients, and they're only relevant
to
> > extensions insofar as extensions expose one public API surface (and a
generic
> > permissions layer) to get at them.
> > I'd like us to try very hard to get to that place, and that would mean not
> > requiring explicit browser-side support for every extension API and
function.
> > Does this not seem feasible?
> Permissions scare me (more below), as does the scope of the project.
> 
> > It seems like unnecessary complexity. You're receiving an IPC, creating a
> > ref-counted impl object, calling it synchronously to do work for you, and
then
> > deleting it. Maybe it will make sense to establish a common pattern in cases
> > where you have to persist state during an async callback, but this isn't one
of
> > those cases.
> Receiving an IPC will almost certainly be necessary in all cases, no?  And
creating/destroying an object really isn't a con (we have plenty of
stack-allocated objects to do work that are created/destroyed).  The refcounting
is a bit of a shame, but really only matters when we need it.

I just don't see the point in this particular case. This function class isn't
used by anything else. It's essentially taking logic which could have been
written inline, and moving it into its own ExtensionFunction object to be called
it from here. Of course if there were a bunch of boilerplate browser-side logic
that needed to run for every extension function, then I could see the point of
this. But again, I'd like to see if we can avoid that.

> 
> > As far as permission checking goes, I'd expect the renderer to be unable to
> > acquire an interface connection in the first place if it didn't have
permission
> > to use a given interface. That's an interesting problem in itself but it
seems
> > like we could avoid having boilerplate impl-side support if permissions were
> > handled at a higher layer when possible.
> I can see how this would be possible with a 1:1 mapping between services and
apis, but if you have a general service which is used for multiple generic
things and an extension has permission to do only some of them, how do you
control whether or not the extension gets a port?  And renderer-side permission
checking is definitely not sufficient.  So it seems like we'll need some
permissions component somewhere anyway, right?  In the eventual world, it could
be that the extensions api services become permission check -> send to the right
mojo service, but it seems like we'll likely still need some piece of that
somewhere.

I am not suggesting renderer-side permission checking. I'm suggesting that
(extensions) renderer code asks for interface pipes through a mechanism (maybe
similar to ServiceRegistry) which (browser-side) knows about the renderer and
about extension permissions, and which can decide whether or not to bind the
pipe to an implementation. There will likely be cases where this alone is not
sufficient because of more granular extension permissions (e.g. sockets APIs
being able to bind only certain addresses) but this can still be managed with a
layered approach, e.g., there would still be a generic network socket interface
implemented by //net or //services/network or something, but it wouldn't be
exposed to extensions APIs directly; instead there'd be an extensions-specific
service which was permission-aware and could vend socket pipes to the underlying
network service as permissions allowed. The important win there is that the
majority of the actual browser-side implementation would be in the network
service itself (which would also be consumed by blink and other system
components) and have no knowledge of extensions.

> 
> Finally, there really is the practicality aspect to this.  I don't want
converting to blink bindings to be blocked on rewriting 100+ APIs to use generic
mojo services (which, in no exaggeration, largely boils down to rewriting the
whole browser if we want to do it The Right Way from the start).  If you feel
very strongly that we shouldn't have an incremental step that converts to mojo
here, then we could hypothetically do the blink binding conversion and have it
send over the single monolithic IPC (ExtensionMsg_Request), but that really does
seem like a shame to me.

I 100% agree that we should do something incremental, but why do you think it's
a shame to separate bindings conversion from IPC conversion? That seems like a
logical separation to me: the blink bindings conversion can be done completely
independently by using a monolithic IPC surface up front, and then individual
interfaces could be brought up incrementally and exposed for the blink-side APIs
to consume. I don't think it's necessary for any of the suggested changes (e.g.
renderer-driven keepalive/reporting) to be made simultaneously for every API.

[Devlin, 2016-06-20 21:35:25]

At some point, a meeting for this might make sense. :)

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 19:54:20, Ken Rockot wrote:
> Hmm. I'm not sure I understand the problem. Is it really more reliable to do:
> 
> R            B
> Call ->
>           -> Call
>              +keepalive
>              CallImpl()
>              -keepalive
>           <- reply
> reply <-
> 
> 
> 
> than it is to do
> 
> 
> 
> R               B
> +keepalive ->
> Call ->
>              -> +keepalive
>              -> Call
>                 CallImpl()
>              <- reply
> reply <-
> -keepalive ->
>              -> -keepalive
> 
> 
> The latter does rely on the renderer sending extra messages, but it seems no
> more difficult to bake this into renderer-side helpers than it would be to
bake
> it into browser-side helpers.

Yes, it is more reliable, because we don't have to deal with things like the
renderer crashing, navigating, getting swapped out, etc, and never decrementing
the count.  This could be solved by watching the frames that sent the keepalive
counts for these events in the browser process, but then we're still left with
the browser process doing complex keep alive counts, so there's no real win
there, IMO.

> Not so much an advantage to that move (browser-to-renderer) in itself, but
more
> generally in reducing the total volume of code that exists. If you have an
> interface impl in some browser component which can be consumed by various
> clients unprivileged or otherwise, ideally it wouldn't need to know anything
> about extensions or extensions functions in order to lend its functionality to
> the extensions system. At worst the extensions layer in the browser might need
> to provide a lightweight facade interface to acquire the component's
> interface(s) subject to some additional extensions-specific capability
> constraints. But the core interface implementation itself could remain unaware
> of the extensions system, and in many cases that facade wouldn't be necessary
at
> all.

More on this below.

> I just don't see the point in this particular case. This function class isn't
> used by anything else. It's essentially taking logic which could have been
> written inline, and moving it into its own ExtensionFunction object to be
called
> it from here. Of course if there were a bunch of boilerplate browser-side
logic
> that needed to run for every extension function, then I could see the point of
> this. But again, I'd like to see if we can avoid that.
In the long run, I could see this going either way.  My guess is that we'll
always have some kind of boilerplate browserside, and that there will always be
some functions that need to store state, etc.  Which solution is the right one
isn't really clear, and the answer is probably that any of them will work.  That
said, I have a strong preference for not completely rewriting these, so that
preference extends to using the current idiom that we've always had, and largely
works.

> I am not suggesting renderer-side permission checking. I'm suggesting that
> (extensions) renderer code asks for interface pipes through a mechanism (maybe
> similar to ServiceRegistry) which (browser-side) knows about the renderer and
> about extension permissions, and which can decide whether or not to bind the
> pipe to an implementation. There will likely be cases where this alone is not
> sufficient because of more granular extension permissions (e.g. sockets APIs
> being able to bind only certain addresses) but this can still be managed with
a
> layered approach, e.g., there would still be a generic network socket
interface
> implemented by //net or //services/network or something, but it wouldn't be
> exposed to extensions APIs directly; instead there'd be an extensions-specific
> service which was permission-aware and could vend socket pipes to the
underlying
> network service as permissions allowed. The important win there is that the
> majority of the actual browser-side implementation would be in the network
> service itself (which would also be consumed by blink and other system
> components) and have no knowledge of extensions.

I think I tried to describe this a couple of times.  What I would imagine would
be:
Step 1: something like this - mojo extension functions that do the same thing
they always did.
Step 2: Mojofy and componetize all of chrome and rewrite extension functions to
use those services.

I'm all for having clean, reusable interfaces throughout all of chrome, but I
don't think it should block us transitioning to mojo.  Also, given that in many
of these scenarios we still have some kind of thin, extensiony layer (for
permission checking, transforming arguments/calls to the types expected by the
mojo service, etc), I don't think that they are contradictory.

> I 100% agree that we should do something incremental, but why do you think
it's
> a shame to separate bindings conversion from IPC conversion? That seems like a
> logical separation to me: the blink bindings conversion can be done completely
> independently by using a monolithic IPC surface up front, and then individual
> interfaces could be brought up incrementally and exposed for the blink-side
APIs
> to consume. I don't think it's necessary for any of the suggested changes
(e.g.
> renderer-driven keepalive/reporting) to be made simultaneously for every API.

Being able to use mojo would have a very real performance impact.  We would be
able to completely skip several serializations, and I think (esprehn@ would know
for sure) that blink/v8 do some awesomeness with sharing the same data, so we
don't have to even make many copies on the renderer side (impossible with
serialization).  This would take out a good chunk of the work done by each API
call, and would be fantastically useful in the case of e.g. messaging, where
very long strings may be passed and performing copies is quite expensive. 
Additionally, the transition to blink bindings is a prime time to do this, since
the handlers would already accept individual arguments (rather than a v8 array).
 It just seems such a shame to pass up a significant performance (and in many
ways, readability) improvement when it's relatively simple (once boilerplate is
in place).

[Ken Rockot(use gerrit already), 2016-06-20 22:36:34]

A meeting does probably make sense. As a general high-level comment, I think it
*could* be reasonable to start with something like what you want, i.e.
auto-generating blink IDL/C++ and mojom from extensions IDL, having a 1:1
mapping from extension function to mojo method, and having some helpers to make
it easy to wire up an ExtensionFunction to a mojo binding method impl. I only
worry that even this approach is quite a lot of work to adopt everywhere, and I
don't think it reflects what the system should look like in the long term.

IMHO it doesn't really make sense to optimize the new system's design based on
what will be quickest/easiest to adopt everywhere, since individual API
conversions shouldn't really be blocked on any aspect of the new system being
adopted everywhere. If improved performance is the big win you're targeting up
front, maybe it would make sense to identify the APIs that are most performance
critical, most widely used, etc, in the wild and focus on converting those
first?

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/2074263002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/developer_private/developer_private_api.h:332:
class DeveloperPrivateUpdateExtensionConfigurationFunction
On 2016/06/20 at 21:35:25, Devlin wrote:
> On 2016/06/20 19:54:20, Ken Rockot wrote:
> > Hmm. I'm not sure I understand the problem. Is it really more reliable to
do:
> > 
> > R            B
> > Call ->
> >           -> Call
> >              +keepalive
> >              CallImpl()
> >              -keepalive
> >           <- reply
> > reply <-
> > 
> > 
> > 
> > than it is to do
> > 
> > 
> > 
> > R               B
> > +keepalive ->
> > Call ->
> >              -> +keepalive
> >              -> Call
> >                 CallImpl()
> >              <- reply
> > reply <-
> > -keepalive ->
> >              -> -keepalive
> > 
> > 
> > The latter does rely on the renderer sending extra messages, but it seems no
> > more difficult to bake this into renderer-side helpers than it would be to
bake
> > it into browser-side helpers.
> 
> Yes, it is more reliable, because we don't have to deal with things like the
renderer crashing, navigating, getting swapped out, etc, and never decrementing
the count.  This could be solved by watching the frames that sent the keepalive
counts for these events in the browser process, but then we're still left with
the browser process doing complex keep alive counts, so there's no real win
there, IMO.

If the keepalive is only used to keep the process alive, process crashes don't
seem like a relevant concern.

The frame lifetime issue is interesting, but then is there really any sane
reason to maintain a keepalive count after navigation anyway? Shouldn't
keepalive count reset on navigation regardless of any outstanding requests? And
if a new keepalive pipe is created per navigation, there's no issues with races
or complicated lifetime tracking in the browser.

To be totally fair, I might be overlooking other complexity here. I just want to
make sure we aren't artificially limiting the design of the system due to
imagined complications which aren't as bad as they first seemed. I catch myself
doing this often enough, so maybe I'm just projecting. ;)

> 
> > Not so much an advantage to that move (browser-to-renderer) in itself, but
more
> > generally in reducing the total volume of code that exists. If you have an
> > interface impl in some browser component which can be consumed by various
> > clients unprivileged or otherwise, ideally it wouldn't need to know anything
> > about extensions or extensions functions in order to lend its functionality
to
> > the extensions system. At worst the extensions layer in the browser might
need
> > to provide a lightweight facade interface to acquire the component's
> > interface(s) subject to some additional extensions-specific capability
> > constraints. But the core interface implementation itself could remain
unaware
> > of the extensions system, and in many cases that facade wouldn't be
necessary at
> > all.
> 
> More on this below.
> 
> > I just don't see the point in this particular case. This function class
isn't
> > used by anything else. It's essentially taking logic which could have been
> > written inline, and moving it into its own ExtensionFunction object to be
called
> > it from here. Of course if there were a bunch of boilerplate browser-side
logic
> > that needed to run for every extension function, then I could see the point
of
> > this. But again, I'd like to see if we can avoid that.
> In the long run, I could see this going either way.  My guess is that we'll
always have some kind of boilerplate browserside, and that there will always be
some functions that need to store state, etc.  Which solution is the right one
isn't really clear, and the answer is probably that any of them will work.  That
said, I have a strong preference for not completely rewriting these, so that
preference extends to using the current idiom that we've always had, and largely
works.

I'd like to understand this preference better. Again I'm trying to emphasize
code reuse here and I believe that most browser-side code supporting extensions
APIs will ultimately be reusable by other parts of the system and shouldn't need
to be exclusive to or dependent upon the extensions system.

Having a 1:1 correspondence between API surface and IPC surface (well, sort of -
let's say, public API surface and browser API surface) seems like an artifact of
the old system's design, which doesn't scale particularly well, and which isn't
necessarily worth persisting just for its own sake.

> 
> > I am not suggesting renderer-side permission checking. I'm suggesting that
> > (extensions) renderer code asks for interface pipes through a mechanism
(maybe
> > similar to ServiceRegistry) which (browser-side) knows about the renderer
and
> > about extension permissions, and which can decide whether or not to bind the
> > pipe to an implementation. There will likely be cases where this alone is
not
> > sufficient because of more granular extension permissions (e.g. sockets APIs
> > being able to bind only certain addresses) but this can still be managed
with a
> > layered approach, e.g., there would still be a generic network socket
interface
> > implemented by //net or //services/network or something, but it wouldn't be
> > exposed to extensions APIs directly; instead there'd be an
extensions-specific
> > service which was permission-aware and could vend socket pipes to the
underlying
> > network service as permissions allowed. The important win there is that the
> > majority of the actual browser-side implementation would be in the network
> > service itself (which would also be consumed by blink and other system
> > components) and have no knowledge of extensions.
> 
> I think I tried to describe this a couple of times.  What I would imagine
would be:
> Step 1: something like this - mojo extension functions that do the same thing
they always did.
> Step 2: Mojofy and componetize all of chrome and rewrite extension functions
to use those services.
> 
> I'm all for having clean, reusable interfaces throughout all of chrome, but I
don't think it should block us transitioning to mojo.  Also, given that in many
of these scenarios we still have some kind of thin, extensiony layer (for
permission checking, transforming arguments/calls to the types expected by the
mojo service, etc), I don't think that they are contradictory.
> 
> > I 100% agree that we should do something incremental, but why do you think
it's
> > a shame to separate bindings conversion from IPC conversion? That seems like
a
> > logical separation to me: the blink bindings conversion can be done
completely
> > independently by using a monolithic IPC surface up front, and then
individual
> > interfaces could be brought up incrementally and exposed for the blink-side
APIs
> > to consume. I don't think it's necessary for any of the suggested changes
(e.g.
> > renderer-driven keepalive/reporting) to be made simultaneously for every
API.
> 
> Being able to use mojo would have a very real performance impact.  We would be
able to completely skip several serializations, and I think (esprehn@ would know
for sure) that blink/v8 do some awesomeness with sharing the same data, so we
don't have to even make many copies on the renderer side (impossible with
serialization).  This would take out a good chunk of the work done by each API
call, and would be fantastically useful in the case of e.g. messaging, where
very long strings may be passed and performing copies is quite expensive. 
Additionally, the transition to blink bindings is a prime time to do this, since
the handlers would already accept individual arguments (rather than a v8 array).
 It just seems such a shame to pass up a significant performance (and in many
ways, readability) improvement when it's relatively simple (once boilerplate is
in place).

That's fair, but it doesn't change the fact that APIs can be converted
incrementally regardless of what approach is taken.

I don't think any of the proposed approaches require transformations to be
applied uniformly across the entire extensions system at once.

[Devlin, 2016-06-21 20:51:35]

I came back to this a day later, and I can't parse this wall of text anymore. :)
 I'll set up a meeting to chat a bit more.  A few other quick responses to your
high level comments.

> IMHO it doesn't really make sense to optimize the new system's design based on
> what will be quickest/easiest to adopt everywhere, since individual API
> conversions shouldn't really be blocked on any aspect of the new system being
> adopted everywhere. If improved performance is the big win you're targeting up
> front, maybe it would make sense to identify the APIs that are most
performance
> critical, most widely used, etc, in the wild and focus on converting those
> first?

Right.  I don't want to convert everything to this.  I want to convert the apis
that will have the biggest impact - those that are most commonly used, often
during content scripts (since those can block page loads).  Additionally, being
able to say "please use this for all future APIs" is a big benefit.  I don't
anticipate converting all 100+ apis by myself to this. :)  However, given
there's no real time frame (AFAIK) for the much larger project of converting the
browser to use entirely mojo services, I think it still makes sense to spend a
bit of time optimizing the solution for the mean time.

[Devlin, 2016-06-21 20:51:54]

esprehn@, comments from a blink bindings perspective?

[Ken Rockot(use gerrit already), 2016-06-21 20:55:21]

Just one additional comment - it seems incorrect to me to have Blink know
any details of Chrome extensions APIs. It would be nice if as an embedder
of Blink we had an easy way to use Blink IDL and other bindings facilities
without requiring all Blink embedders to build the extensions stuff.

On Tue, Jun 21, 2016 at 1:51 PM, <rdevlin.cronin@chromium.org> wrote:

> esprehn@, comments from a blink bindings perspective?
>
> https://codereview.chromium.org/2074263002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2016-06-21 20:55:21]

Just one additional comment - it seems incorrect to me to have Blink know
any details of Chrome extensions APIs. It would be nice if as an embedder
of Blink we had an easy way to use Blink IDL and other bindings facilities
without requiring all Blink embedders to build the extensions stuff.

On Tue, Jun 21, 2016 at 1:51 PM, <rdevlin.cronin@chromium.org> wrote:

> esprehn@, comments from a blink bindings perspective?
>
> https://codereview.chromium.org/2074263002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2016-06-21 20:58:54]

On 2016/06/21 20:55:21, Ken Rockot wrote:
> Just one additional comment - it seems incorrect to me to have Blink know
> any details of Chrome extensions APIs. It would be nice if as an embedder
> of Blink we had an easy way to use Blink IDL and other bindings facilities
> without requiring all Blink embedders to build the extensions stuff.

Agreed, I mention that here:
https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m....
 Though esprehn@ has pointed out to me in the past that modules/ is basically
not (traditional?) blink and easily compiled out on a per-module basis.  Even
so, I agree that this would be nicer in an extension-y spot.

[esprehn, 2016-06-21 21:11:10]

On 2016/06/21 at 20:58:54, rdevlin.cronin wrote:
> On 2016/06/21 20:55:21, Ken Rockot wrote:
> > Just one additional comment - it seems incorrect to me to have Blink know
> > any details of Chrome extensions APIs. It would be nice if as an embedder
> > of Blink we had an easy way to use Blink IDL and other bindings facilities
> > without requiring all Blink embedders to build the extensions stuff.
> 
> Agreed, I mention that here:
https://codereview.chromium.org/2074263002/diff/1/third_party/WebKit/Source/m....
 Though esprehn@ has pointed out to me in the past that modules/ is basically
not (traditional?) blink and easily compiled out on a per-module basis.  Even
so, I agree that this would be nicer in an extension-y spot.

Blink is not some reusable system, we can keep it in modules and have a separate
build target if needed, in general though there's no reason to "keep stuff out
of blink".