IPC::Message Refactoring: Move POSIX specific bits to PlatformFileAttachment

This change:

 * Gets rid of MessageAttachmentSet::owning_descriptors_ by moving
   the responsibility to PlatformFileAttachment, where owning_
   member variable is used to track the lifetime
 * Replaces MessageAttachmetnSet::Add*File() and TakePlatformFile()
   with platform agnostic AddAttachment() and GetAttachmentAt()
 * Repalces Message::ReadFile(), Write*File() with ReadAttachment()
   and WriteAttachmente(), which are also platform agnostic.

This also adds MessageAttachment::TakePlatformFile() virtual function,
which will be implemented by upcoming MojoHandleAttachment as well.

This is another preparation for introducing MojoHandleAttachment,
but it also simplifies Message and MessageAttachmentSet.

R=agl@chromium.org
TBR=mtomasz@chromium.org
BUG=448190

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

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

chrome/browser/extensions/api/file_system/file_system_api.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/common/extensions/api/file_system.h"

ipc/BUILD.gn

@@ -42,8 +42,8 @@ component("ipc") {
     "ipc_param_traits.h",
     "ipc_platform_file.cc",
     "ipc_platform_file.h",
-    "ipc_platform_file_attachment.cc",
-    "ipc_platform_file_attachment.h",
+    "ipc_platform_file_attachment_posix.cc",
+    "ipc_platform_file_attachment_posix.h",
     "ipc_sender.h",
     "ipc_switches.cc",
     "ipc_switches.h",

ipc/ipc.gypi

@@ -47,8 +47,8 @@
           'ipc_param_traits.h',
           'ipc_platform_file.cc',
           'ipc_platform_file.h',
-          'ipc_platform_file_attachment.cc',
-          'ipc_platform_file_attachment.h',
+          'ipc_platform_file_attachment_posix.cc',
+          'ipc_platform_file_attachment_posix.h',
           'ipc_sender.h',
           'ipc_switches.cc',
           'ipc_switches.h',

ipc/ipc_channel_posix.cc

@@ -42,6 +42,7 @@
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_message_attachment_set.h"
 #include "ipc/ipc_message_utils.h"
+#include "ipc/ipc_platform_file_attachment_posix.h"
 #include "ipc/ipc_switches.h"
 #include "ipc/unix_domain_socket_util.h"
 
@@ -793,7 +794,8 @@ void ChannelPosix::QueueHelloMessage() {
 #if defined(IPC_USES_READWRITE)
   scoped_ptr<Message> hello;
   if (remote_fd_pipe_.is_valid()) {
-    if (!msg->WriteBorrowingFile(remote_fd_pipe_.get())) {
+    if (!msg->WriteAttachment(
+            new internal::PlatformFileAttachment(remote_fd_pipe_.get()))) {
       NOTREACHED() << "Unable to pickle hello message file descriptors";
     }
     DCHECK_EQ(msg->attachment_set()->size(), 1U);
@@ -1014,11 +1016,11 @@ void ChannelPosix::HandleInternalMessage(const Message& msg) {
         // server also contains the fd_pipe_, which  will be used for all
         // subsequent file descriptor passing.
         DCHECK_EQ(msg.attachment_set()->size(), 1U);
-        base::ScopedFD descriptor;
-        if (!msg.ReadFile(&iter, &descriptor)) {
+        scoped_refptr<MessageAttachment> attachment;
+        if (!msg.ReadAttachment(&iter, &attachment)) {
           NOTREACHED();
         }
-        fd_pipe_.reset(descriptor.release());
+        fd_pipe_.reset(attachment->TakePlatformFile());
       }
 #endif  // IPC_USES_READWRITE
       peer_pid_ = pid;

ipc/ipc_message.cc

@@ -7,10 +7,12 @@
 #include "base/atomic_sequence_num.h"
 #include "base/logging.h"
 #include "build/build_config.h"
+#include "ipc/ipc_message_attachment.h"
 #include "ipc/ipc_message_attachment_set.h"
 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
+#include "ipc/ipc_platform_file_attachment_posix.h"
 #endif
 
 namespace {
@@ -127,22 +129,16 @@ void Message::set_received_time(int64 time) const {
 }
 #endif
 
-#if defined(OS_POSIX)
-bool Message::WriteFile(base::ScopedFD descriptor) {
-  // We write the index of the descriptor so that we don't have to
-  // keep the current descriptor as extra decoding state when deserialising.
-  WriteInt(attachment_set()->size());
-  return attachment_set()->AddToOwn(descriptor.Pass());
-}
-
-bool Message::WriteBorrowingFile(const base::PlatformFile& descriptor) {
+bool Message::WriteAttachment(scoped_refptr<MessageAttachment> attachment) {
   // We write the index of the descriptor so that we don't have to
   // keep the current descriptor as extra decoding state when deserialising.
   WriteInt(attachment_set()->size());
-  return attachment_set()->AddToBorrow(descriptor);
+  return attachment_set()->AddAttachment(attachment);
 }
 
-bool Message::ReadFile(PickleIterator* iter, base::ScopedFD* descriptor) const {
+bool Message::ReadAttachment(
+    PickleIterator* iter,
+    scoped_refptr<MessageAttachment>* attachment) const {
   int descriptor_index;
   if (!iter->ReadInt(&descriptor_index))
     return false;
@@ -151,18 +147,12 @@ bool Message::ReadFile(PickleIterator* iter, base::ScopedFD* descriptor) const {
   if (!attachment_set)
     return false;
 
-  base::PlatformFile file = attachment_set->TakeDescriptorAt(descriptor_index);
-  if (file < 0)
-    return false;
-
-  descriptor->reset(file);
-  return true;
+  *attachment = attachment_set->GetAttachmentAt(descriptor_index);
+  return nullptr != attachment->get();
 }
 
-bool Message::HasFileDescriptors() const {
+bool Message::HasAttachments() const {
   return attachment_set_.get() && !attachment_set_->empty();
 }
 
-#endif
-
 }  // namespace IPC

ipc/ipc_message.h

@@ -8,7 +8,8 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/files/file.h"
+#include "base/debug/trace_event.h"
+#include "base/memory/ref_counted.h"
 #include "base/pickle.h"
 #include "base/trace_event/trace_event.h"
 #include "ipc/ipc_export.h"
@@ -17,15 +18,12 @@
 #define IPC_MESSAGE_LOG_ENABLED
 #endif
 
-#if defined(OS_POSIX)
-#include "base/memory/ref_counted.h"
-#endif
-
 namespace IPC {
 
 //------------------------------------------------------------------------------
 
 struct LogData;
+class MessageAttachment;
 class MessageAttachmentSet;
 
 class IPC_EXPORT Message : public Pickle {
@@ -169,21 +167,15 @@ class IPC_EXPORT Message : public Pickle {
     return Pickle::FindNext(sizeof(Header), range_start, range_end);
   }
 
-#if defined(OS_POSIX)
-  // On POSIX, a message supports reading / writing FileDescriptor objects.
-  // This is used to pass a file descriptor to the peer of an IPC channel.
-
-  // Add a descriptor to the end of the set. Returns false if the set is full.
-  bool WriteFile(base::ScopedFD descriptor);
-  bool WriteBorrowingFile(const base::PlatformFile& descriptor);
-
-  // Get a file descriptor from the message. Returns false on error.
-  //   iter: a Pickle iterator to the current location in the message.
-  bool ReadFile(PickleIterator* iter, base::ScopedFD* file) const;
-
-  // Returns true if there are any file descriptors in this message.
-  bool HasFileDescriptors() const;
-#endif
+  // WriteAttachment appends |attachment| to the end of the set. It returns
+  // false iff the set is full.
+  bool WriteAttachment(scoped_refptr<MessageAttachment> attachment);
+  // ReadAttachment parses an attachment given the parsing state |iter| and
+  // writes it to |*attachment|. It returns true on success.
+  bool ReadAttachment(PickleIterator* iter,
+                      scoped_refptr<MessageAttachment>* attachment) const;
+  // Returns true if there are any attachment in this message.
+  bool HasAttachments() const;
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
   // Adds the outgoing time from Time::Now() at the end of the message and sets

ipc/ipc_message_attachment.cc

@@ -6,6 +6,9 @@
 
 namespace IPC {
 
+MessageAttachment::MessageAttachment() {
+}
+
 MessageAttachment::~MessageAttachment() {
 }
 

ipc/ipc_message_attachment.h

@@ -5,6 +5,8 @@
 #ifndef IPC_IPC_MESSAGE_ATTACHMENT_H_
 #define IPC_IPC_MESSAGE_ATTACHMENT_H_
 
+#include "base/files/file.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "ipc/ipc_export.h"
 
@@ -21,10 +23,14 @@ class IPC_EXPORT MessageAttachment
   };
 
   virtual Type GetType() const = 0;
+  virtual base::PlatformFile TakePlatformFile() = 0;
 
  protected:
   friend class base::RefCounted<MessageAttachment>;
+  MessageAttachment();
   virtual ~MessageAttachment();
+
+  DISALLOW_COPY_AND_ASSIGN(MessageAttachment);
 };
 
 }  // namespace IPC

ipc/ipc_message_attachment_set.cc

@@ -8,12 +8,12 @@
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
 #include "ipc/ipc_message_attachment.h"
-#include "ipc/ipc_platform_file_attachment.h"
 
 #if defined(OS_POSIX)
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
+#include "ipc/ipc_platform_file_attachment_posix.h"
 #endif // OS_POSIX
 
 namespace IPC {
@@ -49,9 +49,18 @@ unsigned MessageAttachmentSet::size() const {
   return static_cast<unsigned>(attachments_.size());
 }
 
-void MessageAttachmentSet::AddAttachment(
+bool MessageAttachmentSet::AddAttachment(
     scoped_refptr<MessageAttachment> attachment) {
+#if defined(OS_POSIX)
+  if (attachment->GetType() != MessageAttachment::TYPE_PLATFORM_FILE ||
+      num_descriptors() == kMaxDescriptorsPerMessage) {
+    DLOG(WARNING) << "Cannot add file descriptor. MessageAttachmentSet full.";
+    return false;
+  }
+#endif
+
   attachments_.push_back(attachment);
+  return true;
 }
 
 scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
@@ -95,55 +104,6 @@ scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
 
 #if defined(OS_POSIX)
 
-bool MessageAttachmentSet::AddToBorrow(base::PlatformFile fd) {
-  DCHECK_EQ(consumed_descriptor_highwater_, 0u);
-
-  if (num_descriptors() == kMaxDescriptorsPerMessage) {
-    DLOG(WARNING) << "Cannot add file descriptor. MessageAttachmentSet full.";
-    return false;
-  }
-
-  AddAttachment(new internal::PlatformFileAttachment(fd));
-  return true;
-}
-
-bool MessageAttachmentSet::AddToOwn(base::ScopedFD fd) {
-  DCHECK_EQ(consumed_descriptor_highwater_, 0u);
-
-  if (num_descriptors() == kMaxDescriptorsPerMessage) {
-    DLOG(WARNING) << "Cannot add file descriptor. MessageAttachmentSet full.";
-    return false;
-  }
-
-  AddAttachment(new internal::PlatformFileAttachment(fd.get()));
-  owned_descriptors_.push_back(new base::ScopedFD(fd.Pass()));
-  DCHECK(num_descriptors() <= kMaxDescriptorsPerMessage);
-  return true;
-}
-
-base::PlatformFile MessageAttachmentSet::TakeDescriptorAt(unsigned index) {
-  scoped_refptr<MessageAttachment> attachment = GetAttachmentAt(index);
-  if (!attachment)
-    return -1;
-
-  base::PlatformFile file = internal::GetPlatformFile(attachment);
-
-  // TODO(morrita): In production, attachments_.size() should be same as
-  // owned_descriptors_.size() as all read descriptors are owned by Message.
-  // We have to do this because unit test breaks this assumption. It should be
-  // changed to exercise with own-able descriptors.
-  for (ScopedVector<base::ScopedFD>::const_iterator i =
-           owned_descriptors_.begin();
-       i != owned_descriptors_.end(); ++i) {
-    if ((*i)->get() == file) {
-      ignore_result((*i)->release());
-      break;
-    }
-  }
-
-  return file;
-}
-
 void MessageAttachmentSet::PeekDescriptors(base::PlatformFile* buffer) const {
   for (size_t i = 0; i != attachments_.size(); ++i)
     buffer[i] = internal::GetPlatformFile(attachments_[i]);
@@ -162,15 +122,16 @@ bool MessageAttachmentSet::ContainsDirectoryDescriptor() const {
 
 void MessageAttachmentSet::CommitAll() {
   attachments_.clear();
-  owned_descriptors_.clear();
   consumed_descriptor_highwater_ = 0;
 }
 
 void MessageAttachmentSet::ReleaseFDsToClose(
     std::vector<base::PlatformFile>* fds) {
-  for (ScopedVector<base::ScopedFD>::iterator i = owned_descriptors_.begin();
-       i != owned_descriptors_.end(); ++i) {
-    fds->push_back((*i)->release());
+  for (size_t i = 0; i < attachments_.size(); ++i) {
+    internal::PlatformFileAttachment* file =
+        static_cast<internal::PlatformFileAttachment*>(attachments_[i].get());
+    if (file->Owns())
+      fds->push_back(file->TakePlatformFile());
   }
 
   CommitAll();
@@ -183,11 +144,9 @@ void MessageAttachmentSet::AddDescriptorsToOwn(const base::PlatformFile* buffer,
   DCHECK_EQ(consumed_descriptor_highwater_, 0u);
 
   attachments_.reserve(count);
-  owned_descriptors_.reserve(count);
-  for (unsigned i = 0; i < count; ++i) {
-    AddAttachment(new internal::PlatformFileAttachment(buffer[i]));
-    owned_descriptors_.push_back(new base::ScopedFD(buffer[i]));
-  }
+  for (unsigned i = 0; i < count; ++i)
+    AddAttachment(
+        new internal::PlatformFileAttachment(base::ScopedFD(buffer[i])));
 }
 
 #endif  // OS_POSIX

ipc/ipc_message_attachment_set.h

@@ -21,9 +21,10 @@ namespace IPC {
 class MessageAttachment;
 
 // -----------------------------------------------------------------------------
-// A MessageAttachmentSet is an ordered set of POSIX file descriptors. These are
-// associated with IPC messages so that descriptors can be transmitted over a
-// UNIX domain socket.
+// A MessageAttachmentSet is an ordered set of MessageAttachment objects. These
+// are associated with IPC messages so that attachments, each of which is either
+// a platform file or a mojo handle, can be transmitted over the underlying UNIX
+// domain socket (for ChannelPosix) or Mojo MessagePipe (for ChannelMojo).
 // -----------------------------------------------------------------------------
 class IPC_EXPORT MessageAttachmentSet
     : public base::RefCountedThreadSafe<MessageAttachmentSet> {
@@ -37,7 +38,14 @@ class IPC_EXPORT MessageAttachmentSet
   // Return true if no unconsumed descriptors remain
   bool empty() const { return 0 == size(); }
 
-  void AddAttachment(scoped_refptr<MessageAttachment> attachment);
+  bool AddAttachment(scoped_refptr<MessageAttachment> attachment);
+
+  // Take the nth attachment from the beginning of the set, Code using this
+  // /must/ access the attachments in order, and must do it at most once.
+  //
+  // This interface is designed for the deserialising code as it doesn't
+  // support close flags.
+  //   returns: an attachment, or nullptr on error
   scoped_refptr<MessageAttachment> GetAttachmentAt(unsigned index);
 
 #if defined(OS_POSIX)
@@ -51,30 +59,6 @@ class IPC_EXPORT MessageAttachmentSet
   // of descriptors to a MessageAttachmentSet.
   static const size_t kMaxDescriptorsPerMessage = 7;
 
-  // ---------------------------------------------------------------------------
-  // Interfaces for building during message serialisation...
-
-  // Add a descriptor to the end of the set. Returns false iff the set is full.
-  bool AddToBorrow(base::PlatformFile fd);
-  // Add a descriptor to the end of the set and automatically close it after
-  // transmission. Returns false iff the set is full.
-  bool AddToOwn(base::ScopedFD fd);
-
-  // ---------------------------------------------------------------------------
-  // ---------------------------------------------------------------------------
-  // Interfaces for accessing during message deserialisation...
-
-  // Take the nth descriptor from the beginning of the set,
-  // transferring the ownership of the descriptor taken. Code using this
-  // /must/ access the descriptors in order, and must do it at most once.
-  //
-  // This interface is designed for the deserialising code as it doesn't
-  // support close flags.
-  //   returns: file descriptor, or -1 on error
-  base::PlatformFile TakeDescriptorAt(unsigned n);
-
-  // ---------------------------------------------------------------------------
-
   // ---------------------------------------------------------------------------
   // Interfaces for transmission...
 
@@ -116,14 +100,6 @@ class IPC_EXPORT MessageAttachmentSet
   // |MessagePipe|.
   std::vector<scoped_refptr<MessageAttachment>> attachments_;
 
-#if defined(OS_POSIX)
-  // A vector of owning descriptors. If this message is sent, then file
-  // descriptors are sent as control data. After sending, any owning descriptors
-  // are closed. If this message has been received then all received
-  // descriptors are owned by this message.
-  ScopedVector<base::ScopedFD> owned_descriptors_;
-#endif
-
   // This contains the index of the next descriptor which should be consumed.
   // It's used in a couple of ways. Firstly, at destruction we can check that
   // all the descriptors have been read (with GetNthDescriptor). Secondly, we

ipc/ipc_message_attachment_set_posix_unittest.cc

@@ -11,6 +11,7 @@
 
 #include "base/basictypes.h"
 #include "base/posix/eintr_wrapper.h"
+#include "ipc/ipc_platform_file_attachment_posix.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace IPC {
@@ -42,7 +43,8 @@ TEST(MessageAttachmentSet, BasicAdd) {
 
   ASSERT_EQ(set->size(), 0u);
   ASSERT_TRUE(set->empty());
-  ASSERT_TRUE(set->AddToBorrow(kFDBase));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
   ASSERT_EQ(set->size(), 1u);
   ASSERT_TRUE(!set->empty());
 
@@ -57,7 +59,8 @@ TEST(MessageAttachmentSet, BasicAddAndClose) {
   ASSERT_EQ(set->size(), 0u);
   ASSERT_TRUE(set->empty());
   const int fd = GetSafeFd();
-  ASSERT_TRUE(set->AddToOwn(base::ScopedFD(fd)));
+  ASSERT_TRUE(set->AddAttachment(
+      new internal::PlatformFileAttachment(base::ScopedFD(fd))));
   ASSERT_EQ(set->size(), 1u);
   ASSERT_TRUE(!set->empty());
 
@@ -69,9 +72,11 @@ TEST(MessageAttachmentSet, MaxSize) {
   scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
 
   for (size_t i = 0; i < MessageAttachmentSet::kMaxDescriptorsPerMessage; ++i)
-    ASSERT_TRUE(set->AddToBorrow(kFDBase + 1 + i));
+    ASSERT_TRUE(set->AddAttachment(
+        new internal::PlatformFileAttachment(kFDBase + 1 + i)));
 
-  ASSERT_TRUE(!set->AddToBorrow(kFDBase));
+  ASSERT_TRUE(
+      !set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
 
   set->CommitAll();
 }
@@ -98,7 +103,8 @@ TEST(MessageAttachmentSet, PeekDescriptors) {
   scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
 
   set->PeekDescriptors(NULL);
-  ASSERT_TRUE(set->AddToBorrow(kFDBase));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
 
   int fds[1];
   fds[0] = 0;
@@ -113,13 +119,16 @@ TEST(MessageAttachmentSet, WalkInOrder) {
 
   // TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
   // used to retrieve borrowed descriptors. That never happens in production.
-  ASSERT_TRUE(set->AddToBorrow(kFDBase));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 1));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 2));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 1)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2)));
 
-  ASSERT_EQ(set->TakeDescriptorAt(0), kFDBase);
-  ASSERT_EQ(set->TakeDescriptorAt(1), kFDBase + 1);
-  ASSERT_EQ(set->TakeDescriptorAt(2), kFDBase + 2);
+  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetAttachmentAt(1)->TakePlatformFile(), kFDBase + 1);
+  ASSERT_EQ(set->GetAttachmentAt(2)->TakePlatformFile(), kFDBase + 2);
 
   set->CommitAll();
 }
@@ -129,12 +138,15 @@ TEST(MessageAttachmentSet, WalkWrongOrder) {
 
   // TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
   // used to retrieve borrowed descriptors. That never happens in production.
-  ASSERT_TRUE(set->AddToBorrow(kFDBase));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 1));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 2));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 1)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2)));
 
-  ASSERT_EQ(set->TakeDescriptorAt(0), kFDBase);
-  ASSERT_EQ(set->TakeDescriptorAt(2), -1);
+  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetAttachmentAt(2), nullptr);
 
   set->CommitAll();
 }
@@ -144,19 +156,22 @@ TEST(MessageAttachmentSet, WalkCycle) {
 
   // TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
   // used to retrieve borrowed descriptors. That never happens in production.
-  ASSERT_TRUE(set->AddToBorrow(kFDBase));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 1));
-  ASSERT_TRUE(set->AddToBorrow(kFDBase + 2));
-
-  ASSERT_EQ(set->TakeDescriptorAt(0), kFDBase);
-  ASSERT_EQ(set->TakeDescriptorAt(1), kFDBase + 1);
-  ASSERT_EQ(set->TakeDescriptorAt(2), kFDBase + 2);
-  ASSERT_EQ(set->TakeDescriptorAt(0), kFDBase);
-  ASSERT_EQ(set->TakeDescriptorAt(1), kFDBase + 1);
-  ASSERT_EQ(set->TakeDescriptorAt(2), kFDBase + 2);
-  ASSERT_EQ(set->TakeDescriptorAt(0), kFDBase);
-  ASSERT_EQ(set->TakeDescriptorAt(1), kFDBase + 1);
-  ASSERT_EQ(set->TakeDescriptorAt(2), kFDBase + 2);
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 1)));
+  ASSERT_TRUE(
+      set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2)));
+
+  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetAttachmentAt(1)->TakePlatformFile(), kFDBase + 1);
+  ASSERT_EQ(set->GetAttachmentAt(2)->TakePlatformFile(), kFDBase + 2);
+  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetAttachmentAt(1)->TakePlatformFile(), kFDBase + 1);
+  ASSERT_EQ(set->GetAttachmentAt(2)->TakePlatformFile(), kFDBase + 2);
+  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetAttachmentAt(1)->TakePlatformFile(), kFDBase + 1);
+  ASSERT_EQ(set->GetAttachmentAt(2)->TakePlatformFile(), kFDBase + 2);
 
   set->CommitAll();
 }
