Reland "[service-manager] Simplify connection logic"

This is a reland of 18526eed

Diff PS3->PS4 is fix for breakage.

Original change's description:
> [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/+/1593722
> Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/master@{#657401}

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

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,
+      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);

services/service_manager/tests/connect/connect_unittest.cc

@@ -558,15 +558,16 @@ TEST_F(ConnectTest, MAYBE_PackagedApp_BlockedInterface) {
 // Connection to another application provided by the same package, blocked
 // because it's not in the capability filter whitelist.
 TEST_F(ConnectTest, MAYBE_BlockedPackagedApplication) {
-  mojom::ConnectResult result;
   base::RunLoop run_loop;
   test::mojom::ConnectTestServicePtr service_b;
   connector()->BindInterface(
       ServiceFilter::ByName(kTestAppBName), mojo::MakeRequest(&service_b),
-      base::BindOnce(&StartServiceResponse, nullptr, &result, nullptr));
-  service_b.set_connection_error_handler(run_loop.QuitClosure());
-  run_loop.Run();
+      base::BindLambdaForTesting([&](mojom::ConnectResult result,
+                                     const base::Optional<Identity>& identity) {
         EXPECT_EQ(mojom::ConnectResult::ACCESS_DENIED, result);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
 }
 
 TEST_F(ConnectTest, CapabilityClasses) {