[ServiceManager] Eliminate parent-child relationship between services

This change eliminates the property that non-singleton services are
owned by the first service that connects to them. Instead, singleton
and non-singleton services are treated identically by the Service
Manager: they go away either (1) when the service requests so or
(2) when the Service Manager itself goes away.

The motivation for this change is that (a) there are situations where
this "tree ownership" model doesn't make sense (see linked bug), and
(b) there is nothing currently in the codebase that is utilizing the
tree ownership model. If desired, the tree ownership model could be
resurrected on a per-service basis by adding a "session manager" attribute
to a service's manifest, which would denote that that service should own
all of the services that it directly or transitively connects to.

This CL also changes Service Manager's destructor to perform
two-phase shutdown on its owned Instances. This change eliminates
possible deadlock in the following situation:
- An Instance A blocks in its destructor for its Runner to complete
- That Runner blocks completion on the corresponding Service A shutting down
- Service A blocks its shutdown on some distinct Service B shutting down
- Service B is waiting to receive a connection error in order to initiate
  shutdown
- ServiceManager never deletes Instance B since it is blocked waiting for
  the destructor of Instance A to return

An example of such a situation is found in connect_test_package.cc:
ConnectTestService clears its ProvidedService instances in its OnStop() method,
while ProvidedService does not exit its destructor until it receives a
connection error. Before this CL, deadlock was not tickled because the
ProvidedServices were always killed when the first service that
connected to them was killed, which happened to be before ConnectTestService
was killed.

BUG=672863

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

services/service_manager/service_manager.cc

@@ -101,9 +101,9 @@ class ServiceManager::Instance
     DCHECK_NE(mojom::kInvalidInstanceID, id_);
   }
 
-  ~Instance() override {
-    // Shutdown all bindings before we close the runner. This way the process
-    // should see the pipes closed and exit, as well as waking up any potential
+  void Stop() {
+    // Shutdown all bindings. This way the process should see the pipes closed
+    // and exit, as well as waking up any potential
     // sync/WaitForIncomingResponse().
     service_.reset();
     if (pid_receiver_binding_.is_bound())
@@ -118,6 +118,10 @@ class ServiceManager::Instance
       // Notify the ServiceManager that this Instance is really going away.
       service_manager_->OnInstanceStopped(identity_);
     }
+  }
+
+  ~Instance() override {
+    Stop();
 
     // Release |runner_| so that if we are called back to OnRunnerCompleted()
     // we know we're in the destructor.
@@ -125,21 +129,6 @@ class ServiceManager::Instance
     runner.reset();
   }
 
