[MBI]: Make AgentSchedulingGroup(Host) real RouteProviders & Associat…

…edInterfaceProviders

This CL removes the content.mojom.RouteProvider and
blink.mojom.AssociatedInterfaceProvider implementations from
RenderThreadImpl and RenderProcessHostImpl. As a result, we make
AgentSchedulingGroup and AgentSchedulingGroupHost fully-fledged
RouteProviders and AssociatedInterfaceProviders.

Consequently, we:
 (1) Remove the RenderThread public API addition done in
     crrev.com/c/2433793, since AgentSchedulingGroup is now wholly
     responsible for providing a remote mojom::RouteProvider*, and no
     longer delegates to RenderThreadImpl.
 (2) Introduce MockAgentSchedulingGroup alongside
     AgentSchedulingGroupHost to be used in unit tests. This provides a
     mock for GetRemoteRouteProvider() so we don't have to have a
     test-only path when RenderThreadImpl::current() == false alongside
     production code
 (3) Introduce temporary
     {RenderProcessHostImpl,ChildThreadimpl}::GetListener() methods for
     AgentSchedulingGroup and its host to use, to get IPC::Listeners
     from the routing ID map. This is important for the implementation
     of AssociatedInterfaceProvider, however it is temporary, and these
     methods will be removed when the legacy IPC functionality is fully
     transferred to ASG(H), and these objects have their own
     IPC::Listener* map.

One subtle change that this CL makes is the way that the RouteProvider
interface is bound between the RenderProcessHostImpl:

Before this CL, the Renderer => Browser RouteProvider was bound lazily,
whenever RenderThreadImpl::GetRemoteRouteProvider() was called for the
first time.

After this CL, the Renderer => Browser RouteProvider is bound at the
exact same time as the Renderer => Browser interface. This is done in
AgentSchedulingGroupHost::SetUpMojoIfNeeded(), where we call
mojom::AgentSchedulingGroup::BindAssociatedRouteProvider(). We pass
both a remote pointing to AgentSchedulingGroupHost, and a receiver that
AgentSchedulingGroup will bind. Once bound, bi-directional communication
is immediately available. One upside to this is that the bind lifetimes
of the browser-side remote and receiver pair are 100% consistent.

----

This change is fully described in:
https://docs.google.com/document/d/12jyv-8aBwMlSS_Y5roEl_ykZT0_eQszGiiNs7Hxktmk/edit#heading=h.3dn0j414iw3j

R=haraken@chromium.org, kinuko@chromium.org, kouhei@chromium.org, talp@chromium.org

Bug: 1132901
Change-Id: I819289c4d8d254229d5ad639ec8eb5fbe96d7381
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438232
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Tal Pressman <talp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819176}

content/browser/renderer_host/agent_scheduling_group_host.cc

