Add OnServicePIDReceived to ServiceManagerListener

This provides a more consistent way for listeners to get the PID of a service. The existing APIs for doing this (for example the pid argument of OnServiceStarted) will be removed in a follow up. Bug: 739710 Change-Id: Ia41cdb12e412bd478aefb1481431db1cd2824265 Reviewed-on: https://chromium-review.googlesource.com/563366 Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Reviewed-by: Ken Rockot <rockot@chromium.org> Commit-Queue: Hector Dearman <hjd@chromium.org> Cr-Commit-Position: refs/heads/master@{#485596}
Denger-Network · Jul 11, 2017 · 64b7ed1 · 64b7ed1
1 parent b27231d
commit 64b7ed1
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 4 deletions.
diff --git a/mash/task_viewer/task_viewer.cc b/mash/task_viewer/task_viewer.cc
@@ -203,6 +203,8 @@ class TaskViewerContents
     }
     NOTREACHED();
   }
+  void OnServicePIDReceived(const service_manager::Identity& identity,
+                            uint32_t pid) override {}
 
   bool ContainsIdentity(const service_manager::Identity& identity) const {
     for (auto& it : instances_) {

diff --git a/services/resource_coordinator/memory_instrumentation/process_map.cc b/services/resource_coordinator/memory_instrumentation/process_map.cc
@@ -55,6 +55,9 @@ void ProcessMap::OnServiceStopped(const service_manager::Identity& identity) {
   instances_.erase(identity);
 }
 
+void ProcessMap::OnServicePIDReceived(const service_manager::Identity& identity,
+                                      uint32_t pid) {}
+
 base::ProcessId ProcessMap::GetProcessId(
     const service_manager::Identity& identity) const {
   auto it = instances_.find(identity);

diff --git a/services/resource_coordinator/memory_instrumentation/process_map.h b/services/resource_coordinator/memory_instrumentation/process_map.h
@@ -50,6 +50,8 @@ class ProcessMap : public service_manager::mojom::ServiceManagerListener {
   void OnServiceStarted(const Identity&, uint32_t pid) override;
   void OnServiceFailedToStart(const Identity&) override;
   void OnServiceStopped(const Identity&) override;
+  void OnServicePIDReceived(const service_manager::Identity& identity,
+                            uint32_t pid) override;
 
   mojo::Binding<service_manager::mojom::ServiceManagerListener> binding_;
   std::map<Identity, base::ProcessId> instances_;

diff --git a/services/service_manager/public/interfaces/service_manager.mojom b/services/service_manager/public/interfaces/service_manager.mojom
@@ -30,6 +30,9 @@ interface ServiceManagerListener {
   // assigned to it.
   OnServiceStarted(Identity identity, uint32 pid);
 
+  // Called when a pid is available for the service.
+  OnServicePIDReceived(Identity identity, uint32 pid);
+
   // Called when a service failed to start. (typically because it was shutdown
   // before the manager heard back from the service).
   OnServiceFailedToStart(Identity identity);

diff --git a/services/service_manager/service_manager.cc b/services/service_manager/service_manager.cc
@@ -612,6 +612,7 @@ class ServiceManager::Instance
     }
 #endif
     pid_ = pid;
+    service_manager_->NotifyServicePIDReceived(identity_, pid_);
   }
 
   void OnServiceLost(
@@ -976,7 +977,7 @@ void ServiceManager::OnInstanceUnreachable(Instance* instance) {
 }
 
 void ServiceManager::OnInstanceStopped(const Identity& identity) {
-  listeners_.ForAllPtrs([identity](mojom::ServiceManagerListener* listener) {
+  listeners_.ForAllPtrs([&identity](mojom::ServiceManagerListener* listener) {
     listener->OnServiceStopped(identity);
   });
   if (!instance_quit_callback_.is_null())
@@ -1026,15 +1027,22 @@ void ServiceManager::EraseInstanceIdentity(Instance* instance) {
 void ServiceManager::NotifyServiceStarted(const Identity& identity,
                                           base::ProcessId pid) {
   listeners_.ForAllPtrs(
-      [identity, pid](mojom::ServiceManagerListener* listener) {
+      [&identity, pid](mojom::ServiceManagerListener* listener) {
         listener->OnServiceStarted(identity, pid);
       });
 }
 
 void ServiceManager::NotifyServiceFailedToStart(const Identity& identity) {
+  listeners_.ForAllPtrs([&identity](mojom::ServiceManagerListener* listener) {
+    listener->OnServiceFailedToStart(identity);
+  });
+}
+
+void ServiceManager::NotifyServicePIDReceived(const Identity& identity,
+                                              base::ProcessId pid) {
   listeners_.ForAllPtrs(
-      [identity](mojom::ServiceManagerListener* listener) {
-        listener->OnServiceFailedToStart(identity);
+      [&identity, pid](mojom::ServiceManagerListener* listener) {
+        listener->OnServicePIDReceived(identity, pid);
       });
 }
 

diff --git a/services/service_manager/service_manager.h b/services/service_manager/service_manager.h
@@ -121,6 +121,8 @@ class ServiceManager {
   void NotifyServiceStarted(const Identity& identity, base::ProcessId pid);
   void NotifyServiceFailedToStart(const Identity& identity);
 
+  void NotifyServicePIDReceived(const Identity& identity, base::ProcessId pid);
+
   // Attempt 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.

diff --git a/services/service_manager/tests/lifecycle/lifecycle_unittest.cc b/services/service_manager/tests/lifecycle/lifecycle_unittest.cc
@@ -110,6 +110,15 @@ class InstanceState : public mojom::ServiceManagerListener {
       destruction_loop_ = nullptr;
     }
   }
+  void OnServicePIDReceived(const service_manager::Identity& identity,
+                            uint32_t pid) override {
+    for (auto& instance : instances_) {
+      if (instance.second.identity == identity) {
+        instance.second.pid = pid;
+        break;
+      }
+    }
+  }
 
   // All currently running instances.
   std::map<std::string, Instance> instances_;

diff --git a/services/service_manager/tests/service_manager/service_manager_unittest.cc b/services/service_manager/tests/service_manager/service_manager_unittest.cc
@@ -154,6 +154,13 @@ class ServiceManagerTest : public test::ServiceTest,
     service_failed_to_start_callback_ = callback;
   }
 
+  using ServicePIDReceivedCallback =
+      base::Callback<void(const service_manager::Identity&, uint32_t pid)>;
+  void set_service_pid_received_callback(
+      const ServicePIDReceivedCallback& callback) {
+    service_pid_received_callback_ = callback;
+  }
+
   void StartTarget() {
     base::FilePath target_path;
     CHECK(base::PathService::Get(base::DIR_EXE, &target_path));
@@ -256,6 +263,11 @@ class ServiceManagerTest : public test::ServiceTest,
       }
     }
   }
+  void OnServicePIDReceived(const service_manager::Identity& identity,
+                            uint32_t pid) override {
+    if (!service_pid_received_callback_.is_null())
+      service_pid_received_callback_.Run(identity, pid);
+  }
 
   void OnConnectionCompleted(mojom::ConnectResult, const Identity&) {}
 
@@ -266,6 +278,7 @@ class ServiceManagerTest : public test::ServiceTest,
   std::unique_ptr<base::RunLoop> wait_for_instances_loop_;
   ServiceStartedCallback service_started_callback_;
   ServiceFailedToStartCallback service_failed_to_start_callback_;
+  ServicePIDReceivedCallback service_pid_received_callback_;
   base::Process target_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceManagerTest);
@@ -318,6 +331,16 @@ void OnServiceFailedToStartCallback(
   continuation.Run();
 }
 
+void OnServicePIDReceivedCallback(std::string* service_name,
+                                  uint32_t* serivce_pid,
+                                  const base::Closure& continuation,
+                                  const service_manager::Identity& identity,
+                                  uint32_t pid) {
+  *service_name = identity.name();
+  *serivce_pid = pid;
+  continuation.Run();
+}
+
 // Tests that creating connecting to a singleton packaged service work.
 TEST_F(ServiceManagerTest, CreatePackagedSingletonInstance) {
   AddListenerAndWaitForApplications();
@@ -363,4 +386,26 @@ TEST_F(ServiceManagerTest, CreatePackagedSingletonInstance) {
   }
 }
 
+TEST_F(ServiceManagerTest, PIDReceivedCallback) {
+  AddListenerAndWaitForApplications();
+
+  {
+    base::RunLoop loop;
+    std::string service_name;
+    uint32_t pid = 0u;
+    set_service_pid_received_callback(
+        base::BindRepeating(&OnServicePIDReceivedCallback, &service_name, &pid,
+                            loop.QuitClosure()));
+    bool failed_to_start = false;
+    set_service_failed_to_start_callback(base::BindRepeating(
+        &OnServiceFailedToStartCallback, &failed_to_start, loop.QuitClosure()));
+
+    connector()->StartService("service_manager_unittest_embedder");
+    loop.Run();
+    EXPECT_FALSE(failed_to_start);
+    EXPECT_EQ("service_manager_unittest_embedder", service_name);
+    EXPECT_NE(pid, 0u);
+  }
+}
+
 }  // namespace service_manager
diff --git a/services/video_capture/test/device_factory_provider_test.h b/services/video_capture/test/device_factory_provider_test.h
@@ -34,6 +34,8 @@ class ServiceManagerListenerImpl
                         uint32_t pid) override {}
   void OnServiceFailedToStart(
       const service_manager::Identity& identity) override {}
+  void OnServicePIDReceived(const service_manager::Identity& identity,
+                            uint32_t pid) override {}
 
   MOCK_METHOD1(OnServiceStopped,
                void(const service_manager::Identity& identity));