RC: Make the graph resilient to PID reuse.

Change-Id: I9695d4c554ce45c339282dcd2c70adc1e83da557
Reviewed-on: https://chromium-review.googlesource.com/1099257
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567241}

services/resource_coordinator/coordination_unit/coordination_unit_graph.cc

@@ -169,15 +169,18 @@ void CoordinationUnitGraph::DestroyCoordinationUnit(CoordinationUnitBase* cu) {
 void CoordinationUnitGraph::BeforeProcessPidChange(
     ProcessCoordinationUnitImpl* process,
     base::ProcessId new_pid) {
+  // On Windows, PIDs are agressively reused, and because not all process
+  // creation/death notifications are synchronized, it's possible for more than
+  // one CU to have the same PID. To handle this, the second and subsequent
+  // registration override earlier registrations, while unregistration will only
+  // unregister the current holder of the PID.
   if (process->process_id() != base::kNullProcessId) {
-    size_t erased = processes_by_pid_.erase(process->process_id());
-    DCHECK_EQ(1u, erased);
-  }
-  if (new_pid != base::kNullProcessId) {
-    bool inserted =
-        processes_by_pid_.insert(std::make_pair(new_pid, process)).second;
-    DCHECK(inserted);
+    auto it = processes_by_pid_.find(process->process_id());
+    if (it != processes_by_pid_.end() && it->second == process)
+      processes_by_pid_.erase(it);
   }
+  if (new_pid != base::kNullProcessId)
+    processes_by_pid_[new_pid] = process;
 }
 
 }  // namespace resource_coordinator

services/resource_coordinator/coordination_unit/coordination_unit_graph_unittest.cc

@@ -8,15 +8,22 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace resource_coordinator {
+namespace {
+
+ProcessCoordinationUnitImpl* CreateProcessCU(CoordinationUnitGraph* graph) {
+  return graph->CreateProcessCoordinationUnit(
+      CoordinationUnitID(CoordinationUnitType::kProcess,
+                         CoordinationUnitID::RANDOM_ID),
+      nullptr);
+}
+
+}  // namespace
 
 TEST(CoordinationUnitGraphTest, DestructionWhileCUSOutstanding) {
   std::unique_ptr<CoordinationUnitGraph> graph(new CoordinationUnitGraph());
 
   for (size_t i = 0; i < 10; ++i) {
-    auto* process = graph->CreateProcessCoordinationUnit(
-        CoordinationUnitID(CoordinationUnitType::kProcess,
-                           CoordinationUnitID::RANDOM_ID),
-        nullptr);
+    ProcessCoordinationUnitImpl* process = CreateProcessCU(graph.get());
     EXPECT_NE(nullptr, process);
 
     process->SetPID(i + 100);
@@ -47,11 +54,7 @@ TEST(CoordinationUnitGraphTest, FindOrCreateSystemCoordinationUnit) {
 TEST(CoordinationUnitGraphTest, GetProcessCoordinationUnitByPid) {
   CoordinationUnitGraph graph;
 
-  ProcessCoordinationUnitImpl* process = graph.CreateProcessCoordinationUnit(
-      CoordinationUnitID(CoordinationUnitType::kProcess,
-                         CoordinationUnitID::RANDOM_ID),
-      nullptr);
-
+  ProcessCoordinationUnitImpl* process = CreateProcessCU(&graph);
   EXPECT_EQ(base::kNullProcessId, process->process_id());
 
   static constexpr base::ProcessId kPid = 10;
@@ -65,4 +68,28 @@ TEST(CoordinationUnitGraphTest, GetProcessCoordinationUnitByPid) {
   EXPECT_EQ(nullptr, graph.GetProcessCoordinationUnitByPid(12));
 }
 
+TEST(CoordinationUnitGraphTest, PIDReuse) {
+  // This test emulates what happens on Windows under aggressive PID reuse,
+  // where a process termination notification can be delayed until after the
+  // PID has been reused for a new process.
+  CoordinationUnitGraph graph;
+
+  static constexpr base::ProcessId kPid = 10;
+
+  ProcessCoordinationUnitImpl* process1 = CreateProcessCU(&graph);
+  ProcessCoordinationUnitImpl* process2 = CreateProcessCU(&graph);
+
+  process1->SetPID(kPid);
+  EXPECT_EQ(process1, graph.GetProcessCoordinationUnitByPid(kPid));
+
+  // The second registration for the same PID should override the first one.
+  process2->SetPID(kPid);
+  EXPECT_EQ(process2, graph.GetProcessCoordinationUnitByPid(kPid));
+
+  // The destruction of the first process CU shouldn't clear the PID
+  // registration.
+  process1->Destruct();
+  EXPECT_EQ(process2, graph.GetProcessCoordinationUnitByPid(kPid));
+}
+
 }  // namespace resource_coordinator