Perform InterfaceProviderSpec intersection in the ServiceManager.

. Introduces a parallel flow in ServiceManager to Connect called BindInterface which carries (via ConnectParams) interface name and pipe.
. Introduces an OnBindInterface() method to mojom::Service. In ServiceContext, this just calls through to OnConnect still since updating callsites is a pretty large task for a subsequent change.

Note that policy checking is still happening in the client - there was no easy way to avoid this without adding an unsafe mode to InterfaceRegistry that skips the check that I'd be concerned might be misused. And IR should probably just die once this work is complete anyway.

R=rockot@chromium.org,tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2610853013
Cr-Commit-Position: refs/heads/master@{#443119}

services/service_manager/connect_params.h

@@ -52,6 +52,22 @@ class ConnectParams {
     return std::move(pid_receiver_request_);
   }
 
+  void set_interface_request_info(
+      const std::string& interface_name,
+      mojo::ScopedMessagePipeHandle interface_pipe) {
+    interface_name_ = interface_name;
+    interface_pipe_ = std::move(interface_pipe);
+  }
+  const std::string& interface_name() const {
+    return interface_name_;
+  }
+  bool HasInterfaceRequestInfo() const {
+    return !interface_name_.empty() && interface_pipe_.is_valid();
+  }
+  mojo::ScopedMessagePipeHandle TakeInterfaceRequestPipe() {
+    return std::move(interface_pipe_);
+  }
+
   void set_connect_callback(const mojom::Connector::ConnectCallback& value) {
     connect_callback_ = value;
   }
@@ -59,6 +75,15 @@ class ConnectParams {
     return connect_callback_;
   }
 
+  void set_bind_interface_callback(
+      const mojom::Connector::BindInterfaceCallback& callback) {
+    bind_interface_callback_ = callback;
+  }
+  const mojom::Connector::BindInterfaceCallback&
+      bind_interface_callback() const {
+    return bind_interface_callback_;
+  }
+
  private:
   // It may be null (i.e., is_null() returns true) which indicates that there is
   // no source (e.g., for the first application or in tests).
@@ -69,7 +94,10 @@ class ConnectParams {
   mojom::InterfaceProviderRequest remote_interfaces_;
   mojom::ServicePtr service_;
   mojom::PIDReceiverRequest pid_receiver_request_;
+  std::string interface_name_;
+  mojo::ScopedMessagePipeHandle interface_pipe_;
   mojom::Connector::ConnectCallback connect_callback_;
+  mojom::Connector::BindInterfaceCallback bind_interface_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectParams);
 };

services/service_manager/public/cpp/interface_registry.h

