[service-manager] Simplify connection logic

This reduces the complexity of Service Manager's logic for handling
interconnection between service instances. Much of the complexity was
due to connection operations once upon a time being highly
asynchronous within SM, but that hasn't been the case for a while.

Rather than having ServiceManager::Connect do way too much stuff, it's
replaced with a simpler and more straightforward
FindOrCreateMatchingTargetInstance, which does only what the name says.
Previous work done by Connect() in various edge cases has been moved out
to its prior call sites, now replaced with calls to FOCMTI.

Bug: 904240
Change-Id: Ia276f6fa61477ddc141724d30484237498eef070
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1593722Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#657401}

services/service_manager/service_instance.cc

@@ -220,7 +220,7 @@ void ServiceInstance::StartWithRemote(
                            base::BindOnce(&ServiceInstance::OnStartCompleted,
                                           base::Unretained(this)));
 
-  service_manager_->NotifyServiceCreated(this);
+  service_manager_->NotifyServiceCreated(*this);
 }
 
 bool ServiceInstance::StartWithExecutablePath(const base::FilePath& path,
@@ -246,30 +246,24 @@ bool ServiceInstance::StartWithExecutablePath(const base::FilePath& path,
 #endif
 }
 
-void ServiceInstance::AuthorizeAndForwardConnectionRequestFromOtherService(
-    const Identity& source_identity,
+bool ServiceInstance::MaybeAcceptConnectionRequest(
+    const ServiceInstance& source_instance,
     const std::string& interface_name,
     mojo::ScopedMessagePipeHandle receiving_pipe,
-    mojom::BindInterfacePriority priority,
-    mojom::Connector::BindInterfaceCallback callback) {
-  ServiceInstance* const source =
-      service_manager_->GetExistingInstance(source_identity);
-  if (!source || state_ == mojom::InstanceState::kUnreachable) {
-    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, identity_);
-    return;
-  }
+    mojom::BindInterfacePriority priority) {
+  if (state_ == mojom::InstanceState::kUnreachable)
+    return false;
 
-  const Manifest& source_manifest = source->manifest();
+  const Manifest& source_manifest = source_instance.manifest();
   const bool bindable_on_any_service = base::ContainsKey(
       source_manifest.interfaces_bindable_on_any_service, interface_name);
   const bool allowed_by_capabilities =
       AllowsInterface(source_manifest.required_capabilities, identity_.name(),
                       manifest_.exposed_capabilities, interface_name);
   if (!bindable_on_any_service && !allowed_by_capabilities) {
-    ReportBlockedInterface(source_identity.name(), identity_.name(),
+    ReportBlockedInterface(source_instance.identity().name(), identity_.name(),
                            interface_name);
-    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, identity_);
-    return;
+    return false;
   }
 
   base::OnceClosure on_bind_interface_complete;
@@ -280,13 +274,14 @@ void ServiceInstance::AuthorizeAndForwardConnectionRequestFromOtherService(
   }
 
   service_remote_->OnBindInterface(
-      BindSourceInfo(source_identity, GetRequiredCapabilities(
-                                          source_manifest.required_capabilities,
-                                          identity_.name())),
+      BindSourceInfo(
+          source_instance.identity(),
+          GetRequiredCapabilities(source_manifest.required_capabilities,
+                                  identity_.name())),
       interface_name, std::move(receiving_pipe),
       std::move(on_bind_interface_complete));
 
-  std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, identity_);
+  return true;
 }
 
 void ServiceInstance::Stop() {
@@ -379,12 +374,12 @@ void ServiceInstance::HandleServiceOrConnectorDisconnection() {
   }
 }
 