@@ -199,10 +199,6 @@ bool AgentSchedulingGroupHost::InitProcessAndMojos() {
 }
 
 ChannelProxy* AgentSchedulingGroupHost::GetChannel() {
-  // TODO(crbug.com/1111231): If the process is not initialized, it also implies
-  // that it is not Ready, meaning the channel we return here will not be valid.
-  // In that case we should return |nullptr|, but that causes certain tests to
-  // fail. This should be changed once those tests are fixed.
   if (process_.IsInitializedAndNotDead())
     SetUpMojoIfNeeded();
 
@@ -232,11 +228,8 @@ void AgentSchedulingGroupHost::RemoveRoute(int32_t routing_id) {
 }
 
 mojom::RouteProvider* AgentSchedulingGroupHost::GetRemoteRouteProvider() {
-  // TODO(domfarolino): Remove `GetRemoteRouteProvider` from `RenderProcessHost`
-  // and make `AgentSchedulingGroupHost` a fully-fledged RouteProvider.
-  RenderProcessHostImpl& process =
-      static_cast<RenderProcessHostImpl&>(process_);
-  return process.GetRemoteRouteProvider(PassKey());
+  SetUpMojoIfNeeded();
+  return remote_route_provider_.get();
 }
 
 void AgentSchedulingGroupHost::CreateFrame(mojom::CreateFrameParamsPtr params) {
@@ -277,34 +270,52 @@ void AgentSchedulingGroupHost::GetRoute(
     int32_t routing_id,
     mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterfaceProvider>
         receiver) {
-  // TODO(crbug.com/1111231): Make AgentSchedulingGroupHost a fully-fledged
-  // RouteProvider, so we can register routes directly with an
-  // AgentSchedulingGroupHost rather than RenderProcessHostImpl.
-  static_cast<RenderProcessHostImpl&>(process_).GetRoute(routing_id,
-                                                         std::move(receiver));
+  DCHECK(receiver.is_valid());
+  associated_interface_provider_receivers_.Add(this, std::move(receiver),
+                                               routing_id);
 }
 
 void AgentSchedulingGroupHost::GetAssociatedInterface(
     const std::string& name,
     mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterface>
         receiver) {
-  // TODO(crbug.com/1111231): Make AgentSchedulingGroupHost a fully-fledged
-  // AssociatedInterfaceProvider, so we can start associating interfaces
-  // directly with the AgentSchedulingGroupHost interface.
-  static_cast<RenderProcessHostImpl&>(process_).GetAssociatedInterface(
-      name, std::move(receiver));
+  int32_t routing_id =
+      associated_interface_provider_receivers_.current_context();
+  IPC::Listener* listener =
+      static_cast<RenderProcessHostImpl&>(process_).GetListener(PassKey(),
+                                                                routing_id);
+  if (listener)
+    listener->OnAssociatedInterfaceRequest(name, receiver.PassHandle());
 }
 
 void AgentSchedulingGroupHost::ResetMojo() {
   receiver_.reset();
   mojo_remote_.reset();
+  remote_route_provider_.reset();
+  route_provider_receiver_.reset();
+  associated_interface_provider_receivers_.Clear();
+  // TODO(domfarolino): Move the SetUpMojoIfNeeded() logic to this method, along
+  // with invoking RenderProcessHostImpl::EnableSendQueue(), so that upon
+  // renderer process crash, we immediately reset our mojos.
 }
 
 void AgentSchedulingGroupHost::SetUpMojoIfNeeded() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(process_.IsInitializedAndNotDead());
+  // We don't DCHECK |process_.IsInitializedAndNotDead()| here because we may
+  // end up here after the render process has died but before the
+  // RenderProcessHostImpl is re-initialized (and thus not considered dead
+  // anymore).
+
+  // The bind states of all of |AgentSchedulingGroupHost|'s remotes and
+  // receivers are expected to be equivalent.
+  DCHECK(process_.GetRendererInterface());
+
+  // Make sure that the bind state of all mojos are equivalent.
+  DCHECK_EQ(mojo_remote_.is_bound(), receiver_.is_bound());
+  DCHECK_EQ(receiver_.is_bound(), remote_route_provider_.is_bound());
+  DCHECK_EQ(remote_route_provider_.is_bound(),
+            route_provider_receiver_.is_bound());
 
-  DCHECK_EQ(receiver_.is_bound(), mojo_remote_.is_bound());
   if (receiver_.is_bound())
     return;
 
@@ -317,6 +328,10 @@ void AgentSchedulingGroupHost::SetUpMojoIfNeeded() {
         receiver_.BindNewPipeAndPassRemote(),
         mojo_remote_.BindNewPipeAndPassReceiver());
   }
+
+  mojo_remote_.get()->BindAssociatedRouteProvider(
+      route_provider_receiver_.BindNewEndpointAndPassRemote(),
+      remote_route_provider_.BindNewEndpointAndPassReceiver());
 }
 
 }  // namespace content

content/browser/renderer_host/agent_scheduling_group_host.h

@@ -13,6 +13,7 @@
 #include "content/common/renderer.mojom-forward.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_receiver_set.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -188,6 +189,20 @@ class CONTENT_EXPORT AgentSchedulingGroupHost
   // Remote stub of `mojom::AgentSchedulingGroup`, used for sending calls to the
   // (renderer-side) `AgentSchedulingGroup`.
   MaybeAssociatedRemote mojo_remote_;
