Removed the ProcessHandle from the RendererClosedDetails payload

content/browser/mach_broker_mac.mm

Index: content/browser/mach_broker_mac.mm
diff --git a/content/browser/mach_broker_mac.mm b/content/browser/mach_broker_mac.mm
index f2e5f985c32a792b19455d96e9b762bc3a655eb7..d2aea0789261de3e65257000345c5f4308ea85cc 100644
--- a/content/browser/mach_broker_mac.mm
+++ b/content/browser/mach_broker_mac.mm
@@ -195,11 +195,13 @@ void MachBroker::EnsureRunning() {
   }
 }
 
-void MachBroker::AddPlaceholderForPid(base::ProcessHandle pid) {
+void MachBroker::AddPlaceholderForPid(base::ProcessHandle pid,
+                                      int child_process_id) {
   lock_.AssertAcquired();
 
   DCHECK_EQ(0u, mach_map_.count(pid));
   mach_map_[pid] = MACH_PORT_NULL;
+  child_process_id_map_[pid] = child_process_id;
 }
 
 mach_port_t MachBroker::TaskForPid(base::ProcessHandle pid) const {
@@ -222,23 +224,17 @@ void MachBroker::BrowserChildProcessCrashed(const ChildProcessData& data) {
 void MachBroker::Observe(int type,
                          const NotificationSource& source,
                          const NotificationDetails& details) {
-  // TODO(rohitrao): These notifications do not always carry the proper PIDs,
-  // especially when the renderer is already gone or has crashed.  Find a better
-  // way to listen for child process deaths.  http://crbug.com/55734
-  base::ProcessHandle handle = 0;
   switch (type) {
-    case NOTIFICATION_RENDERER_PROCESS_CLOSED:
-      handle = Details<RenderProcessHost::RendererClosedDetails>(
-          details)->handle;
-      break;
     case NOTIFICATION_RENDERER_PROCESS_TERMINATED:
-      handle = Source<RenderProcessHost>(source)->GetHandle();
+    case NOTIFICATION_RENDERER_PROCESS_CLOSED: {
+      RenderProcessHost* host = Source<RenderProcessHost>(source).ptr();
+      InvalidateChildProcessId(host->GetID());
       break;
+    }
     default:
       NOTREACHED() << "Unexpected notification";
       break;
   }
-  InvalidatePid(handle);
 }
 
 MachBroker::MachBroker() : listener_thread_started_(false) {
@@ -272,8 +268,22 @@ void MachBroker::InvalidatePid(base::ProcessHandle pid) {
                                           it->second);
   MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr) << "mach_port_deallocate";
   mach_map_.erase(it);
+  child_process_id_map_.erase(pid);
+}
+
+void MachBroker::InvalidateChildProcessId(int child_process_id) {
+  base::ProcessHandle pid = 0;
+  for (const auto& it : child_process_id_map_) {

[Mark Mentovai, 2014/11/13 14:00:10]

You need to assert that the lock is held to be able to walk this map, but that
won’t work because the lock isn’t taken until InvalidatePid().

[sramajay, 2014/11/13 15:45:28]

Ok. I can remove the InvalidatePid with the above mentioned change, in which
case i can apply the lock before using the map.

+    if (it.second == child_process_id) {

[Mark Mentovai, 2014/11/13 14:00:10]

It seems like you really want a bidirectional map, but since we don’t have that,
two maps?

But if you restructure things to only have InvalidateChildProcessId and get rid
of InvalidatePid as suggested, you can probably do this with a single map.

[sramajay, 2014/11/13 15:45:28]

Its not only InvalidatePid(), but FinalizePid() is called with ProcessHandle &
mach_prot_t obtained from the kernel, for which we need the ProcessHandle in the
map. We can't have a single map. Your comments please.

[Mark Mentovai, 2014/11/13 16:04:53]

Then it sounds like we should have a map from PID to child process ID to be used
by FinalizePid and TaskForPid, and a map from child process ID to {mach_port_t,
pid}.

I don’t see a good reason to have two parallel maps that share the same key,
which is what your change has done. Use a structure for the value type instead.
On the other hand, if you do need to access things using a child process ID key
sometimes and a PID key other times, it’s reasonable to have a map deal with
that for you, rather than manually iterating over a single map’s values.

+      pid = it.first;
+      break;
+    }
+  }
+  if (pid)
+    InvalidatePid(pid);
 }
 
+
 // static
 std::string MachBroker::GetMachPortName() {
   const base::CommandLine* command_line =