IPC: Remove unnecessary conversions of BrokerableAttachment.

BrokerableAttachments are typically stored and passed around in a scoped_refptr.
There were several locations where they were being unnecessarily converted to a
raw pointer. This was probably responsible for a non-deterministic crash on GPU
bots (https://code.google.com/p/chromium/issues/detail?id=535711#c28), although
I haven't been able to reproduce the problem locally.

BUG=535711

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

Cr-Commit-Position: refs/heads/master@{#356968}

18 files changed, 48 insertions, 47 deletions

diff --git a/ipc/attachment_broker.h b/ipc/attachment_broker.h
index 36489d4..a015587 100644
--- a/ipc/attachment_broker.h
+++ b/ipc/attachment_broker.h
@@ -7,6 +7,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/process/process_handle.h"
 #include "ipc/brokerable_attachment.h"
 #include "ipc/ipc_export.h"
@@ -65,8 +66,9 @@ class IPC_EXPORT AttachmentBroker : public Listener {
   // IPC::Channel to communicate with the broker process. This may be the same
   // IPC::Channel that is requesting the brokering of an attachment.
   // Returns true on success and false otherwise.
-  virtual bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                                       base::ProcessId destination_process) = 0;
+  virtual bool SendAttachmentToProcess(
+      const scoped_refptr<BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) = 0;
 
   // Returns whether the attachment was available. If the attachment was
   // available, populates the output parameter |attachment|.
diff --git a/ipc/attachment_broker_mac_unittest.cc b/ipc/attachment_broker_mac_unittest.cc
index 4d1f5bc..e3062b5 100644
--- a/ipc/attachment_broker_mac_unittest.cc
+++ b/ipc/attachment_broker_mac_unittest.cc
@@ -264,8 +264,9 @@ class MockBroker : public IPC::AttachmentBrokerUnprivilegedMac {
  public:
   MockBroker() {}
   ~MockBroker() override {}
-  bool SendAttachmentToProcess(IPC::BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override {
+  bool SendAttachmentToProcess(
+      const scoped_refptr<IPC::BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override {
     return IPC::AttachmentBrokerUnprivilegedMac::SendAttachmentToProcess(
         attachment, base::Process::Current().Pid());
   }
diff --git a/ipc/attachment_broker_privileged_mac.cc b/ipc/attachment_broker_privileged_mac.cc
index 61694bf..8965eb5 100644
--- a/ipc/attachment_broker_privileged_mac.cc
+++ b/ipc/attachment_broker_privileged_mac.cc
@@ -60,12 +60,12 @@ AttachmentBrokerPrivilegedMac::AttachmentBrokerPrivilegedMac(
 AttachmentBrokerPrivilegedMac::~AttachmentBrokerPrivilegedMac() {}
 
 bool AttachmentBrokerPrivilegedMac::SendAttachmentToProcess(
-    BrokerableAttachment* attachment,
+    const scoped_refptr<IPC::BrokerableAttachment>& attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::MACH_PORT: {
       internal::MachPortAttachmentMac* mach_port_attachment =
-          static_cast<internal::MachPortAttachmentMac*>(attachment);
+          static_cast<internal::MachPortAttachmentMac*>(attachment.get());
       MachPortWireFormat wire_format =
           mach_port_attachment->GetWireFormat(destination_process);
 
diff --git a/ipc/attachment_broker_privileged_mac.h b/ipc/attachment_broker_privileged_mac.h
index 329a376..d20bcc5 100644
--- a/ipc/attachment_broker_privileged_mac.h
+++ b/ipc/attachment_broker_privileged_mac.h
@@ -55,8 +55,9 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac
   ~AttachmentBrokerPrivilegedMac() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override;
+  bool SendAttachmentToProcess(
+      const scoped_refptr<IPC::BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.
   bool OnMessageReceived(const Message& message) override;
diff --git a/ipc/attachment_broker_privileged_win.cc b/ipc/attachment_broker_privileged_win.cc
index a638752f..ec84940 100644
--- a/ipc/attachment_broker_privileged_win.cc
+++ b/ipc/attachment_broker_privileged_win.cc
@@ -19,12 +19,12 @@ AttachmentBrokerPrivilegedWin::AttachmentBrokerPrivilegedWin() {}
 AttachmentBrokerPrivilegedWin::~AttachmentBrokerPrivilegedWin() {}
 
 bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess(
-    BrokerableAttachment* attachment,
+    const scoped_refptr<IPC::BrokerableAttachment>& attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::WIN_HANDLE: {
       const internal::HandleAttachmentWin* handle_attachment =
-          static_cast<const internal::HandleAttachmentWin*>(attachment);
+          static_cast<const internal::HandleAttachmentWin*>(attachment.get());
       HandleWireFormat wire_format =
           handle_attachment->GetWireFormat(destination_process);
       HandleWireFormat new_wire_format =
diff --git a/ipc/attachment_broker_privileged_win.h b/ipc/attachment_broker_privileged_win.h
index f40ccf5..e12e505 100644
--- a/ipc/attachment_broker_privileged_win.h
+++ b/ipc/attachment_broker_privileged_win.h
@@ -20,8 +20,9 @@ class IPC_EXPORT AttachmentBrokerPrivilegedWin
   ~AttachmentBrokerPrivilegedWin() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override;
+  bool SendAttachmentToProcess(
+      const scoped_refptr<IPC::BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.
   bool OnMessageReceived(const Message& message) override;
diff --git a/ipc/attachment_broker_privileged_win_unittest.cc b/ipc/attachment_broker_privileged_win_unittest.cc
index 555262a..b0353d6 100644
--- a/ipc/attachment_broker_privileged_win_unittest.cc
+++ b/ipc/attachment_broker_privileged_win_unittest.cc
@@ -276,8 +276,9 @@ class MockBroker : public IPC::AttachmentBrokerUnprivilegedWin {
  public:
   MockBroker() {}
   ~MockBroker() override {}
-  bool SendAttachmentToProcess(IPC::BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override {
+  bool SendAttachmentToProcess(
+      const scoped_refptr<IPC::BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override {
     return IPC::AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
         attachment, base::Process::Current().Pid());
   }
diff --git a/ipc/attachment_broker_unprivileged_mac.cc b/ipc/attachment_broker_unprivileged_mac.cc
index b5a7e84..2bba01d 100644
--- a/ipc/attachment_broker_unprivileged_mac.cc
+++ b/ipc/attachment_broker_unprivileged_mac.cc
@@ -50,12 +50,12 @@ AttachmentBrokerUnprivilegedMac::AttachmentBrokerUnprivilegedMac() {}
 AttachmentBrokerUnprivilegedMac::~AttachmentBrokerUnprivilegedMac() {}
 
 bool AttachmentBrokerUnprivilegedMac::SendAttachmentToProcess(
-    BrokerableAttachment* attachment,
+    const scoped_refptr<IPC::BrokerableAttachment>& attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::MACH_PORT: {
       internal::MachPortAttachmentMac* mach_port_attachment =
-          static_cast<internal::MachPortAttachmentMac*>(attachment);
+          static_cast<internal::MachPortAttachmentMac*>(attachment.get());
       internal::MachPortAttachmentMac::WireFormat format =
           mach_port_attachment->GetWireFormat(destination_process);
       bool success =
diff --git a/ipc/attachment_broker_unprivileged_mac.h b/ipc/attachment_broker_unprivileged_mac.h
index 26cf443..b47233f 100644
--- a/ipc/attachment_broker_unprivileged_mac.h
+++ b/ipc/attachment_broker_unprivileged_mac.h
@@ -22,8 +22,9 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedMac
   ~AttachmentBrokerUnprivilegedMac() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override;
+  bool SendAttachmentToProcess(
+      const scoped_refptr<IPC::BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.
   bool OnMessageReceived(const Message& message) override;
diff --git a/ipc/attachment_broker_unprivileged_win.cc b/ipc/attachment_broker_unprivileged_win.cc
index e1d9941..9e9e7b2 100644
--- a/ipc/attachment_broker_unprivileged_win.cc
+++ b/ipc/attachment_broker_unprivileged_win.cc
@@ -17,12 +17,12 @@ AttachmentBrokerUnprivilegedWin::AttachmentBrokerUnprivilegedWin() {}
 AttachmentBrokerUnprivilegedWin::~AttachmentBrokerUnprivilegedWin() {}
 
 bool AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
-    BrokerableAttachment* attachment,
+    const scoped_refptr<BrokerableAttachment>& attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::WIN_HANDLE: {
       const internal::HandleAttachmentWin* handle_attachment =
-          static_cast<const internal::HandleAttachmentWin*>(attachment);
+          static_cast<const internal::HandleAttachmentWin*>(attachment.get());
       internal::HandleAttachmentWin::WireFormat format =
           handle_attachment->GetWireFormat(destination_process);
       return get_sender()->Send(
diff --git a/ipc/attachment_broker_unprivileged_win.h b/ipc/attachment_broker_unprivileged_win.h
index bcb1500..9fa15ee 100644
--- a/ipc/attachment_broker_unprivileged_win.h
+++ b/ipc/attachment_broker_unprivileged_win.h
@@ -22,8 +22,9 @@ class IPC_EXPORT AttachmentBrokerUnprivilegedWin
   ~AttachmentBrokerUnprivilegedWin() override;
 
   // IPC::AttachmentBroker overrides.
-  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override;
+  bool SendAttachmentToProcess(
+      const scoped_refptr<BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override;
 
   // IPC::Listener overrides.
   bool OnMessageReceived(const Message& message) override;
diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
index 85f4475..1f73e78 100644
--- a/ipc/ipc_channel_posix.cc
+++ b/ipc/ipc_channel_posix.cc
@@ -697,7 +697,7 @@ bool ChannelPosix::ProcessMessageForDelivery(Message* message) {
   if (message->HasBrokerableAttachments()) {
     DCHECK(GetAttachmentBroker());
     DCHECK(peer_pid_ != base::kNullProcessId);
-    for (BrokerableAttachment* attachment :
+    for (const scoped_refptr<IPC::BrokerableAttachment>& attachment :
          message->attachment_set()->GetBrokerableAttachments()) {
       if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment,
                                                           peer_pid_)) {
diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc
index 3f3a56e..a82093e 100644
--- a/ipc/ipc_channel_reader.cc
+++ b/ipc/ipc_channel_reader.cc
@@ -246,9 +246,9 @@ ChannelReader::AttachmentIdSet ChannelReader::GetBrokeredAttachments(
 
 #if USE_ATTACHMENT_BROKER
   MessageAttachmentSet* set = msg->attachment_set();
-  std::vector<BrokerableAttachment*> brokerable_attachments_copy =
-      set->GetBrokerableAttachments();
-  for (const BrokerableAttachment* attachment : brokerable_attachments_copy) {
+  std::vector<scoped_refptr<IPC::BrokerableAttachment>>
+      brokerable_attachments_copy(set->GetBrokerableAttachments());
+  for (const auto& attachment : brokerable_attachments_copy) {
     if (attachment->NeedsBrokering()) {
       AttachmentBroker* broker = GetAttachmentBroker();
       DCHECK(broker);
diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc
index c528996..fdf3144 100644
--- a/ipc/ipc_channel_reader_unittest.cc
+++ b/ipc/ipc_channel_reader_unittest.cc
@@ -42,8 +42,9 @@ class MockAttachmentBroker : public AttachmentBroker {
  public:
   typedef std::set<scoped_refptr<BrokerableAttachment>> AttachmentSet;
 
-  bool SendAttachmentToProcess(BrokerableAttachment* attachment,
-                               base::ProcessId destination_process) override {
+  bool SendAttachmentToProcess(
+      const scoped_refptr<BrokerableAttachment>& attachment,
+      base::ProcessId destination_process) override {
     return false;
   }
 
diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc
index 2a487ff..19f7cb5 100644
--- a/ipc/ipc_channel_win.cc
+++ b/ipc/ipc_channel_win.cc
@@ -107,7 +107,7 @@ bool ChannelWin::ProcessMessageForDelivery(Message* message) {
   if (message->HasBrokerableAttachments()) {
     DCHECK(GetAttachmentBroker());
     DCHECK(peer_pid_ != base::kNullProcessId);
-    for (BrokerableAttachment* attachment :
+    for (const scoped_refptr<IPC::BrokerableAttachment>& attachment :
          message->attachment_set()->GetBrokerableAttachments()) {
       if (!GetAttachmentBroker()->SendAttachmentToProcess(attachment,
                                                           peer_pid_)) {
diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc
index 2738bf1..d64c8028 100644
--- a/ipc/ipc_message.cc
+++ b/ipc/ipc_message.cc
@@ -147,16 +147,15 @@ Message::NextMessageInfo::~NextMessageInfo() {}
 Message::SerializedAttachmentIds
 Message::SerializedIdsOfBrokerableAttachments() {
   DCHECK(HasBrokerableAttachments());
-  std::vector<BrokerableAttachment*> attachments =
-      attachment_set_->GetBrokerableAttachments();
+  std::vector<scoped_refptr<IPC::BrokerableAttachment>> attachments(
+      attachment_set_->GetBrokerableAttachments());
   CHECK_LE(attachments.size(), std::numeric_limits<size_t>::max() /
                                    BrokerableAttachment::kNonceSize);
   size_t size = attachments.size() * BrokerableAttachment::kNonceSize;
   char* buffer = static_cast<char*>(malloc(size));
   for (size_t i = 0; i < attachments.size(); ++i) {
-    const BrokerableAttachment* attachment = attachments[i];
     char* start_range = buffer + i * BrokerableAttachment::kNonceSize;
-    BrokerableAttachment::AttachmentId id = attachment->GetIdentifier();
+    BrokerableAttachment::AttachmentId id = attachments[i]->GetIdentifier();
     id.SerializeToBuffer(start_range, BrokerableAttachment::kNonceSize);
   }
   SerializedAttachmentIds ids;
diff --git a/ipc/ipc_message_attachment_set.cc b/ipc/ipc_message_attachment_set.cc
index b290e0a..16446c5 100644
--- a/ipc/ipc_message_attachment_set.cc
+++ b/ipc/ipc_message_attachment_set.cc
@@ -175,26 +175,18 @@ void MessageAttachmentSet::CommitAllDescriptors() {
   consumed_descriptor_highwater_ = 0;
 }
 
-std::vector<BrokerableAttachment*>
+std::vector<scoped_refptr<IPC::BrokerableAttachment>>
 MessageAttachmentSet::GetBrokerableAttachments() const {
-  std::vector<BrokerableAttachment*> output;
-  for (const scoped_refptr<MessageAttachment>& attachment :
-       brokerable_attachments_) {
-      output.push_back(static_cast<BrokerableAttachment*>(attachment.get()));
-  }
-  return output;
+  return brokerable_attachments_;
 }
 
 void MessageAttachmentSet::ReplacePlaceholderWithAttachment(
     const scoped_refptr<BrokerableAttachment>& attachment) {
+  DCHECK_NE(BrokerableAttachment::PLACEHOLDER, attachment->GetBrokerableType());
   for (auto it = brokerable_attachments_.begin();
        it != brokerable_attachments_.end(); ++it) {
-    BrokerableAttachment* brokerable_attachment =
-        static_cast<BrokerableAttachment*>(it->get());
-
-    if (brokerable_attachment->GetBrokerableType() ==
-            BrokerableAttachment::PLACEHOLDER &&
-        brokerable_attachment->GetIdentifier() == attachment->GetIdentifier()) {
+    if ((*it)->GetBrokerableType() == BrokerableAttachment::PLACEHOLDER &&
+        (*it)->GetIdentifier() == attachment->GetIdentifier()) {
       *it = attachment;
       return;
     }
diff --git a/ipc/ipc_message_attachment_set.h b/ipc/ipc_message_attachment_set.h
index 4276fcd..6a94b19 100644
--- a/ipc/ipc_message_attachment_set.h
+++ b/ipc/ipc_message_attachment_set.h
@@ -87,7 +87,8 @@ class IPC_EXPORT MessageAttachmentSet
   void CommitAllDescriptors();
 
   // Returns a vector of all brokerable attachments.
-  std::vector<BrokerableAttachment*> GetBrokerableAttachments() const;
+  std::vector<scoped_refptr<IPC::BrokerableAttachment>>
+  GetBrokerableAttachments() const;
 
   // Replaces a placeholder brokerable attachment with |attachment|, matching
   // them by their id.