Renderers should obtain browser InterfaceProvider by connecting to browser

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index de825b5852f59b51a71e389a97740977679d1548..4856b9220804d546d305b99a3447e62123108abd 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -148,6 +148,7 @@
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/worker_service.h"
 #include "content/public/common/child_process_host.h"
+#include "content/public/common/connection_filter.h"
 #include "content/public/common/content_constants.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
@@ -172,6 +173,7 @@
 #include "mojo/edk/embedder/embedder.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "ppapi/shared_impl/ppapi_switches.h"
+#include "services/shell/public/cpp/connection.h"
 #include "services/shell/public/cpp/interface_provider.h"
 #include "services/shell/public/cpp/interface_registry.h"
 #include "services/shell/runner/common/switches.h"
@@ -453,15 +455,64 @@ RendererMainThreadFactoryFunction g_renderer_main_thread_factory = NULL;
 
 base::MessageLoop* g_in_process_thread;
 
+// Stores the maximum number of renderer processes the content module can
+// create.
+static size_t g_max_renderer_count_override = 0;
+
+// static
+bool g_run_renderer_in_process_ = false;
+
+// Held by the RPH's BrowserContext's MojoShellConnection, ownership transferred
+// back to RPH upon RPH destruction.
+class RenderProcessHostImpl::ConnectionFilterImpl : public ConnectionFilter {
+ public:
+  ConnectionFilterImpl(
+      const shell::Identity& child_identity,
+      std::unique_ptr<shell::InterfaceRegistry> registry)
+      : child_identity_(child_identity),
+        registry_(std::move(registry)) {}
+  ~ConnectionFilterImpl() override {}
+
+ private:
+  // ConnectionFilter:
+  bool OnConnect(shell::Connection* connection,
+                 shell::Connector* connector) override {
+    // We only fulfill connections from the renderer we host.
+    const shell::Identity& remote_identity = connection->GetRemoteIdentity();
+    if (child_identity_.name() != remote_identity.name() ||

[Ken Rockot(use gerrit already), 2016/07/29 17:09:45]

Could we just add a != operator to Identity? Shouldn't the user_id also be
checked?

[Ben Goodger (Google), 2016/07/29 19:35:02]

Can't use user_id since user_id for outbound connections is kInheritUserId

+        child_identity_.instance() != remote_identity.instance()) {
+      return false;
+    }
+
+    std::set<std::string> interface_names;
+    registry_->GetInterfaceNames(&interface_names);
+    for (auto& interface_name : interface_names) {
+      connection->GetInterfaceRegistry()->AddInterface(
+          interface_name,
+          base::Bind(&ConnectionFilterImpl::GetInterface,
+                     base::Unretained(this),

[Ken Rockot(use gerrit already), 2016/07/29 17:09:46]

This is not safe. The registered callbacks may call into the
ConnectionFilterImpl as long as the Connection (and thus the IOThreadContext of
the MojoChildConnection) is alive. That can live arbitrarily longer than the
RPHI which created it, since it lives on the IO thread.

I think we need to use WeakPtrs here.

+                     interface_name));
+    }
+    return true;
+  }
+
+  void GetInterface(const std::string& interface_name,
+                    mojo::ScopedMessagePipeHandle handle) {
+    shell::mojom::InterfaceProvider* provider = registry_.get();
+    provider->GetInterface(interface_name, std::move(handle));
+  }
+
+  shell::Identity child_identity_;
+  std::unique_ptr<shell::InterfaceRegistry> registry_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConnectionFilterImpl);
+};
+
 base::MessageLoop*
 RenderProcessHostImpl::GetInProcessRendererThreadForTesting() {
   return g_in_process_thread;
 }
 
