Support exposing Mojo services between render frames, render threads, and their respective hosts.

Support exposing Mojo services between render frames, render threads, and their respective hosts.

This introduces ServiceRegistry as an abstraction around providing
services to and accessing services from a remote peer. In particular,
this adds peered service registries to RenderProcessHost and
RenderThread, and to RenderFrameHost and RenderFrame - the RenderFrame
setup is implemented using the RenderProcessHost/RenderThread
ServiceRegistry pair.

This replaces the existing WebUI handle setup by adding a webUI
controller service to the frame host registry and a corresponding
request for the webUI controller service to the frame registry.

BUG=386155
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279557

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279623

[koz (OOO until 15th September), 2014-05-23 00:37:18]

This lg at a high level. I haven't had a super fine go over the code though.

[Sam McNally, 2014-05-25 23:46:53]

+darin

[koz (OOO until 15th September), 2014-05-26 00:54:26]

lgtm

[Sam McNally, 2014-06-12 02:47:07]

This should be ready to take another look at now.

[darin (slow to review), 2014-06-12 05:27:13]

Had you considered pushing the frame-specific InterfaceProvider down to the
renderer process upon creating of a new RenderFrameHost? Is it better to have
the RenderFrame request the InterfaceProvider as opposed to being given that
early on?

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

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:131: virtual void
ConnectToInterfaceProvider(
@davemoore and I were again discussing the names of things today. We seem to
have arrived at InterfaceProvider -> ServiceProvider because "applications vend
services" and "services implement interfaces".

I think this may be a good fit for this CL. Maybe a ServiceRegistry is a
ServiceProvider? That makes sense.

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:174: void
BindServiceRegistry(mojo::ScopedMessagePipeHandle shell_handle);
nit: shell_handle seems like the wrong parameter name here.

https://codereview.chromium.org/285333003/diff/650001/content/browser/mojo/mo...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/285333003/diff/650001/content/browser/mojo/mo...
content/browser/mojo/mojo_application_host.h:32: bool
Init(mojo::ScopedMessagePipeHandle* shell_handle);
nit: |shell_handle| doesn't seem like the right name for this one.

https://codereview.chromium.org/285333003/diff/670001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.cc (right):

https://codereview.chromium.org/285333003/diff/670001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.cc:29: "webui_controller",
perhaps instead of binding a generic "webui_controller" service, we should be
using the name of the Mojom-defined interface the webui page is intended to talk
to? e.g., "OmniboxUIHandlerMojo"

speaking of which, perhaps some HTML / JS files need to be updated to make
about:omnibox work with these changes?

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
content/public/common/service_registry.h:24: // Make the service created by
|service_factory| available to the remote
nit: Would help to clarify that the callback is run to create a new connection
to the service. The pipe handle represents that new connection.

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
content/public/common/service_registry.h:27: void AddService(const
base::Callback<void(mojo::ScopedMessagePipeHandle)>
Since we know the interface here, I wonder if it might be nice to use
InterfaceRequest<Interface> here instead of mojo::ScopedMessagePipeHandle.
Perhaps it would enable callers of AddService to be more typesafe? The downside
is that you can't just pass that callback along to the virtual AddService method
:-/ Maybe that makes it not worth it.

[Sam McNally, 2014-06-12 08:56:40]

On 2014/06/12 05:27:13, darin wrote:
> Had you considered pushing the frame-specific InterfaceProvider down to the
> renderer process upon creating of a new RenderFrameHost? Is it better to have
> the RenderFrame request the InterfaceProvider as opposed to being given that
> early on?
> 
I hadn't, and it turns out to be a bit simpler to push from the browser.

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

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:131: virtual void
ConnectToInterfaceProvider(
On 2014/06/12 05:27:12, darin wrote:
> @davemoore and I were again discussing the names of things today. We seem to
> have arrived at InterfaceProvider -> ServiceProvider because "applications
vend
> services" and "services implement interfaces".
> 
> I think this may be a good fit for this CL. Maybe a ServiceRegistry is a
> ServiceProvider? That makes sense.

Sounds reasonable, but I'd like to stick with IInterfaceProvider for now just to
avoid clashing with the old ServiceProvider.

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/285333003/diff/650001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:174: void
BindServiceRegistry(mojo::ScopedMessagePipeHandle shell_handle);
On 2014/06/12 05:27:12, darin wrote:
> nit: shell_handle seems like the wrong parameter name here.

Done.

https://codereview.chromium.org/285333003/diff/650001/content/browser/mojo/mo...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/285333003/diff/650001/content/browser/mojo/mo...
content/browser/mojo/mojo_application_host.h:32: bool
Init(mojo::ScopedMessagePipeHandle* shell_handle);
On 2014/06/12 05:27:12, darin wrote:
> nit: |shell_handle| doesn't seem like the right name for this one.

Done.

https://codereview.chromium.org/285333003/diff/670001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.cc (right):

https://codereview.chromium.org/285333003/diff/670001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.cc:29: "webui_controller",
On 2014/06/12 05:27:12, darin wrote:
> perhaps instead of binding a generic "webui_controller" service, we should be
> using the name of the Mojom-defined interface the webui page is intended to
talk
> to? e.g., "OmniboxUIHandlerMojo"
> 
> speaking of which, perhaps some HTML / JS files need to be updated to make
> about:omnibox work with these changes?

I'm planning to change it to use the auto-generated interface names in a
follow-up CL. Currently, the renderer side of the connection is set up in C++
(which doesn't know which interface the JS will want to connect to) and only the
handle is passed into the JS code. My plan is to make GetInterface available
from JS so the JS code will be able to connect directly to the browser-side
controller.

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
content/public/common/service_registry.h:24: // Make the service created by
|service_factory| available to the remote
On 2014/06/12 05:27:12, darin wrote:
> nit: Would help to clarify that the callback is run to create a new connection
> to the service. The pipe handle represents that new connection.

Done.

https://codereview.chromium.org/285333003/diff/670001/content/public/common/s...
content/public/common/service_registry.h:27: void AddService(const
base::Callback<void(mojo::ScopedMessagePipeHandle)>
On 2014/06/12 05:27:12, darin wrote:
> Since we know the interface here, I wonder if it might be nice to use
> InterfaceRequest<Interface> here instead of mojo::ScopedMessagePipeHandle.
> Perhaps it would enable callers of AddService to be more typesafe? The
downside
> is that you can't just pass that callback along to the virtual AddService
method
> :-/ Maybe that makes it not worth it.

Done.

[darin (slow to review), 2014-06-18 00:09:29]

https://codereview.chromium.org/285333003/diff/880001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.cc (right):

https://codereview.chromium.org/285333003/diff/880001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.cc:27:
render_view_host->GetProcess()->ActivateMojo();
I don't think we want to call ActivateMojo() here. This should really happen
more transparently as there could be a variety of services that would otherwise
need to call ActivateMojo(). Maybe we can do this from GetServiceRegistry()?

https://codereview.chromium.org/285333003/diff/880001/content/common/mojo/ser...
File content/common/mojo/service_registry_impl.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/common/mojo/ser...
content/common/mojo/service_registry_impl.h:33: void
Bind(mojo::ScopedMessagePipeHandle handle);
nit: You might call this method BindRemoteServiceProvider for clarity.

https://codereview.chromium.org/285333003/diff/880001/content/common/render_f...
File content/common/render_frame_setup.mojom (right):

https://codereview.chromium.org/285333003/diff/880001/content/common/render_f...
content/common/render_frame_setup.mojom:10: ConnectToServiceProvider(int32
frame_routing_id,
Hmm, how about naming this method GetServiceProvider?

https://codereview.chromium.org/285333003/diff/880001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/public/browser/...
content/public/browser/render_process_host.h:240: virtual void ActivateMojo() =
0;
It'd be nice to make ActivateMojo() a private implementation detail. Eventually,
we will just activate mojo immediately. We only need a signal from the
browser-side for now as we don't want to expose mojo to all renderers yet.

https://codereview.chromium.org/285333003/diff/880001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/public/common/s...
content/public/common/service_registry.h:53: void
GetInterface(mojo::InterfacePtr<Interface>* ptr) {
nit: You might call this GetRemoteInterface for clarity.

[Sam McNally, 2014-06-18 01:31:41]

https://codereview.chromium.org/285333003/diff/880001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/mojo_web_ui_controller.cc (right):

https://codereview.chromium.org/285333003/diff/880001/chrome/browser/ui/webui...
chrome/browser/ui/webui/mojo_web_ui_controller.cc:27:
render_view_host->GetProcess()->ActivateMojo();
On 2014/06/18 00:09:28, darin wrote:
> I don't think we want to call ActivateMojo() here. This should really happen
> more transparently as there could be a variety of services that would
otherwise
> need to call ActivateMojo(). Maybe we can do this from GetServiceRegistry()?

Done.

https://codereview.chromium.org/285333003/diff/880001/content/common/mojo/ser...
File content/common/mojo/service_registry_impl.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/common/mojo/ser...
content/common/mojo/service_registry_impl.h:33: void
Bind(mojo::ScopedMessagePipeHandle handle);
On 2014/06/18 00:09:28, darin wrote:
> nit: You might call this method BindRemoteServiceProvider for clarity.

Done.

https://codereview.chromium.org/285333003/diff/880001/content/common/render_f...
File content/common/render_frame_setup.mojom (right):

https://codereview.chromium.org/285333003/diff/880001/content/common/render_f...
content/common/render_frame_setup.mojom:10: ConnectToServiceProvider(int32
frame_routing_id,
On 2014/06/18 00:09:28, darin wrote:
> Hmm, how about naming this method GetServiceProvider?

Done.

https://codereview.chromium.org/285333003/diff/880001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/public/browser/...
content/public/browser/render_process_host.h:240: virtual void ActivateMojo() =
0;
On 2014/06/18 00:09:28, darin wrote:
> It'd be nice to make ActivateMojo() a private implementation detail.
Eventually,
> we will just activate mojo immediately. We only need a signal from the
> browser-side for now as we don't want to expose mojo to all renderers yet.

Done.

https://codereview.chromium.org/285333003/diff/880001/content/public/common/s...
File content/public/common/service_registry.h (right):

https://codereview.chromium.org/285333003/diff/880001/content/public/common/s...
content/public/common/service_registry.h:53: void
GetInterface(mojo::InterfacePtr<Interface>* ptr) {
On 2014/06/18 00:09:29, darin wrote:
> nit: You might call this GetRemoteInterface for clarity.

Done.

[darin (slow to review), 2014-06-21 04:33:35]

LGTM w/ nits:

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

https://codereview.chromium.org/285333003/diff/900001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:171:
setup->GetServiceProvider(routing_id_, mojo::Get(&service_provider));
nit: It might read better if this were GetServiceProviderForFrame.

https://codereview.chromium.org/285333003/diff/900001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:256:
static_cast<RenderProcessHostImpl*>(GetProcess())->ActivateMojo();
nit: perhaps we should rename ActivateMojo to EnsureMojoActivated since it is
OK to call it multiple times.

https://codereview.chromium.org/285333003/diff/900001/content/common/mojo/ser...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/common/mojo/ser...
content/common/mojo/service_registry_impl.cc:21: mojo::ScopedMessagePipeHandle
handle(pending_connects_.front().second);
You could also use mojo::CloseRaw here. That might make your intent slightly
clearer.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
content/renderer/render_thread_impl.cc:273: if (!frame) {
Under what conditions is there no frame yet for this routing ID? Is there
some race condition you are dealing with here? Some comments would be
helpful. I'm also curious if we could avoid the complexity of
RegisterPendingRenderFrameConnect somehow.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
content/renderer/render_thread_impl.cc:542: mojo::ScopedMessagePipeHandle
handle(it->second);
nit: again, CloseRaw() might be clearer.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/web_ui...
File content/renderer/web_ui_mojo.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/renderer/web_ui...
content/renderer/web_ui_mojo.cc:91: ->GetServiceRegistry()
nit: "Wrap after binary operators, not before." from
http://dev.chromium.org/developers/coding-style

RenderFrame::FromWebFrame(...)->
    GetServiceRegistry()->
        GetRemoteInterface(...);

[Sam McNally, 2014-06-24 04:00:29]

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

https://codereview.chromium.org/285333003/diff/900001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:171:
setup->GetServiceProvider(routing_id_, mojo::Get(&service_provider));
On 2014/06/21 04:33:35, darin wrote:
> nit: It might read better if this were GetServiceProviderForFrame.

Done.

https://codereview.chromium.org/285333003/diff/900001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:256:
static_cast<RenderProcessHostImpl*>(GetProcess())->ActivateMojo();
On 2014/06/21 04:33:35, darin wrote:
> nit: perhaps we should rename ActivateMojo to EnsureMojoActivated since it is
> OK to call it multiple times.

Done.

https://codereview.chromium.org/285333003/diff/900001/content/common/mojo/ser...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/common/mojo/ser...
content/common/mojo/service_registry_impl.cc:21: mojo::ScopedMessagePipeHandle
handle(pending_connects_.front().second);
On 2014/06/21 04:33:35, darin wrote:
> You could also use mojo::CloseRaw here. That might make your intent slightly
> clearer.

Done.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
content/renderer/render_thread_impl.cc:273: if (!frame) {
On 2014/06/21 04:33:35, darin wrote:
> Under what conditions is there no frame yet for this routing ID? Is there
> some race condition you are dealing with here? Some comments would be
> helpful. I'm also curious if we could avoid the complexity of
> RegisterPendingRenderFrameConnect somehow.

There's a race between the IPC that causes the RenderFrame to be created and the
Mojo message for GetServiceProviderForFrame. It seems to hit this condition for
top-level frames created after the first one. The delayed Mojo activation is
probably giving us the better ordering for the first frame.

Passing the handle in the other direction may fix the problem in this case, but
I suspect there are some other cases where it would be racy in the other
direction.

I think we may be stuck with this until we can enforce ordering between the two
messages or send the handle with the create message.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/render...
content/renderer/render_thread_impl.cc:542: mojo::ScopedMessagePipeHandle
handle(it->second);
On 2014/06/21 04:33:35, darin wrote:
> nit: again, CloseRaw() might be clearer.

Done.

https://codereview.chromium.org/285333003/diff/900001/content/renderer/web_ui...
File content/renderer/web_ui_mojo.cc (right):

https://codereview.chromium.org/285333003/diff/900001/content/renderer/web_ui...
content/renderer/web_ui_mojo.cc:91: ->GetServiceRegistry()
On 2014/06/21 04:33:35, darin wrote:
> nit: "Wrap after binary operators, not before." from
> http://dev.chromium.org/developers/coding-style
> 
> RenderFrame::FromWebFrame(...)->
>     GetServiceRegistry()->
>         GetRemoteInterface(...);

Done.

[Sam McNally, 2014-06-24 07:48:40]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-06-24 07:49:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/285333003/1040001

[commit-bot: I haz the power, 2014-06-24 10:26:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 11:04:02]

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

[commit-bot: I haz the power, 2014-06-24 11:04:04]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Sam McNally, 2014-06-24 22:48:13]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-06-24 22:50:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/285333003/1040001

[commit-bot: I haz the power, 2014-06-25 00:20:21]

Change committed as 279557

[Sam McNally, 2014-06-25 00:55:25]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-06-25 00:58:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/285333003/1080001

[raymes, 2014-06-25 01:43:30]

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:29: return;
Should this ever happen in normal execution?

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:49:
service_factories_[service_name] = service_factory;
It seems like it would be good to DCHECK that there isn't already a service of
that name in there?

[Sam McNally, 2014-06-25 01:49:22]

The CQ bit was unchecked by sammc@chromium.org

[Sam McNally, 2014-06-25 02:36:11]

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
File content/common/mojo/service_registry_impl.cc (right):

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:29: return;
On 2014/06/25 01:43:30, raymes wrote:
> Should this ever happen in normal execution?

No. Switched to a DCHECK.

https://codereview.chromium.org/285333003/diff/1080001/content/common/mojo/se...
content/common/mojo/service_registry_impl.cc:49:
service_factories_[service_name] = service_factory;
On 2014/06/25 01:43:30, raymes wrote:
> It seems like it would be good to DCHECK that there isn't already a service of
> that name in there?

Done.

[Sam McNally, 2014-06-25 02:37:11]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-06-25 02:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/285333003/1100001

[commit-bot: I haz the power, 2014-06-25 06:09:36]

Change committed as 279623