+
+  // The `mojom::RouteProvider` mojo pair to setup
+  // `blink::AssociatedInterfaceProvider` routes between this and the
+  // renderer-side `AgentSchedulingGroup`.
+  mojo::AssociatedRemote<mojom::RouteProvider> remote_route_provider_;
+  mojo::AssociatedReceiver<mojom::RouteProvider> route_provider_receiver_{this};
+
+  // The `blink::mojom::AssociatedInterfaceProvider` receiver set that *all*
+  // renderer-side `blink::AssociatedInterfaceProvider` objects own a remote to.
+  // `AgentSchedulingGroupHost` will be responsible for routing each associated
+  // interface request to the appropriate renderer host object.
+  mojo::AssociatedReceiverSet<blink::mojom::AssociatedInterfaceProvider,
+                              int32_t>
+      associated_interface_provider_receivers_;
 };
 
 }  // namespace content

content/browser/renderer_host/render_process_host_impl.cc

@@ -1947,8 +1947,6 @@ void RenderProcessHostImpl::InitializeChannelProxy() {
   //
   // See OnProcessLaunched() for some additional details of this somewhat
   // surprising behavior.
-  remote_route_provider_.reset();
-  channel_->GetRemoteAssociatedInterface(&remote_route_provider_);
   renderer_interface_.reset();
   channel_->GetRemoteAssociatedInterface(&renderer_interface_);
 
@@ -2482,8 +2480,6 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
 
   // This base::Unretained() usage is safe since the associated_registry is
   // owned by this RPHI.
-  associated_registry->AddInterface(base::BindRepeating(
-      &RenderProcessHostImpl::BindRouteProvider, base::Unretained(this)));
   associated_registry->AddInterface(base::BindRepeating(
       &RenderProcessHostImpl::CreateRendererHost, base::Unretained(this)));
 
@@ -2537,31 +2533,10 @@ void RenderProcessHostImpl::RegisterMojoInterfaces() {
       std::move(registry), std::move(child_host_pending_receiver_));
 }
 
