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 97 matching lines @@
       // unspoofable pid of the task that sent the message.
       //
       // TODO(rsesek): In the 10.7 SDK, there's audit_token_to_pid().
       pid_t child_pid;
       audit_token_to_au32(msg.trailer.msgh_audit,
           NULL, NULL, NULL, NULL, NULL, &child_pid, NULL, NULL);
 
       mach_port_t child_task_port = msg.child_task_port.name;
 
       // Take the lock and update the broker information.
       base::AutoLock lock(broker_->GetLock());

[Mark Mentovai, 2014/11/17 19:17:46]

I would move this lock into FinalizePid(), replacing the lock_.AssertAcquired()
in that function. If you do this, also adjust the comment in the header file for
FinalizePid().

[sramajay, 2014/11/18 14:15:19]

I would take this suggestion as a new CL as it is not only FinalizePid() but it
is the same for even other methods AddPlaceholderFodPid() and EnsureRunning().
And more over this change is a standalone enhancement.

       broker_->FinalizePid(child_pid, child_task_port);
     }
 
     MACH_LOG(ERROR, kr) << "mach_msg";
   }
 
  private:
   // The MachBroker to use when new child task rights are received.  Can be
   // NULL.
   MachBroker* broker_;  // weak
@@ skipping 59 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_[child_process_id] = pid;
 }
 
 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);
+  InvalidateChildProcessId(data.id);
 }
 
 void MachBroker::BrowserChildProcessCrashed(const ChildProcessData& data) {
-  InvalidatePid(data.handle);
+  InvalidateChildProcessId(data.id);
 }
 
 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();
 
   MachMap::iterator it = mach_map_.find(pid);
   if (it == mach_map_.end()) {
     // Do nothing for unknown pids.
     LOG(ERROR) << "Unknown process " << pid << " is sending Mach IPC messages!";
     return;
   }
 
   DCHECK(it->second == MACH_PORT_NULL);
   if (it->second == MACH_PORT_NULL)
     it->second = task_port;
 }
 
-void MachBroker::InvalidatePid(base::ProcessHandle pid) {
+void MachBroker::InvalidateChildProcessId(int child_process_id) {
   base::AutoLock lock(lock_);
-  MachBroker::MachMap::iterator it = mach_map_.find(pid);
-  if (it == mach_map_.end())
+  MachBroker::ChildProcessIdMap::iterator it =
+    child_process_id_map_.find(child_process_id);

[Mark Mentovai, 2014/11/17 19:17:46]

Continuation lines get a 4-space indent, not a 2-space one.

[sramajay, 2014/11/18 14:15:19]

Done.

+  if (it == child_process_id_map_.end())
     return;
 
+  base::ProcessHandle pid = it->second;
   kern_return_t kr = mach_port_deallocate(mach_task_self(),

[Mark Mentovai, 2014/11/17 19:17:46]

You should use mach_map_.find() and handle the case where the PID isn’t in
mach_map_ by skipping the mach_port_deallocate() and mach_map_.erase(). Since
you’ll have an iterator, you can also use it with the erase() on line 272,
instead of having to do the same map lookup twice.

It will happen that you’ll get an entry for child_process_id in
child_process_id_map_ but no entry for the pid in mach_map_ if FinalizePid()
doesn’t get called before the child process dies.

[sramajay, 2014/11/18 14:15:19]

Done.

-                                          it->second);
+                                          mach_map_[pid]);
   MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr) << "mach_port_deallocate";
-  mach_map_.erase(it);
+  mach_map_.erase(pid);
+  child_process_id_map_.erase(it);
 }
 
 // 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