memory-infra: Use new OnServicePIDReceived API

Bug: 739710
Change-Id: Ib926ae1ec013b33b7a90dcedb78960713cfa97e0
Reviewed-on: https://chromium-review.googlesource.com/565510Reviewed-by: Siddhartha S <ssid@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: Hector Dearman <hjd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487442}

services/resource_coordinator/memory_instrumentation/process_map.cc

@@ -30,23 +30,20 @@ ProcessMap::ProcessMap(service_manager::Connector* connector) : binding_(this) {
 ProcessMap::~ProcessMap() {}
 
 void ProcessMap::OnInit(std::vector<RunningServiceInfoPtr> instances) {
-  for (RunningServiceInfoPtr& instance : instances)
-    OnServiceCreated(std::move(instance));
+  for (const RunningServiceInfoPtr& instance : instances) {
+    if (instance->pid == base::kNullProcessId)
+      continue;
+    const service_manager::Identity& identity = instance->identity;
+    auto it_and_inserted = instances_.emplace(identity, instance->pid);
+    DCHECK(it_and_inserted.second);
+  }
 }
 
 void ProcessMap::OnServiceCreated(RunningServiceInfoPtr instance) {
-  if (instance->pid == base::kNullProcessId)
-    return;
-  const service_manager::Identity& identity = instance->identity;
-  DCHECK_EQ(0u, instances_.count(identity));
-  instances_.emplace(identity, instance->pid);
 }
 
 void ProcessMap::OnServiceStarted(const service_manager::Identity& identity,
                                   uint32_t pid) {
-  if (pid == base::kNullProcessId)
-    return;
-  instances_[identity] = pid;
 }
 
 void ProcessMap::OnServiceFailedToStart(const service_manager::Identity&) {}
@@ -56,7 +53,12 @@ void ProcessMap::OnServiceStopped(const service_manager::Identity& identity) {
 }
 
 void ProcessMap::OnServicePIDReceived(const service_manager::Identity& identity,
-                                      uint32_t pid) {}
+                                      uint32_t pid) {
+  if (pid == base::kNullProcessId)
+    return;
+  auto it_and_inserted = instances_.emplace(identity, pid);
+  DCHECK(it_and_inserted.second);
+}
 
 base::ProcessId ProcessMap::GetProcessId(
     const service_manager::Identity& identity) const {

services/resource_coordinator/memory_instrumentation/process_map.h

@@ -37,9 +37,7 @@ class ProcessMap : public service_manager::mojom::ServiceManagerListener {
  private:
   FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, TypicalCase);
   FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, PresentInInit);
-  FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, MissedOnServiceCreated);
-  FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, ZeroPidOnCreate_NonZeroOnStart);
-  FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, NonZeroPidOnCreate_ZeroOnStart);
+  FRIEND_TEST_ALL_PREFIXES(ProcessMapTest, NullsInInitIgnored);
 
   using Identity = service_manager::Identity;
   using RunningServiceInfoPtr = service_manager::mojom::RunningServiceInfoPtr;

services/resource_coordinator/memory_instrumentation/process_map_unittest.cc

@@ -25,30 +25,6 @@ RunningServiceInfoPtr MakeTestServiceInfo(
   return info;
 }
 
