Revert of Remove IPC::BrokerableAttachment. (patchset chromium#1 id:1…

…20001 of https://codereview.chromium.org/2494943002/ )

Reason for revert:
Caused crash in NewlibPackagedAppTest.SuccessfulLoad browser_tests.

See http://crbug.com/665678#c1

BUG=665678

Original issue's description:
> Remove IPC::BrokerableAttachment.
>
> With only ChannelMojo in use, the distinction between brokerable and
> non-brokerable attachments no longer makes sense. This CL removes that
> distinction by removing BrokerableAttachment and flattening the
> hierarchy of attachment types.
>
> This also trims some POSIX-specific parts of IPC::MessageAttachmentSet.
>
> BUG=659448
>
> Committed: https://crrev.com/f6e03ce56c4d2370b79d0c3dd4ceb89cf5528e56
> Cr-Commit-Position: refs/heads/master@{#432153}

TBR=rockot@chromium.org,dcheng@chromium.org,mseaborn@chromium.org,raymes@chromium.org,erikchen@chromium.org,sammc@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=659448

Review-Url: https://codereview.chromium.org/2504063002
Cr-Commit-Position: refs/heads/master@{#432351}

base/pickle.cc

@@ -233,6 +233,7 @@ void PickleSizer::AddBytes(int length) {
 
 void PickleSizer::AddAttachment() {
   // From IPC::Message::WriteAttachment
+  AddBool();
   AddInt();
 }
 

components/nacl/loader/nacl_ipc_adapter.cc

@@ -626,6 +626,7 @@ std::unique_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);

ipc/BUILD.gn

@@ -9,6 +9,8 @@ import("//tools/ipc_fuzzer/ipc_fuzzer.gni")
 
 component("ipc") {
   sources = [
+    "brokerable_attachment.cc",
+    "brokerable_attachment.h",
     "export_template.h",
     "handle_attachment_win.cc",
     "handle_attachment_win.h",

ipc/brokerable_attachment.cc

@@ -0,0 +1,34 @@
+// Copyright 2015 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 "ipc/brokerable_attachment.h"
+
+#include <stddef.h>
+
+#include "build/build_config.h"
+
+namespace IPC {
+
+// BrokerableAttachment::BrokerableAttachment ----------------------------------
+
+BrokerableAttachment::BrokerableAttachment() = default;
+
+BrokerableAttachment::~BrokerableAttachment() = default;
+
+bool BrokerableAttachment::NeedsBrokering() const {
+  return GetBrokerableType() == PLACEHOLDER;
+}
+
+BrokerableAttachment::Type BrokerableAttachment::GetType() const {
+  return TYPE_BROKERABLE_ATTACHMENT;
+}
+
+#if defined(OS_POSIX)
+base::PlatformFile BrokerableAttachment::TakePlatformFile() {
+  NOTREACHED();
+  return base::PlatformFile();
+}
+#endif  // OS_POSIX
+
+}  // namespace IPC

ipc/brokerable_attachment.h

@@ -0,0 +1,54 @@
+// Copyright 2015 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.
+
+#ifndef IPC_BROKERABLE_ATTACHMENT_H_
+#define IPC_BROKERABLE_ATTACHMENT_H_
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <algorithm>
+
+#include "base/macros.h"
+#include "build/build_config.h"
+#include "ipc/ipc_export.h"
+#include "ipc/ipc_message_attachment.h"
+
+namespace IPC {
+
+// This subclass of MessageAttachment requires an AttachmentBroker to be
+// attached to a Chrome IPC message.
+class IPC_EXPORT BrokerableAttachment : public MessageAttachment {
+ public:
+  enum BrokerableType {
+    PLACEHOLDER,
+    WIN_HANDLE,
+    MACH_PORT,
+  };
+
+  // Whether the attachment still needs information from the broker before it
+  // can be used.
+  bool NeedsBrokering() const;
+
+  // Returns TYPE_BROKERABLE_ATTACHMENT
+  Type GetType() const override;
+
+  virtual BrokerableType GetBrokerableType() const = 0;
+
+// MessageAttachment override.
+#if defined(OS_POSIX)
+  base::PlatformFile TakePlatformFile() override;
+#endif  // OS_POSIX
+
+ protected:
+  BrokerableAttachment();
+  ~BrokerableAttachment() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BrokerableAttachment);
+};
+
+}  // namespace IPC
+
+#endif  // IPC_BROKERABLE_ATTACHMENT_H_

ipc/handle_attachment_win.cc

@@ -28,13 +28,24 @@ HandleAttachmentWin::HandleAttachmentWin(const HANDLE& handle,
                                          FromWire from_wire)
     : handle_(handle), permissions_(HandleWin::INVALID), owns_handle_(true) {}
 
+HandleAttachmentWin::HandleAttachmentWin(const WireFormat& wire_format)
+    : handle_(LongToHandle(wire_format.handle)),
+      permissions_(wire_format.permissions),
+      owns_handle_(true) {}
+
 HandleAttachmentWin::~HandleAttachmentWin() {
   if (handle_ != INVALID_HANDLE_VALUE && owns_handle_)
     ::CloseHandle(handle_);
 }
 
-MessageAttachment::Type HandleAttachmentWin::GetType() const {
-  return Type::WIN_HANDLE;
+HandleAttachmentWin::BrokerableType HandleAttachmentWin::GetBrokerableType()
+    const {
+  return WIN_HANDLE;
+}
+
+HandleAttachmentWin::WireFormat HandleAttachmentWin::GetWireFormat(
+    const base::ProcessId& destination) const {
+  return WireFormat(HandleToLong(handle_), destination, permissions_);
 }
 
 }  // namespace internal

ipc/handle_attachment_win.h

@@ -8,16 +8,48 @@
 #include <stdint.h>
 
 #include "base/process/process_handle.h"
+#include "ipc/brokerable_attachment.h"
 #include "ipc/handle_win.h"
 #include "ipc/ipc_export.h"
-#include "ipc/ipc_message_attachment.h"
 
 namespace IPC {
 namespace internal {
 
 // This class represents a Windows HANDLE attached to a Chrome IPC message.
-class IPC_EXPORT HandleAttachmentWin : public MessageAttachment {
+class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment {
  public:
+  // The wire format for this handle.
+  struct IPC_EXPORT WireFormat {
+    // IPC translation requires that classes passed through IPC have a default
+    // constructor.
+    WireFormat()
+        : handle(0),
+          destination_process(0),
+          permissions(HandleWin::INVALID) {}
+
+    WireFormat(int32_t handle,
+               const base::ProcessId& destination_process,
+               HandleWin::Permissions permissions)
+        : handle(handle),
+          destination_process(destination_process),
+          permissions(permissions) {}
+
+    // The HANDLE that is intended for duplication, or the HANDLE that has been
+    // duplicated, depending on context.
+    // The type is int32_t instead of HANDLE because HANDLE gets typedefed to
+    // void*, whose size varies between 32 and 64-bit processes. Using a
+    // int32_t means that 64-bit processes will need to perform both up-casting
+    // and down-casting. This is performed using the appropriate Windows APIs.
+    // A value of 0 is equivalent to an invalid handle.
+    int32_t handle;
+
+    // The id of the destination process that the handle is duplicated into.
+    base::ProcessId destination_process;
+
+    // The permissions to use when duplicating the handle.
+    HandleWin::Permissions permissions;
+  };
+
   // This constructor makes a copy of |handle| and takes ownership of the
   // result. Should only be called by the sender of a Chrome IPC message.
   HandleAttachmentWin(const HANDLE& handle, HandleWin::Permissions permissions);
@@ -29,7 +61,14 @@ class IPC_EXPORT HandleAttachmentWin : public MessageAttachment {
   // receiver of a Chrome IPC message.
   HandleAttachmentWin(const HANDLE& handle, FromWire from_wire);
 
-  Type GetType() const override;
+  // This constructor takes ownership of |wire_format.handle| without making a
+  // copy. Should only be called by the receiver of a Chrome IPC message.
+  explicit HandleAttachmentWin(const WireFormat& wire_format);
+
+  BrokerableType GetBrokerableType() const override;
+
+  // Returns the wire format of this attachment.
+  WireFormat GetWireFormat(const base::ProcessId& destination) const;
 
   HANDLE get_handle() const { return handle_; }
 

ipc/handle_win.cc

@@ -38,10 +38,16 @@ bool ParamTraits<HandleWin>::Read(const base::Pickle* m,
     return false;
   MessageAttachment* attachment =
       static_cast<MessageAttachment*>(base_attachment.get());
-  if (attachment->GetType() != MessageAttachment::Type::WIN_HANDLE)
+  if (attachment->GetType() != MessageAttachment::TYPE_BROKERABLE_ATTACHMENT)
     return false;
+  BrokerableAttachment* brokerable_attachment =
+      static_cast<BrokerableAttachment*>(attachment);
+  if (brokerable_attachment->GetBrokerableType() !=
+      BrokerableAttachment::WIN_HANDLE) {
+    return false;
+  }
   IPC::internal::HandleAttachmentWin* handle_attachment =
-      static_cast<IPC::internal::HandleAttachmentWin*>(attachment);
+      static_cast<IPC::internal::HandleAttachmentWin*>(brokerable_attachment);
   r->set_handle(handle_attachment->get_handle());
   handle_attachment->reset_handle_ownership();
   return true;

ipc/ipc_channel_mojo.cc

@@ -124,14 +124,14 @@ base::ScopedFD TakeOrDupFile(internal::PlatformFileAttachment* attachment) {
 
 MojoResult WrapAttachmentImpl(MessageAttachment* attachment,
                               mojom::SerializedHandlePtr* serialized) {
-  if (attachment->GetType() == MessageAttachment::Type::MOJO_HANDLE) {
+  if (attachment->GetType() == MessageAttachment::TYPE_MOJO_HANDLE) {
     *serialized = CreateSerializedHandle(
         static_cast<internal::MojoHandleAttachment&>(*attachment).TakeHandle(),
         mojom::SerializedHandle::Type::MOJO_HANDLE);
     return MOJO_RESULT_OK;
   }
 #if defined(OS_POSIX)
-  if (attachment->GetType() == MessageAttachment::Type::PLATFORM_FILE) {
+  if (attachment->GetType() == MessageAttachment::TYPE_PLATFORM_FILE) {
     // 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.
@@ -148,15 +148,21 @@ MojoResult WrapAttachmentImpl(MessageAttachment* attachment,
   }
 #endif
 #if defined(OS_MACOSX)
-  DCHECK_EQ(attachment->GetType(), MessageAttachment::Type::MACH_PORT);
+  DCHECK_EQ(attachment->GetType(),
+            MessageAttachment::TYPE_BROKERABLE_ATTACHMENT);
+  DCHECK_EQ(static_cast<BrokerableAttachment&>(*attachment).GetBrokerableType(),
+            BrokerableAttachment::MACH_PORT);
   internal::MachPortAttachmentMac& mach_port_attachment =
       static_cast<internal::MachPortAttachmentMac&>(*attachment);
   MojoResult result = WrapMachPort(mach_port_attachment.get_mach_port(),
                                    serialized);
   mach_port_attachment.reset_mach_port_ownership();
   return result;
 #elif defined(OS_WIN)
-  DCHECK_EQ(attachment->GetType(), MessageAttachment::Type::WIN_HANDLE);
+  DCHECK_EQ(attachment->GetType(),
+            MessageAttachment::TYPE_BROKERABLE_ATTACHMENT);
+  DCHECK_EQ(static_cast<BrokerableAttachment&>(*attachment).GetBrokerableType(),
+            BrokerableAttachment::WIN_HANDLE);
   internal::HandleAttachmentWin& handle_attachment =
       static_cast<internal::HandleAttachmentWin&>(*attachment);
   MojoResult result = WrapPlatformHandle(
@@ -403,9 +409,18 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet(
   std::vector<mojom::SerializedHandlePtr> output_handles;
   MessageAttachmentSet* set = message->attachment_set();
 
-  for (unsigned i = 0; result == MOJO_RESULT_OK && i < set->size(); ++i) {
-    result = WrapAttachment(set->GetAttachmentAt(i).get(), &output_handles);
+  for (unsigned i = 0;
+       result == MOJO_RESULT_OK && i < set->num_non_brokerable_attachments();
+       ++i) {
+    result = WrapAttachment(set->GetNonBrokerableAttachmentAt(i).get(),
+                            &output_handles);
   }
+  for (unsigned i = 0;
+       result == MOJO_RESULT_OK && i < set->num_brokerable_attachments(); ++i) {
+    result = WrapAttachment(set->GetBrokerableAttachmentAt(i).get(),
+                            &output_handles);
+  }
+
   set->CommitAllDescriptors();
 
   if (!output_handles.empty())

ipc/ipc_channel_mojo_unittest.cc

@@ -289,12 +289,11 @@ class HandleSendingHelper {
     base::ScopedFD fd;
     scoped_refptr<base::Pickle::Attachment> attachment;
     EXPECT_TRUE(message.ReadAttachment(iter, &attachment));
-    EXPECT_EQ(
-        IPC::MessageAttachment::Type::PLATFORM_FILE,
-        static_cast<IPC::MessageAttachment*>(attachment.get())->GetType());
-    base::File file(
-        static_cast<IPC::internal::PlatformFileAttachment*>(attachment.get())
-            ->TakePlatformFile());
+    EXPECT_EQ(IPC::MessageAttachment::TYPE_PLATFORM_FILE,
+              static_cast<IPC::MessageAttachment*>(attachment.get())
+                  ->GetType());
+    base::File file(static_cast<IPC::MessageAttachment*>(attachment.get())
+                        ->TakePlatformFile());
     std::string content(GetSendingFileContent().size(), ' ');
     file.Read(0, &content[0], content.size());
     EXPECT_EQ(content, GetSendingFileContent());

ipc/ipc_channel_nacl.cc

@@ -23,7 +23,6 @@
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_message_attachment_set.h"
-#include "ipc/ipc_platform_file_attachment_posix.h"
 #include "native_client/src/public/imc_syscalls.h"
 #include "native_client/src/public/imc_types.h"
 
@@ -224,11 +223,8 @@ void ChannelNacl::DidRecvMsg(std::unique_ptr<MessageContents> contents) {
   data->swap(contents->data);
   read_queue_.push_back(data);
 
-  input_attachments_.reserve(contents->fds.size());
-  for (int fd : contents->fds) {
-    input_attachments_.push_back(
-        new internal::PlatformFileAttachment(base::ScopedFD(fd)));
-  }
+  input_fds_.insert(input_fds_.end(),
+                    contents->fds.begin(), contents->fds.end());
   contents->fds.clear();
 
   // In POSIX, we would be told when there are bytes to read by implementing
@@ -276,22 +272,16 @@ bool ChannelNacl::ProcessOutgoingMessages() {
     linked_ptr<Message> msg = output_queue_.front();
     output_queue_.pop_front();
 
-    const size_t num_fds = msg->attachment_set()->size();
+    int fds[MessageAttachmentSet::kMaxDescriptorsPerMessage];
+    const size_t num_fds =
+        msg->attachment_set()->num_non_brokerable_attachments();
     DCHECK(num_fds <= MessageAttachmentSet::kMaxDescriptorsPerMessage);
-    std::vector<int> fds;
-    fds.reserve(num_fds);
-    for (size_t i = 0; i < num_fds; i++) {
-      scoped_refptr<MessageAttachment> attachment =
-          msg->attachment_set()->GetAttachmentAt(i);
-      DCHECK_EQ(MessageAttachment::Type::PLATFORM_FILE, attachment->GetType());
-      fds.push_back(static_cast<internal::PlatformFileAttachment&>(*attachment)
-                        .TakePlatformFile());
-    }
+    msg->attachment_set()->PeekDescriptors(fds);
 
     NaClAbiNaClImcMsgIoVec iov = {
       const_cast<void*>(msg->data()), msg->size()
     };
-    NaClAbiNaClImcMsgHdr msgh = {&iov, 1, fds.data(), num_fds};
+    NaClAbiNaClImcMsgHdr msgh = { &iov, 1, fds, num_fds };
     ssize_t bytes_written = imc_sendmsg(pipe_, &msgh, 0);
 
     DCHECK(bytes_written);  // The trusted side shouldn't return 0.
@@ -355,22 +345,23 @@ bool ChannelNacl::ShouldDispatchInputMessage(Message* msg) {
   return true;
 }
 
-bool ChannelNacl::GetAttachments(Message* msg) {
+bool ChannelNacl::GetNonBrokeredAttachments(Message* msg) {
   uint16_t header_fds = msg->header()->num_fds;
-  CHECK(header_fds == input_attachments_.size());
+  CHECK(header_fds == input_fds_.size());
   if (header_fds == 0)
     return true;  // Nothing to do.
 
-  for (auto& attachment : input_attachments_) {
-    msg->attachment_set()->AddAttachment(std::move(attachment));
-  }
-  input_attachments_.clear();
+  // The shenaniganery below with &foo.front() requires input_fds_ to have
+  // contiguous underlying storage (such as a simple array or a std::vector).
+  // This is why the header warns not to make input_fds_ a deque<>.
+  msg->attachment_set()->AddDescriptorsToOwn(&input_fds_.front(), header_fds);
+  input_fds_.clear();
   return true;
 }
 
 bool ChannelNacl::DidEmptyInputBuffers() {
-  // When the input data buffer is empty, the attachments should be too.
-  return input_attachments_.empty();
+  // When the input data buffer is empty, the fds should be too.
+  return input_fds_.empty();
 }
 
 void ChannelNacl::HandleInternalMessage(const Message& msg) {