Revert of ipc: Clean up interface of attachment broker. (patchset chr…

…omium#5 id:80001 of https://codereview.chromium.org/1269553003/) Reason for revert: Causes failures in: IPCAttachmentBrokerPrivilegedWinTest.SendHandle IPCAttachmentBrokerPrivilegedWinTest.SendHandleToSelf IPCAttachmentBrokerPrivilegedWinTest.SendHandleWithoutPermissions on the Win7 Tests (dbg)(1) bot http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%281%29/builds/40372/steps/ipc_tests/logs/stdio Original issue's description: > ipc: Clean up interface of attachment broker. > > AttachmentBrokerUnprivileged now has a method > DesignateBrokerCommunicationChannel which is used by non-broker processes to > designate an IPC::Channel as the communication medium for brokerable attachment > messages. IPC::Channel has a new member attachment_broker_endpoint_ which causes > it to pass all messages through the attachment broker before forwarding them to > the listener. > > BUG=493414 > > Committed: https://crrev.com/9a06836982214f2edced21bbd1615b49e7f231bf > Cr-Commit-Position: refs/heads/master@{#341143} TBR=tsepez@chromium.org,erikchen@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=493414 Review URL: https://codereview.chromium.org/1259953005 Cr-Commit-Position: refs/heads/master@{#341171}
cbchan · Jul 30, 2015 · 0905996 · 0905996
1 parent 2764561
commit 0905996
Show file tree

Hide file tree

Showing 14 changed files with 4 additions and 62 deletions.
diff --git a/ipc/attachment_broker_privileged.cc b/ipc/attachment_broker_privileged.cc
@@ -16,7 +16,6 @@ AttachmentBrokerPrivileged::~AttachmentBrokerPrivileged() {}
 
 void AttachmentBrokerPrivileged::RegisterCommunicationChannel(
     Channel* channel) {
-  channel->set_attachment_broker_endpoint(true);
   auto it = std::find(channels_.begin(), channels_.end(), channel);
   DCHECK(channels_.end() == it);
   channels_.push_back(channel);

diff --git a/ipc/attachment_broker_privileged_win_unittest.cc b/ipc/attachment_broker_privileged_win_unittest.cc
@@ -161,7 +161,7 @@ class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase {
     broker_->AddObserver(&observer_);
     set_attachment_broker(broker_.get());
     CreateChannel(&proxy_listener_);
-    broker_->DesignateBrokerCommunicationChannel(channel());
+    broker_->set_sender(channel());
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
   }
@@ -278,9 +278,6 @@ TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleToSelf) {
 
   set_broker(new MockBroker);
   CommonSetUp();
-  // Technically, the channel is an endpoint, but we need the proxy listener to
-  // receive the messages so that it can quit the message loop.
-  channel()->set_attachment_broker_endpoint(false);
   get_proxy_listener()->set_listener(get_broker());
 
   HANDLE h = CreateTempFile();

diff --git a/ipc/attachment_broker_unprivileged.cc b/ipc/attachment_broker_unprivileged.cc
@@ -4,20 +4,10 @@
 
 #include "ipc/attachment_broker_unprivileged.h"
 
-#include "ipc/ipc_channel.h"
-
 namespace IPC {
 
 AttachmentBrokerUnprivileged::AttachmentBrokerUnprivileged() {}
 
 AttachmentBrokerUnprivileged::~AttachmentBrokerUnprivileged() {}
 
-void AttachmentBrokerUnprivileged::DesignateBrokerCommunicationChannel(
-    IPC::Channel* channel) {
-  DCHECK(channel);
-  DCHECK(!sender_);
-  sender_ = channel;
-  channel->set_attachment_broker_endpoint(true);
-}
-
 }  // namespace IPC
diff --git a/ipc/attachment_broker_unprivileged.h b/ipc/attachment_broker_unprivileged.h
@@ -10,7 +10,6 @@
 
 namespace IPC {
 
-class Channel;
 class Sender;
 
 // This abstract subclass of AttachmentBroker is intended for use in
@@ -20,9 +19,7 @@ class IPC_EXPORT AttachmentBrokerUnprivileged : public IPC::AttachmentBroker {
   AttachmentBrokerUnprivileged();
   ~AttachmentBrokerUnprivileged() override;
 
-  // In each unprivileged process, exactly one channel should be used to
-  // communicate brokerable attachments with the broker process.
-  void DesignateBrokerCommunicationChannel(IPC::Channel* channel);
+  void set_sender(IPC::Sender* sender) { sender_ = sender; }
 
  protected:
   IPC::Sender* get_sender() { return sender_; }

diff --git a/ipc/ipc_channel.h b/ipc/ipc_channel.h
@@ -165,7 +165,6 @@ class IPC_EXPORT Channel : public Sender {
       Listener* listener,
       AttachmentBroker* broker = nullptr);
 
-  Channel() : attachment_broker_endpoint_(false) {}
   ~Channel() override;
 
   // Connect the pipe.  On the server side, this will initiate
@@ -253,17 +252,6 @@ class IPC_EXPORT Channel : public Sender {
   static void NotifyProcessForkedForTesting();
 #endif
 
-  void set_attachment_broker_endpoint(bool is_endpoint) {
-    attachment_broker_endpoint_ = is_endpoint;
-  }
-
- protected:
-  bool is_attachment_broker_endpoint() { return attachment_broker_endpoint_; }
-
- private:
-  // Whether this channel is used as an endpoint for sending and receiving
-  // brokerable attachment messages to/from the broker process.
-  bool attachment_broker_endpoint_;
 };
 
 #if defined(OS_POSIX)

diff --git a/ipc/ipc_channel_nacl.cc b/ipc/ipc_channel_nacl.cc
@@ -385,10 +385,6 @@ base::ProcessId ChannelNacl::GetSenderPID() {
   return base::kNullProcessId;
 }
 
-bool ChannelNacl::IsAttachmentBrokerEndpoint() {
-  return is_attachment_broker_endpoint();
-}
-
 // Channel's methods
 
 // static

diff --git a/ipc/ipc_channel_nacl.h b/ipc/ipc_channel_nacl.h
@@ -70,7 +70,6 @@ class ChannelNacl : public Channel,
   bool DidEmptyInputBuffers() override;
   void HandleInternalMessage(const Message& msg) override;
   base::ProcessId GetSenderPID() override;
-  bool IsAttachmentBrokerEndpoint() override;
 
   Mode mode_;
   bool waiting_connect_;

diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
@@ -955,10 +955,6 @@ base::ProcessId ChannelPosix::GetSenderPID() {
   return GetPeerPID();
 }
 
-bool ChannelPosix::IsAttachmentBrokerEndpoint() {
-  return is_attachment_broker_endpoint();
-}
-
 void ChannelPosix::Close() {
   // Close can be called multiple time, so we need to make sure we're
   // idempotent.

diff --git a/ipc/ipc_channel_posix.h b/ipc/ipc_channel_posix.h
@@ -84,7 +84,6 @@ class IPC_EXPORT ChannelPosix : public Channel,
   bool DidEmptyInputBuffers() override;
   void HandleInternalMessage(const Message& msg) override;
   base::ProcessId GetSenderPID() override;
-  bool IsAttachmentBrokerEndpoint() override;
 
   // Finds the set of file descriptors in the given message.  On success,
   // appends the descriptors to the input_fds_ member and returns true

diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc
@@ -163,18 +163,9 @@ void ChannelReader::DispatchMessage(Message* m) {
                IPC_MESSAGE_ID_LINE(m->type()));
 #endif
   m->TraceMessageEnd();
-
-  bool handled = false;
-  if (IsInternalMessage(*m)) {
+  if (IsInternalMessage(*m))
     HandleInternalMessage(*m);
-    handled = true;
-  }
-#if USE_ATTACHMENT_BROKER
-  if (!handled && IsAttachmentBrokerEndpoint() && GetAttachmentBroker()) {
-    handled = GetAttachmentBroker()->OnMessageReceived(*m);
-  }
-#endif  // USE_ATTACHMENT_BROKER
-  if (!handled)
+  else
     listener_->OnMessageReceived(*m);
   if (m->dispatch_error())
     listener_->OnBadMessageReceived(*m);

diff --git a/ipc/ipc_channel_reader.h b/ipc/ipc_channel_reader.h
@@ -117,9 +117,6 @@ class IPC_EXPORT ChannelReader : public SupportsAttachmentBrokering,
   // Get the process ID for the sender of the message.
   virtual base::ProcessId GetSenderPID() = 0;
 
-  // Whether the channel is an endpoint of attachment brokering.
-  virtual bool IsAttachmentBrokerEndpoint() = 0;
-
  private:
   FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, AttachmentAlreadyBrokered);
   FRIEND_TEST_ALL_PREFIXES(ChannelReaderTest, AttachmentNotYetBrokered);

diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc
@@ -81,8 +81,6 @@ class MockChannelReader : public ChannelReader {
 
   base::ProcessId GetSenderPID() override { return base::kNullProcessId; }
 
-  bool IsAttachmentBrokerEndpoint() override { return false; }
-
   AttachmentBroker* GetAttachmentBroker() override { return broker_; }
 
   // This instance takes ownership of |m|.

diff --git a/ipc/ipc_channel_win.cc b/ipc/ipc_channel_win.cc
@@ -247,10 +247,6 @@ base::ProcessId ChannelWin::GetSenderPID() {
   return GetPeerPID();
 }
 
-bool ChannelWin::IsAttachmentBrokerEndpoint() {
-  return is_attachment_broker_endpoint();
-}
-
 bool ChannelWin::DidEmptyInputBuffers() {
   // We don't need to do anything here.
   return true;

diff --git a/ipc/ipc_channel_win.h b/ipc/ipc_channel_win.h
@@ -53,7 +53,6 @@ class ChannelWin : public Channel,
   bool DidEmptyInputBuffers() override;
   void HandleInternalMessage(const Message& msg) override;
   base::ProcessId GetSenderPID() override;
-  bool IsAttachmentBrokerEndpoint() override;
 
   static const base::string16 PipeName(const std::string& channel_id,
                                        int32* secret);