-TEST(ProcessMapTest, TypicalCase) {
-  ProcessMap process_map(nullptr);
-  service_manager::Identity id1("id1");
-  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
-  process_map.OnInit(std::vector<RunningServiceInfoPtr>());
-  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
-
-  process_map.OnServiceCreated(MakeTestServiceInfo(id1, 1 /* pid */));
-  process_map.OnServiceStarted(id1, 1 /* pid */);
-  EXPECT_EQ(static_cast<base::ProcessId>(1), process_map.GetProcessId(id1));
-
-  // Adding a separate service with a different identity should have no effect
-  // on the first identity registered.
-  service_manager::Identity id2("id2");
-  process_map.OnServiceCreated(MakeTestServiceInfo(id2, 2 /* pid */));
-  EXPECT_EQ(static_cast<base::ProcessId>(1), process_map.GetProcessId(id1));
-  EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
-
-  // Once the service is stopped, searching for its id should return a null pid.
-  process_map.OnServiceStopped(id1);
-  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
-  EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
-}
-
 TEST(ProcessMapTest, PresentInInit) {
   // Add a dummy entry so that the actual |ids| indexes are 1-based.
   std::vector<service_manager::Identity> ids{service_manager::Identity()};
@@ -68,57 +44,54 @@ TEST(ProcessMapTest, PresentInInit) {
   EXPECT_EQ(static_cast<base::ProcessId>(3), process_map.GetProcessId(ids[3]));
 }
 
-// Test that the PID for a given service is still recorded if we miss the
-// OnServiceCreated.
-TEST(ProcessMapTest, MissedOnServiceCreated) {
+TEST(ProcessMapTest, NullsInInitIgnored) {
   ProcessMap process_map(nullptr);
-
   service_manager::Identity id1("id1");
   service_manager::Identity id2("id2");
 
-  process_map.OnServiceCreated(MakeTestServiceInfo(id1, 0 /* pid */));
-  process_map.OnServiceStarted(id1, 1 /* pid */);
+  std::vector<RunningServiceInfoPtr> instances;
+  instances.push_back(MakeTestServiceInfo(id1, base::kNullProcessId));
+  instances.push_back(MakeTestServiceInfo(id2, 2));
 
-  process_map.OnServiceStarted(id2, 2 /* pid */);
+  process_map.OnInit(std::move(instances));
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
+  EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
 
+  process_map.OnServicePIDReceived(id1, 1 /* pid */);
   EXPECT_EQ(static_cast<base::ProcessId>(1), process_map.GetProcessId(id1));
   EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
 }
 
-// In some platforms The OnServiceCreated() reports a null pid, and the real
-// pid comes only in the OnServiceStarted() callback.
-TEST(ProcessMapTest, ZeroPidOnCreate_NonZeroOnStart) {
+TEST(ProcessMapTest, TypicalCase) {
   ProcessMap process_map(nullptr);
-
   service_manager::Identity id1("id1");
-  service_manager::Identity id2("id2");
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
+  process_map.OnInit(std::vector<RunningServiceInfoPtr>());
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
 
-  process_map.OnServiceCreated(MakeTestServiceInfo(id1, 0 /* pid */));
+  process_map.OnServiceCreated(MakeTestServiceInfo(id1, 1 /* pid */));
   process_map.OnServiceStarted(id1, 1 /* pid */);
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
 
-  process_map.OnServiceCreated(MakeTestServiceInfo(id2, 0 /* pid */));
-  process_map.OnServiceStarted(id2, 2 /* pid */);
-
+  process_map.OnServicePIDReceived(id1, 1 /* pid */);
   EXPECT_EQ(static_cast<base::ProcessId>(1), process_map.GetProcessId(id1));
-  EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
-}
-
-// In the opposite case, instead, test that the valid PID seen in
-// OnServiceCreated() is preserved.
-TEST(ProcessMapTest, NonZeroPidOnCreate_ZeroOnStart) {
-  ProcessMap process_map(nullptr);
 
-  service_manager::Identity id1("id1");
+  // Adding a separate service with a different identity should have no effect
+  // on the first identity registered.
   service_manager::Identity id2("id2");
-
-  process_map.OnServiceCreated(MakeTestServiceInfo(id1, 1 /* pid */));
-  process_map.OnServiceStarted(id1, 0 /* pid */);
-
   process_map.OnServiceCreated(MakeTestServiceInfo(id2, 2 /* pid */));
-  process_map.OnServiceStarted(id2, 0 /* pid */);
-
+  process_map.OnServicePIDReceived(id2, 2 /* pid */);
   EXPECT_EQ(static_cast<base::ProcessId>(1), process_map.GetProcessId(id1));
   EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
+
+  // Once the service is stopped, searching for its id should return a null pid.
+  process_map.OnServiceStopped(id1);
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id1));
+  EXPECT_EQ(static_cast<base::ProcessId>(2), process_map.GetProcessId(id2));
+
+  // Unknown identities return a null pid.
+  service_manager::Identity id3("id3");
+  EXPECT_EQ(base::kNullProcessId, process_map.GetProcessId(id3));
 }
 
 }  // namespace memory_instrumentation