@@ -165,7 +180,7 @@ TEST(MessageAttachmentSet, DontClose) {
   scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
 
   const int fd = GetSafeFd();
-  ASSERT_TRUE(set->AddToBorrow(fd));
+  ASSERT_TRUE(set->AddAttachment(new internal::PlatformFileAttachment(fd)));
   set->CommitAll();
 
   ASSERT_FALSE(VerifyClosed(fd));
@@ -175,7 +190,8 @@ TEST(MessageAttachmentSet, DoClose) {
   scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
 
   const int fd = GetSafeFd();
-  ASSERT_TRUE(set->AddToOwn(base::ScopedFD(fd)));
+  ASSERT_TRUE(set->AddAttachment(
+      new internal::PlatformFileAttachment(base::ScopedFD(fd))));
   set->CommitAll();
 
   ASSERT_TRUE(VerifyClosed(fd));

ipc/ipc_message_utils.cc

@@ -13,8 +13,13 @@
 #include "base/time/time.h"
 #include "base/values.h"
 #include "ipc/ipc_channel_handle.h"
+#include "ipc/ipc_message_attachment.h"
 #include "ipc/ipc_message_attachment_set.h"
 
+#if defined(OS_POSIX)
+#include "ipc/ipc_platform_file_attachment_posix.h"
+#endif
+
 #if defined(OS_WIN)
 #include <tchar.h>
 #endif
@@ -465,10 +470,11 @@ void ParamTraits<base::FileDescriptor>::Write(Message* m, const param_type& p) {
     return;
 
   if (p.auto_close) {
-    if (!m->WriteFile(base::ScopedFD(p.fd)))
+    if (!m->WriteAttachment(
+            new internal::PlatformFileAttachment(base::ScopedFD(p.fd))))
       NOTREACHED();
   } else {
-    if (!m->WriteBorrowingFile(p.fd))
+    if (!m->WriteAttachment(new internal::PlatformFileAttachment(p.fd)))
       NOTREACHED();
   }
 }
@@ -486,11 +492,11 @@ bool ParamTraits<base::FileDescriptor>::Read(const Message* m,
   if (!valid)
     return true;
 
-  base::ScopedFD fd;
-  if (!m->ReadFile(iter, &fd))
+  scoped_refptr<MessageAttachment> attachment;
+  if (!m->ReadAttachment(iter, &attachment))
     return false;
 
-  *r = base::FileDescriptor(fd.release(), true);
+  *r = base::FileDescriptor(attachment->TakePlatformFile(), true);
   return true;
 }
 
@@ -726,7 +732,7 @@ void ParamTraits<Message>::Write(Message* m, const Message& p) {
 #if defined(OS_POSIX)
   // We don't serialize the file descriptors in the nested message, so there
   // better not be any.
-  DCHECK(!p.HasFileDescriptors());
+  DCHECK(!p.HasAttachments());
 #endif
 
   // Don't just write out the message. This is used to send messages between

ipc/ipc_platform_file_attachment.cc → ipc/ipc_platform_file_attachment_posix.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ipc/ipc_platform_file_attachment.h"
+#include "ipc/ipc_platform_file_attachment_posix.h"
 
 namespace IPC {
 namespace internal {
@@ -11,6 +11,10 @@ PlatformFileAttachment::PlatformFileAttachment(base::PlatformFile file)
     : file_(file) {
 }
 
+PlatformFileAttachment::PlatformFileAttachment(base::ScopedFD file)
+    : file_(file.get()), owning_(file.Pass()) {
+}
+
 PlatformFileAttachment::~PlatformFileAttachment() {
 }
 
@@ -18,6 +22,11 @@ MessageAttachment::Type PlatformFileAttachment::GetType() const {
   return TYPE_PLATFORM_FILE;
 }
 
+base::PlatformFile PlatformFileAttachment::TakePlatformFile() {
+  ignore_result(owning_.release());
+  return file_;
+}
+
 base::PlatformFile GetPlatformFile(
     scoped_refptr<MessageAttachment> attachment) {
   DCHECK_EQ(attachment->GetType(), MessageAttachment::TYPE_PLATFORM_FILE);

ipc/ipc_platform_file_attachment.h → ipc/ipc_platform_file_attachment_posix.h

@@ -5,26 +5,34 @@
 #ifndef IPC_IPC_PLATFORM_FILE_ATTACHMENT_H_
 #define IPC_IPC_PLATFORM_FILE_ATTACHMENT_H_
 
-#include "base/files/file.h"
+#include "ipc/ipc_export.h"
 #include "ipc/ipc_message_attachment.h"
 
 namespace IPC {
 namespace internal {
 
 // A platform file that is sent over |Channel| as a part of |Message|.
-// |PlatformFileAttachment| doesn't own |file_|. The lifecycle of |file_| is
-// managed by |MessageAttachmentSet|.
-class PlatformFileAttachment : public MessageAttachment {
+// PlatformFileAttachment optionally owns the file and |owning_| is set in that
+// case. Also, |file_| is not cleared even after the ownership is taken.
+// Some old clients require this strange behavior.
+class IPC_EXPORT PlatformFileAttachment : public MessageAttachment {
  public:
+  // Non-owning constructor
   explicit PlatformFileAttachment(base::PlatformFile file);
+  // Owning constructor
+  explicit PlatformFileAttachment(base::ScopedFD file);
 
   Type GetType() const override;
+  base::PlatformFile TakePlatformFile() override;
+
   base::PlatformFile file() const { return file_; }
+  bool Owns() const { return owning_.is_valid(); }
 
  private:
   ~PlatformFileAttachment() override;
 
   base::PlatformFile file_;
+  base::ScopedFD owning_;
 };
 
 base::PlatformFile GetPlatformFile(scoped_refptr<MessageAttachment> attachment);

ipc/mojo/ipc_channel_mojo.cc

@@ -16,6 +16,10 @@
 #include "third_party/mojo/src/mojo/edk/embedder/embedder.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/error_handler.h"
 
+#if defined(OS_POSIX) && !defined(OS_NACL)
+#include "ipc/ipc_platform_file_attachment_posix.h"
+#endif
+
 namespace IPC {
 
 namespace {
@@ -352,8 +356,9 @@ MojoResult ChannelMojo::WriteToMessageAttachmentSet(
       return unwrap_result;
     }
 
-    bool ok = message->attachment_set()->AddToOwn(
-        base::ScopedFD(platform_handle.release().fd));
+    bool ok = message->attachment_set()->AddAttachment(
+        new internal::PlatformFileAttachment(
+            base::ScopedFD(platform_handle.release().fd)));
     DCHECK(ok);
   }
 
@@ -367,7 +372,7 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
   // We dup() the handles in IPC::Message to transmit.
   // IPC::MessageAttachmentSet has intricate lifecycle semantics
   // of FDs, so just to dup()-and-own them is the safest option.
-  if (message->HasFileDescriptors()) {
+  if (message->HasAttachments()) {
     MessageAttachmentSet* fdset = message->attachment_set();
     std::vector<base::PlatformFile> fds_to_send(fdset->size());
     fdset->PeekDescriptors(&fds_to_send[0]);

ipc/mojo/ipc_channel_mojo_unittest.cc

@@ -17,6 +17,7 @@
 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
+#include "ipc/ipc_platform_file_attachment_posix.h"
 #endif
 
 namespace {
@@ -328,8 +329,9 @@ class ListenerThatExpectsFile : public IPC::Listener {
     PickleIterator iter(message);
 
     base::ScopedFD fd;
-    EXPECT_TRUE(message.ReadFile(&iter, &fd));
-    base::File file(fd.release());
+    scoped_refptr<IPC::MessageAttachment> attachment;
+    EXPECT_TRUE(message.ReadAttachment(&iter, &attachment));
+    base::File file(attachment->TakePlatformFile());
     std::string content(GetSendingFileContent().size(), ' ');
     file.Read(0, &content[0], content.size());
     EXPECT_EQ(content, GetSendingFileContent());
@@ -359,7 +361,8 @@ class ListenerThatExpectsFile : public IPC::Listener {
     file.Flush();
     IPC::Message* message = new IPC::Message(
         0, 2, IPC::Message::PRIORITY_NORMAL);
-    message->WriteFile(base::ScopedFD(file.TakePlatformFile()));
+    message->WriteAttachment(new IPC::internal::PlatformFileAttachment(
+        base::ScopedFD(file.TakePlatformFile())));
     ASSERT_TRUE(sender->Send(message));
   }