@@ -24,6 +24,16 @@ namespace service_manager {
 class Connection;
 class InterfaceBinder;
 
+// Returns the set of capabilities required from the target.
+CapabilitySet GetRequestedCapabilities(const InterfaceProviderSpec& source_spec,
+                                       const Identity& target);
+
+// Generates a single set of interfaces that is the union of all interfaces
+// exposed by the target for the capabilities requested by the source.
+InterfaceSet GetInterfacesToExpose(const InterfaceProviderSpec& source_spec,
+                                   const Identity& target,
+                                   const InterfaceProviderSpec& target_spec);
+
 // An implementation of mojom::InterfaceProvider that allows the user to
 // register services to be exposed to another application.
 //

services/service_manager/public/cpp/lib/interface_registry.cc

@@ -13,7 +13,29 @@
 namespace service_manager {
 namespace {
 
-// Returns the set of capabilities required from the target.
+void SerializeIdentity(const Identity& identity, std::stringstream* stream) {
+  *stream << identity.name() << "@" << identity.instance() << " run as: "
+          << identity.user_id();
+}
+
+void SerializeSpec(const InterfaceProviderSpec& spec,
+                   std::stringstream* stream) {
+  *stream << "  Providing:\n";
+  for (const auto& entry : spec.provides) {
+    *stream << "    capability: " << entry.first << " containing interfaces:\n";
+    for (const auto& interface_name : entry.second)
+      *stream << "      " << interface_name << "\n";
+  }
+  *stream << "\n  Requiring:\n";
+  for (const auto& entry : spec.requires) {
+    *stream << "    From: " << entry.first << ":\n";
+    for (const auto& capability_name : entry.second)
+      *stream << "      " << capability_name << "\n";
+  }
+}
+
+}  // namespace
+
 CapabilitySet GetRequestedCapabilities(const InterfaceProviderSpec& source_spec,
                                        const Identity& target) {
   CapabilitySet capabilities;
@@ -34,8 +56,6 @@ CapabilitySet GetRequestedCapabilities(const InterfaceProviderSpec& source_spec,
   return capabilities;
 }
 
-// Generates a single set of interfaces that is the union of all interfaces
-// exposed by the target for the capabilities requested by the source.
 InterfaceSet GetInterfacesToExpose(
     const InterfaceProviderSpec& source_spec,
     const Identity& target,
@@ -58,29 +78,6 @@ InterfaceSet GetInterfacesToExpose(
   return exposed_interfaces;
 }
 
-void SerializeIdentity(const Identity& identity, std::stringstream* stream) {
-  *stream << identity.name() << "@" << identity.instance() << " run as: "
-          << identity.user_id();
-}
-
-void SerializeSpec(const InterfaceProviderSpec& spec,
-                   std::stringstream* stream) {
-  *stream << "  Providing:\n";
-  for (const auto& entry : spec.provides) {
-    *stream << "    capability: " << entry.first << " containing interfaces:\n";
-    for (const auto& interface_name : entry.second)
-      *stream << "      " << interface_name << "\n";
-  }
-  *stream << "\n  Requiring:\n";
-  for (const auto& entry : spec.requires) {
-    *stream << "    From: " << entry.first << ":\n";
-    for (const auto& capability_name : entry.second)
-      *stream << "      " << capability_name << "\n";
-  }
-}
-
-}  // namespace
-
 InterfaceRegistry::InterfaceRegistry(const std::string& name)
     : binding_(this),
       name_(name),

services/service_manager/public/cpp/lib/service_context.cc

@@ -98,14 +98,46 @@ void ServiceContext::OnConnect(
                            local_info_.interface_provider_specs, &target_spec);
   GetInterfaceProviderSpec(mojom::kServiceManager_ConnectorSpec,
                            source_info.interface_provider_specs, &source_spec);
+
+  // Acknowledge the request regardless of whether it's accepted.
+  callback.Run();
+
+  CallOnConnect(source_info, source_spec, target_spec, std::move(interfaces));
+}
+
+void ServiceContext::OnBindInterface(
+    const ServiceInfo& source_info,
+    const std::string& interface_name,
+    mojo::ScopedMessagePipeHandle interface_pipe,
+    const OnBindInterfaceCallback& callback) {
+  // Acknowledge the request regardless of whether it's accepted.
+  callback.Run();
+
+  mojom::InterfaceProviderPtr interface_provider;
+  // TODO(beng): This should be replaced with a call to OnBindInterface() in a
+  //             subsequent change.
+  InterfaceProviderSpec source_spec, target_spec;
+  GetInterfaceProviderSpec(mojom::kServiceManager_ConnectorSpec,
+                           local_info_.interface_provider_specs, &target_spec);
+  GetInterfaceProviderSpec(mojom::kServiceManager_ConnectorSpec,
+                           source_info.interface_provider_specs, &source_spec);
+  CallOnConnect(source_info, source_spec, target_spec,
+                MakeRequest(&interface_provider));
+  interface_provider->GetInterface(interface_name, std::move(interface_pipe));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// ServiceContext, private:
+
+void ServiceContext::CallOnConnect(const ServiceInfo& source_info,
+                                   const InterfaceProviderSpec& source_spec,
+                                   const InterfaceProviderSpec& target_spec,
+                                   mojom::InterfaceProviderRequest interfaces) {
   auto registry =
       base::MakeUnique<InterfaceRegistry>(mojom::kServiceManager_ConnectorSpec);
   registry->Bind(std::move(interfaces), local_info_.identity, target_spec,
                  source_info.identity, source_spec);
 
-  // Acknowledge the request regardless of whether it's accepted.
-  callback.Run();
-
   if (!service_->OnConnect(source_info, registry.get()))
     return;
 
@@ -117,9 +149,6 @@ void ServiceContext::OnConnect(
       std::make_pair(raw_registry, std::move(registry)));
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// ServiceContext, private:
-
 void ServiceContext::OnConnectionError() {
   // Note that the Service doesn't technically have to quit now, it may live
   // on to service existing connections. All existing Connectors however are

services/service_manager/public/cpp/service_context.h

@@ -112,6 +112,16 @@ class ServiceContext : public mojom::Service {
   void OnConnect(const ServiceInfo& source_info,
                  mojom::InterfaceProviderRequest interfaces,
                  const OnConnectCallback& callback) override;
+  void OnBindInterface(
+      const ServiceInfo& source_info,
+      const std::string& interface_name,
+      mojo::ScopedMessagePipeHandle interface_pipe,
+      const OnBindInterfaceCallback& callback) override;
+
+  void CallOnConnect(const ServiceInfo& source_info,
+                     const InterfaceProviderSpec& source_spec,
+                     const InterfaceProviderSpec& target_spec,
+                     mojom::InterfaceProviderRequest request);
 
   void OnConnectionError();
   void OnRegistryConnectionError(InterfaceRegistry* registry);

services/service_manager/public/interfaces/service.mojom

@@ -67,4 +67,32 @@ interface Service {
   //    seek to bind interface implementations exported by the target.
   //
   OnConnect(ServiceInfo source_info, InterfaceProvider&? interfaces) => ();
+
+  // Called when a request to bind an interface is received from another
+  // ("source") service. This is the result of that service calling
+  // BindInterface() on a Connector. By the time this method is called, the
+  // service manager has already completed a policy check to determine that this
+  // interface can be bound.
+  //
+  // The Service must respond to acknowledge receipt of the request.
+  //
+  // Parameters:
+  //
+  //  source_info
+  //    Contains the source identity and interface provider specs.
+  //
+  //  interface_name
+  //    The name of the interface to be bound.
+  //
+  //  interface_pipe
+  //    The message pipe to bind the interface implementation to.
+  //
+  // TODO(beng): It occurs to me that |source_info| leaks all metadata about
+  //             the source's capability requirements to the target. This seems
+  //             undesirable. The metadata supplied here should be germane to
+  //             fulfilling this request and no more.
+  // TODO(beng): This should replace OnConnect().
+  OnBindInterface(ServiceInfo source_info,
+                  string interface_name,
+                  handle<message_pipe> interface_pipe) => ();
 };

services/service_manager/service_manager.cc

@@ -47,6 +47,21 @@ const char kCapability_InstancePerChild[] =
     "service_manager:instance_per_child";
 const char kCapability_ServiceManager[] = "service_manager:service_manager";
 
+bool Succeeded(mojom::ConnectResult result) {
+  return result == mojom::ConnectResult::SUCCEEDED;
+}
+
+void RunCallback(ConnectParams* params,
+                 mojom::ConnectResult result,
+                 const std::string& user_id) {
+  if (!params->connect_callback().is_null()) {
+    params->connect_callback().Run(result, user_id);
+    return;
+  }
+  if (!params->bind_interface_callback().is_null())
+    params->bind_interface_callback().Run(result, user_id);
+}
+
 }  // namespace
 
 Identity CreateServiceManagerIdentity() {
@@ -125,18 +140,19 @@ class ServiceManager::Instance
     Stop();
   }
 
-  bool ConnectToService(std::unique_ptr<ConnectParams>* connect_params) {
+  bool OnConnect(std::unique_ptr<ConnectParams>* in_params) {
     if (!service_.is_bound())
       return false;
 
-    std::unique_ptr<ConnectParams> params(std::move(*connect_params));
+    std::unique_ptr<ConnectParams> params(std::move(*in_params));
     if (!params->connect_callback().is_null()) {
       params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED,
                                      identity_.user_id());
     }
 
     InterfaceProviderSpecMap specs;
-    Instance* source = service_manager_->GetExistingInstance(params->source());
+    Instance* source =
+        service_manager_->GetExistingInstance(params->source());
     if (source)
       specs = source->interface_provider_specs_;
 
@@ -148,6 +164,48 @@ class ServiceManager::Instance
     return true;
   }
 
+  bool OnBindInterface(std::unique_ptr<ConnectParams>* in_params) {
+    if (!service_.is_bound())
+      return false;
+
+    std::unique_ptr<ConnectParams> params(std::move(*in_params));
+    InterfaceProviderSpecMap source_specs;
+    InterfaceProviderSpec source_connection_spec;
+    Instance* source =
+        service_manager_->GetExistingInstance(params->source());
+    if (source) {
+      source_specs = source->interface_provider_specs_;
+      source_connection_spec = source->GetConnectionSpec();
+    }
+
+    InterfaceSet exposed = GetInterfacesToExpose(source_connection_spec,
+                                                 identity_,
+                                                 GetConnectionSpec());
+    bool allowed = (exposed.size() == 1 && exposed.count("*") == 1) ||
+        exposed.count(params->interface_name()) > 0;
+    if (!allowed) {
+      std::stringstream ss;
+      ss << "Connection InterfaceProviderSpec prevented service: "
+         << params->source().name() << " from binding interface: "
+         << params->interface_name() << " exposed by: " << identity_.name();
+      LOG(ERROR) << ss.str();
+      params->bind_interface_callback().Run(mojom::ConnectResult::ACCESS_DENIED,
+                                            identity_.user_id());
+      return false;
+    }
+
+    params->bind_interface_callback().Run(mojom::ConnectResult::SUCCEEDED,
+                                          identity_.user_id());
+
+    pending_service_connections_++;
+    service_->OnBindInterface(
+        ServiceInfo(params->source(), source_specs),
+        params->interface_name(),
+        params->TakeInterfaceRequestPipe(),
+        base::Bind(&Instance::OnConnectComplete, base::Unretained(this)));
+    return true;
+  }
+
   void OnConnectComplete() {
     DCHECK_GT(pending_service_connections_, 0);
     pending_service_connections_--;
@@ -229,72 +287,65 @@ class ServiceManager::Instance
 
   // mojom::Connector implementation:
   void StartService(
-      const Identity& target,
+      const Identity& in_target,
       mojo::ScopedMessagePipeHandle service_handle,
       mojom::PIDReceiverRequest pid_receiver_request) override {
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result))
+      return;
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+
     mojom::ServicePtr service;
     service.Bind(mojom::ServicePtrInfo(std::move(service_handle), 0));
-    ConnectImpl(
-        target,
-        mojom::InterfaceProviderRequest(),
-        std::move(service),
-        std::move(pid_receiver_request),
-        base::Bind(
-            &service_manager::ServiceManager::Instance::EmptyConnectCallback,
-            weak_factory_.GetWeakPtr()));
+    params->set_client_process_info(std::move(service),
+                                    std::move(pid_receiver_request));
+    service_manager_->Connect(
+        std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
 
-  void Connect(const service_manager::Identity& target,
+  void Connect(const service_manager::Identity& in_target,
                mojom::InterfaceProviderRequest remote_interfaces,
                const ConnectCallback& callback) override {
-    ConnectImpl(target, std::move(remote_interfaces), mojom::ServicePtr(),
-                mojom::PIDReceiverRequest(), callback);
+    Identity target = in_target;
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, mojom::kInheritUserID);
+      return;
+    }
+
+    std::unique_ptr<ConnectParams> params(new ConnectParams);
+    params->set_source(identity_);
+    params->set_target(target);
+    params->set_remote_interfaces(std::move(remote_interfaces));
+    params->set_connect_callback(callback);
+    service_manager_->Connect(
+        std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
 
-  void BindInterface(const service_manager::Identity& target,
+  void BindInterface(const service_manager::Identity& in_target,
                      const std::string& interface_name,
                      mojo::ScopedMessagePipeHandle interface_pipe,
                      const BindInterfaceCallback& callback) override {
-    mojom::InterfaceProviderPtr remote_interfaces;
-    ConnectImpl(
-        target,
-        MakeRequest(&remote_interfaces),
-        mojom::ServicePtr(),
-        mojom::PIDReceiverRequest(),
-        base::Bind(
-            &service_manager::ServiceManager::Instance::BindCallbackWrapper,
-            weak_factory_.GetWeakPtr(),
-            callback));
-    remote_interfaces->GetInterface(interface_name, std::move(interface_pipe));
-    // TODO(beng): Rather than just forwarding thru to InterfaceProvider, do
-    //             manifest policy intersection here.
-  }
-
-  void ConnectImpl(const service_manager::Identity& in_target,
-                   mojom::InterfaceProviderRequest remote_interfaces,
-                   mojom::ServicePtr service,
-                   mojom::PIDReceiverRequest pid_receiver_request,
-                   const ConnectCallback& callback) {
     Identity target = in_target;
-    if (target.user_id() == mojom::kInheritUserID)
-      target.set_user_id(identity_.user_id());
-
-    if (!ValidateIdentity(target, callback))
-      return;
-    if (!ValidateClientProcessInfo(&service, &pid_receiver_request, target,
-                                   callback)) {
+    mojom::ConnectResult result =
+        ValidateConnectParams(&target, nullptr, nullptr);
+    if (!Succeeded(result)) {
+      callback.Run(result, mojom::kInheritUserID);
       return;
     }
-    if (!ValidateConnectionSpec(target, callback))
-      return;
 
     std::unique_ptr<ConnectParams> params(new ConnectParams);
     params->set_source(identity_);
     params->set_target(target);
-    params->set_remote_interfaces(std::move(remote_interfaces));
-    params->set_client_process_info(std::move(service),
-                                    std::move(pid_receiver_request));
-    params->set_connect_callback(callback);
+    params->set_interface_request_info(interface_name,
+                                       std::move(interface_pipe));
+    params->set_bind_interface_callback(callback);
     service_manager_->Connect(
         std::move(params), nullptr, weak_factory_.GetWeakPtr());
   }
@@ -321,61 +372,66 @@ class ServiceManager::Instance
     service_manager_->AddListener(std::move(listener));
   }
 
-  bool ValidateIdentity(const Identity& identity,
-                        const ConnectCallback& callback) {
+  mojom::ConnectResult ValidateConnectParams(
+      Identity* target,
+      mojom::ServicePtr* service,
+      mojom::PIDReceiverRequest* pid_receiver_request) {
+    if (target->user_id() == mojom::kInheritUserID)
+      target->set_user_id(identity_.user_id());
+
+    mojom::ConnectResult result = ValidateIdentity(*target);
+    if (!Succeeded(result))
+      return result;
+
+    result = ValidateClientProcessInfo(service, pid_receiver_request, *target);
+    if (!Succeeded(result))
+      return result;
+    return ValidateConnectionSpec(*target);
+  }
+
+  mojom::ConnectResult ValidateIdentity(const Identity& identity) {
     if (identity.name().empty()) {
       LOG(ERROR) << "Error: empty service name.";
-      callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::INVALID_ARGUMENT;
     }
     if (!base::IsValidGUID(identity.user_id())) {
       LOG(ERROR) << "Error: invalid user_id: " << identity.user_id();
-      callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::INVALID_ARGUMENT;
     }
-    return true;
+    return mojom::ConnectResult::SUCCEEDED;
   }
 
-  bool ValidateClientProcessInfo(
+  mojom::ConnectResult ValidateClientProcessInfo(
       mojom::ServicePtr* service,
       mojom::PIDReceiverRequest* pid_receiver_request,
-      const Identity& target,
-      const ConnectCallback& callback) {
-    if (service->is_bound() || pid_receiver_request->is_pending()) {
+      const Identity& target) {
+    if (service && pid_receiver_request &&
+        (service->is_bound() || pid_receiver_request->is_pending())) {
       if (!HasCapability(GetConnectionSpec(), kCapability_ClientProcess)) {
         LOG(ERROR) << "Instance: " << identity_.name() << " attempting "
                    << "to register an instance for a process it created for "
                    << "target: " << target.name() << " without the "
                    << "service_manager{client_process} capability "
                    << "class.";
-        callback.Run(mojom::ConnectResult::ACCESS_DENIED,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::ACCESS_DENIED;
       }
 
       if (!service->is_bound() || !pid_receiver_request->is_pending()) {
         LOG(ERROR) << "Must supply both service AND "
                    << "pid_receiver_request when sending client process info";
-        callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::INVALID_ARGUMENT;
       }
       if (service_manager_->GetExistingInstance(target)) {
         LOG(ERROR) << "Cannot client process matching existing identity:"
                    << "Name: " << target.name() << " User: "
                    << target.user_id() << " Instance: " << target.instance();
-        callback.Run(mojom::ConnectResult::INVALID_ARGUMENT,
-                     mojom::kInheritUserID);
-        return false;
+        return mojom::ConnectResult::INVALID_ARGUMENT;
       }
     }
-    return true;
+    return mojom::ConnectResult::SUCCEEDED;
   }
 
-  bool ValidateConnectionSpec(const Identity& target,
-                              const ConnectCallback& callback) {
+  mojom::ConnectResult ValidateConnectionSpec(const Identity& target) {
     InterfaceProviderSpec connection_spec = GetConnectionSpec();
     // TODO(beng): Need to do the following additional policy validation of
     // whether this instance is allowed to connect using:
@@ -388,9 +444,7 @@ class ServiceManager::Instance
                  << " attempting to connect to: " << target.name()
                  << " as: " << target.user_id() << " without "
                  << " the service:service_manager{user_id} capability.";
-      callback.Run(mojom::ConnectResult::ACCESS_DENIED,
-                   mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::ACCESS_DENIED;
     }
     if (!target.instance().empty() &&
         target.instance() != target.name() &&
@@ -400,19 +454,17 @@ class ServiceManager::Instance
                  << " using Instance name: " << target.instance()
                  << " without the "
                  << "service_manager{instance_name} capability.";
-      callback.Run(mojom::ConnectResult::ACCESS_DENIED, mojom::kInheritUserID);
-      return false;
+      return mojom::ConnectResult::ACCESS_DENIED;
     }
 
     if (allow_any_application_ ||
         connection_spec.requires.find(target.name()) !=
             connection_spec.requires.end()) {
-      return true;
+      return mojom::ConnectResult::SUCCEEDED;
     }
     LOG(ERROR) << "InterfaceProviderSpec prevented connection from: "
                << identity_.name() << " to: " << target.name();
-    callback.Run(mojom::ConnectResult::ACCESS_DENIED, mojom::kInheritUserID);
-    return false;
+    return mojom::ConnectResult::ACCESS_DENIED;
   }
 
   uint32_t GenerateUniqueID() const {
@@ -782,7 +834,14 @@ void ServiceManager::NotifyServiceFailedToStart(const Identity& identity) {
 bool ServiceManager::ConnectToExistingInstance(
     std::unique_ptr<ConnectParams>* params) {
   Instance* instance = GetExistingInstance((*params)->target());
-  return instance && instance->ConnectToService(params);
+  if (instance) {
+    if ((*params)->HasInterfaceRequestInfo()) {
+      instance->OnBindInterface(params);
+      return true;
+    }
+    return instance->OnConnect(params);
+  }
+  return false;
 }
 
 ServiceManager::Instance* ServiceManager::CreateInstance(
@@ -868,10 +927,7 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
   // If name resolution failed, we drop the connection.
   if (!result) {
     LOG(ERROR) << "Failed to resolve service name: " << params->target().name();
-    if (!params->connect_callback().is_null()) {
-      params->connect_callback().Run(
-          mojom::ConnectResult::INVALID_ARGUMENT, "");
-    }
+    RunCallback(params.get(), mojom::ConnectResult::INVALID_ARGUMENT, "");
     return;
   }
 
@@ -941,8 +997,7 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
       LOG(ERROR)
           << "Error: The catalog was unable to read a manifest for service \""
           << result->name << "\".";
-      if (!params->connect_callback().is_null())
-        params->connect_callback().Run(mojom::ConnectResult::ACCESS_DENIED, "");
+      RunCallback(params.get(), mojom::ConnectResult::ACCESS_DENIED, "");
       return;
     }
 
@@ -980,18 +1035,19 @@ void ServiceManager::OnGotResolvedName(std::unique_ptr<ConnectParams> params,
 
       if (!instance->StartWithFilePath(package_path)) {
         OnInstanceError(instance);
-        if (!params->connect_callback().is_null()) {
-          params->connect_callback().Run(
-              mojom::ConnectResult::INVALID_ARGUMENT, "");
-        }
+        RunCallback(params.get(), mojom::ConnectResult::INVALID_ARGUMENT, "");
         return;
       }
     }
   }
 
   // Now that the instance has a Service, we can connect to it.
-  bool connected = instance->ConnectToService(&params);
-  DCHECK(connected);
+  if (params->HasInterfaceRequestInfo()) {
+    instance->OnBindInterface(&params);
+  } else {
+    bool connected = instance->OnConnect(&params);
+    DCHECK(connected);
+  }
 }
 
 base::WeakPtr<ServiceManager> ServiceManager::GetWeakPtr() {