-// Stores the maximum number of renderer processes the content module can
-// create.
-static size_t g_max_renderer_count_override = 0;
-
 // static
 size_t RenderProcessHost::GetMaxRendererProcessCount() {
   if (g_max_renderer_count_override)
@@ -509,9 +560,6 @@ size_t RenderProcessHost::GetMaxRendererProcessCount() {
 }
 
 // static
-bool g_run_renderer_in_process_ = false;
-
-// static
 void RenderProcessHost::SetMaxRendererProcessCount(size_t count) {
   g_max_renderer_count_override = count;
 }
@@ -749,7 +797,23 @@ bool RenderProcessHostImpl::Init() {
 #endif  // defined(OS_ANDROID)
 
   CreateMessageFilters();
-  RegisterMojoInterfaces();
+  std::unique_ptr<shell::InterfaceRegistry> registry(

[Ken Rockot(use gerrit already), 2016/07/29 17:09:45]

Can't you just put all this junk in RegisterMojoInterfaces() with no argument?

+      new shell::InterfaceRegistry(nullptr));
+#if defined(OS_ANDROID)
+  interface_registry_android_ =
+      InterfaceRegistryAndroid::Create(registry.get());
+  InterfaceRegistrarAndroid::ExposeInterfacesToRenderer(
+      interface_registry_android_.get());
+#endif
+
+  RegisterMojoInterfaces(registry.get());
+  MojoShellConnection* mojo_shell_connection =
+      BrowserContext::GetMojoShellConnectionFor(browser_context_);
+  std::unique_ptr<ConnectionFilterImpl> connection_filter(
+      new ConnectionFilterImpl(mojo_child_connection_->child_identity(),
+                               std::move(registry)));
+  connection_filter_ = connection_filter.get();
+  mojo_shell_connection->AddConnectionFilter(std::move(connection_filter));
 
   if (run_renderer_in_process()) {
     DCHECK(g_renderer_main_thread_factory);
@@ -1047,74 +1111,67 @@ void RenderProcessHostImpl::CreateMessageFilters() {
 #endif
 }
 
-void RenderProcessHostImpl::RegisterMojoInterfaces() {
+void RenderProcessHostImpl::RegisterMojoInterfaces(
+    shell::InterfaceRegistry* registry) {
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner =
+      BrowserThread::GetTaskRunnerForThread(BrowserThread::UI);
 #if !defined(OS_ANDROID)
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&device::BatteryMonitorImpl::Create));
+  registry->AddInterface(base::Bind(&device::BatteryMonitorImpl::Create),
+                         ui_task_runner);

[Ken Rockot(use gerrit already), 2016/07/29 17:09:45]

It's pretty unfortunate that we have to add ui_task_runner arguments to so many
AddInterface calls. I guess I don't have any better ideas for now.

 #endif
-
-  GetInterfaceRegistry()->AddInterface(
+  registry->AddInterface(
       base::Bind(&PermissionServiceContext::CreateService,
-                 base::Unretained(permission_service_context_.get())));
-
+                 base::Unretained(permission_service_context_.get())),
+      ui_task_runner);
   // TODO(mcasas): finalize arguments.
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&ImageCaptureImpl::Create));
-
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&OffscreenCanvasSurfaceImpl::Create));
-
-  GetInterfaceRegistry()->AddInterface(base::Bind(
-      &BackgroundSyncContext::CreateService,
-      base::Unretained(storage_partition_impl_->GetBackgroundSyncContext())));
-
-  GetInterfaceRegistry()->AddInterface(base::Bind(
-      &PlatformNotificationContextImpl::CreateService,
-      base::Unretained(
-          storage_partition_impl_->GetPlatformNotificationContext()), GetID()));
-
-  GetInterfaceRegistry()->AddInterface(
+  registry->AddInterface(base::Bind(&ImageCaptureImpl::Create),
+                         ui_task_runner);
+  registry->AddInterface(base::Bind(&OffscreenCanvasSurfaceImpl::Create),
+                         ui_task_runner);
+  registry->AddInterface(
+      base::Bind(&BackgroundSyncContext::CreateService,
+                 base::Unretained(
+                     storage_partition_impl_->GetBackgroundSyncContext())),
+      ui_task_runner);
+  registry->AddInterface(
+      base::Bind(&PlatformNotificationContextImpl::CreateService,
+                 base::Unretained(
+                     storage_partition_impl_->GetPlatformNotificationContext()),
+                 GetID()),
+      ui_task_runner);
+  registry->AddInterface(
       base::Bind(&RenderProcessHostImpl::CreateStoragePartitionService,
-                 base::Unretained(this)));
-
-  GetInterfaceRegistry()->AddInterface(
+                 base::Unretained(this)),
+      ui_task_runner);
+  registry->AddInterface(
       base::Bind(&BroadcastChannelProvider::Connect,
                  base::Unretained(
-                     storage_partition_impl_->GetBroadcastChannelProvider())));
+                     storage_partition_impl_->GetBroadcastChannelProvider())),
+      ui_task_runner);
+  if (memory_coordinator::IsEnabled()) {
+    registry->AddInterface(base::Bind(&CreateMemoryCoordinatorHandle, GetID()),
+                           ui_task_runner);
+  }
 
   scoped_refptr<base::SingleThreadTaskRunner> file_task_runner =
       BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE);
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&MimeRegistryImpl::Create), file_task_runner);
-
+  registry->AddInterface(base::Bind(&MimeRegistryImpl::Create),
+                         file_task_runner);
 #if defined(USE_MINIKIN_HYPHENATION)
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&hyphenation::HyphenationImpl::Create), file_task_runner);
+  registry->AddInterface(base::Bind(&hyphenation::HyphenationImpl::Create),
+                         file_task_runner);
 #endif
 
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner =
       BrowserThread::GetTaskRunnerForThread(BrowserThread::IO);