-  Instance* parent() const { return parent_; }
-
-  void AddChild(std::unique_ptr<Instance> child) {
-    child->parent_ = this;
-    children_.insert(std::make_pair(child.get(), std::move(child)));
-  }
-
-  void RemoveChild(Instance* child) {
-    auto it = children_.find(child);
-    DCHECK(it != children_.end());
-
-    // Deletes |child|.
-    children_.erase(it);
-  }
-
   bool ConnectToService(std::unique_ptr<ConnectParams>* connect_params) {
     if (!service_.is_bound())
       return false;
@@ -473,8 +462,6 @@ class ServiceManager::Instance
   mojo::BindingSet<mojom::ServiceManager> service_manager_bindings_;
   mojo::AssociatedBinding<mojom::ServiceControl> control_binding_;
   base::ProcessId pid_ = base::kNullProcessId;
-  Instance* parent_ = nullptr;
-  InstanceMap children_;
   State state_;
 
   // The number of outstanding OnConnect requests which are in flight.
@@ -567,11 +554,20 @@ ServiceManager::~ServiceManager() {
   // hitting bindings DCHECKs, since the ServiceManager or Catalog may at any
   // given time own in-flight responders for Instances' Connector requests.
   std::unique_ptr<Instance> service_manager_instance;
-  auto iter = root_instances_.find(service_manager_instance_);
-  DCHECK(iter != root_instances_.end());
+  auto iter = instances_.find(service_manager_instance_);
+  DCHECK(iter != instances_.end());
   service_manager_instance = std::move(iter->second);
 
-  root_instances_.clear();
+  // Stop all of the instances before destroying any of them. This ensures that
+  // all Services will receive connection errors and avoids possible deadlock in
+  // the case where one Instance's destructor blocks waiting for its Runner to
+  // quit, while that Runner's corresponding Service blocks its shutdown on a
+  // distinct Service receiving a connection error.
+  for (const auto& instance : instances_)
+    instance.first->Stop();
+  service_manager_instance->Stop();
+
+  instances_.clear();
 }
 
 void ServiceManager::SetServiceOverrides(
@@ -642,16 +638,11 @@ void ServiceManager::OnInstanceError(Instance* instance) {
     return;
 
   EraseInstanceIdentity(instance);
-  if (instance->parent()) {
-    // Deletes |instance|.
-    instance->parent()->RemoveChild(instance);
-  } else {
-    auto it = root_instances_.find(instance);
-    DCHECK(it != root_instances_.end());
+  auto it = instances_.find(instance);
+  DCHECK(it != instances_.end());
 
-    // Deletes |instance|.
-    root_instances_.erase(it);
-  }
+  // Deletes |instance|.
+  instances_.erase(it);
 }
 
 void ServiceManager::OnInstanceUnreachable(Instance* instance) {
@@ -769,11 +760,7 @@ ServiceManager::Instance* ServiceManager::CreateInstance(
   auto instance = base::MakeUnique<Instance>(this, target, specs);
   Instance* raw_instance = instance.get();
 
-  Instance* source_instance = GetExistingInstance(source);
-  if (source_instance)
-    source_instance->AddChild(std::move(instance));
-  else
-    root_instances_.insert(std::make_pair(raw_instance, std::move(instance)));
+  instances_.insert(std::make_pair(raw_instance, std::move(instance)));
 
   // NOTE: |instance| has been passed elsewhere. Use |raw_instance| from this
   // point forward. It's safe for the extent of this method.

services/service_manager/service_manager.h

@@ -162,17 +162,16 @@ class ServiceManager {
 
   std::unique_ptr<ServiceOverrides> service_overrides_;
 
-  // Ownership of all root Instances. Non-root Instances are owned by their
-  // parent Instance.
+  // Ownership of all Instances.
   using InstanceMap = std::map<Instance*, std::unique_ptr<Instance>>;
-  InstanceMap root_instances_;
+  InstanceMap instances_;
 
   // Maps service identities to reachable instances. Note that the Instance*
   // values here are NOT owned by this map.
   std::map<Identity, Instance*> identity_to_instance_;
 
   // Always points to the ServiceManager's own Instance. Note that this
-  // Instance still has an entry in |root_instances_|.
+  // Instance still has an entry in |instances_|.
   Instance* service_manager_instance_;
 
   // Tracks the names of instances that are allowed to field connection requests

services/service_manager/tests/lifecycle/lifecycle_unittest.cc

@@ -23,7 +23,6 @@ namespace service_manager {
 namespace {
 
 const char kTestAppName[] = "lifecycle_unittest_app";
-const char kTestParentName[] = "lifecycle_unittest_parent";
 const char kTestExeName[] = "lifecycle_unittest_exe";
 const char kTestPackageName[] = "lifecycle_unittest_package";
 const char kTestPackageAppNameA[] = "lifecycle_unittest_package_app_a";
@@ -426,33 +425,4 @@ TEST_F(LifecycleTest, Exe_TerminateProcess) {
   EXPECT_EQ(0u, instances()->GetNewInstanceCount());
 }
 
-TEST_F(LifecycleTest, ShutdownTree) {
-  // Verifies that Instances are destroyed when their creator is.
-  std::unique_ptr<Connection> parent_connection =
-      connector()->Connect(kTestParentName);
-  test::mojom::ParentPtr parent;
-  parent_connection->GetInterface(&parent);
-
-  // This asks kTestParentName to open a connection to kTestAppName and blocks
-  // on a response from a Ping().
-  {
-    base::RunLoop loop;
-    parent->ConnectToChild(base::Bind(&QuitLoop, &loop));
-    loop.Run();
-  }
-
-  // Should now have two new instances (parent and child).
-  EXPECT_EQ(2u, instances()->GetNewInstanceCount());
-  EXPECT_TRUE(instances()->HasInstanceForName(kTestParentName));
-  EXPECT_TRUE(instances()->HasInstanceForName(kTestAppName));
-
-  parent->Quit();
-
-  // Quitting the parent should cascade-quit the child.
-  WaitForInstanceDestruction();
-  EXPECT_EQ(0u, instances()->GetNewInstanceCount());
-  EXPECT_FALSE(instances()->HasInstanceForName(kTestParentName));
-  EXPECT_FALSE(instances()->HasInstanceForName(kTestAppName));
-}
-
 }  // namespace service_manager