[Experimental] Fool around with exposing extension bindings via blink

chrome/browser/extensions/api/developer_private/developer_private_api.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_EXTENSIONS_API_DEVELOPER_PRIVATE_DEVELOPER_PRIVATE_API_H_
 #define CHROME_BROWSER_EXTENSIONS_API_DEVELOPER_PRIVATE_DEVELOPER_PRIVATE_API_H_
 
 #include <set>
 
 #include "base/files/file.h"
@@ skipping 10 matching lines @@
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
 #include "chrome/browser/extensions/pack_extension_job.h"
 #include "chrome/common/extensions/api/developer_private.h"
 #include "chrome/common/extensions/webstore_install_result.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_prefs_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
+#include "extensions/browser/mojo_extension_function.h"
 #include "extensions/browser/process_manager_observer.h"
 #include "extensions/browser/warning_service.h"
+#include "extensions/common/api/mojo/developer_private.mojom.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_operation.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
 
 class Profile;
 
 namespace extensions {
 
 class EventRouter;
 class ExtensionError;
@@ skipping 277 matching lines @@
     : public DeveloperPrivateAPIFunction {
  public:
   DECLARE_EXTENSION_FUNCTION("developerPrivate.updateProfileConfiguration",
                              DEVELOPERPRIVATE_UPDATEPROFILECONFIGURATION);
 
  private:
   ~DeveloperPrivateUpdateProfileConfigurationFunction() override;
   ResponseAction Run() override;
 };
 
 class DeveloperPrivateUpdateExtensionConfigurationFunction

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

Why does this even still need to exist?

[Devlin, 2016/06/20 17:49:23]

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?

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

- 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?
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]

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.
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).
Permissions scare me (more below), as does the scope of the project.
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 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]

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

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.
More on this below.
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 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.
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]

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

-    : public DeveloperPrivateAPIFunction {
+    : public MojoExtensionFunction<
+          void,
+          mojom::UpdateExtensionConfigurationParamsPtr> {
  public:
-  DECLARE_EXTENSION_FUNCTION("developerPrivate.updateExtensionConfiguration",
-                             DEVELOPERPRIVATE_UPDATEEXTENSIONCONFIGURATION);
+  DeveloperPrivateUpdateExtensionConfigurationFunction(
+      const mojo::Callback<void()>& callback);
+  RunResult Run(mojom::UpdateExtensionConfigurationParamsPtr) override;
 
  protected:
   ~DeveloperPrivateUpdateExtensionConfigurationFunction() override;
-  ResponseAction Run() override;
 };
 
 class DeveloperPrivateReloadFunction : public DeveloperPrivateAPIFunction {
  public:
   DECLARE_EXTENSION_FUNCTION("developerPrivate.reload",
                              DEVELOPERPRIVATE_RELOAD);
 
  protected:
   ~DeveloperPrivateReloadFunction() override;
 
@@ skipping 259 matching lines @@
  protected:
   ~DeveloperPrivateUpdateExtensionCommandFunction() override;
   ResponseAction Run() override;
 };
 
 }  // namespace api
 
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_EXTENSIONS_API_DEVELOPER_PRIVATE_DEVELOPER_PRIVATE_API_H_