ipc: Make a new class PlaceholderBrokerableAttachment.

Instances of this class need to be replaced before the message can be
dispatched. This mechanism is a lot cleaner than the previous mechanism, which
had an "unbrokered" state for every type of BrokerableAttachment, which had to
be mutated before being dispatched.

BUG=493414

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

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

ipc/BUILD.gn

@@ -83,6 +83,8 @@ component("ipc") {
     "param_traits_macros.h",
     "param_traits_read_macros.h",
     "param_traits_write_macros.h",
+    "placeholder_brokerable_attachment.cc",
+    "placeholder_brokerable_attachment.h",
     "struct_constructor_macros.h",
     "struct_destructor_macros.h",
     "unix_domain_socket_util.cc",

ipc/attachment_broker_privileged_win.cc

@@ -22,7 +22,7 @@ bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess(
     const BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
-    case BrokerableAttachment::WIN_HANDLE:
+    case BrokerableAttachment::WIN_HANDLE: {
       const internal::HandleAttachmentWin* handle_attachment =
           static_cast<const internal::HandleAttachmentWin*>(attachment);
       HandleWireFormat wire_format =
@@ -33,6 +33,10 @@ bool AttachmentBrokerPrivilegedWin::SendAttachmentToProcess(
         return false;
       RouteDuplicatedHandle(new_wire_format);
       return true;
+    }
+    case BrokerableAttachment::PLACEHOLDER:
+      NOTREACHED();
+      return false;
   }
   return false;
 }

ipc/attachment_broker_unprivileged_win.cc

@@ -20,13 +20,17 @@ bool AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
     const BrokerableAttachment* attachment,
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
-    case BrokerableAttachment::WIN_HANDLE:
+    case BrokerableAttachment::WIN_HANDLE: {
       const internal::HandleAttachmentWin* handle_attachment =
           static_cast<const internal::HandleAttachmentWin*>(attachment);
       internal::HandleAttachmentWin::WireFormat format =
           handle_attachment->GetWireFormat(destination_process);
       return get_sender()->Send(
           new AttachmentBrokerMsg_DuplicateWinHandle(format));
+    }
+    case BrokerableAttachment::PLACEHOLDER:
+      NOTREACHED();
+      return false;
   }
   return false;
 }

ipc/brokerable_attachment.cc

@@ -29,30 +29,29 @@ void BrokerableAttachment::AttachmentId::SerializeToBuffer(char* start_address,
     start_address[i] = nonce[i];
 }
 
-BrokerableAttachment::BrokerableAttachment()
-    : needs_brokering_(false) {}
+BrokerableAttachment::BrokerableAttachment() {}
 
-BrokerableAttachment::BrokerableAttachment(const AttachmentId& id,
-                                           bool needs_brokering)
-    : id_(id), needs_brokering_(needs_brokering) {}
+BrokerableAttachment::BrokerableAttachment(const AttachmentId& id) : id_(id) {}
 