-mojom::ConnectResult ServiceInstance::ValidateConnectionRequestToOtherService(
+bool ServiceInstance::CanConnectToOtherInstance(
     const ServiceFilter& target_filter,
     const base::Optional<std::string>& target_interface_name) {
   if (target_filter.service_name().empty()) {
     DLOG(ERROR) << "ServiceFilter has no service name.";
-    return mojom::ConnectResult::INVALID_ARGUMENT;
+    return false;
   }
 
   bool skip_instance_group_check =
@@ -402,7 +397,7 @@ mojom::ConnectResult ServiceInstance::ValidateConnectionRequestToOtherService(
                << "group " << target_filter.instance_group()->ToString()
                << " without |can_connect_to_instances_in_any_group| set to "
                << "|true|.";
-    return mojom::ConnectResult::ACCESS_DENIED;
+    return false;
   }
   if (target_filter.instance_id() && !target_filter.instance_id()->is_zero() &&
       !manifest_.options.can_connect_to_instances_with_any_id) {
@@ -411,14 +406,14 @@ mojom::ConnectResult ServiceInstance::ValidateConnectionRequestToOtherService(
                << " with instance ID "
                << target_filter.instance_id()->ToString() << " without "
                << "|can_connect_to_instances_with_any_id| set to |true|.";
-    return mojom::ConnectResult::ACCESS_DENIED;
+    return false;
   }
 
   if (can_contact_all_services_ ||
       !manifest_.interfaces_bindable_on_any_service.empty() ||
       manifest_.required_capabilities.find(target_filter.service_name()) !=
           manifest_.required_capabilities.end()) {
-    return mojom::ConnectResult::SUCCEEDED;
+    return true;
   }
 
   if (target_interface_name) {
@@ -428,7 +423,7 @@ mojom::ConnectResult ServiceInstance::ValidateConnectionRequestToOtherService(
     ReportBlockedStartService(identity_.name(), target_filter.service_name());
   }
 
-  return mojom::ConnectResult::ACCESS_DENIED;
+  return false;
 }
 
 void ServiceInstance::BindInterface(
@@ -437,16 +432,25 @@ void ServiceInstance::BindInterface(
     mojo::ScopedMessagePipeHandle receiving_pipe,
     mojom::BindInterfacePriority priority,
     BindInterfaceCallback callback) {
-  mojom::ConnectResult result =
-      ValidateConnectionRequestToOtherService(target_filter, interface_name);
-  if (result != mojom::ConnectResult::SUCCEEDED) {
-    std::move(callback).Run(result, base::nullopt);
+  if (!CanConnectToOtherInstance(target_filter, interface_name)) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, base::nullopt);
+    return;
+  }
+
+  ServiceInstance* target_instance =
+      service_manager_->FindOrCreateMatchingTargetInstance(*this,
+                                                           target_filter);
+  bool allowed =
+      target_instance &&
+      target_instance->MaybeAcceptConnectionRequest(
+          *this, interface_name, std::move(receiving_pipe), priority);
+  if (!allowed) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, base::nullopt);
     return;
   }
 
-  service_manager_->Connect(target_filter, identity_, interface_name,
-                            std::move(receiving_pipe), priority,
-                            std::move(callback));
+  std::move(callback).Run(mojom::ConnectResult::SUCCEEDED,
+                          target_instance->identity());
 }
 
 void ServiceInstance::QueryService(const std::string& service_name,
@@ -462,14 +466,22 @@ void ServiceInstance::QueryService(const std::string& service_name,
 
 void ServiceInstance::WarmService(const ServiceFilter& target_filter,
                                   WarmServiceCallback callback) {
-  mojom::ConnectResult result = ValidateConnectionRequestToOtherService(
-      target_filter, base::nullopt /* interface_name */);
-  if (result != mojom::ConnectResult::SUCCEEDED) {
-    std::move(callback).Run(result, base::nullopt);
+  if (!CanConnectToOtherInstance(target_filter,
+                                 base::nullopt /* interface_name */)) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, base::nullopt);
     return;
   }
 
-  service_manager_->Connect(target_filter, identity_, std::move(callback));
+  ServiceInstance* target_instance =
+      service_manager_->FindOrCreateMatchingTargetInstance(*this,
+                                                           target_filter);
+  if (!target_instance) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED, base::nullopt);
+    return;
+  }
+
+  std::move(callback).Run(mojom::ConnectResult::SUCCEEDED,
+                          target_instance->identity());
 }
 
 void ServiceInstance::RegisterServiceInstance(
@@ -477,6 +489,13 @@ void ServiceInstance::RegisterServiceInstance(
     mojo::ScopedMessagePipeHandle service_remote_handle,
     mojom::PIDReceiverRequest pid_receiver_request,
     RegisterServiceInstanceCallback callback) {
+  auto target_filter = ServiceFilter::ForExactIdentity(identity);
+  if (!CanConnectToOtherInstance(target_filter,
+                                 base::nullopt /* interface_name */)) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED);
+    return;
+  }
+
   mojo::PendingRemote<mojom::Service> service_remote(
       std::move(service_remote_handle), 0);
 
