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);

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

Why don’t you get rid of InvalidatePid and use InvalidateChildProcessId
everywhere? data.id carries the child process ID. Line 221 too.

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

Acknowledged.

 }
 
 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 (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 =
       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