-  GetInterfaceRegistry()->AddInterface(base::Bind(&DeviceLightHost::Create),
-                                       io_task_runner);
-  GetInterfaceRegistry()->AddInterface(base::Bind(&DeviceMotionHost::Create),
-                                       io_task_runner);
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&DeviceOrientationHost::Create), io_task_runner);
-  GetInterfaceRegistry()->AddInterface(
-      base::Bind(&DeviceOrientationAbsoluteHost::Create), io_task_runner);
-
-  if (memory_coordinator::IsEnabled()) {
-    GetInterfaceRegistry()->AddInterface(
-        base::Bind(&CreateMemoryCoordinatorHandle, GetID()));
-  }
+  registry->AddInterface(base::Bind(&DeviceLightHost::Create), io_task_runner);

[Ken Rockot(use gerrit already), 2016/07/29 17:09:45]

io_task_runner isn't necessary now, it just needlessly defers the binding

+  registry->AddInterface(base::Bind(&DeviceMotionHost::Create), io_task_runner);
+  registry->AddInterface(base::Bind(&DeviceOrientationHost::Create),
+                         io_task_runner);
+  registry->AddInterface(base::Bind(&DeviceOrientationAbsoluteHost::Create),
+                         io_task_runner);
 
-#if defined(OS_ANDROID)
-  InterfaceRegistrarAndroid::ExposeInterfacesToRenderer(
-      mojo_child_connection_->interface_registry_android());
-#endif
-
-  GetContentClient()->browser()->ExposeInterfacesToRenderer(
-      GetInterfaceRegistry(), this);
+  GetContentClient()->browser()->ExposeInterfacesToRenderer(registry, this);
 }
 
 void RenderProcessHostImpl::CreateStoragePartitionService(
@@ -1920,6 +1977,13 @@ void RenderProcessHostImpl::Cleanup() {
         NOTIFICATION_RENDERER_PROCESS_TERMINATED,
         Source<RenderProcessHost>(this), NotificationService::NoDetails());
 
+    if (connection_filter_) {
+      MojoShellConnection* mojo_shell_connection =
+          BrowserContext::GetMojoShellConnectionFor(browser_context_);
+      // Throw the result away, so the connection filter is deleted.
+      mojo_shell_connection->RemoveConnectionFilter(connection_filter_);
+    }
+
 #ifndef NDEBUG
     is_self_deleted_ = true;
 #endif