Removed the ProcessHandle from the RendererClosedDetails payload

content/browser/mach_broker_mac.mm

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/mach_broker_mac.h"
 
 #include <bsm/libbsm.h>
 #include <servers/bootstrap.h>
 
 #include "base/bind.h"
@@ skipping 177 matching lines @@
     // Intentional leak.  This thread is never joined or reaped.
     MachListenerThreadDelegate* thread = new MachListenerThreadDelegate(this);
     if (thread->Init()) {
       base::PlatformThread::CreateNonJoinable(0, thread);
     } else {
       LOG(ERROR) << "Failed to initialize the MachListenerThreadDelegate";
     }
   }
 }
 
-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 {
   base::AutoLock lock(lock_);
   MachBroker::MachMap::const_iterator it = mach_map_.find(pid);
   if (it == mach_map_.end())
     return MACH_PORT_NULL;
   return it->second;
 }
 
 void MachBroker::BrowserChildProcessHostDisconnected(
     const ChildProcessData& data) {
   InvalidatePid(data.handle);
 }
 
 void MachBroker::BrowserChildProcessCrashed(const ChildProcessData& data) {
   InvalidatePid(data.handle);
 }
 
 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;
+    case NOTIFICATION_RENDERER_PROCESS_TERMINATED:
+    case NOTIFICATION_RENDERER_PROCESS_CLOSED: {
+      RenderProcessHost* host = Source<RenderProcessHost>(source).ptr();
+      InvalidateChildProcessId(host->GetID());
       break;
-    case NOTIFICATION_RENDERER_PROCESS_TERMINATED:
-      handle = Source<RenderProcessHost>(source)->GetHandle();
-      break;
+    }
     default:
       NOTREACHED() << "Unexpected notification";
       break;
   }
-  InvalidatePid(handle);
 }
 
 MachBroker::MachBroker() : listener_thread_started_(false) {
 }
 
 MachBroker::~MachBroker() {}
 
 void MachBroker::FinalizePid(base::ProcessHandle pid,
                              mach_port_t task_port) {
   lock_.AssertAcquired();
@@ skipping 13 matching lines @@
 void MachBroker::InvalidatePid(base::ProcessHandle pid) {
   base::AutoLock lock(lock_);
   MachBroker::MachMap::iterator it = mach_map_.find(pid);
   if (it == mach_map_.end())
     return;
 
   kern_return_t kr = mach_port_deallocate(mach_task_self(),
                                           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 (ChildProcessIdMap::iterator it = child_process_id_map_.begin();

[Charlie Reis, 2014/11/10 19:57:25]

Why not have the map go the other way?  Then you can look up the handle from
child_process_id (without iterating), and erase it from the map by
child_process_id after calling InvalidatePid.

[sramajay, 2014/11/11 14:16:05]

InvalidatePid is called from other BrowserChildProcessHostDisconnected() and
BrowserChildProcessCrashed(), in which ProcessHandle is passed on. If i change
as per above comment, then iteration has to be done inside InvalidatePid().
Iteration cannot be avoided. 

+      it != child_process_id_map_.end(); ++it) {
+    if (it->second == child_process_id) {
+      pid = it->first;
+      break;
+    }
+  }
+  if (pid)
+    InvalidatePid(pid);
+}
+
+
 // static
 std::string MachBroker::GetMachPortName() {
   const base::CommandLine* command_line =
       base::CommandLine::ForCurrentProcess();
   const bool is_child = command_line->HasSwitch(switches::kProcessType);
 
   // In non-browser (child) processes, use the parent's pid.
   const pid_t pid = is_child ? getppid() : getpid();
   return base::StringPrintf("%s.rohitfork.%d", base::mac::BaseBundleID(), pid);
 }
 
 void MachBroker::RegisterNotifications() {
   registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CLOSED,
                  NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  NotificationService::AllBrowserContextsAndSources());
 
   // No corresponding StopObservingBrowserChildProcesses,
   // we leak this singleton.
   BrowserChildProcessObserver::Add(this);
 }
 
 }  // namespace content