-void RenderProcessHostImpl::BindRouteProvider(
-    mojo::PendingAssociatedReceiver<mojom::RouteProvider> receiver) {
-  if (route_provider_receiver_.is_bound())
-    return;
-  route_provider_receiver_.Bind(std::move(receiver));
-}
-
-void RenderProcessHostImpl::GetRoute(
-    int32_t routing_id,
-    mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterfaceProvider>
-        receiver) {
-  DCHECK(receiver.is_valid());
-  associated_interface_provider_receivers_.Add(this, std::move(receiver),
-                                               routing_id);
-}
-
-void RenderProcessHostImpl::GetAssociatedInterface(
-    const std::string& name,
-    mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterface>
-        receiver) {
-  int32_t routing_id =
-      associated_interface_provider_receivers_.current_context();
-  IPC::Listener* listener = listeners_.Lookup(routing_id);
-  if (listener)
-    listener->OnAssociatedInterfaceRequest(name, receiver.PassHandle());
+IPC::Listener* RenderProcessHostImpl::GetListener(
+    util::PassKey<AgentSchedulingGroupHost>,
+    int32_t routing_id) {
+  return listeners_.Lookup(routing_id);
 }
 
 void RenderProcessHostImpl::CreateEmbeddedFrameSinkProvider(
@@ -2856,11 +2831,6 @@ void RenderProcessHostImpl::SetIsUsed() {
   is_unused_ = false;
 }
 
-mojom::RouteProvider* RenderProcessHostImpl::GetRemoteRouteProvider(
-    util::PassKey<AgentSchedulingGroupHost>) {
-  return remote_route_provider_.get();
-}
-
 void RenderProcessHostImpl::AddRoute(int32_t routing_id,
                                      IPC::Listener* listener) {
   TRACE_EVENT2("shutdown", "RenderProcessHostImpl::AddRoute",
@@ -4634,8 +4604,6 @@ void RenderProcessHostImpl::ProcessDied(
 void RenderProcessHostImpl::ResetIPC() {
   renderer_host_receiver_.reset();
   io_thread_host_impl_.reset();
-  route_provider_receiver_.reset();
-  associated_interface_provider_receivers_.Clear();
   associated_interfaces_.reset();
   coordinator_connector_receiver_.reset();
   tracing_registration_.reset();

content/browser/renderer_host/render_process_host_impl.h

@@ -29,6 +29,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/sequence_bound.h"
+#include "base/util/type_safety/pass_key.h"
 #include "build/build_config.h"
 #include "content/browser/child_process_launcher.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
@@ -159,8 +160,6 @@ typedef base::Thread* (*RendererMainThreadFactoryFunction)(
 class CONTENT_EXPORT RenderProcessHostImpl
     : public RenderProcessHost,
       public ChildProcessLauncher::Client,
-      public mojom::RouteProvider,
-      public blink::mojom::AssociatedInterfaceProvider,
       public mojom::RendererHost,
       public blink::mojom::DomStorageProvider,
       public memory_instrumentation::mojom::CoordinatorConnector {
@@ -273,9 +272,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
   void DumpProfilingData(base::OnceClosure callback) override;
 #endif
 
-  mojom::RouteProvider* GetRemoteRouteProvider(
-      util::PassKey<AgentSchedulingGroupHost>);
-
   // IPC::Sender via RenderProcessHost.
   bool Send(IPC::Message* msg) override;
 
@@ -747,17 +743,11 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Registers Mojo interfaces to be exposed to the renderer.
   void RegisterMojoInterfaces();
 
-  // mojom::RouteProvider:
-  void GetRoute(
-      int32_t routing_id,
-      mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterfaceProvider>
-          receiver) override;
-
-  // blink::mojom::AssociatedInterfaceProvider:
-  void GetAssociatedInterface(
-      const std::string& name,
-      mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterface>
-          receiver) override;
+  // TODO(crbug.com/1132901): We'll be able to remove this method once
+  // `AgentSchedulingGroupHost` maintains its own map of IPC::Listeners, but for
+  // now we'll let it delegate to this class.
+  IPC::Listener* GetListener(util::PassKey<AgentSchedulingGroupHost>,
+                             int32_t routing_id);
 
   // mojom::RendererHost
   using BrowserHistogramCallback =
@@ -770,9 +760,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
       const GURL& url,
       mojom::RendererHost::ResolveProxyCallback callback) override;
 
-  void BindRouteProvider(
-      mojo::PendingAssociatedReceiver<mojom::RouteProvider> receiver);
-
   void CreateEmbeddedFrameSinkProvider(
       mojo::PendingReceiver<blink::mojom::EmbeddedFrameSinkProvider> receiver);
   void BindCompositingModeReporter(
@@ -986,11 +973,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // nature (e.g. metrics, memory usage).
   std::unique_ptr<blink::AssociatedInterfaceRegistry> associated_interfaces_;
 
-  mojo::AssociatedReceiver<mojom::RouteProvider> route_provider_receiver_{this};
-  mojo::AssociatedReceiverSet<blink::mojom::AssociatedInterfaceProvider,
-                              int32_t>
-      associated_interface_provider_receivers_;
-
   // These fields are cached values that are updated in
   // UpdateProcessPriorityInputs, and are used to compute priority sent to
   // ChildProcessLauncher.
@@ -1169,7 +1151,6 @@ class CONTENT_EXPORT RenderProcessHostImpl
   mojo::Remote<mojom::ChildProcess> child_process_;
   // This will be bound to |io_thread_host_impl_|.
   mojo::PendingReceiver<mojom::ChildProcessHost> child_host_pending_receiver_;
-  mojo::AssociatedRemote<mojom::RouteProvider> remote_route_provider_;
   mojo::AssociatedRemote<mojom::Renderer> renderer_interface_;
   mojo::AssociatedReceiver<mojom::RendererHost> renderer_host_receiver_{this};
   mojo::Receiver<memory_instrumentation::mojom::CoordinatorConnector>

content/child/child_thread_impl.cc

@@ -843,14 +843,8 @@ bool ChildThreadImpl::IsInBrowserProcess() const {
   return static_cast<bool>(browser_process_io_runner_);
 }
 
-void ChildThreadImpl::GetAssociatedInterface(
-    int32_t routing_id,
-    const std::string& name,
-    mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterface>
-        receiver) {
-  Listener* route = router_.GetRoute(routing_id);
-  if (route)
-    route->OnAssociatedInterfaceRequest(name, receiver.PassHandle());
+IPC::Listener* ChildThreadImpl::GetListener(int32_t routing_id) {
+  return router_.GetRoute(routing_id);
 }
 
 }  // namespace content

content/child/child_thread_impl.h

@@ -161,11 +161,10 @@ class CONTENT_EXPORT ChildThreadImpl : public IPC::Listener,
 
   bool IsInBrowserProcess() const;
 
-  void GetAssociatedInterface(
-      int32_t routing_id,
-      const std::string& name,
-      mojo::PendingAssociatedReceiver<blink::mojom::AssociatedInterface>
-          receiver);
+  // TODO(1132901) We'll be able to move this method once |AgentSchedulingGroup|
+  // maintains its own map of IPC::Listeners, but for now we'll let it delegate
+  // to this class.
+  virtual IPC::Listener* GetListener(int32_t routing_id);
 
  private:
   // TODO(crbug.com/1111231): This class is a friend so that it can call our

content/common/agent_scheduling_group.mojom

@@ -4,6 +4,7 @@
 
 module content.mojom;
 
+import "content/common/associated_interfaces.mojom";
 import "content/common/document_scoped_interface_bundle.mojom";
 import "content/common/native_types.mojom";
 import "ipc/constants.mojom";
@@ -206,6 +207,21 @@ interface AgentSchedulingGroupHost {
 // AgentSchedulingGroupHost and the renderer process's AgentSchedulingGroup.
 // Implemented by content::AgentSchedulingGroup (in the renderer process).
 interface AgentSchedulingGroup {
+  // Tells the renderer to bind the receiver to a backing RouteProvider
+  // implementation, which in practice is itself. Also passes a remote to the
+  // RouteProvider interface owned by AgentSchedulingGroupHost so that we
+  // immediately have bidirectional RouteProvider communication between
+  // AgentSchedulingGroup <=> AgentSchedulingGroupHost. We have this as a
+  // method on this interface, as opposed to passing this remote/receiver pair
+  // over the method that creates the remote AgentSchedulingGroup. This is
+  // because we need the RouteProvider remote/receiver pair to be associated
+  // with the message pipe that the AgentSchedulingGroup is associated with,
+  // which may be different than the message pipe that we create the
+  // AgentSchedulingGroup over.
+  BindAssociatedRouteProvider(
+    pending_associated_remote<RouteProvider> remote,
+    pending_associated_receiver<RouteProvider> receiver);
+
   // Tells the renderer to create a new view.
   CreateView(CreateViewParams params);
 

content/public/renderer/render_thread.h

@@ -12,7 +12,6 @@
 #include "base/callback.h"
 #include "base/metrics/user_metrics_action.h"
 #include "base/single_thread_task_runner.h"
-#include "base/util/type_safety/pass_key.h"
 #include "content/common/content_export.h"
 #include "content/public/child/child_thread.h"
 #include "ipc/ipc_channel_proxy.h"
@@ -43,12 +42,6 @@ class Extension;
 }  // namespace v8
 
 namespace content {
-class AgentSchedulingGroup;
-
-namespace mojom {
-class RouteProvider;
-}  // namespace mojom
-
 class RenderThreadObserver;
 class ResourceDispatcherDelegate;
 
@@ -82,9 +75,6 @@ class CONTENT_EXPORT RenderThread : virtual public ChildThread {
   virtual void AddObserver(RenderThreadObserver* observer) = 0;
   virtual void RemoveObserver(RenderThreadObserver* observer) = 0;
 
-  virtual mojom::RouteProvider* GetRemoteRouteProvider(
-      util::PassKey<AgentSchedulingGroup>) = 0;
-
   // Set the ResourceDispatcher delegate object for this process.
   virtual void SetResourceDispatcherDelegate(
       ResourceDispatcherDelegate* delegate) = 0;