@@ -495,23 +514,13 @@ void ServiceInstance::RegisterServiceInstance(
     return;
   }
 
-  auto target_filter = ServiceFilter::ForExactIdentity(identity);
-  mojom::ConnectResult result = ValidateConnectionRequestToOtherService(
-      target_filter, base::nullopt /* interface_name */);
-  if (result != mojom::ConnectResult::SUCCEEDED) {
-    std::move(callback).Run(result);
-    return;
+  mojom::ServicePtr service(std::move(service_remote));
+  if (!service_manager_->RegisterService(identity, std::move(service),
+                                         std::move(pid_receiver_request))) {
+    std::move(callback).Run(mojom::ConnectResult::ACCESS_DENIED);
   }
 
-  service_manager_->Connect(target_filter, identity_, std::move(service_remote),
-                            std::move(pid_receiver_request),
-                            base::BindOnce(
-                                [](RegisterServiceInstanceCallback callback,
-                                   mojom::ConnectResult result,
-                                   const base::Optional<Identity>& identity) {
-                                  std::move(callback).Run(result);
-                                },
-                                std::move(callback)));
+  std::move(callback).Run(mojom::ConnectResult::SUCCEEDED);
 }
 
 void ServiceInstance::Clone(mojom::ConnectorRequest request) {

services/service_manager/service_instance.h

@@ -73,14 +73,14 @@ class ServiceInstance : public mojom::Connector,
   bool StartWithExecutablePath(const base::FilePath& path,
                                SandboxType sandbox_type);
 
-  // Forwards a BindInterface request to the service instance, iff it should be
-  // allowed based on manifest constraints.
-  void AuthorizeAndForwardConnectionRequestFromOtherService(
-      const Identity& source_identity,
+  // Forwards a BindInterface request from |source_instance| to this instance,
+  // iff it should be allowed based on manifest constraints. Returns |true| if
+  // the request was allowed, or |false| otherwise.
+  bool MaybeAcceptConnectionRequest(
+      const ServiceInstance& source_instance,
       const std::string& interface_name,
       mojo::ScopedMessagePipeHandle receiving_pipe,
-      mojom::BindInterfacePriority priority,
-      mojom::Connector::BindInterfaceCallback callback);
+      mojom::BindInterfacePriority priority);
 
   // Stops receiving any new messages from the service instance and renders the
   // instance permanently unreachable. Note that this does NOT make any attempt
@@ -112,10 +112,11 @@ class ServiceInstance : public mojom::Connector,
 
   // Examines an interface connection request coming from this service instance
   // and determines whether it should be allowed to reach any designated target
-  // instance. A return value of |mojom::ConnectResult::SUCCESS| indicates that
-  // the request can be processed further. Any other result means the request
-  // should be rejected.
-  mojom::ConnectResult ValidateConnectionRequestToOtherService(
+  // instance. Returns |true| if so, or |false| otherwise.
+  //
+  // If |target_interface_name| is null, it is sufficient for this (the source)
+  // service to have access to *any* arbitrary interface on the target service.
+  bool CanConnectToOtherInstance(
       const ServiceFilter& target_filter,
       const base::Optional<std::string>& target_interface_name);
 

services/service_manager/service_manager.h

@@ -64,7 +64,9 @@ class ServiceManager : public Service {
   //
   // |pid_receiver_request| may be null, in which case the service manager
   // assumes the new service is running in this process.
-  void RegisterService(const Identity& identity,
+  //
+  // Returns |true| if registration succeeded, or |false| otherwise.
+  bool RegisterService(const Identity& identity,
                        mojom::ServicePtr service,
                        mojom::PIDReceiverRequest pid_receiver_request);
 
@@ -74,40 +76,17 @@ class ServiceManager : public Service {
                     const base::Token& instance_group,
                     std::string* sandbox_type);
 
-  // Completes a connection between a source and target application as defined
-  // by |params|. If no existing instance of the target service is running, one
-  // will be loaded.
-  void Connect(const ServiceFilter& partial_target_filter,
-               const Identity& source_identity,
-               const base::Optional<std::string>& interface_name,
-               mojo::ScopedMessagePipeHandle receiving_pipe,
-               mojo::PendingRemote<mojom::Service> service_remote,
-               mojo::PendingReceiver<mojom::PIDReceiver> pid_receiver,
-               mojom::BindInterfacePriority priority,
-               mojom::Connector::BindInterfaceCallback callback);
-
-  // Variant of the above when no Service remote or PIDReceiver is provided by
-  // the caller.
-  void Connect(const ServiceFilter& partial_target_filter,
-               const Identity& source_identity,
-               const std::string& interface_name,
-               mojo::ScopedMessagePipeHandle receiving_pipe,
-               mojom::BindInterfacePriority priority,
-               mojom::Connector::BindInterfaceCallback callback);
-
-  // Variant of the above where no interface name, receiving pipe, Service
-  // remote, or PIDReceiver is provided by the caller.
-  void Connect(const ServiceFilter& partial_target_filter,
-               const Identity& source_identity,
-               mojom::Connector::BindInterfaceCallback callback);
-
-  // Variant of the above where no interface name or receiving pipe is provided
-  // by the caller, but a Service remote and PIDReceiver are provided.
-  void Connect(const ServiceFilter& partial_target_filter,
-               const Identity& source_identity,
-               mojo::PendingRemote<mojom::Service> service_remote,
-               mojo::PendingReceiver<mojom::PIDReceiver> pid_receiver,
-               mojom::Connector::BindInterfaceCallback callback);
+  // Attempts to locate a ServiceInstance as a target for a connection request
+  // from |source_instance| by matching against |partial_target_filter|. If
+  // a suitable instance exists it is returned, otherwise the Service Manager
+  // attempts to create a new suitable instance.
+  //
+  // Returns null if a matching instance did not exist and could not be created,
+  // otherwise returns a valid ServiceInstance which matches
+  // |partial_target_filter| from |source_instance|'s perspective.
+  ServiceInstance* FindOrCreateMatchingTargetInstance(
+      const ServiceInstance& source_instance,
+      const ServiceFilter& partial_target_filter);
 
  private:
   friend class ServiceInstance;
@@ -129,7 +108,7 @@ class ServiceManager : public Service {
   // Returns a running instance identified by |identity|.
   ServiceInstance* GetExistingInstance(const Identity& identity) const;
 
-  void NotifyServiceCreated(ServiceInstance* instance);
+  void NotifyServiceCreated(const ServiceInstance& instance);
   void NotifyServiceStarted(const Identity& identity, base::ProcessId pid);
   void NotifyServiceFailedToStart(const Identity& identity);
 
@@ -141,11 +120,6 @@ class ServiceManager : public Service {
   // Called from the instance implementing mojom::ServiceManager.
   void AddListener(mojom::ServiceManagerListenerPtr listener);
 
-  void CreateServiceWithFactory(const ServiceFilter& service_factory_filter,
-                                const std::string& name,
-                                mojom::ServiceRequest request,
-                                mojom::PIDReceiverPtr pid_receiver);
-
   // Returns a running ServiceFactory for |filter|. If there is not one running,
   // one is started.
   mojom::ServiceFactory* GetServiceFactory(const ServiceFilter& filter);