[service-manager] Require valid fields in Identity

Changes Identity so that none of its fields are optional. All Identity
objects are now either empty and invalid, or fully populated and refer
to a specific, concrete service instance that did, does, or may exist at
some point.

Bug: 902590,904240
Change-Id: I5b35909afb39b2588f0841d5b1713a6f7c06c800
Reviewed-on: https://chromium-review.googlesource.com/c/1333126Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Oksana Zhuravlova <oksamyt@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#608634}

chrome/browser/chrome_content_browser_client_unittest.cc

@@ -389,7 +389,7 @@ TEST(ChromeContentBrowserClientTest, ShouldTerminateOnServiceQuit) {
     bool expect_terminate;
   } kTestCases[] = {
       // Don't terminate for invalid service names.
-      {"", false},
+      {"x", false},
       {"unknown-name", false},
       // Don't terminate for some well-known browser services.
       {content::mojom::kBrowserServiceName, false},
@@ -401,7 +401,8 @@ TEST(ChromeContentBrowserClientTest, ShouldTerminateOnServiceQuit) {
   };
   ChromeContentBrowserClient client;
   for (const auto& test : kTestCases) {
-    service_manager::Identity id(test.service_name);
+    service_manager::Identity id(test.service_name, base::Token{1, 2},
+                                 base::Token{}, base::Token{3, 4});
     EXPECT_EQ(test.expect_terminate, client.ShouldTerminateOnServiceQuit(id))
         << "for service name " << test.service_name;
   }

chrome/browser/chrome_site_per_process_browsertest.cc

@@ -1023,7 +1023,7 @@ class TestBrowserClientForSpellCheck : public ChromeContentBrowserClient {
       const service_manager::BindSourceInfo& source_info) {
     content::RenderProcessHost* host =
         content::RenderProcessHost::FromRendererInstanceId(
-            *source_info.identity.instance_id());
+            source_info.identity.instance_id());
     auto spell_check_host = std::make_unique<MockSpellCheckHost>(host);
     spell_check_host->SpellCheckHostRequest(std::move(request));
     spell_check_hosts_.push_back(std::move(spell_check_host));

chrome/browser/safe_browsing/client_side_detection_service.cc

@@ -261,8 +261,8 @@ void ClientSideDetectionService::SendModelToProcess(
   ChromeService::GetInstance()->connector()->BindInterface(
       service_manager::ServiceFilter::ByNameWithIdInGroup(
           chrome::mojom::kRendererServiceName,
-          *process->GetChildIdentity().instance_id(),
-          *process->GetChildIdentity().instance_group()),
+          process->GetChildIdentity().instance_id(),
+          process->GetChildIdentity().instance_group()),
       &phishing);
   phishing->SetPhishingModel(model);
 }

chrome/browser/spellchecker/spell_check_host_chrome_impl.cc

@@ -82,7 +82,7 @@ void SpellCheckHostChromeImpl::CallSpellingService(
   // service, calls CallSpellingServiceDone.
   content::BrowserContext* context =
       content::BrowserContext::GetBrowserContextForServiceInstanceGroup(
-          *renderer_identity_.instance_group());
+          renderer_identity_.instance_group());
   client_.RequestTextCheck(
       context, SpellingServiceClient::SPELLCHECK, text,
       base::BindOnce(&SpellCheckHostChromeImpl::CallSpellingServiceDone,

chrome/browser/spellchecker/spell_check_host_chrome_impl_mac.cc

@@ -138,7 +138,7 @@ void SpellingRequest::RequestRemoteCheck(
     const service_manager::Identity& renderer_identity) {
   BrowserContext* context =
       content::BrowserContext::GetBrowserContextForServiceInstanceGroup(
-          *renderer_identity.instance_group());
+          renderer_identity.instance_group());
 
   // |this| may be gone at callback invocation if the owner has been removed.
   client->RequestTextCheck(

chrome/browser/spellchecker/spellcheck_factory.cc

@@ -29,7 +29,7 @@ SpellcheckService* SpellcheckServiceFactory::GetForRenderer(
     const service_manager::Identity& renderer_identity) {
   content::BrowserContext* context =
       content::BrowserContext::GetBrowserContextForServiceInstanceGroup(
-          *renderer_identity.instance_group());
+          renderer_identity.instance_group());
   if (!context)
     return nullptr;
   return GetForContext(context);

chrome/browser/spellchecker/spellcheck_service.cc

@@ -183,7 +183,7 @@ void SpellcheckService::InitForRenderer(
 
   content::BrowserContext* context =
       content::BrowserContext::GetBrowserContextForServiceInstanceGroup(
-          *renderer_identity.instance_group());
+          renderer_identity.instance_group());
   if (SpellcheckServiceFactory::GetForContext(context) != this)
     return;
 
@@ -208,8 +208,8 @@ void SpellcheckService::InitForRenderer(
   spellcheck::mojom::SpellCheckerPtr spellchecker;
   ChromeService::GetInstance()->connector()->BindInterface(
       service_manager::ServiceFilter::ByNameWithIdInGroup(
-          chrome::mojom::kRendererServiceName, *renderer_identity.instance_id(),
-          *renderer_identity.instance_group()),
+          chrome::mojom::kRendererServiceName, renderer_identity.instance_id(),
+          renderer_identity.instance_group()),
       &spellchecker);
   spellchecker->Initialize(std::move(dictionaries), custom_words, enable);
 }
@@ -299,8 +299,8 @@ void SpellcheckService::OnCustomDictionaryChanged(
     ChromeService::GetInstance()->connector()->BindInterface(
         service_manager::ServiceFilter::ByNameWithIdInGroup(
             chrome::mojom::kRendererServiceName,
-            *renderer_identity.instance_id(),
-            *renderer_identity.instance_group()),
+            renderer_identity.instance_id(),
+            renderer_identity.instance_group()),
         &spellchecker);
     spellchecker->CustomDictionaryChanged(additions, deletions);
   }

chrome/browser/spellchecker/spellcheck_service_browsertest.cc

@@ -88,8 +88,8 @@ class SpellcheckServiceBrowserTest : public InProcessBrowserTest,
     ChromeService::GetInstance()->connector()->OverrideBinderForTesting(
         service_manager::ServiceFilter::ByNameWithIdInGroup(
             chrome::mojom::kRendererServiceName,
-            *renderer_identity.instance_id(),
-            *renderer_identity.instance_group()),
+            renderer_identity.instance_id(),
+            renderer_identity.instance_group()),
         spellcheck::mojom::SpellChecker::Name_,
         base::BindRepeating(&SpellcheckServiceBrowserTest::Bind,
                             base::Unretained(this)));

chrome/browser/spellchecker/test/spellcheck_content_browser_client.cc

@@ -55,7 +55,7 @@ void SpellCheckContentBrowserClient::BindSpellCheckPanelHostRequest(
     const service_manager::BindSourceInfo& source_info) {
   content::RenderProcessHost* render_process_host =
       content::RenderProcessHost::FromRendererInstanceId(
-          *source_info.identity.instance_id());
+          source_info.identity.instance_id());
   auto spell_check_panel_host =
       std::make_unique<SpellCheckMockPanelHost>(render_process_host);
   spell_check_panel_host->SpellCheckPanelHostRequest(std::move(request));

content/common/service_manager/service_manager_connection_impl_unittest.cc