-BrokerableAttachment::~BrokerableAttachment() {
-}
+BrokerableAttachment::~BrokerableAttachment() {}
 
 BrokerableAttachment::AttachmentId BrokerableAttachment::GetIdentifier() const {
   return id_;
 }
 
 bool BrokerableAttachment::NeedsBrokering() const {
-  return needs_brokering_;
-}
-
-void BrokerableAttachment::SetNeedsBrokering(bool needs_brokering) {
-  needs_brokering_ = needs_brokering;
+  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

@@ -51,6 +51,7 @@ class IPC_EXPORT BrokerableAttachment : public MessageAttachment {
   };
 
   enum BrokerableType {
+    PLACEHOLDER,
     WIN_HANDLE,
   };
 
@@ -61,31 +62,26 @@ class IPC_EXPORT BrokerableAttachment : public MessageAttachment {
   // can be used.
   bool NeedsBrokering() const;
 
-  // Fills in the data of this instance with the data from |attachment|.
-  // This instance must require brokering, |attachment| must be brokered, and
-  // both instances must have the same identifier.
-  virtual void PopulateWithAttachment(
-      const BrokerableAttachment* attachment) = 0;
-
   // 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(const AttachmentId& id, bool needs_brokering);
+  BrokerableAttachment(const AttachmentId& id);
   ~BrokerableAttachment() override;
 
-  void SetNeedsBrokering(bool needs_brokering);
-
  private:
   // This member uniquely identifies a BrokerableAttachment across all Chrome
   // processes.
   const AttachmentId id_;
 
-  // Whether the attachment still needs to be filled in by an AttachmentBroker.
-  bool needs_brokering_;
   DISALLOW_COPY_AND_ASSIGN(BrokerableAttachment);
 };
 

ipc/handle_attachment_win.cc

@@ -14,13 +14,13 @@ HandleAttachmentWin::HandleAttachmentWin(const HANDLE& handle,
     : handle_(handle), permissions_(permissions) {}
 
 HandleAttachmentWin::HandleAttachmentWin(const WireFormat& wire_format)
-    : BrokerableAttachment(wire_format.attachment_id, false),
+    : BrokerableAttachment(wire_format.attachment_id),
       handle_(LongToHandle(wire_format.handle)),
       permissions_(wire_format.permissions) {}
 
 HandleAttachmentWin::HandleAttachmentWin(
     const BrokerableAttachment::AttachmentId& id)
-    : BrokerableAttachment(id, true),
+    : BrokerableAttachment(id),
       handle_(INVALID_HANDLE_VALUE),
       permissions_(HandleWin::INVALID) {}
 
@@ -32,19 +32,6 @@ HandleAttachmentWin::BrokerableType HandleAttachmentWin::GetBrokerableType()
   return WIN_HANDLE;
 }
 
-void HandleAttachmentWin::PopulateWithAttachment(
-    const BrokerableAttachment* attachment) {
-  DCHECK(NeedsBrokering());
-  DCHECK(!attachment->NeedsBrokering());
-  DCHECK(GetIdentifier() == attachment->GetIdentifier());
-  DCHECK_EQ(GetBrokerableType(), attachment->GetBrokerableType());
-
-  const HandleAttachmentWin* handle_attachment =
-      static_cast<const HandleAttachmentWin*>(attachment);
-  handle_ = handle_attachment->handle_;
-  SetNeedsBrokering(false);
-}
-
 HandleAttachmentWin::WireFormat HandleAttachmentWin::GetWireFormat(
     const base::ProcessId& destination) const {
   WireFormat format;

ipc/handle_attachment_win.h

@@ -40,7 +40,6 @@ class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment {
   explicit HandleAttachmentWin(const BrokerableAttachment::AttachmentId& id);
 
   BrokerableType GetBrokerableType() const override;
-  void PopulateWithAttachment(const BrokerableAttachment* attachment) override;
 
   // Returns the wire format of this attachment.
   WireFormat GetWireFormat(const base::ProcessId& destination) const;

ipc/ipc.gypi

@@ -88,6 +88,8 @@
           'param_traits_macros.h',
           'param_traits_read_macros.h',
           'param_traits_write_macros.h',
+          'placeholder_brokerable_attachment.cc',
+          'placeholder_brokerable_attachment.h',
           'struct_constructor_macros.h',
           'struct_destructor_macros.h',
           'unix_domain_socket_util.cc',

ipc/ipc_channel_reader.cc

@@ -97,8 +97,8 @@ bool ChannelReader::TranslateInputData(const char* input_data,
       int pickle_len = static_cast<int>(info.pickle_end - p);
       Message translated_message(p, pickle_len);
 
-      // TODO(erikchen): Make attachments for info.attachment_ids.
-      // http://crbug.com/493414.
+      for (const auto& id : info.attachment_ids)
+        translated_message.AddPlaceholderBrokerableAttachmentWithId(id);
 
       if (!GetNonBrokeredAttachments(&translated_message))
         return false;
@@ -201,8 +201,9 @@ ChannelReader::AttachmentIdSet ChannelReader::GetBrokeredAttachments(
 
 #if USE_ATTACHMENT_BROKER
   MessageAttachmentSet* set = msg->attachment_set();
-  for (const scoped_refptr<BrokerableAttachment>& attachment :
-       set->GetBrokerableAttachmentsForUpdating()) {
+  std::vector<const BrokerableAttachment*> brokerable_attachments_copy =
+      set->PeekBrokerableAttachments();
+  for (const BrokerableAttachment* attachment : brokerable_attachments_copy) {
     if (attachment->NeedsBrokering()) {
       AttachmentBroker* broker = GetAttachmentBroker();
       scoped_refptr<BrokerableAttachment> brokered_attachment;
@@ -213,7 +214,7 @@ ChannelReader::AttachmentIdSet ChannelReader::GetBrokeredAttachments(
         continue;
       }
 
-      attachment->PopulateWithAttachment(brokered_attachment.get());
+      set->ReplacePlaceholderWithAttachment(brokered_attachment);
     }
   }
 #endif  // USE_ATTACHMENT_BROKER

ipc/ipc_channel_reader_unittest.cc

@@ -9,6 +9,7 @@
 #include "ipc/attachment_broker.h"
 #include "ipc/brokerable_attachment.h"
 #include "ipc/ipc_channel_reader.h"
+#include "ipc/placeholder_brokerable_attachment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if USE_ATTACHMENT_BROKER
@@ -19,15 +20,9 @@ namespace {
 
 class MockAttachment : public BrokerableAttachment {
  public:
-  MockAttachment(int internal_state) : internal_state_(internal_state) {}
+  MockAttachment() {}
   MockAttachment(BrokerableAttachment::AttachmentId id)
-      : BrokerableAttachment(id, true), internal_state_(-1) {}
-
-  void PopulateWithAttachment(const BrokerableAttachment* attachment) override {
-    const MockAttachment* mock_attachment =
-        static_cast<const MockAttachment*>(attachment);
-    internal_state_ = mock_attachment->internal_state_;
-  }
+      : BrokerableAttachment(id) {}
 
 #if defined(OS_POSIX)
   base::PlatformFile TakePlatformFile() override {
@@ -39,8 +34,6 @@ class MockAttachment : public BrokerableAttachment {
 
  private:
   ~MockAttachment() override {}
-  // Internal state differentiates MockAttachments.
-  int internal_state_;
 };
 
 class MockAttachmentBroker : public AttachmentBroker {
@@ -105,12 +98,12 @@ TEST(ChannelReaderTest, AttachmentAlreadyBrokered) {
   MockAttachmentBroker broker;
   MockChannelReader reader;
   reader.set_broker(&broker);
-  scoped_refptr<MockAttachment> attachment(new MockAttachment(5));
+  scoped_refptr<MockAttachment> attachment(new MockAttachment);
   broker.AddAttachment(attachment);
 
   Message* m = new Message;
-  MockAttachment* needs_brokering_attachment =
-      new MockAttachment(attachment->GetIdentifier());
+  PlaceholderBrokerableAttachment* needs_brokering_attachment =
+      new PlaceholderBrokerableAttachment(attachment->GetIdentifier());
   EXPECT_TRUE(m->WriteAttachment(needs_brokering_attachment));
   reader.AddMessageForDispatch(m);
   EXPECT_EQ(ChannelReader::DISPATCH_FINISHED, reader.DispatchMessages());
@@ -121,11 +114,11 @@ TEST(ChannelReaderTest, AttachmentNotYetBrokered) {
   MockAttachmentBroker broker;
   MockChannelReader reader;
   reader.set_broker(&broker);
-  scoped_refptr<MockAttachment> attachment(new MockAttachment(5));
+  scoped_refptr<MockAttachment> attachment(new MockAttachment);
 
   Message* m = new Message;
-  MockAttachment* needs_brokering_attachment =
-      new MockAttachment(attachment->GetIdentifier());
+  PlaceholderBrokerableAttachment* needs_brokering_attachment =
+      new PlaceholderBrokerableAttachment(attachment->GetIdentifier());
   EXPECT_TRUE(m->WriteAttachment(needs_brokering_attachment));
   reader.AddMessageForDispatch(m);
   EXPECT_EQ(ChannelReader::DISPATCH_WAITING_ON_BROKER,

ipc/ipc_message.cc

@@ -11,6 +11,7 @@
 #include "build/build_config.h"
 #include "ipc/ipc_message_attachment.h"
 #include "ipc/ipc_message_attachment_set.h"
+#include "ipc/placeholder_brokerable_attachment.h"
 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
@@ -180,6 +181,13 @@ void Message::FindNext(const char* range_start,
   info->message_found = true;
 }
 
+bool Message::AddPlaceholderBrokerableAttachmentWithId(
+    BrokerableAttachment::AttachmentId id) {
+  scoped_refptr<PlaceholderBrokerableAttachment> attachment(
+      new PlaceholderBrokerableAttachment(id));
+  return attachment_set()->AddAttachment(attachment);
+}
+
 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.

ipc/ipc_message.h

@@ -194,6 +194,11 @@ class IPC_EXPORT Message : public base::Pickle {
                        const char* range_end,
                        NextMessageInfo* info);
 
+  // Adds a placeholder brokerable attachment that must be replaced before the
+  // message can be dispatched.
+  bool AddPlaceholderBrokerableAttachmentWithId(
+      BrokerableAttachment::AttachmentId id);
+
   // WriteAttachment appends |attachment| to the end of the set. It returns
   // false iff the set is full.
   bool WriteAttachment(scoped_refptr<MessageAttachment> attachment);

ipc/ipc_message_attachment_set.cc

@@ -143,17 +143,25 @@ MessageAttachmentSet::PeekBrokerableAttachments() const {
   return output;
 }
 
-std::vector<scoped_refptr<BrokerableAttachment>>
-MessageAttachmentSet::GetBrokerableAttachmentsForUpdating() {
-  std::vector<scoped_refptr<BrokerableAttachment>> output;
-  for (const scoped_refptr<MessageAttachment>& attachment : attachments_) {
-    if (attachment->GetType() ==
-        MessageAttachment::TYPE_BROKERABLE_ATTACHMENT) {
-      output.push_back(scoped_refptr<BrokerableAttachment>(
-          static_cast<BrokerableAttachment*>(attachment.get())));
+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;
+    BrokerableAttachment* brokerable_attachment =
+        static_cast<BrokerableAttachment*>(it->get());
+
+    if (brokerable_attachment->GetBrokerableType() ==
+            BrokerableAttachment::PLACEHOLDER &&
+        brokerable_attachment->GetIdentifier() == attachment->GetIdentifier()) {
+      *it = attachment;
+      return;
     }
   }
-  return output;
+
+  // This function should only be called if there is a placeholder ready to be
+  // replaced.
+  NOTREACHED();
 }
 
 #if defined(OS_POSIX)

ipc/ipc_message_attachment_set.h

@@ -60,8 +60,11 @@ class IPC_EXPORT MessageAttachmentSet
 
   // Returns a vector of all brokerable attachments.
   std::vector<const BrokerableAttachment*> PeekBrokerableAttachments() const;
-  std::vector<scoped_refptr<BrokerableAttachment>>
-  GetBrokerableAttachmentsForUpdating();
+
+  // Replaces a placeholder brokerable attachment with |attachment|, matching
+  // them by their id.
+  void ReplacePlaceholderWithAttachment(
+      const scoped_refptr<BrokerableAttachment>& attachment);
 
 #if defined(OS_POSIX)
   // This is the maximum number of descriptors per message. We need to know this

ipc/placeholder_brokerable_attachment.cc

@@ -0,0 +1,14 @@
+// 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/placeholder_brokerable_attachment.h"
+
+namespace IPC {
+
+BrokerableAttachment::BrokerableType
+PlaceholderBrokerableAttachment::GetBrokerableType() const {
+  return PLACEHOLDER;
+}
+
+}  // namespace IPC