[Merge M48] ipc: Minor fixes to AttachmentBrokerPrivilegedMac.

> Two small changes:
>  - Better cleanup when a process has gone away, but extractors or precursors are
>  still present.
>  - Remove an unused method.
>
> BUG=561105
>
> Review URL: https://codereview.chromium.org/1473883002
>
> Cr-Commit-Position: refs/heads/master@{#362057}

(cherry picked from commit 28f0a58)
TBR=tsepez@chromium.org, isherman@chromium.org
BUG=563762

Review URL: https://codereview.chromium.org/1491023002 .

Cr-Commit-Position: refs/branch-heads/2564@{#189}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}

ipc/attachment_broker_privileged.h

@@ -79,6 +79,9 @@ class IPC_EXPORT AttachmentBrokerPrivileged : public IPC::AttachmentBroker {
     ERROR_DECREASE_REF = 10,
     // Couldn't extract a right from the source.
     ERROR_EXTRACT_SOURCE_RIGHT = 11,
+    // The broker did not have a channel of communication with the source
+    // process.
+    ERROR_SOURCE_NOT_FOUND = 12,
     ERROR_MAX
   };
 

ipc/attachment_broker_privileged_mac.cc

@@ -96,6 +96,36 @@ bool AttachmentBrokerPrivilegedMac::SendAttachmentToProcess(
   return false;
 }
 
+void AttachmentBrokerPrivilegedMac::DeregisterCommunicationChannel(
+    Endpoint* endpoint) {
+  AttachmentBrokerPrivileged::DeregisterCommunicationChannel(endpoint);
+
+  if (!endpoint)
+    return;
+
+  base::ProcessId pid = endpoint->GetPeerPID();
+  if (pid == base::kNullProcessId)
+    return;
+
+  {
+    base::AutoLock l(precursors_lock_);
+    auto it = precursors_.find(pid);
+    if (it != precursors_.end()) {
+      delete it->second;
+      precursors_.erase(pid);
+    }
+  }
+
+  {
+    base::AutoLock l(extractors_lock_);
+    auto it = extractors_.find(pid);
+    if (it != extractors_.end()) {
+      delete it->second;
+      extractors_.erase(pid);
+    }
+  }
+}
+
 bool AttachmentBrokerPrivilegedMac::OnMessageReceived(const Message& msg) {
   bool handled = true;
   switch (msg.type()) {
@@ -169,7 +199,7 @@ void AttachmentBrokerPrivilegedMac::RoutePrecursorToSelf(
   HandleReceivedAttachment(attachment);
 }
 
-void AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother(
+bool AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother(
     const MachPortWireFormat& wire_format) {
   DCHECK_NE(wire_format.destination_process, base::Process::Current().Pid());
 
@@ -183,11 +213,12 @@ void AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother(
     LOG(ERROR) << "Failed to deliver brokerable attachment to process with id: "
                << dest;
     LogError(DESTINATION_NOT_FOUND);
-    return;
+    return false;
   }
 
   LogError(DESTINATION_FOUND);
   sender->Send(new AttachmentBrokerMsg_MachPortHasBeenDuplicated(wire_format));
+  return true;
 }
 
 mach_port_name_t AttachmentBrokerPrivilegedMac::CreateIntermediateMachPort(
@@ -245,21 +276,6 @@ mach_port_name_t AttachmentBrokerPrivilegedMac::CreateIntermediateMachPort(
   return endpoint;
 }
 
-base::mac::ScopedMachSendRight AttachmentBrokerPrivilegedMac::AcquireSendRight(
-    base::ProcessId pid,
-    mach_port_name_t named_right) {
-  if (pid == base::GetCurrentProcId()) {
-    kern_return_t kr = mach_port_mod_refs(mach_task_self(), named_right,
-                                          MACH_PORT_RIGHT_SEND, 1);
-    if (kr != KERN_SUCCESS)
-      return base::mac::ScopedMachSendRight(MACH_PORT_NULL);
-    return base::mac::ScopedMachSendRight(named_right);
-  }
-
-  mach_port_t task_port = port_provider_->TaskForPid(pid);
-  return ExtractNamedRight(task_port, named_right);
-}
-
 base::mac::ScopedMachSendRight AttachmentBrokerPrivilegedMac::ExtractNamedRight(
     mach_port_t task_port,
     mach_port_name_t named_right) {
@@ -305,24 +321,40 @@ void AttachmentBrokerPrivilegedMac::SendPrecursorsForProcess(
   // Whether this process is the destination process.
   bool to_self = pid == base::GetCurrentProcId();
 
+  if (!to_self) {
+    if (!GetSenderWithProcessId(pid)) {
+      // If there is no sender, then the destination process is no longer
+      // running, or never existed to begin with.
+      LogError(DESTINATION_NOT_FOUND);
+      delete it->second;
+      precursors_.erase(it);
+      return;
+    }
+  }
+
   mach_port_t task_port = port_provider_->TaskForPid(pid);
+
+  // It's possible that the destination process has not yet provided the
+  // privileged process with its task port.
   if (!to_self && task_port == MACH_PORT_NULL)
     return;
 
   while (!it->second->empty()) {
     auto precursor_it = it->second->begin();
-    if (to_self)
+    if (to_self) {
       RoutePrecursorToSelf(*precursor_it);
-    else
-      SendPrecursor(*precursor_it, task_port);
+    } else {
+      if (!SendPrecursor(*precursor_it, task_port))
+        break;
+    }
     it->second->erase(precursor_it);
   }
 
   delete it->second;
   precursors_.erase(it);
 }
 
-void AttachmentBrokerPrivilegedMac::SendPrecursor(
+bool AttachmentBrokerPrivilegedMac::SendPrecursor(
     AttachmentPrecursor* precursor,
     mach_port_t task_port) {
   DCHECK(task_port);
@@ -334,7 +366,8 @@ void AttachmentBrokerPrivilegedMac::SendPrecursor(
     intermediate_port = CreateIntermediateMachPort(
         task_port, base::mac::ScopedMachSendRight(port_to_insert.release()));
   }
-  RouteWireFormatToAnother(CopyWireFormat(wire_format, intermediate_port));
+  return RouteWireFormatToAnother(
+      CopyWireFormat(wire_format, intermediate_port));
 }
 
 void AttachmentBrokerPrivilegedMac::AddPrecursor(
@@ -357,7 +390,18 @@ void AttachmentBrokerPrivilegedMac::ProcessExtractorsForProcess(
   if (it == extractors_.end())
     return;
 
+  if (!GetSenderWithProcessId(pid)) {
+    // If there is no sender, then the source process is no longer running.
+    LogError(ERROR_SOURCE_NOT_FOUND);
+    delete it->second;
+    extractors_.erase(it);
+    return;
+  }
+
   mach_port_t task_port = port_provider_->TaskForPid(pid);
+
+  // It's possible that the source process has not yet provided the privileged
+  // process with its task port.
   if (task_port == MACH_PORT_NULL)
     return;
 

ipc/attachment_broker_privileged_mac.h

@@ -64,6 +64,7 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac
   bool SendAttachmentToProcess(
       const scoped_refptr<IPC::BrokerableAttachment>& attachment,
       base::ProcessId destination_process) override;
+  void DeregisterCommunicationChannel(Endpoint* endpoint) override;
 
   // IPC::Listener overrides.
   bool OnMessageReceived(const Message& message) override;
@@ -150,11 +151,6 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac
       mach_port_t task_port,
       base::mac::ScopedMachSendRight port_to_insert);
 
-  // Acquire a send right to a named right in |pid|.
-  // Returns MACH_PORT_NULL on error.
-  base::mac::ScopedMachSendRight AcquireSendRight(base::ProcessId pid,
-                                                  mach_port_name_t named_right);
-
   // Extracts a copy of the send right to |named_right| from |task_port|.
   // Returns MACH_PORT_NULL on error.
   base::mac::ScopedMachSendRight ExtractNamedRight(
@@ -176,14 +172,16 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac
   // |wire_format.mach_port| must be the intermediate Mach port.
   // Ownership of |wire_format.mach_port| is implicitly passed to the process
   // that receives the Chrome IPC message.
-  void RouteWireFormatToAnother(const MachPortWireFormat& wire_format);
+  // Returns |false| on irrecoverable error.
+  bool RouteWireFormatToAnother(const MachPortWireFormat& wire_format);
 
   // Atempts to broker all precursors whose destination is |pid|. Has no effect
   // if |port_provider_| does not have the task port for |pid|.
   void SendPrecursorsForProcess(base::ProcessId pid);
 
   // Brokers a single precursor into the task represented by |task_port|.
-  void SendPrecursor(AttachmentPrecursor* precursor, mach_port_t task_port);
+  // Returns |false| on irrecoverable error.
+  bool SendPrecursor(AttachmentPrecursor* precursor, mach_port_t task_port);
 
   // Add a precursor to |precursors_|. Takes ownership of |port|.
   void AddPrecursor(base::ProcessId pid,

tools/metrics/histograms/histograms.xml

@@ -65067,6 +65067,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="11" label="ERROR_EXTRACT_RIGHT_SOURCE">
     Couldn't extract a right from the source process.
   </int>
+  <int value="12" label="ERROR_SOURCE_NOT_FOUND">
+    Broker didn't have a channel of communication with the source process.
+  </int>
 </enum>
 
 <enum name="IPCAttachmentBrokerUnprivilegedBrokerAttachmentError" type="int">