@@ -42,8 +42,9 @@ TEST(ServiceManagerConnectionImplTest, ServiceLaunchThreading) {
   connection.AddEmbeddedService(kTestServiceName, info);
   connection.Start();
   service_manager::BindSourceInfo source_info(
-      {service_manager::mojom::kServiceName,
-       service_manager::kSystemInstanceGroup},
+      service_manager::Identity(service_manager::mojom::kServiceName,
+                                service_manager::kSystemInstanceGroup,
+                                base::Token{}, base::Token::CreateRandom()),
       service_manager::CapabilitySet());
   service_manager::mojom::ServiceFactoryPtr factory;
   service->OnBindInterface(

content/public/test/mock_render_process_host.cc

@@ -60,6 +60,7 @@ MockRenderProcessHost::MockRenderProcessHost(BrowserContext* browser_context)
       child_identity_(
           mojom::kRendererServiceName,
           BrowserContext::GetServiceInstanceGroupFor(browser_context),
+          base::Token::CreateRandom(),
           base::Token::CreateRandom()),
       url_loader_factory_(nullptr),
       weak_ptr_factory_(this) {

ios/web/service_manager_connection_impl_unittest.cc

@@ -51,8 +51,9 @@ TEST_F(ServiceManagerConnectionImplTest, ServiceLaunchThreading) {
   connection.AddEmbeddedService(kTestServiceName, info);
   connection.Start();
   service_manager::BindSourceInfo source_info(
-      {service_manager::mojom::kServiceName,
-       service_manager::kSystemInstanceGroup},
+      service_manager::Identity{service_manager::mojom::kServiceName,
+                                service_manager::kSystemInstanceGroup,
+                                base::Token{}, base::Token::CreateRandom()},
       service_manager::CapabilitySet());
   service_manager::mojom::ServiceFactoryPtr factory;
   service->OnBindInterface(

services/catalog/catalog.cc

@@ -226,7 +226,7 @@ void Catalog::BindCatalogRequest(
     mojom::CatalogRequest request,
     const service_manager::BindSourceInfo& source_info) {
   Instance* instance =
-      GetInstanceForGroup(*source_info.identity.instance_group());
+      GetInstanceForGroup(source_info.identity.instance_group());
   instance->BindCatalog(std::move(request));
 }
 

services/file/file_service.cc

@@ -94,7 +94,7 @@ FileService::~FileService() {
 
 void FileService::OnStart() {
   file_system_objects_.reset(new FileService::FileSystemObjects(
-      GetUserDirForInstanceGroup(*context()->identity().instance_group())));
+      GetUserDirForInstanceGroup(context()->identity().instance_group())));
   leveldb_objects_.reset(
       new FileService::LevelDBServiceObjects(file_service_runner_));
 }

services/service_manager/BUILD.gn

@@ -27,8 +27,6 @@ source_set("service_manager") {
   sources = [
     "connect_params.cc",
     "connect_params.h",
-    "connect_util.cc",
-    "connect_util.h",
     "service_manager.cc",
     "service_manager.h",
     "service_overrides.cc",

services/service_manager/connect_params.h

@@ -13,6 +13,7 @@
 #include "base/optional.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "services/service_manager/public/cpp/identity.h"
+#include "services/service_manager/public/cpp/service_filter.h"
 #include "services/service_manager/public/mojom/connector.mojom.h"
 #include "services/service_manager/public/mojom/service.mojom.h"
 
@@ -20,6 +21,12 @@ namespace service_manager {
 
 // This class represents a request for the application manager to connect to an
 // application.
+//
+// TODO(https://crbug.com/904240): This should really just be an internal type
+// within ServiceManager. At this point its only real value is in providing a
+// default response to a |BindInterface()| or |WarmService()| request when the
+// ServiceManager otherwise drops the request without responding. This is of
+// questionable value.
 class ConnectParams {
  public:
   using ConnectionCallback =
@@ -31,8 +38,9 @@ class ConnectParams {
 
   void set_source(const Identity& source) { source_ = source;  }
   const Identity& source() const { return source_; }
-  void set_target(const Identity& target) { target_ = target; }
-  const Identity& target() const { return target_; }
+
+  void set_target(const ServiceFilter& target) { target_ = target; }
+  const ServiceFilter& target() const { return target_; }
 
   void set_client_process_info(
       mojom::ServicePtr service,
@@ -70,6 +78,10 @@ class ConnectParams {
     connection_callback_ = std::move(callback);
   }
 
+  ConnectionCallback TakeConnectionCallback() {
+    return std::move(connection_callback_);
+  }
+
   void set_response_data(mojom::ConnectResult result,
                          const base::Optional<Identity>& resolved_identity) {
     result_ = result;
@@ -77,11 +89,12 @@ class ConnectParams {
   }
 
  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).
+  // The identity of the instance making this connection request.
   Identity source_;
-  // The identity of the application being connected to.
-  Identity target_;
+
+  // A filter defining the search criteria for a running target instance to
+  // which the source would like to connect.
+  ServiceFilter target_;
 
   mojom::ServicePtr service_;
   mojom::PIDReceiverRequest pid_receiver_request_;

services/service_manager/connect_util.cc

-// Copyright 2015 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "services/service_manager/connect_util.h"
-
-#include <memory>
-#include <utility>
-
-#include "base/bind.h"
-#include "services/service_manager/connect_params.h"
-#include "services/service_manager/service_manager.h"
-
-namespace service_manager {
-
-mojo::ScopedMessagePipeHandle BindInterface(
-    ServiceManager* service_manager,
-    const Identity& source,
-    const Identity& target,
-    const std::string& interface_name) {
-  std::unique_ptr<ConnectParams> params(new ConnectParams);
-  params->set_source(source);
-  params->set_target(target);
-  mojo::MessagePipe pipe;
-  params->set_interface_request_info(interface_name, std::move(pipe.handle1));
-  params->set_connection_callback(base::DoNothing());
-  service_manager->Connect(std::move(params));
-  return std::move(pipe.handle0);
-}
-
-}  // namespace service_manager

services/service_manager/connect_util.h

-// Copyright 2015 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef SERVICES_SERVICE_MANAGER_CONNECT_UTIL_H_
-#define SERVICES_SERVICE_MANAGER_CONNECT_UTIL_H_
-
-#include "mojo/public/cpp/bindings/interface_ptr.h"
-#include "mojo/public/cpp/system/handle.h"
-#include "services/service_manager/public/cpp/identity.h"
-#include "services/service_manager/public/mojom/connector.mojom.h"
-
-namespace service_manager {
-
-class ServiceManager;
-
-mojo::ScopedMessagePipeHandle BindInterface(
-    ServiceManager* service_manager,
-    const Identity& source,
-    const Identity& target,
-    const std::string& interface_name);
-
-// Must only be used by Service Manager internals and test code as it does not
-// forward capability filters. Runs |name| with a permissive capability filter.
-template <typename Interface>
-inline void BindInterface(ServiceManager* service_manager,
-                               const Identity& source,
-                               const Identity& target,
-                               mojo::InterfacePtr<Interface>* ptr) {
-  mojo::ScopedMessagePipeHandle service_handle =
-      BindInterface(service_manager, source, target, Interface::Name_);
-  ptr->Bind(mojo::InterfacePtrInfo<Interface>(std::move(service_handle), 0u));
-}
-
-template <typename Interface>
-inline void BindInterface(ServiceManager* service_manager,
-                               const Identity& source,
-                               const std::string& name,
-                               mojo::InterfacePtr<Interface>* ptr) {
-  mojo::ScopedMessagePipeHandle service_handle = BindInterface(
-      service_manager, source,
-      Identity(name, base::nullopt /* instance_group */), Interface::Name_);
-  ptr->Bind(mojo::InterfacePtrInfo<Interface>(std::move(service_handle), 0u));
-}
-
-}  // namespace service_manager
-
-#endif  // SERVICES_SERVICE_MANAGER_CONNECT_UTIL_H_

services/service_manager/public/cpp/BUILD.gn

@@ -92,6 +92,8 @@ component("cpp_types") {
 component("mojom_traits") {
   output_name = "service_manager_mojom_traits"
   sources = [
+    "identity_mojom_traits.cc",
+    "identity_mojom_traits.h",
     "service_filter_mojom_traits.cc",
     "service_filter_mojom_traits.h",
   ]

services/service_manager/public/cpp/connector.cc

@@ -42,10 +42,7 @@ void Connector::RegisterServiceInstance(
   if (!BindConnectorIfNecessary())
     return;
 
-  DCHECK(identity.instance_group() && !identity.instance_group()->is_zero());
-  DCHECK(identity.instance_id());
-  DCHECK(identity.globally_unique_id() &&
-         !identity.globally_unique_id()->is_zero());
+  DCHECK(identity.IsValid());
   DCHECK(service.is_bound() && pid_receiver_request.is_pending());
   connector_->RegisterServiceInstance(
       identity, service.PassInterface().PassHandle(),
@@ -98,6 +95,7 @@ void Connector::FilterInterfaces(const std::string& spec,
                                  mojom::InterfaceProviderPtr target) {
   if (!BindConnectorIfNecessary())
     return;
+  DCHECK(source_identity.IsValid());
   connector_->FilterInterfaces(spec, source_identity, std::move(request),
                                std::move(target));
 }

services/service_manager/public/cpp/identity.cc

@@ -10,29 +10,19 @@
 
 namespace service_manager {
 
-Identity::Identity() : Identity("") {}
-
-Identity::Identity(const std::string& name) : Identity(name, base::nullopt) {}
-
-Identity::Identity(const std::string& name,
-                   const base::Optional<base::Token>& instance_group)
-    : Identity(name, instance_group, base::nullopt) {}
+Identity::Identity() = default;
 
 Identity::Identity(const std::string& name,
-                   const base::Optional<base::Token>& instance_group,
-                   const base::Optional<base::Token>& instance_id)
-    : Identity(name, instance_group, instance_id, base::nullopt) {}
-
-Identity::Identity(const std::string& name,
-                   const base::Optional<base::Token>& instance_group,
-                   const base::Optional<base::Token>& instance_id,
-                   const base::Optional<base::Token>& globally_unique_id)
+                   const base::Token& instance_group,
+                   const base::Token& instance_id,
+                   const base::Token& globally_unique_id)
     : name_(name),
       instance_group_(instance_group),
       instance_id_(instance_id),
       globally_unique_id_(globally_unique_id) {
-  DCHECK(!instance_group_ || !instance_group_->is_zero());
-  DCHECK(!globally_unique_id_ || !globally_unique_id_->is_zero());
+  DCHECK(!name_.empty());
+  DCHECK(!instance_group_.is_zero());
+  DCHECK(!globally_unique_id_.is_zero());
 }
 
 Identity::Identity(const Identity& other) = default;
@@ -48,28 +38,20 @@ bool Identity::operator<(const Identity& other) const {
 }
 
 bool Identity::operator==(const Identity& other) const {
-  return Matches(other) && globally_unique_id_ == other.globally_unique_id_;
+  return name_ == other.name_ && instance_group_ == other.instance_group_ &&
+         instance_id_ == other.instance_id_ &&
+         globally_unique_id_ == other.globally_unique_id_;
 }
 
 bool Identity::IsValid() const {
-  return !name_.empty();
+  return !name_.empty() && !instance_group_.is_zero() &&
+         !globally_unique_id_.is_zero();
 }
 
 std::string Identity::ToString() const {
-  return base::StringPrintf(
-      "%s/%s/%s/%s",
-      instance_group_ ? instance_group_->ToString().c_str() : "*",
-      name_.c_str(), instance_id_ ? instance_id_->ToString().c_str() : "*",
-      globally_unique_id_ ? globally_unique_id_->ToString().c_str() : "*");
-}
-
-bool Identity::Matches(const Identity& other) const {
-  return name_ == other.name_ && instance_group_ == other.instance_group_ &&
-         instance_id_ == other.instance_id_;
-}
-
-Identity Identity::WithoutGloballyUniqueId() const {
-  return Identity(name_, instance_group_, instance_id_);
+  return base::StringPrintf("%s/%s/%s/%s", instance_group_.ToString().c_str(),
+                            name_.c_str(), instance_id_.ToString().c_str(),
+                            globally_unique_id_.ToString().c_str());
 }
 
 }  // namespace service_manager

services/service_manager/public/cpp/identity.h

@@ -7,55 +7,37 @@
 
 #include <string>
 
-#include "base/optional.h"
+#include "base/component_export.h"
 #include "base/token.h"
-#include "services/service_manager/public/cpp/types_export.h"
 
 namespace service_manager {
 
-// Represents the identity of a service instance. Useful to clients who want to
-// connect to running (or lazily started) service instances through the Service
-// Manager. Every Service also has a unique Identity assigned to it by the
-// Service Manager on service startup, communicated to the service instance via
-// |Service::OnStart()|.
+// Represents the identity of a single service instance running in the system.
+// Every service instance has an Identity assigned to it, either by the Service
+// Manager or by a trusted client using the privileged
+// |Connector.RegisterServiceInstance()| API. Each Identity is globally unique
+// across space and time.
 //
 // |name| is the name of the service, as specified in the service's manifest.
 //
 // |instance_group| is a base::Token representing the identity of an isolated
-// group of instances running in the system. Most services may only connect to
-// instances in the same instance group as themselves. This group is implied
-// when |instance_group| is null.
+// group of instances running in the system.
 //
-// |instance_id| identifies a more specific instance within the targeted
-// instance group, or globally if the target's instances are shared across
-// instance groups. Typically this is also null unless a client knows there are
-// multiple instances of the target service and wants to connect to a specific
-// one.
+// |instance_id| identifies a more specific instance within the instance's
+// group, or globally if the service's instances are shared across groups. Note
+// that at any given time there is only a single running service instance with
+// any given combination of |name|, |instance_group|, and |instance_id|.
 //
-// Finally, every service instance also has a globally unique ID Token
-// associated with it. This Token is generated and assigned by the Service
-// Manager on instance startup.
-//
-// If an interface request targets an Identity with a specific globally unique
-// ID, that request will be processed only if the instance identified by that ID
-// is still running.
-//
-// TODO(https://crbug.com/902590): Also consider whether partial Identity
-// values used for service instance resolution (i.e. passed to
-// |Connector.BindInterface()|) should be their own kind of type.
-class SERVICE_MANAGER_PUBLIC_CPP_TYPES_EXPORT Identity {
+// |globally_unique_id| is a randomly generated token whose sole purpose is to
+// differentiate identities of instances over time where all three other fields
+// are identical.
+class COMPONENT_EXPORT(SERVICE_MANAGER_CPP_TYPES) Identity {
  public:
   Identity();
-  explicit Identity(const std::string& name);
-  Identity(const std::string& name,
-           const base::Optional<base::Token>& instance_group);
-  Identity(const std::string& name,
-           const base::Optional<base::Token>& instance_group,
-           const base::Optional<base::Token>& instance_id);
-  Identity(const std::string& name,
-           const base::Optional<base::Token>& instance_group,
-           const base::Optional<base::Token>& instance_id,
-           const base::Optional<base::Token>& globally_unique_id);
+  explicit Identity(const std::string& name,
+                    const base::Token& instance_group,
+                    const base::Token& instance_id,
+                    const base::Token& globally_unique_id);
   Identity(const Identity& other);
   ~Identity();
 
@@ -68,37 +50,16 @@ class SERVICE_MANAGER_PUBLIC_CPP_TYPES_EXPORT Identity {
 
   std::string ToString() const;
 
-  // Indicates whether this Identity matches |other|, meaning that the name,
-  // instance group, and instance ID are identical in both. Note in particular
-  // that GUID is completely ignored, so this is not a strict equality test.
-  // For strict equality including GUID, use |operator==|.
-  bool Matches(const Identity& other) const;
-
-  // Returns a copy of this Identity with no globally unique ID specified.
-  Identity WithoutGloballyUniqueId() const;
-
   const std::string& name() const { return name_; }
-  const base::Optional<base::Token>& instance_group() const {
-    return instance_group_;
-  }
-  void set_instance_group(const base::Optional<base::Token>& instance_group) {
-    instance_group_ = instance_group;
-  }
-  const base::Optional<base::Token>& instance_id() const {
-    return instance_id_;
-  }
-  void set_instance_id(const base::Optional<base::Token>& instance_id) {
-    instance_id_ = instance_id;
-  }
-  const base::Optional<base::Token>& globally_unique_id() const {
-    return globally_unique_id_;
-  }
+  const base::Token& instance_group() const { return instance_group_; }
+  const base::Token& instance_id() const { return instance_id_; }
+  const base::Token& globally_unique_id() const { return globally_unique_id_; }
 
  private:
   std::string name_;
-  base::Optional<base::Token> instance_group_;
-  base::Optional<base::Token> instance_id_;
-  base::Optional<base::Token> globally_unique_id_;
+  base::Token instance_group_;
+  base::Token instance_id_;
+  base::Token globally_unique_id_;
 };
 
 }  // namespace service_manager

services/service_manager/public/cpp/identity.typemap

@@ -8,6 +8,7 @@ traits_headers =
     [ "//services/service_manager/public/cpp/identity_mojom_traits.h" ]
 public_deps = [
   "//services/service_manager/public/cpp:cpp_types",
+  "//services/service_manager/public/cpp:mojom_traits",
 ]
 
 type_mappings = [ "service_manager.mojom.Identity=::service_manager::Identity" ]

services/service_manager/public/cpp/identity_mojom_traits.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/service_manager/public/cpp/identity_mojom_traits.h"
+
+#include <string>
+
+#include "mojo/public/cpp/base/token_mojom_traits.h"
+
+namespace mojo {
+
+bool StructTraits<service_manager::mojom::IdentityDataView,
+                  service_manager::Identity>::
+    Read(service_manager::mojom::IdentityDataView data,
+         service_manager::Identity* out) {
+  std::string name;
+  if (!data.ReadName(&name) || name.empty())
+    return false;
+
+  base::Token instance_group;
+  if (!data.ReadInstanceGroup(&instance_group) || instance_group.is_zero())
+    return false;
+
+  base::Token instance_id;
+  if (!data.ReadInstanceId(&instance_id))
+    return false;
+
+  base::Token globally_unique_id;
+  if (!data.ReadGloballyUniqueId(&globally_unique_id) ||
+      globally_unique_id.is_zero()) {
+    return false;
+  }
+
+  *out = service_manager::Identity(name, instance_group, instance_id,
+                                   globally_unique_id);
+  return true;
+}
+
+}  // namespace mojo

services/service_manager/public/cpp/identity_mojom_traits.h

@@ -5,53 +5,35 @@
 #ifndef SERVICES_SERVICE_MANAGER_PUBLIC_CPP_IDENTITY_STRUCT_TRAITS_H_
 #define SERVICES_SERVICE_MANAGER_PUBLIC_CPP_IDENTITY_STRUCT_TRAITS_H_
 
+#include "base/component_export.h"
 #include "services/service_manager/public/cpp/identity.h"
-#include "services/service_manager/public/mojom/connector.mojom.h"
+#include "services/service_manager/public/mojom/connector.mojom-shared.h"
 
 namespace mojo {
 
 template <>
-struct COMPONENT_EXPORT(SERVICE_MANAGER_MOJOM)
+struct COMPONENT_EXPORT(SERVICE_MANAGER_MOJOM_TRAITS)
     StructTraits<service_manager::mojom::IdentityDataView,
                  service_manager::Identity> {
   static const std::string& name(const service_manager::Identity& identity) {
+    DCHECK(!identity.name().empty());
     return identity.name();
   }
-  static const base::Optional<base::Token>& instance_group(
+  static const base::Token& instance_group(
       const service_manager::Identity& identity) {
     return identity.instance_group();
   }
-  static const base::Optional<base::Token>& instance_id(
+  static const base::Token& instance_id(
       const service_manager::Identity& identity) {
     return identity.instance_id();
   }
-  static const base::Optional<base::Token>& globally_unique_id(
+  static const base::Token& globally_unique_id(
       const service_manager::Identity& identity) {
     return identity.globally_unique_id();
   }
 
   static bool Read(service_manager::mojom::IdentityDataView data,
-                   service_manager::Identity* out) {
-    std::string name;
-    if (!data.ReadName(&name))
-      return false;
-
-    base::Optional<base::Token> instance_group;
-    if (!data.ReadInstanceGroup(&instance_group))
-      return false;
-
-    base::Optional<base::Token> instance_id;
-    if (!data.ReadInstanceId(&instance_id))
-      return false;
-
-    base::Optional<base::Token> globally_unique_id;
-    if (!data.ReadGloballyUniqueId(&globally_unique_id))
-      return false;
-
-    *out = service_manager::Identity(name, instance_group, instance_id,
-                                     globally_unique_id);
-    return true;
-  }
+                   service_manager::Identity* out);
 };
 
 }  // namespace mojo

services/service_manager/public/cpp/service_filter.cc

@@ -20,6 +20,8 @@ ServiceFilter::ServiceFilter(const Identity& identity)
 
 ServiceFilter::~ServiceFilter() = default;
 
+ServiceFilter& ServiceFilter::operator=(const ServiceFilter& other) = default;
+
 // static
 ServiceFilter ServiceFilter::ByName(const std::string& service_name) {
   return ServiceFilter(service_name, base::nullopt /* instance_group */,
@@ -53,10 +55,7 @@ ServiceFilter ServiceFilter::ByNameWithIdInGroup(
 
 // static
 ServiceFilter ServiceFilter::ForExactIdentity(const Identity& identity) {
-  DCHECK(identity.instance_group() && !identity.instance_group()->is_zero());
-  DCHECK(identity.instance_id());
-  DCHECK(identity.globally_unique_id() &&
-         !identity.globally_unique_id()->is_zero());
+  DCHECK(identity.IsValid());
   return ServiceFilter(identity.name(), identity.instance_group(),
                        identity.instance_id(), identity.globally_unique_id());
 }

services/service_manager/public/cpp/service_filter.h

@@ -35,6 +35,8 @@ class COMPONENT_EXPORT(SERVICE_MANAGER_CPP_TYPES) ServiceFilter {
 
   ~ServiceFilter();
 
+  ServiceFilter& operator=(const ServiceFilter& other);
+
   // Returns a ServiceFilter which only filters by service name. When locating a
   // service instance using such a filter, the Service Manager will consider an
   // instance to be a potential match as long as its service name matches

services/service_manager/public/cpp/service_test.cc

@@ -23,7 +23,7 @@ ServiceTestClient::~ServiceTestClient() {}
 
 void ServiceTestClient::OnStart() {
   test_->OnStartCalled(context()->connector(), context()->identity().name(),
-                       *context()->identity().instance_group());
+                       context()->identity().instance_group());
 }
 
 void ServiceTestClient::OnBindInterface(
@@ -83,7 +83,9 @@ void ServiceTest::SetUp() {
   context_ = std::make_unique<ServiceContext>(CreateService(),
                                               mojo::MakeRequest(&service));
   background_service_manager_->RegisterService(
-      Identity(test_name_, kSystemInstanceGroup), std::move(service), nullptr);
+      Identity(test_name_, kSystemInstanceGroup, base::Token{},
+               base::Token::CreateRandom()),
+      std::move(service), nullptr);
   connector_ = context_->connector();
   run_loop.Run();
 }

services/service_manager/public/cpp/test/test_connector_factory.cc

@@ -36,7 +36,8 @@ class TestConnectorImplBase : public mojom::Connector {
  public:
   TestConnectorImplBase(const base::Token& test_instance_group,
                         bool release_service_on_quit_request)
-      : test_instance_group_(test_instance_group),
+      : fake_guid_(base::Token::CreateRandom()),
+        test_instance_group_(test_instance_group),
         release_service_on_quit_request_(release_service_on_quit_request) {}
   ~TestConnectorImplBase() override = default;
 
@@ -53,7 +54,8 @@ class TestConnectorImplBase : public mojom::Connector {
                             base::Unretained(this), service_context_ptr));
     service_contexts_.emplace(service_context_ptr, std::move(service_context));
     (*service_ptr)
-        ->OnStart(Identity("TestConnectorFactory", test_instance_group_),
+        ->OnStart(Identity("TestConnectorFactory", test_instance_group_,
+                           base::Token{}, base::Token::CreateRandom()),
                   base::BindOnce(&TestConnectorImplBase::OnStartCallback,
                                  base::Unretained(this), service_context_ptr));
   }
@@ -90,12 +92,13 @@ class TestConnectorImplBase : public mojom::Connector {
     DCHECK(service_ptr) << "Binding interface for unregistered service "
                         << service_filter.service_name();
     (*service_ptr)
-        ->OnBindInterface(
-            BindSourceInfo(
-                Identity("TestConnectorFactory", test_instance_group_),
-                CapabilitySet()),
-            interface_name, std::move(interface_pipe), base::DoNothing());
-    std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, Identity());
+        ->OnBindInterface(BindSourceInfo(Identity("TestConnectorFactory",
+                                                  test_instance_group_,
+                                                  base::Token{}, fake_guid_),
+                                         CapabilitySet()),
+                          interface_name, std::move(interface_pipe),
+                          base::DoNothing());
+    std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, base::nullopt);
   }
 
   void WarmService(const ServiceFilter& service_filter,
@@ -127,6 +130,7 @@ class TestConnectorImplBase : public mojom::Connector {
     NOTREACHED();
   }
 
+  const base::Token fake_guid_;
   base::Token test_instance_group_;
   const bool release_service_on_quit_request_;
   std::map<ServiceContext*, std::unique_ptr<ServiceContext>> service_contexts_;
@@ -195,7 +199,8 @@ class ProxiedServiceConnector : public mojom::Connector {
       TestConnectorFactory::NameToServiceProxyMap* proxies,
       TestConnectorFactory::NameToServiceHandlerMap* handlers,
       const base::Token& test_instance_group)
-      : proxies_(proxies),
+      : fake_guid_(base::Token::CreateRandom()),
+        proxies_(proxies),
         handlers_(handlers),
         test_instance_group_(test_instance_group) {}
 
@@ -229,10 +234,11 @@ class ProxiedServiceConnector : public mojom::Connector {
         << "TestConnectorFactory received a BindInterface request for an "
         << "unregistered service '" << service_filter.service_name() << "'";
     proxy->OnBindInterface(
-        BindSourceInfo(Identity("TestConnectorFactory", test_instance_group_),
+        BindSourceInfo(Identity("TestConnectorFactory", test_instance_group_,
+                                base::Token{}, fake_guid_),
                        CapabilitySet()),
         interface_name, std::move(interface_pipe), base::DoNothing());
-    std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, Identity());
+    std::move(callback).Run(mojom::ConnectResult::SUCCEEDED, base::nullopt);
   }
 
   void WarmService(const ServiceFilter& filter,
@@ -264,6 +270,7 @@ class ProxiedServiceConnector : public mojom::Connector {
     NOTREACHED();
   }
 
+  const base::Token fake_guid_;
   TestConnectorFactory::NameToServiceProxyMap* const proxies_;
   TestConnectorFactory::NameToServiceHandlerMap* const handlers_;
   const base::Token test_instance_group_;
@@ -322,7 +329,8 @@ mojom::ServiceRequest TestConnectorFactory::RegisterInstance(
     const std::string& service_name) {
   mojom::ServicePtr proxy;
   mojom::ServiceRequest request = mojo::MakeRequest(&proxy);
-  proxy->OnStart(Identity(service_name, test_instance_group_),
+  proxy->OnStart(Identity(service_name, test_instance_group_, base::Token{},
+                          base::Token::CreateRandom()),
                  base::BindOnce(&TestConnectorFactory::OnStartResponseHandler,
                                 base::Unretained(this), service_name));
   service_proxies_[service_name] = std::move(proxy);

services/service_manager/public/mojom/connector.mojom

@@ -34,51 +34,24 @@ struct Identity {
   // running service instances can only reach (via Connector) other service
   // instances running in the same instance group. This facilitates a mode of
   // isolation which is useful for modeling, e.g., different users in a multi-
-  // user system.
-  //
-  // If this is null in an Identity passed to |Connector.BindInterface()|, it is
-  // implied that the connecting service wants to connect to an instance in its
-  // own instance group.
-  //
-  // Only services with appropriate permissions (declared in manifest options
-  // via |can_connect_to_instances_in_any_group|) may explicitly choose other
-  // instance groups when making outgoing service interface requests.
-  //
-  // When a new service instance is initialized and the Service Manager provides
-  // its Identity via |Service.OnStart()|, that Identity always includes a valid
-  // instance group ID.
-  mojo_base.mojom.Token? instance_group;
+  // user system. Must always be non-zero.
+  mojo_base.mojom.Token instance_group;
 
-  // When making outgoing connections to a service, the |instance_id| field may
-  // be used to select one of multiple instances within the instance group. This
-  // is only meaningful if the target service is not designated as a singleton
-  // service in its manifest.
+  // Disambiguates multiple instances of the same service potentially running
+  // within the same instance group. Used for more granular isolation of
+  // instances. May be zero.
   //
-  // May be (and is typically) null, indicating an instance with the default
-  // instance ID.
-  mojo_base.mojom.Token? instance_id;
+  // At any given moment, there can be at most ONE service instance running with
+  // any given combination of |name|, |instance_group|, and |instance_id|.
+  mojo_base.mojom.Token instance_id;
 
   // A Token which globally and uniquely identifies a service instance across
-  // space and time.
-  //
-  // This value is generated and assigned by the Service Manager when starting
-  // a new instance and is given in the Identity passed to the instance via
-  // |Service.OnStart()|.
-  //
-  // This may also be used to reliably correlate events on a
-  // ServiceManagerListener interface with a specific service instance, given
-  // that other metadata (like |instance_id| above, or service process PID) may
-  // be reused over short periods of time and can thus lead to ambiguity in the
-  // relationship among multiple such events.
-  //
-  // NOTE: If a specific |globally_unique_id| is provided in an Identity passed
-  // to |Connector.BindInterface()|, the Service Manager will only establish a
-  // connection to the target service if all other provided fields above match a
-  // running instance AND that instance has the provided |globally_unique_id|.
-  // The Service Manager will never start a new instance on behalf of such a
-  // request. That is to say, clients cannot elicit new instance creation with
-  // a specific |globally_unique_id|.
-  mojo_base.mojom.Token? globally_unique_id;
+  // space and time. This value is generated and assigned by the Service Manager
+  // when starting a new instance, or is given to the Service Manager by a
+  // privileged client using the |Connector.RegisterServiceInstance()| API.
+  //
+  // In either case, this value is always unique to a single service instance.
+  mojo_base.mojom.Token globally_unique_id;
 };
 
 // Static metadata about a service the Service Manager knows about. This is

services/service_manager/runner/host/service_process_launcher.cc

@@ -106,8 +106,8 @@ mojom::ServicePtr ServiceProcessLauncher::Start(const Identity& target,
   child_command_line->AppendArguments(parent_command_line, false);
   child_command_line->AppendSwitchASCII(switches::kServiceName, target.name());
 #ifndef NDEBUG
-  child_command_line->AppendSwitchASCII(
-      "g", target.instance_group().value_or(base::Token{}).ToString());
+  child_command_line->AppendSwitchASCII("g",
+                                        target.instance_group().ToString());
 #endif
 
   if (!IsUnsandboxedSandboxType(sandbox_type)) {

services/service_manager/service_manager.h

@@ -36,9 +36,9 @@ namespace service_manager {
 
 class ServiceContext;
 
-// Creates an identity for the Service Manager, used when the Service Manager
-// connects to services.
-Identity CreateServiceManagerIdentity();
+// Creates an identity for the singular Service Manager instance which is always
+// present in the system.
+const Identity& GetServiceManagerInstanceIdentity();
 
 class ServiceManager {
  public:
@@ -78,9 +78,11 @@ class ServiceManager {
                        mojom::ServicePtr service,
                        mojom::PIDReceiverRequest pid_receiver_request);
 
-  // Determine information about |Identity| from its manifests. Returns
+  // Determine information about |service_name| from its manifests. Returns
   // false if the identity does not have a catalog entry.
-  bool QueryCatalog(const Identity& identity, std::string* sandbox_type);
+  bool QueryCatalog(const std::string& service_name,
+                    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
@@ -124,9 +126,7 @@ class ServiceManager {
   // Called by an Instance as it's being destroyed.
   void OnInstanceStopped(const Identity& identity);
 
-  // Returns a running instance matching |identity|. This might be an instance
-  // running as in a different instance group or with a different instance ID if
-  // the service is shared across instance groups or is a singleton.
+  // Returns a running instance identified by |identity|.
   Instance* GetExistingInstance(const Identity& identity) const;
 
   // Erases any identities mapping to |instance|. Following this call it is
@@ -138,13 +138,7 @@ class ServiceManager {
 
   void NotifyServicePIDReceived(const Identity& identity, base::ProcessId pid);
 
-  // Attempts to complete the connection requested by |params| by connecting to
-  // an existing instance. If there is an existing instance, |params| is taken,
-  // and this function returns true.
-  bool ConnectToExistingInstance(std::unique_ptr<ConnectParams>* params);
-
-  Instance* CreateInstance(const Identity& source,
-                           const Identity& target,
+  Instance* CreateInstance(const Identity& identity,
                            InstanceType instance_type,
                            const InterfaceProviderSpecMap& specs,
                            const catalog::ServiceOptions& options);
@@ -152,15 +146,15 @@ class ServiceManager {
   // Called from the instance implementing mojom::ServiceManager.
   void AddListener(mojom::ServiceManagerListenerPtr listener);
 
-  void CreateServiceWithFactory(const Identity& service_factory,
+  void CreateServiceWithFactory(const ServiceFilter& service_factory_filter,
                                 const std::string& name,
                                 mojom::ServiceRequest request,
                                 mojom::PIDReceiverPtr pid_receiver);
-  // Returns a running ServiceFactory for |service_factory_identity|.
-  // If there is not one running one is started for |source_identity|.
-  mojom::ServiceFactory* GetServiceFactory(
-      const Identity& service_factory_identity);
-  void OnServiceFactoryLost(const Identity& which);
+
+  // Returns a running ServiceFactory for |filter|. If there is not one running,
+  // one is started.
+  mojom::ServiceFactory* GetServiceFactory(const ServiceFilter& filter);
+  void OnServiceFactoryLost(const ServiceFilter& which);
 
   base::WeakPtr<ServiceManager> GetWeakPtr();
 
@@ -178,7 +172,7 @@ class ServiceManager {
   // Instance still has an entry in |instances_|.
   Instance* service_manager_instance_;
 
-  std::map<Identity, mojom::ServiceFactoryPtr> service_factories_;
+  std::map<ServiceFilter, mojom::ServiceFactoryPtr> service_factories_;
   mojo::InterfacePtrSet<mojom::ServiceManagerListener> listeners_;
   base::Callback<void(const Identity&)> instance_quit_callback_;
   std::unique_ptr<ServiceProcessLauncherFactory>

services/service_manager/standalone/context.cc

@@ -26,8 +26,8 @@
 #include "build/build_config.h"
 #include "services/catalog/catalog.h"
 #include "services/service_manager/connect_params.h"
-#include "services/service_manager/connect_util.h"
 #include "services/service_manager/public/cpp/constants.h"
+#include "services/service_manager/public/cpp/service_filter.h"
 #include "services/service_manager/runner/common/switches.h"
 #include "services/service_manager/runner/host/service_process_launcher_factory.h"
 #include "services/service_manager/service_manager.h"
@@ -107,9 +107,9 @@ void Context::Run(const std::string& name, base::RepeatingClosure on_quit) {
   service_manager_->SetInstanceQuitCallback(
       base::BindRepeating(&OnInstanceQuit, name, std::move(on_quit)));
 
-  std::unique_ptr<ConnectParams> params(new ConnectParams);
-  params->set_source(CreateServiceManagerIdentity());
-  params->set_target(Identity(name, kSystemInstanceGroup));
+  auto params = std::make_unique<ConnectParams>();
+  params->set_source(GetServiceManagerInstanceIdentity());
+  params->set_target(ServiceFilter::ByNameInGroup(name, kSystemInstanceGroup));
   service_manager_->Connect(std::move(params));
 }
 

services/service_manager/tests/connect/connect_test.mojom

@@ -6,6 +6,7 @@ module service_manager.test.mojom;
 
 import "mojo/public/mojom/base/token.mojom";
 import "services/service_manager/public/mojom/connector.mojom";
+import "services/service_manager/public/mojom/service_filter.mojom";
 
 interface ConnectTestService {
   GetTitle() => (string title);
@@ -36,9 +37,10 @@ interface StandaloneApp {
 interface IdentityTest {
   // Attempts to connect to mojo:connect_test_class_app as |target|.
   // The callback takes the connection response result, and the identity
-  // mojo:connect_test_class_app was run as, which should match |target|.
-  ConnectToClassAppWithIdentity(service_manager.mojom.Identity target) =>
-      (int32 connection_result, service_manager.mojom.Identity target);
+  // mojo:connect_test_class_app was run as, which should match |filter| or be
+  // null on error.
+  ConnectToClassAppWithFilter(service_manager.mojom.ServiceFilter filter) =>
+      (int32 connection_result, service_manager.mojom.Identity? identity);
 };
 
 struct ConnectionState {

services/service_manager/tests/connect/connect_test_app.cc

@@ -115,7 +115,7 @@ class ConnectTestApp : public Service,
     std::move(callback).Run("APP");
   }
   void GetInstanceId(GetInstanceIdCallback callback) override {
-    std::move(callback).Run(*context()->identity().instance_id());
+    std::move(callback).Run(context()->identity().instance_id());
   }
 
   // test::mojom::StandaloneApp:
@@ -169,18 +169,18 @@ class ConnectTestApp : public Service,
   }
 
   // test::mojom::IdentityTest:
-  void ConnectToClassAppWithIdentity(
-      const service_manager::Identity& target,
-      ConnectToClassAppWithIdentityCallback callback) override {
+  void ConnectToClassAppWithFilter(
+      const service_manager::ServiceFilter& filter,
+      ConnectToClassAppWithFilterCallback callback) override {
     mojom::ConnectResult result;
     base::Optional<Identity> resolved_identity;
     base::RunLoop loop;
     context()->connector()->WarmService(
-        target, base::BindOnce(&OnConnectResult, loop.QuitClosure(), &result,
+        filter, base::BindOnce(&OnConnectResult, loop.QuitClosure(), &result,
                                &resolved_identity));
     base::MessageLoopCurrent::ScopedNestableTaskAllower allow;
     loop.Run();
-    std::move(callback).Run(static_cast<int32_t>(result), *resolved_identity);
+    std::move(callback).Run(static_cast<int32_t>(result), resolved_identity);
   }
 
   void OnGotTitle(ConnectToAllowedAppInBlockedPackageCallback* callback,

services/service_manager/tests/connect/connect_test_class_app.cc

@@ -64,7 +64,7 @@ class ConnectTestClassApp : public Service,
     std::move(callback).Run("CLASS APP");
   }
   void GetInstanceId(GetInstanceIdCallback callback) override {
-    std::move(callback).Run(*context()->identity().instance_id());
+    std::move(callback).Run(context()->identity().instance_id());
   }
 
   // test::mojom::ClassInterface:

services/service_manager/tests/connect/connect_test_exe.cc

@@ -45,7 +45,7 @@ class Target : public service_manager::Service,
   }
 
   void GetInstanceId(GetInstanceIdCallback callback) override {
-    std::move(callback).Run(*context()->identity().instance_id());
+    std::move(callback).Run(context()->identity().instance_id());
   }
 
   service_manager::BinderRegistry registry_;

services/service_manager/tests/connect/connect_test_package.cc

@@ -121,7 +121,7 @@ class ProvidedService : public Service,
   }
 
   void GetInstanceId(GetInstanceIdCallback callback) override {
-    std::move(callback).Run(*service_binding_.identity().instance_id());
+    std::move(callback).Run(service_binding_.identity().instance_id());
   }
 
   // test::mojom::BlockedInterface:
@@ -130,16 +130,16 @@ class ProvidedService : public Service,
   }
 
   // test::mojom::IdentityTest:
-  void ConnectToClassAppWithIdentity(
-      const service_manager::Identity& target,
-      ConnectToClassAppWithIdentityCallback callback) override {
+  void ConnectToClassAppWithFilter(
+      const service_manager::ServiceFilter& filter,
+      ConnectToClassAppWithFilterCallback callback) override {
     mojom::ConnectResult result;
     base::Optional<Identity> resolved_identity;
     base::RunLoop loop(base::RunLoop::Type::kNestableTasksAllowed);
     service_binding_.GetConnector()->WarmService(
-        target, base::BindOnce(&QuitLoop, &loop, &result, &resolved_identity));
+        filter, base::BindOnce(&QuitLoop, &loop, &result, &resolved_identity));
     loop.Run();
-    std::move(callback).Run(static_cast<int32_t>(result), *resolved_identity);
+    std::move(callback).Run(static_cast<int32_t>(result), resolved_identity);
   }
 
   // base::SimpleThread:
@@ -243,7 +243,7 @@ class ConnectTestService : public Service,
   }
 
   void GetInstanceId(GetInstanceIdCallback callback) override {
-    std::move(callback).Run(*service_binding_.identity().instance_id());
+    std::move(callback).Run(service_binding_.identity().instance_id());
   }
 
   void OnConnectionError() {

services/service_manager/tests/connect/connect_unittest.cc

@@ -73,10 +73,10 @@ void ReceiveQueryResult(mojom::ServiceInfoPtr* out_info,
 }
 
 void ReceiveConnectionResult(mojom::ConnectResult* out_result,
-                             Identity* out_target,
+                             base::Optional<Identity>* out_target,
                              base::RunLoop* loop,
                              int32_t in_result,
-                             const Identity& in_identity) {
+                             const base::Optional<Identity>& in_identity) {
   *out_result = static_cast<mojom::ConnectResult>(in_result);
   *out_target = in_identity;
   loop->Quit();
@@ -177,8 +177,9 @@ class ConnectTest : public testing::Test,
     mojom::ServiceRequest request = mojo::MakeRequest(&proxy);
     mojom::PIDReceiverPtr pid_receiver;
     service_manager_.RegisterService(
-        Identity(service_name, kSystemInstanceGroup), std::move(proxy),
-        mojo::MakeRequest(&pid_receiver));
+        Identity(service_name, kSystemInstanceGroup, base::Token{},
+                 base::Token::CreateRandom()),
+        std::move(proxy), mojo::MakeRequest(&pid_receiver));
     pid_receiver->SetPID(1);
     return request;
   }
@@ -236,12 +237,11 @@ TEST_F(ConnectTest, BindInterface) {
 TEST_F(ConnectTest, Instances) {
   const base::Token kInstanceIdA{1, 2};
   const base::Token kInstanceIdB{3, 4};
-  Identity identity_a(kTestAppName, base::nullopt /* instance_group */,
-                      kInstanceIdA);
+  auto filter_a = ServiceFilter::ByNameWithId(kTestAppName, kInstanceIdA);
   base::Token instance_a1, instance_a2;
   test::mojom::ConnectTestServicePtr service_a1;
   {
-    connector()->BindInterface(identity_a, &service_a1);
+    connector()->BindInterface(filter_a, &service_a1);
     base::RunLoop loop;
     service_a1->GetInstanceId(
         base::BindLambdaForTesting([&](const base::Token& instance_id) {
@@ -252,7 +252,7 @@ TEST_F(ConnectTest, Instances) {
   }
   test::mojom::ConnectTestServicePtr service_a2;
   {
-    connector()->BindInterface(identity_a, &service_a2);
+    connector()->BindInterface(filter_a, &service_a2);
     base::RunLoop loop;
     service_a2->GetInstanceId(
         base::BindLambdaForTesting([&](const base::Token& instance_id) {
@@ -263,12 +263,11 @@ TEST_F(ConnectTest, Instances) {
   }
   EXPECT_EQ(instance_a1, instance_a2);
 
-  Identity identity_b(kTestAppName, base::nullopt /* instance_group */,
-                      kInstanceIdB);
+  auto filter_b = ServiceFilter::ByNameWithId(kTestAppName, kInstanceIdB);
   base::Token instance_b;
   test::mojom::ConnectTestServicePtr service_b;
   {
-    connector()->BindInterface(identity_b, &service_b);
+    connector()->BindInterface(filter_b, &service_b);
     base::RunLoop loop;
     service_b->GetInstanceId(
         base::BindLambdaForTesting([&](const base::Token& instance_id) {
@@ -287,8 +286,7 @@ TEST_F(ConnectTest, ConnectWithGloballyUniqueId) {
   target->WaitForStart();
 
   Identity specific_identity = target->identity();
-  EXPECT_TRUE(specific_identity.globally_unique_id().has_value());
-  EXPECT_FALSE(specific_identity.globally_unique_id()->is_zero());
+  EXPECT_TRUE(specific_identity.IsValid());
 
   // First connect with a basic identity.
   test::mojom::ConnectTestServicePtr proxy;
@@ -479,65 +477,68 @@ TEST_F(ConnectTest, MAYBE_ConnectWithoutExplicitClassBlocked) {
   loop.Run();
 }
 
-TEST_F(ConnectTest, ConnectAsDifferentUser_Allowed) {
+TEST_F(ConnectTest, ConnectToDifferentGroup_Allowed) {
   test::mojom::IdentityTestPtr identity_test;
-  connector()->BindInterface(kTestAppName, &identity_test);
+  connector()->BindInterface(ServiceFilter::ByName(kTestAppName),
+                             &identity_test);
   mojom::ConnectResult result;
-  Identity target(kTestClassAppName, base::Token::CreateRandom());
-  Identity result_identity;
+  auto filter = ServiceFilter::ByNameInGroup(kTestClassAppName,
+                                             base::Token::CreateRandom());
+  base::Optional<Identity> result_identity;
   {
     base::RunLoop loop;
-    identity_test->ConnectToClassAppWithIdentity(
-        target,
+    identity_test->ConnectToClassAppWithFilter(
+        filter,
         base::Bind(&ReceiveConnectionResult, &result, &result_identity, &loop));
     loop.Run();
   }
   EXPECT_EQ(result, mojom::ConnectResult::SUCCEEDED);
-  EXPECT_EQ(target.name(), result_identity.name());
-  EXPECT_TRUE(result_identity.instance_group().has_value());
-  EXPECT_EQ(target.instance_group(), result_identity.instance_group());
-  EXPECT_TRUE(result_identity.instance_id().has_value());
-  EXPECT_EQ(result_identity.instance_id().value(), base::Token(0, 0));
-  EXPECT_TRUE(result_identity.globally_unique_id().has_value());
+  ASSERT_TRUE(result_identity);
+  EXPECT_EQ(filter.service_name(), result_identity->name());
+  EXPECT_EQ(*filter.instance_group(), result_identity->instance_group());
+  EXPECT_TRUE(result_identity->instance_id().is_zero());
+  EXPECT_FALSE(result_identity->globally_unique_id().is_zero());
 }
 
-TEST_F(ConnectTest, ConnectAsDifferentUser_Blocked) {
+TEST_F(ConnectTest, ConnectToDifferentGroup_Blocked) {
   test::mojom::IdentityTestPtr identity_test;
-  connector()->BindInterface(kTestAppAName, &identity_test);
+  connector()->BindInterface(ServiceFilter::ByName(kTestAppAName),
+                             &identity_test);
   mojom::ConnectResult result;
-  Identity target(kTestClassAppName, base::Token::CreateRandom());
-  Identity result_identity;
+  auto filter = ServiceFilter::ByNameInGroup(kTestClassAppName,
+                                             base::Token::CreateRandom());
+  base::Optional<Identity> result_identity;
   {
     base::RunLoop loop;
-    identity_test->ConnectToClassAppWithIdentity(
-        target,
+    identity_test->ConnectToClassAppWithFilter(
+        filter,
         base::Bind(&ReceiveConnectionResult, &result, &result_identity, &loop));
     loop.Run();
   }
   EXPECT_EQ(mojom::ConnectResult::ACCESS_DENIED, result);
-  EXPECT_FALSE(target == result_identity);
+  EXPECT_FALSE(result_identity);
 }
 
-TEST_F(ConnectTest, ConnectWithDifferentInstanceName_Blocked) {
+TEST_F(ConnectTest, ConnectWithDifferentInstanceId_Blocked) {
   test::mojom::IdentityTestPtr identity_test;
-  connector()->BindInterface(kTestAppAName, &identity_test);
+  connector()->BindInterface(ServiceFilter::ByName(kTestAppAName),
+                             &identity_test);
 
   mojom::ConnectResult result;
-  Identity target(kTestClassAppName, base::nullopt /* instance_group */,
-                  base::Token::CreateRandom());
-  Identity result_identity;
+  auto filter = ServiceFilter::ByNameWithId(kTestClassAppName,
+                                            base::Token::CreateRandom());
+  base::Optional<Identity> result_identity;
   base::RunLoop loop;
-  identity_test->ConnectToClassAppWithIdentity(
-      target, base::BindRepeating(&ReceiveConnectionResult, &result,
+  identity_test->ConnectToClassAppWithFilter(
+      filter, base::BindRepeating(&ReceiveConnectionResult, &result,
                                   &result_identity, &loop));
   loop.Run();
   EXPECT_EQ(mojom::ConnectResult::ACCESS_DENIED, result);
-  EXPECT_FALSE(target == result_identity);
+  EXPECT_FALSE(result_identity);
 }
 
 // There are various other tests (service manager, lifecycle) that test valid
-// client
-// process specifications. This is the only one for blocking.
+// client process specifications. This is the only one for blocking.
 TEST_F(ConnectTest, ConnectToClientProcess_Blocked) {
   base::Process process;
   mojom::ConnectResult result =
@@ -572,7 +573,7 @@ TEST_F(ConnectTest, AllUsersSingleton) {
                        &first_resolved_identity));
     loop.Run();
     ASSERT_TRUE(first_resolved_identity);
-    EXPECT_NE(*first_resolved_identity->instance_group(),
+    EXPECT_NE(first_resolved_identity->instance_group(),
               singleton_instance_group);
   }
   {

services/service_manager/tests/service_manager/service_manager_listener_unittest.cc

@@ -131,8 +131,9 @@ class ServiceManagerListenerTest : public testing::Test, public Service {
     mojom::ServiceRequest request = mojo::MakeRequest(&proxy);
     mojom::PIDReceiverPtr pid_receiver;
     service_manager_.RegisterService(
-        Identity(service_name, kSystemInstanceGroup), std::move(proxy),
-        mojo::MakeRequest(&pid_receiver));
+        Identity(service_name, kSystemInstanceGroup, base::Token{},
+                 base::Token::CreateRandom()),
+        std::move(proxy), mojo::MakeRequest(&pid_receiver));
     pid_receiver->SetPID(fake_pid);
     return request;
   }
@@ -158,8 +159,7 @@ TEST_F(ServiceManagerListenerTest, InstancesHaveUniqueIdentity) {
   uint32_t pid1;
   WaitForServiceStarted(&identity1, &pid1);
   EXPECT_EQ(kTestTargetServiceName, identity1.name());
-  ASSERT_TRUE(identity1.globally_unique_id().has_value());
-  EXPECT_FALSE(identity1.globally_unique_id()->is_zero());
+  EXPECT_FALSE(identity1.globally_unique_id().is_zero());
   EXPECT_EQ(kTestTargetPid1, pid1);
 
   // We retain a Connector from the first instance before disconnecting it. This
@@ -177,8 +177,7 @@ TEST_F(ServiceManagerListenerTest, InstancesHaveUniqueIdentity) {
   uint32_t pid2;
   WaitForServiceStarted(&identity2, &pid2);
   EXPECT_EQ(kTestTargetServiceName, identity2.name());
-  ASSERT_TRUE(identity2.globally_unique_id().has_value());
-  EXPECT_FALSE(identity2.globally_unique_id()->is_zero());
+  EXPECT_FALSE(identity2.globally_unique_id().is_zero());
   EXPECT_EQ(kTestTargetPid2, pid2);
 
   // This is the important part of the test. The globally unique IDs of both
@@ -187,8 +186,7 @@ TEST_F(ServiceManagerListenerTest, InstancesHaveUniqueIdentity) {
   EXPECT_EQ(identity1.name(), identity2.name());
   EXPECT_EQ(identity1.instance_group(), identity2.instance_group());
   EXPECT_EQ(identity1.instance_id(), identity2.instance_id());
-  EXPECT_NE(identity1.globally_unique_id().value(),
-            identity2.globally_unique_id().value());
+  EXPECT_NE(identity1.globally_unique_id(), identity2.globally_unique_id());
 }
 
 }  // namespace

services/service_manager/tests/service_manager/service_manager_unittest.cc

@@ -306,7 +306,7 @@ class ServiceManagerTest : public test::ServiceTest,
     EXPECT_EQ("service_manager_unittest_embedder", service_name);
   }
 
-  void StartService(const Identity& identity, bool expect_service_started) {
+  void StartService(const ServiceFilter& filter, bool expect_service_started) {
     int start_count = 0;
     base::RunLoop loop;
     std::string service_name;
@@ -317,7 +317,7 @@ class ServiceManagerTest : public test::ServiceTest,
     set_service_failed_to_start_callback(base::BindRepeating(
         &OnServiceFailedToStartCallback, &failed_to_start, loop.QuitClosure()));
 
-    connector()->WarmService(service_manager::ServiceFilter(identity));
+    connector()->WarmService(filter);
     if (!expect_service_started) {
       // Wait briefly and test no new service was created.
       base::MessageLoopCurrent::Get()->task_runner()->PostDelayedTask(
@@ -328,7 +328,7 @@ class ServiceManagerTest : public test::ServiceTest,
     EXPECT_FALSE(failed_to_start);
     if (expect_service_started) {
       EXPECT_EQ(1, start_count);
-      EXPECT_EQ(identity.name(), service_name);
+      EXPECT_EQ(filter.service_name(), service_name);
     } else {
       // The callback was not invoked, nothing should have been set.
       EXPECT_EQ(0, start_count);
@@ -443,22 +443,21 @@ TEST_F(ServiceManagerTest, CreatePackagedRegularInstances) {
   // Connect to the embedder service first.
   StartEmbedderService();
 
-  Identity identity(kRegularServiceName);
-  StartService(identity, /*expect_service_started=*/true);
+  auto filter = ServiceFilter::ByName(kRegularServiceName);
+  StartService(filter, /*expect_service_started=*/true);
 
   // Retstarting with the same identity reuses the existing service.
-  StartService(identity, /*expect_service_started=*/false);
+  StartService(filter, /*expect_service_started=*/false);
 
-  // Starting with a different user ID creates a new service.
-  Identity other_user_identity(kRegularServiceName,
-                               base::Token::CreateRandom());
-  StartService(other_user_identity, /*expect_service_started=*/true);
+  // Starting with a different instance group creates a new service.
+  auto other_group_filter = ServiceFilter::ByNameInGroup(
+      kRegularServiceName, base::Token::CreateRandom());
+  StartService(other_group_filter, /*expect_service_started=*/true);
 
   // Starting with a different instance ID creates a new service as well.
-  Identity instance_identity(kRegularServiceName,
-                             base::nullopt /* instance_group */,
-                             base::Token{1, 2});
-  StartService(instance_identity, /*expect_service_started=*/true);
+  auto other_id_filter =
+      ServiceFilter::ByNameWithId(kRegularServiceName, base::Token{1, 2});
+  StartService(other_id_filter, /*expect_service_started=*/true);
 }
 
 // Tests that starting a shared instance packaged service works, and that when
@@ -473,20 +472,20 @@ TEST_F(ServiceManagerTest, CreatePackagedAllUsersInstances) {
   // Connect to the embedder service first.
   StartEmbedderService();
 
-  Identity identity(kAllUsersServiceName);
-  StartService(identity, /*expect_service_started=*/true);
+  auto filter = ServiceFilter::ByName(kAllUsersServiceName);
+  StartService(filter, /*expect_service_started=*/true);
 
-  // Start again with a different user-id, the existing service should be
+  // Start again with a different instance group. The existing service should be
   // reused.
-  Identity other_user_identity(kAllUsersServiceName,
-                               base::Token::CreateRandom());
-  StartService(other_user_identity, /*expect_service_started=*/false);
+  auto other_group_filter = ServiceFilter::ByNameInGroup(
+      kAllUsersServiceName, base::Token::CreateRandom());
+  StartService(other_group_filter, /*expect_service_started=*/false);
 
-  // Start again with a difference instance ID, in that case a new service
+  // Start again with a difference instance ID. In that case a new service
   // should get created.
-  Identity instance_identity(kAllUsersServiceName, base::Token::CreateRandom(),
-                             base::Token{1, 2});
-  StartService(instance_identity, /*expect_service_started=*/true);
+  auto other_id_filter = ServiceFilter::ByNameWithIdInGroup(
+      kAllUsersServiceName, base::Token{1, 2}, base::Token::CreateRandom());
+  StartService(other_id_filter, /*expect_service_started=*/true);
 }
 
 // Tests that creating a singleton packaged service works, and that when
@@ -498,21 +497,20 @@ TEST_F(ServiceManagerTest, CreatePackagedSingletonInstances) {
   // Connect to the embedder service first.
   StartEmbedderService();
 
-  Identity identity(kSingletonServiceName);
-  StartService(identity, /*expect_service_started=*/true);
+  auto filter = ServiceFilter::ByName(kSingletonServiceName);
+  StartService(filter, /*expect_service_started=*/true);
 
-  // Start again with a different user-id, the existing service should be
+  // Start again with a different instance group. The existing service should be
   // reused.
-  Identity other_user_identity(kSingletonServiceName,
-                               base::Token::CreateRandom());
-  StartService(other_user_identity, /*expect_service_started=*/false);
+  auto other_group_filter = ServiceFilter::ByNameInGroup(
+      kSingletonServiceName, base::Token::CreateRandom());
+  StartService(other_group_filter, /*expect_service_started=*/false);
 
-  // Start again with the same instance group but a difference instance ID, the
+  // Start again with the same instance group but a difference instance ID. The
   // existing service should still be reused.
-  // should get created.
-  Identity instance_identity(kSingletonServiceName,
-                             base::nullopt /* instance_group */, base::Token{});
-  StartService(instance_identity, /*expect_service_started=*/false);
+  auto other_id_filter =
+      ServiceFilter::ByNameWithId(kSingletonServiceName, base::Token{3, 4});
+  StartService(other_id_filter, /*expect_service_started=*/false);
 }
 
 TEST_F(ServiceManagerTest, PIDReceivedCallback) {

services/test/user_id/user_id_service.cc

@@ -34,7 +34,7 @@ void UserIdService::BindUserIdRequest(
 }
 
 void UserIdService::GetInstanceGroup(GetInstanceGroupCallback callback) {
-  std::move(callback).Run(context()->identity().instance_group().value());
+  std::move(callback).Run(context()->identity().instance_group());
 }
 
 }  // namespace user_id