ipc: Make a separate vector for brokerable attachments.

On Mac, an IPC::Message can have both non-brokered and brokered attachments.
During Message serialization, an index of the attachment is written as a
placeholder for the attachment. This relies on the assumption that there exists
a well defined ordering for all attachments. The receiving processes multiplexes
brokered and non-brokered attachments across different communication mediums,
and the ordering between them is non-deterministic.

I changed MessageAttachmentSet to have two vectors - one for brokered
attachments and one for non-brokered attachments. During Message serialization,
a bool is written that indicates whether the attachment is brokered, and then an
index into the appropriate vector.

I decided not to add an equivalent of |consumed_descriptor_highwater_| for
brokered attachments for two reasons:
  1) The sender has clearer ownership semantics for brokered attachments, and
  they will clean themselves up appropriately without assistance from the
  MessageAttachmentSet.
  2) Ownership semantics are fundamentally broken in the receiver, since the
  semantics for Message translation are stateless, whereas ownership transfer is
  stateful. Note the presence of awkward hacks for
  |consumed_descriptor_highwater_|, for which there is no good solution.

BUG=535711

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

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

9 files changed, 186 insertions, 79 deletions

diff --git a/components/nacl/loader/nacl_ipc_adapter.cc b/components/nacl/loader/nacl_ipc_adapter.cc
index 568d3a4..2f3eefc 100644
--- a/components/nacl/loader/nacl_ipc_adapter.cc
+++ b/components/nacl/loader/nacl_ipc_adapter.cc
@@ -615,6 +615,7 @@ scoped_ptr<IPC::Message> CreateOpenResourceReply(
   ppapi::proxy::SerializedHandle::WriteHeader(sh.header(),
                                               new_msg.get());
   new_msg->WriteBool(true);  // valid == true
+  new_msg->WriteBool(false);  // brokerable == false
   // The file descriptor is at index 0. There's only ever one file
   // descriptor provided for this message type, so this will be correct.
   new_msg->WriteInt(0);
diff --git a/ipc/ipc_channel_nacl.cc b/ipc/ipc_channel_nacl.cc
index 798657a..1fe75b6 100644
--- a/ipc/ipc_channel_nacl.cc
+++ b/ipc/ipc_channel_nacl.cc
@@ -286,7 +286,8 @@ bool ChannelNacl::ProcessOutgoingMessages() {
     output_queue_.pop_front();
 
     int fds[MessageAttachmentSet::kMaxDescriptorsPerMessage];
-    const size_t num_fds = msg->attachment_set()->size();
+    const size_t num_fds =
+        msg->attachment_set()->num_non_brokerable_attachments();
     DCHECK(num_fds <= MessageAttachmentSet::kMaxDescriptorsPerMessage);
     msg->attachment_set()->PeekDescriptors(fds);
 
@@ -308,7 +309,7 @@ bool ChannelNacl::ProcessOutgoingMessages() {
                   << msg->size();
       return false;
     } else {
-      msg->attachment_set()->CommitAll();
+      msg->attachment_set()->CommitAllDescriptors();
     }
 
     // Message sent OK!
diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
index abe2f98..51461e8 100644
--- a/ipc/ipc_channel_posix.cc
+++ b/ipc/ipc_channel_posix.cc
@@ -379,7 +379,7 @@ void ChannelPosix::CloseFileDescriptors(Message* msg) {
     QueueCloseFDMessage(to_close[i], 2);
   }
 #else
-  msg->attachment_set()->CommitAll();
+  msg->attachment_set()->CommitAllDescriptors();
 #endif
 }
 
@@ -415,10 +415,11 @@ bool ChannelPosix::ProcessOutgoingMessages() {
 
     Message* msg = element->get_message();
     if (message_send_bytes_written_ == 0 && msg &&
-        !msg->attachment_set()->empty()) {
+        msg->attachment_set()->num_non_brokerable_attachments()) {
       // This is the first chunk of a message which has descriptors to send
       struct cmsghdr *cmsg;
-      const unsigned num_fds = msg->attachment_set()->size();
+      const unsigned num_fds =
+          msg->attachment_set()->num_non_brokerable_attachments();
 
       DCHECK(num_fds <= MessageAttachmentSet::kMaxDescriptorsPerMessage);
       if (msg->attachment_set()->ContainsDirectoryDescriptor()) {
diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc
index 4980b40..1df631b 100644
--- a/ipc/ipc_message.cc
+++ b/ipc/ipc_message.cc
@@ -238,24 +238,46 @@ bool Message::AddPlaceholderBrokerableAttachmentWithId(
 }
 
 bool Message::WriteAttachment(scoped_refptr<MessageAttachment> attachment) {
-  // We write the index of the descriptor so that we don't have to
+  bool brokerable;
+  size_t index;
+  bool success =
+      attachment_set()->AddAttachment(attachment, &index, &brokerable);
+  DCHECK(success);
+
+  // Write the type of descriptor.
+  WriteBool(brokerable);
+
+  // 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()->AddAttachment(attachment);
+  WriteInt(static_cast<int>(index));
+
+#if USE_ATTACHMENT_BROKER && defined(OS_MACOSX) && !defined(OS_IOS)
+  if (brokerable)
+    header()->num_brokered_attachments++;
+#endif
+
+  return success;
 }
 
 bool Message::ReadAttachment(
     base::PickleIterator* iter,
     scoped_refptr<MessageAttachment>* attachment) const {
-  int descriptor_index;
-  if (!iter->ReadInt(&descriptor_index))
+  bool brokerable;
+  if (!iter->ReadBool(&brokerable))
+    return false;
+
+  int index;
+  if (!iter->ReadInt(&index))
     return false;
 
   MessageAttachmentSet* attachment_set = attachment_set_.get();
   if (!attachment_set)
     return false;
 
-  *attachment = attachment_set->GetAttachmentAt(descriptor_index);
+  *attachment = brokerable
+                    ? attachment_set->GetBrokerableAttachmentAt(index)
+                    : attachment_set->GetNonBrokerableAttachmentAt(index);
+
   return nullptr != attachment->get();
 }
 
diff --git a/ipc/ipc_message_attachment_set.cc b/ipc/ipc_message_attachment_set.cc
index faf6462..b290e0a 100644
--- a/ipc/ipc_message_attachment_set.cc
+++ b/ipc/ipc_message_attachment_set.cc
@@ -39,7 +39,7 @@ MessageAttachmentSet::MessageAttachmentSet()
 }
 
 MessageAttachmentSet::~MessageAttachmentSet() {
-  if (consumed_descriptor_highwater_ == size())
+  if (consumed_descriptor_highwater_ == num_non_brokerable_attachments())
     return;
 
   // We close all the owning descriptors. If this message should have
@@ -51,7 +51,7 @@ MessageAttachmentSet::~MessageAttachmentSet() {
   // the descriptors have their close flag set and we free all the extra
   // kernel resources.
   LOG(WARNING) << "MessageAttachmentSet destroyed with unconsumed descriptors: "
-               << consumed_descriptor_highwater_ << "/" << size();
+               << consumed_descriptor_highwater_ << "/" << num_descriptors();
 }
 
 unsigned MessageAttachmentSet::num_descriptors() const {
@@ -65,16 +65,22 @@ unsigned MessageAttachmentSet::num_mojo_handles() const {
 }
 
 unsigned MessageAttachmentSet::num_brokerable_attachments() const {
-  return count_attachments_of_type(
-      attachments_, MessageAttachment::TYPE_BROKERABLE_ATTACHMENT);
+  return static_cast<unsigned>(brokerable_attachments_.size());
 }
 
-unsigned MessageAttachmentSet::size() const {
+unsigned MessageAttachmentSet::num_non_brokerable_attachments() const {
   return static_cast<unsigned>(attachments_.size());
 }
 
+unsigned MessageAttachmentSet::size() const {
+  return static_cast<unsigned>(attachments_.size() +
+                               brokerable_attachments_.size());
+}
+
 bool MessageAttachmentSet::AddAttachment(
-    scoped_refptr<MessageAttachment> attachment) {
+    scoped_refptr<MessageAttachment> attachment,
+    size_t* index,
+    bool* brokerable) {
 #if defined(OS_POSIX)
   if (attachment->GetType() == MessageAttachment::TYPE_PLATFORM_FILE &&
       num_descriptors() == kMaxDescriptorsPerMessage) {
@@ -83,14 +89,37 @@ bool MessageAttachmentSet::AddAttachment(
   }
 #endif
 
-  attachments_.push_back(attachment);
-  return true;
+  switch (attachment->GetType()) {
+    case MessageAttachment::TYPE_PLATFORM_FILE:
+    case MessageAttachment::TYPE_MOJO_HANDLE:
+      attachments_.push_back(attachment);
+      *index = attachments_.size() - 1;
+      *brokerable = false;
+      return true;
+    case MessageAttachment::TYPE_BROKERABLE_ATTACHMENT:
+      BrokerableAttachment* brokerable_attachment =
+          static_cast<BrokerableAttachment*>(attachment.get());
+      scoped_refptr<BrokerableAttachment> a(brokerable_attachment);
+      brokerable_attachments_.push_back(a);
+      *index = brokerable_attachments_.size() - 1;
+      *brokerable = true;
+      return true;
+  }
+  return false;
+}
+
+bool MessageAttachmentSet::AddAttachment(
+    scoped_refptr<MessageAttachment> attachment) {
+  bool brokerable;
+  size_t index;
+  return AddAttachment(attachment, &index, &brokerable);
 }
 
-scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
-    unsigned index) {
-  if (index >= size()) {
-    DLOG(WARNING) << "Accessing out of bound index:" << index << "/" << size();
+scoped_refptr<MessageAttachment>
+MessageAttachmentSet::GetNonBrokerableAttachmentAt(unsigned index) {
+  if (index >= num_non_brokerable_attachments()) {
+    DLOG(WARNING) << "Accessing out of bound index:" << index << "/"
+                  << num_non_brokerable_attachments();
     return scoped_refptr<MessageAttachment>();
   }
 
@@ -115,8 +144,10 @@ scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
   // end of the array and index 0 is requested, we reset the highwater value.
   // TODO(morrita): This is absurd. This "wringle" disallow to introduce clearer
   // ownership model. Only client is NaclIPCAdapter. See crbug.com/415294
-  if (index == 0 && consumed_descriptor_highwater_ == size())
+  if (index == 0 &&
+      consumed_descriptor_highwater_ == num_non_brokerable_attachments()) {
     consumed_descriptor_highwater_ = 0;
+  }
 
   if (index != consumed_descriptor_highwater_)
     return scoped_refptr<MessageAttachment>();
@@ -126,7 +157,20 @@ scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
   return attachments_[index];
 }
 
-void MessageAttachmentSet::CommitAll() {
+scoped_refptr<MessageAttachment>
+MessageAttachmentSet::GetBrokerableAttachmentAt(unsigned index) {
+  if (index >= num_brokerable_attachments()) {
+    DLOG(WARNING) << "Accessing out of bound index:" << index << "/"
+                  << num_brokerable_attachments();
+    return scoped_refptr<MessageAttachment>();
+  }
+
+  scoped_refptr<BrokerableAttachment> brokerable_attachment(
+      brokerable_attachments_[index]);
+  return scoped_refptr<MessageAttachment>(brokerable_attachment.get());
+}
+
+void MessageAttachmentSet::CommitAllDescriptors() {
   attachments_.clear();
   consumed_descriptor_highwater_ = 0;
 }
@@ -134,20 +178,17 @@ void MessageAttachmentSet::CommitAll() {
 std::vector<BrokerableAttachment*>
 MessageAttachmentSet::GetBrokerableAttachments() const {
   std::vector<BrokerableAttachment*> output;
-  for (const scoped_refptr<MessageAttachment>& attachment : attachments_) {
-    if (attachment->GetType() ==
-        MessageAttachment::TYPE_BROKERABLE_ATTACHMENT) {
+  for (const scoped_refptr<MessageAttachment>& attachment :
+       brokerable_attachments_) {
       output.push_back(static_cast<BrokerableAttachment*>(attachment.get()));
-    }
   }
   return output;
 }
 
 void MessageAttachmentSet::ReplacePlaceholderWithAttachment(
     const scoped_refptr<BrokerableAttachment>& attachment) {
-  for (auto it = attachments_.begin(); it != attachments_.end(); ++it) {
-    if ((*it)->GetType() != MessageAttachment::TYPE_BROKERABLE_ATTACHMENT)
-      continue;
+  for (auto it = brokerable_attachments_.begin();
+       it != brokerable_attachments_.end(); ++it) {
     BrokerableAttachment* brokerable_attachment =
         static_cast<BrokerableAttachment*>(it->get());
 
@@ -191,7 +232,7 @@ void MessageAttachmentSet::ReleaseFDsToClose(
       fds->push_back(file->TakePlatformFile());
   }
 
-  CommitAll();
+  CommitAllDescriptors();
 }
 
 void MessageAttachmentSet::AddDescriptorsToOwn(const base::PlatformFile* buffer,
diff --git a/ipc/ipc_message_attachment_set.h b/ipc/ipc_message_attachment_set.h
index 0ed62ac..4276fcd 100644
--- a/ipc/ipc_message_attachment_set.h
+++ b/ipc/ipc_message_attachment_set.h
@@ -21,10 +21,21 @@ class BrokerableAttachment;
 class MessageAttachment;
 
 // -----------------------------------------------------------------------------
-// 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).
+// A MessageAttachmentSet is an ordered set of MessageAttachment objects
+// associated with an IPC message. There are three types of MessageAttachments:
+//   1) TYPE_PLATFORM_FILE is transmitted over the Channel's underlying
+//   UNIX domain socket
+//   2) TYPE_MOJO_HANDLE is transmitted over the Mojo MessagePipe.
+//   3) TYPE_BROKERABLE_ATTACHMENT is transmitted by the Attachment Broker.
+// Any given IPC Message can have attachments of type (1) or (2), but not both.
+// These are stored in |attachments_|. Attachments of type (3) are stored in
+// |brokerable_attachments_|.
+//
+// To produce a deterministic ordering, all attachments in |attachments_| are
+// considered to come before those in |brokerable_attachments_|. These
+// attachments are transmitted across different communication channels, and
+// multiplexed by the receiver, so ordering between them cannot be guaranteed.
+//
 // -----------------------------------------------------------------------------
 class IPC_EXPORT MessageAttachmentSet
     : public base::RefCountedThreadSafe<MessageAttachmentSet> {
@@ -39,24 +50,41 @@ class IPC_EXPORT MessageAttachmentSet
   unsigned num_mojo_handles() const;
   // Return the number of brokerable attachments in the attachment set.
   unsigned num_brokerable_attachments() const;
+  // Return the number of non-brokerable attachments in the attachment set.
+  unsigned num_non_brokerable_attachments() const;
 
   // Return true if no unconsumed descriptors remain
   bool empty() const { return 0 == size(); }
 
+  // Returns whether the attachment was successfully added.
+  // |index| is an output variable. On success, it contains the index of the
+  // newly added attachment.
+  // |brokerable| is an output variable. On success, it describes which vector
+  // the attachment was added to.
+  bool AddAttachment(scoped_refptr<MessageAttachment> attachment,
+                     size_t* index,
+                     bool* brokerable);
+
+  // Similar to the above method, but without output variables.
   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.
+  // Take the nth non-brokerable attachment from the beginning of the vector,
+  // 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);
+  scoped_refptr<MessageAttachment> GetNonBrokerableAttachmentAt(unsigned index);
+
+  // Similar to GetNonBrokerableAttachmentAt, but there are no ordering
+  // requirements.
+  scoped_refptr<MessageAttachment> GetBrokerableAttachmentAt(unsigned index);
 
   // This must be called after transmitting the descriptors returned by
-  // PeekDescriptors. It marks all the descriptors as consumed and closes those
-  // which are auto-close.
-  void CommitAll();
+  // PeekDescriptors. It marks all the non-brokerable descriptors as consumed
+  // and closes those which are auto-close.
+  void CommitAllDescriptors();
 
   // Returns a vector of all brokerable attachments.
   std::vector<BrokerableAttachment*> GetBrokerableAttachments() const;
@@ -80,15 +108,16 @@ class IPC_EXPORT MessageAttachmentSet
   // ---------------------------------------------------------------------------
   // Interfaces for transmission...
 
-  // Fill an array with file descriptors without 'consuming' them. CommitAll
-  // must be called after these descriptors have been transmitted.
+  // Fill an array with file descriptors without 'consuming' them.
+  // CommitAllDescriptors must be called after these descriptors have been
+  // transmitted.
   //   buffer: (output) a buffer of, at least, size() integers.
   void PeekDescriptors(base::PlatformFile* buffer) const;
   // Returns true if any contained file descriptors appear to be handles to a
   // directory.
   bool ContainsDirectoryDescriptor() const;
-  // Fetch all filedescriptors with the "auto close" property.
-  // Used instead of CommitAll() when closing must be handled manually.
+  // Fetch all filedescriptors with the "auto close" property. Used instead of
+  // CommitAllDescriptors() when closing must be handled manually.
   void ReleaseFDsToClose(std::vector<base::PlatformFile>* fds);
 
   // ---------------------------------------------------------------------------
@@ -110,10 +139,12 @@ class IPC_EXPORT MessageAttachmentSet
 
   ~MessageAttachmentSet();
 
-  // A vector of attachments of the message, which might be |PlatformFile| or
-  // |MessagePipe|.
+  // All elements either have type TYPE_PLATFORM_FILE or TYPE_MOJO_HANDLE.
   std::vector<scoped_refptr<MessageAttachment>> attachments_;
 
+  // All elements have type TYPE_BROKERABLE_ATTACHMENT.
+  std::vector<scoped_refptr<BrokerableAttachment>> brokerable_attachments_;
+
   // 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
diff --git a/ipc/ipc_message_attachment_set_posix_unittest.cc b/ipc/ipc_message_attachment_set_posix_unittest.cc
index 8df312f..c7caea6 100644
--- a/ipc/ipc_message_attachment_set_posix_unittest.cc
+++ b/ipc/ipc_message_attachment_set_posix_unittest.cc
@@ -49,7 +49,7 @@ TEST(MessageAttachmentSet, BasicAdd) {
 
   // Empties the set and stops a warning about deleting a set with unconsumed
   // descriptors
-  set->CommitAll();
+  set->CommitAllDescriptors();
 }
 
 TEST(MessageAttachmentSet, BasicAddAndClose) {
@@ -63,7 +63,7 @@ TEST(MessageAttachmentSet, BasicAddAndClose) {
   ASSERT_EQ(set->size(), 1u);
   ASSERT_TRUE(!set->empty());
 
-  set->CommitAll();
+  set->CommitAllDescriptors();
 
   ASSERT_TRUE(VerifyClosed(fd));
 }
@@ -77,7 +77,7 @@ TEST(MessageAttachmentSet, MaxSize) {
   ASSERT_TRUE(
       !set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
 
-  set->CommitAll();
+  set->CommitAllDescriptors();
 }
 
 #if defined(OS_ANDROID)
@@ -98,7 +98,7 @@ TEST(MessageAttachmentSet, MAYBE_SetDescriptors) {
   ASSERT_TRUE(!set->empty());
   ASSERT_EQ(set->size(), 1u);
 
-  set->CommitAll();
+  set->CommitAllDescriptors();
 
   ASSERT_TRUE(VerifyClosed(fd));
 }
@@ -114,7 +114,7 @@ TEST(MessageAttachmentSet, PeekDescriptors) {
   fds[0] = 0;
   set->PeekDescriptors(fds);
   ASSERT_EQ(fds[0], kFDBase);
-  set->CommitAll();
+  set->CommitAllDescriptors();
   ASSERT_TRUE(set->empty());
 }
 
@@ -130,11 +130,13 @@ TEST(MessageAttachmentSet, WalkInOrder) {
   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->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(),
+            kFDBase + 1);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(),
+            kFDBase + 2);
 
-  set->CommitAll();
+  set->CommitAllDescriptors();
 }
 
 TEST(MessageAttachmentSet, WalkWrongOrder) {
@@ -149,10 +151,10 @@ TEST(MessageAttachmentSet, WalkWrongOrder) {
   ASSERT_TRUE(
       set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2)));
 
-  ASSERT_EQ(set->GetAttachmentAt(0)->TakePlatformFile(), kFDBase);
-  ASSERT_EQ(set->GetAttachmentAt(2), nullptr);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2), nullptr);
 
-  set->CommitAll();
+  set->CommitAllDescriptors();
 }
 
 TEST(MessageAttachmentSet, WalkCycle) {
@@ -167,17 +169,23 @@ TEST(MessageAttachmentSet, WalkCycle) {
   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();
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(),
+            kFDBase + 1);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(),
+            kFDBase + 2);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(),
+            kFDBase + 1);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(),
+            kFDBase + 2);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(),
+            kFDBase + 1);
+  ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(),
+            kFDBase + 2);
+
+  set->CommitAllDescriptors();
 }
 
 #if defined(OS_ANDROID)
@@ -190,7 +198,7 @@ TEST(MessageAttachmentSet, MAYBE_DontClose) {
 
   const int fd = GetSafeFd();
   ASSERT_TRUE(set->AddAttachment(new internal::PlatformFileAttachment(fd)));
-  set->CommitAll();
+  set->CommitAllDescriptors();
 
   ASSERT_FALSE(VerifyClosed(fd));
 }
@@ -201,7 +209,7 @@ TEST(MessageAttachmentSet, DoClose) {
   const int fd = GetSafeFd();
   ASSERT_TRUE(set->AddAttachment(
       new internal::PlatformFileAttachment(base::ScopedFD(fd))));
-  set->CommitAll();
+  set->CommitAllDescriptors();
 
   ASSERT_TRUE(VerifyClosed(fd));
 }
diff --git a/ipc/mojo/ipc_channel_mojo.cc b/ipc/mojo/ipc_channel_mojo.cc
index 5239cee..7b91f72 100644
--- a/ipc/mojo/ipc_channel_mojo.cc
+++ b/ipc/mojo/ipc_channel_mojo.cc
@@ -475,8 +475,9 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
   // of FDs, so just to dup()-and-own them is the safest option.
   if (message->HasAttachments()) {
     MessageAttachmentSet* set = message->attachment_set();
-    for (unsigned i = 0; i < set->size(); ++i) {
-      scoped_refptr<MessageAttachment> attachment = set->GetAttachmentAt(i);
+    for (unsigned i = 0; i < set->num_non_brokerable_attachments(); ++i) {
+      scoped_refptr<MessageAttachment> attachment =
+          set->GetNonBrokerableAttachmentAt(i);
       switch (attachment->GetType()) {
         case MessageAttachment::TYPE_PLATFORM_FILE:
 #if defined(OS_POSIX) && !defined(OS_NACL)
@@ -486,7 +487,7 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
                   attachment.get()));
           if (!file.is_valid()) {
             DPLOG(WARNING) << "Failed to dup FD to transmit.";
-            set->CommitAll();
+            set->CommitAllDescriptors();
             return MOJO_RESULT_UNKNOWN;
           }
 
@@ -498,7 +499,7 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
           if (MOJO_RESULT_OK != wrap_result) {
             LOG(WARNING) << "Pipe failed to wrap handles. Closing: "
                          << wrap_result;
-            set->CommitAll();
+            set->CommitAllDescriptors();
             return wrap_result;
           }
 
@@ -522,7 +523,7 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
       }
     }
 
-    set->CommitAll();
+    set->CommitAllDescriptors();
   }
 
   return MOJO_RESULT_OK;
diff --git a/ppapi/proxy/nacl_message_scanner.cc b/ppapi/proxy/nacl_message_scanner.cc
index 3e522f4..769a03f 100644
--- a/ppapi/proxy/nacl_message_scanner.cc
+++ b/ppapi/proxy/nacl_message_scanner.cc
@@ -57,6 +57,7 @@ void WriteHandle(int handle_index,
     // Now write the handle itself in POSIX style.
     // See ParamTraits<FileDescriptor>::Read for where these values are read.
     msg->WriteBool(true);  // valid == true
+    msg->WriteBool(false);  // brokerable == false
     msg->WriteInt(handle_index);
   }
 }