Revert of Clean up public interface of AttachmentBrokerUnprivileged. …

…(patchset chromium#6 id:120001 of https://codereview.chromium.org/1679763002/ ) Reason for revert: failures on Chromium Memory FYI: http://build.chromium.org/p/chromium.memory.fyi/ Failure notification for "memory test: remoting" on "Chromium Mac (valgrind)(2)". Please see if the failures are related to your commit and take appropriate actions (e.g. revert, update suppressions, notify sheriff, etc.). For more info on the memory waterfall please see these links: http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff http://dev.chromium.org/developers/how-tos/using-valgrind http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer By the way, the current memory sheriff is on the CC list. http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28valgrind%29%282%29/builds/37788 Original issue's description: > Clean up public interface of AttachmentBrokerUnprivileged. > > In the old interface, a static factory method returns a scoped_ptr, and the > caller had to manage the lifetime. Since this is a global object with minimal > memory footprint, and is required to outlive every IPC::Channel, it's much > easier for the global to never be destroyed. This also matches the interface for > AttachmentBrokerPrivileged. > > BUG=584297 > > Committed: https://crrev.com/11fea2242b3a197993dbd5a1f977f9a31c6b98e4 > Cr-Commit-Position: refs/heads/master@{#375674} TBR=tsepez@chromium.org,avi@chromium.org,mseaborn@chromium.org,sergeyu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=584297 Review URL: https://codereview.chromium.org/1704743002 Cr-Commit-Position: refs/heads/master@{#375739}
marcosholgado · Feb 17, 2016 · 346a383 · 346a383
1 parent 8fa61a6
commit 346a383
Show file tree

Hide file tree

Showing 17 changed files with 74 additions and 128 deletions.
diff --git a/components/nacl/broker/nacl_broker_listener.cc b/components/nacl/broker/nacl_broker_listener.cc
@@ -32,23 +32,20 @@ void SendReply(IPC::Channel* channel, int32_t pid, bool result) {
 }  // namespace
 
 NaClBrokerListener::NaClBrokerListener() {
-  IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
+  attachment_broker_.reset(
+      IPC::AttachmentBrokerUnprivileged::CreateBroker().release());
 }
 
 NaClBrokerListener::~NaClBrokerListener() {
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker() && channel_)
-    broker->DeregisterBrokerCommunicationChannel(channel_.get());
 }
 
 void NaClBrokerListener::Listen() {
   std::string channel_name =
       base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kProcessChannelID);
   channel_ = IPC::Channel::CreateClient(channel_name, this);
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker())
-    broker->RegisterBrokerCommunicationChannel(channel_.get());
+  if (attachment_broker_.get())
+    attachment_broker_->DesignateBrokerCommunicationChannel(channel_.get());
   CHECK(channel_->Connect());
   base::MessageLoop::current()->Run();
 }

diff --git a/components/nacl/broker/nacl_broker_listener.h b/components/nacl/broker/nacl_broker_listener.h
@@ -15,6 +15,7 @@
 #include "ipc/ipc_listener.h"
 
 namespace IPC {
+class AttachmentBrokerUnprivileged;
 class Channel;
 }
 
@@ -44,6 +45,7 @@ class NaClBrokerListener : public content::SandboxedProcessLauncherDelegate,
   void OnStopBroker();
 
   base::Process browser_process_;
+  scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
   scoped_ptr<IPC::Channel> channel_;
 
   DISALLOW_COPY_AND_ASSIGN(NaClBrokerListener);

diff --git a/components/nacl/loader/nacl_listener.cc b/components/nacl/loader/nacl_listener.cc
@@ -196,7 +196,8 @@ NaClListener::NaClListener()
 #endif
       main_loop_(NULL),
       is_started_(false) {
-  IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
+  attachment_broker_.reset(
+      IPC::AttachmentBrokerUnprivileged::CreateBroker().release());
   io_thread_.StartWithOptions(
       base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
   DCHECK(g_listener == NULL);
@@ -257,9 +258,8 @@ void NaClListener::Listen() {
   filter_ = channel_->CreateSyncMessageFilter();
   channel_->AddFilter(new FileTokenMessageFilter());
   channel_->Init(channel_name, IPC::Channel::MODE_CLIENT, true);
-  IPC::AttachmentBroker* global = IPC::AttachmentBroker::GetGlobal();
-  if (global && !global->IsPrivilegedBroker())
-    global->RegisterBrokerCommunicationChannel(channel_.get());
+  if (attachment_broker_.get())
+    attachment_broker_->DesignateBrokerCommunicationChannel(channel_.get());
   main_loop_ = base::MessageLoop::current();
   main_loop_->Run();
 }

diff --git a/components/nacl/loader/nacl_listener.h b/components/nacl/loader/nacl_listener.h
@@ -22,6 +22,7 @@
 #include "ipc/ipc_listener.h"
 
 namespace IPC {
+class AttachmentBrokerUnprivileged;
 class SyncChannel;
 class SyncMessageFilter;
 }
@@ -79,6 +80,8 @@ class NaClListener : public IPC::Listener {
       const nacl::NaClResourcePrefetchResult& prefetched_resource_file);
   void OnStart(const nacl::NaClStartParams& params);
 
+  scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
+
   // A channel back to the browser.
   scoped_ptr<IPC::SyncChannel> channel_;
 

diff --git a/content/child/child_thread_impl.cc b/content/child/child_thread_impl.cc
@@ -367,7 +367,14 @@ void ChildThreadImpl::Init(const Options& options) {
   IPC::Logging::GetInstance();
 #endif
 
-  IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
+#if USE_ATTACHMENT_BROKER
+  // The only reason a global would already exist is if the thread is being run
+  // in the browser process because of a command line switch.
+  if (!IPC::AttachmentBroker::GetGlobal()) {
+    attachment_broker_.reset(
+        IPC::AttachmentBrokerUnprivileged::CreateBroker().release());
+  }
+#endif
 
   channel_ =
       IPC::SyncChannel::Create(this, ChildProcess::current()->io_task_runner(),
@@ -453,9 +460,8 @@ void ChildThreadImpl::Init(const Options& options) {
   }
 
   ConnectChannel(options.use_mojo_channel);
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker())
-    broker->RegisterBrokerCommunicationChannel(channel_.get());
+  if (attachment_broker_)
+    attachment_broker_->DesignateBrokerCommunicationChannel(channel_.get());
 
   int connection_timeout = kConnectionTimeoutS;
   std::string connection_override =
@@ -491,10 +497,6 @@ ChildThreadImpl::~ChildThreadImpl() {
   IPC::Logging::GetInstance()->SetIPCSender(NULL);
 #endif
 
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker())
-    broker->DeregisterBrokerCommunicationChannel(channel_.get());
-
   channel_->RemoveFilter(histogram_message_filter_.get());
   channel_->RemoveFilter(sync_message_filter_.get());
 

diff --git a/content/child/child_thread_impl.h b/content/child/child_thread_impl.h
@@ -31,6 +31,7 @@ class MessageLoop;
 }  // namespace base
 
 namespace IPC {
+class AttachmentBrokerUnprivileged;
 class MessageFilter;
 class ScopedIPCSupport;
 class SyncChannel;
@@ -239,6 +240,7 @@ class CONTENT_EXPORT ChildThreadImpl
   scoped_ptr<MojoApplication> mojo_application_;
 
   std::string channel_name_;
+  scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
   scoped_ptr<IPC::SyncChannel> channel_;
 
   // Allows threads other than the main thread to send sync messages.

diff --git a/ipc/attachment_broker.cc b/ipc/attachment_broker.cc
@@ -82,20 +82,6 @@ void AttachmentBroker::DeregisterCommunicationChannel(Endpoint* endpoint) {
   NOTREACHED();
 }
 
-void AttachmentBroker::RegisterBrokerCommunicationChannel(Endpoint* endpoint) {
-  NOTREACHED();
-}
-
-void AttachmentBroker::DeregisterBrokerCommunicationChannel(
-    Endpoint* endpoint) {
-  NOTREACHED();
-}
-
-bool AttachmentBroker::IsPrivilegedBroker() {
-  NOTREACHED();
-  return false;
-}
-
 void AttachmentBroker::HandleReceivedAttachment(
     const scoped_refptr<BrokerableAttachment>& attachment) {
   {

diff --git a/ipc/attachment_broker.h b/ipc/attachment_broker.h
@@ -97,14 +97,6 @@ class IPC_EXPORT AttachmentBroker : public Listener {
   virtual void RegisterCommunicationChannel(Endpoint* endpoint);
   virtual void DeregisterCommunicationChannel(Endpoint* endpoint);
 
-  // In each unprivileged process, exactly one channel should be used to
-  // communicate brokerable attachments with the broker process.
-  virtual void RegisterBrokerCommunicationChannel(Endpoint* endpoint);
-  virtual void DeregisterBrokerCommunicationChannel(Endpoint* endpoint);
-
-  // True if and only if this broker is privileged.
-  virtual bool IsPrivilegedBroker();
-
  protected:
   using AttachmentVector = std::vector<scoped_refptr<BrokerableAttachment>>;
 

diff --git a/ipc/attachment_broker_mac_unittest.cc b/ipc/attachment_broker_mac_unittest.cc
@@ -427,7 +427,7 @@ class IPCAttachmentBrokerMacTest : public IPCTestBase {
 
     broker_->AddObserver(&observer_, task_runner());
     CreateChannel(&proxy_listener_);
-    broker_->RegisterBrokerCommunicationChannel(channel());
+    broker_->DesignateBrokerCommunicationChannel(channel());
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
 
@@ -972,7 +972,7 @@ TEST_F(IPCAttachmentBrokerMacTest, SendSharedMemoryHandleChannelProxy) {
   thread->StartWithOptions(options);
 
   CreateChannelProxy(get_proxy_listener(), thread->task_runner().get());
-  get_broker()->RegisterBrokerCommunicationChannel(channel_proxy());
+  get_broker()->DesignateBrokerCommunicationChannel(channel_proxy());
 
   ASSERT_TRUE(StartClient());
 

diff --git a/ipc/attachment_broker_privileged.cc b/ipc/attachment_broker_privileged.cc
@@ -67,7 +67,9 @@ scoped_ptr<AttachmentBrokerPrivileged> CreateBroker() {
 // the global broker.
 class AttachmentBrokerMakeOnce {
  public:
-  AttachmentBrokerMakeOnce() : attachment_broker_(CreateBroker()) {}
+  AttachmentBrokerMakeOnce() {
+    attachment_broker_.reset(CreateBroker().release());
+  }
 
  private:
   scoped_ptr<IPC::AttachmentBrokerPrivileged> attachment_broker_;
@@ -126,10 +128,6 @@ void AttachmentBrokerPrivileged::DeregisterCommunicationChannel(
     endpoints_.erase(it);
 }
 
-bool AttachmentBrokerPrivileged::IsPrivilegedBroker() {
-  return true;
-}
-
 Sender* AttachmentBrokerPrivileged::GetSenderWithProcessId(base::ProcessId id) {
   get_lock()->AssertAcquired();
   auto it = std::find_if(endpoints_.begin(), endpoints_.end(),

diff --git a/ipc/attachment_broker_privileged.h b/ipc/attachment_broker_privileged.h
@@ -50,7 +50,6 @@ class IPC_EXPORT AttachmentBrokerPrivileged : public IPC::AttachmentBroker {
   // AttachmentBroker overrides.
   void RegisterCommunicationChannel(Endpoint* endpoint) override;
   void DeregisterCommunicationChannel(Endpoint* endpoint) override;
-  bool IsPrivilegedBroker() override;
 
  protected:
   // Returns the sender whose peer's process id is |id|.

diff --git a/ipc/attachment_broker_privileged_win_unittest.cc b/ipc/attachment_broker_privileged_win_unittest.cc
@@ -267,7 +267,7 @@ class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase {
       set_broker(new IPC::AttachmentBrokerUnprivilegedWin);
     broker_->AddObserver(&observer_, task_runner());
     CreateChannel(&proxy_listener_);
-    broker_->RegisterBrokerCommunicationChannel(channel());
+    broker_->DesignateBrokerCommunicationChannel(channel());
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
 

diff --git a/ipc/attachment_broker_unprivileged.cc b/ipc/attachment_broker_unprivileged.cc
@@ -4,7 +4,6 @@
 
 #include "ipc/attachment_broker_unprivileged.h"
 
-#include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
 #include "ipc/ipc_channel.h"
@@ -20,47 +19,6 @@
 
 namespace IPC {
 
-namespace {
-
-// On platforms that support attachment brokering, returns a new instance of
-// a platform-specific attachment broker. Otherwise returns |nullptr|.
-// The caller takes ownership of the newly created instance, and is
-// responsible for ensuring that the attachment broker lives longer than
-// every IPC::Channel. The new instance automatically registers itself as the
-// global attachment broker.
-scoped_ptr<AttachmentBrokerUnprivileged> CreateBroker() {
-#if defined(OS_WIN)
-  return scoped_ptr<AttachmentBrokerUnprivileged>(
-      new IPC::AttachmentBrokerUnprivilegedWin);
-#elif defined(OS_MACOSX) && !defined(OS_IOS)
-  return scoped_ptr<AttachmentBrokerUnprivileged>(
-      new IPC::AttachmentBrokerUnprivilegedMac);
-#else
-  return nullptr;
-#endif
-}
-
-// This class is wrapped in a LazyInstance to ensure that its constructor is
-// only called once. The constructor creates an attachment broker and sets it as
-// the global broker.
-class AttachmentBrokerMakeOnce {
- public:
-  AttachmentBrokerMakeOnce() {
-    // Single process tests can cause an attachment broker to already exist.
-    if (AttachmentBroker::GetGlobal())
-      return;
-    attachment_broker_ = CreateBroker();
-  }
-
- private:
-  scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
-};
-
-base::LazyInstance<AttachmentBrokerMakeOnce>::Leaky
-    g_attachment_broker_make_once = LAZY_INSTANCE_INITIALIZER;
-
-}  // namespace
-
 AttachmentBrokerUnprivileged::AttachmentBrokerUnprivileged()
     : sender_(nullptr) {
   IPC::AttachmentBroker::SetGlobal(this);
@@ -71,29 +29,27 @@ AttachmentBrokerUnprivileged::~AttachmentBrokerUnprivileged() {
 }
 
 // static
-void AttachmentBrokerUnprivileged::CreateBrokerIfNeeded() {
-  g_attachment_broker_make_once.Get();
+scoped_ptr<AttachmentBrokerUnprivileged>
+AttachmentBrokerUnprivileged::CreateBroker() {
+#if defined(OS_WIN)
+  return scoped_ptr<AttachmentBrokerUnprivileged>(
+      new IPC::AttachmentBrokerUnprivilegedWin);
+#elif defined(OS_MACOSX) && !defined(OS_IOS)
+  return scoped_ptr<AttachmentBrokerUnprivileged>(
+      new IPC::AttachmentBrokerUnprivilegedMac);
+#else
+  return nullptr;
+#endif
 }
 
-void AttachmentBrokerUnprivileged::RegisterBrokerCommunicationChannel(
+void AttachmentBrokerUnprivileged::DesignateBrokerCommunicationChannel(
     Endpoint* endpoint) {
   DCHECK(endpoint);
   DCHECK(!sender_);
   sender_ = endpoint;
   endpoint->SetAttachmentBrokerEndpoint(true);
 }
 
-void AttachmentBrokerUnprivileged::DeregisterBrokerCommunicationChannel(
-    Endpoint* endpoint) {
-  DCHECK(endpoint);
-  DCHECK_EQ(endpoint, sender_);
-  sender_ = nullptr;
-}
-
-bool AttachmentBrokerUnprivileged::IsPrivilegedBroker() {
-  return false;
-}
-
 void AttachmentBrokerUnprivileged::LogError(UMAError error) {
   UMA_HISTOGRAM_ENUMERATION(
       "IPC.AttachmentBrokerUnprivileged.BrokerAttachmentError", error,

diff --git a/ipc/attachment_broker_unprivileged.h b/ipc/attachment_broker_unprivileged.h
@@ -22,15 +22,17 @@ class IPC_EXPORT AttachmentBrokerUnprivileged : public IPC::AttachmentBroker {
   AttachmentBrokerUnprivileged();
   ~AttachmentBrokerUnprivileged() override;
 
-  // If there is no global attachment broker, makes a new
-  // AttachmentBrokerUnprivileged and sets it as the global attachment broker.
-  // This method is thread safe.
-  static void CreateBrokerIfNeeded();
+   // On platforms that support attachment brokering, returns a new instance of
+   // a platform-specific attachment broker. Otherwise returns |nullptr|.
+   // The caller takes ownership of the newly created instance, and is
+   // responsible for ensuring that the attachment broker lives longer than
+   // every IPC::Channel. The new instance automatically registers itself as the
+   // global attachment broker.
+  static scoped_ptr<AttachmentBrokerUnprivileged> CreateBroker();
 
-  // AttachmentBroker:
-  void RegisterBrokerCommunicationChannel(Endpoint* endpoint) override;
-  void DeregisterBrokerCommunicationChannel(Endpoint* endpoint) override;
-  bool IsPrivilegedBroker() override;
+  // In each unprivileged process, exactly one channel should be used to
+  // communicate brokerable attachments with the broker process.
+  void DesignateBrokerCommunicationChannel(Endpoint* endpoint);
 
  protected:
   IPC::Sender* get_sender() { return sender_; }

diff --git a/remoting/host/desktop_process.cc b/remoting/host/desktop_process.cc
@@ -82,9 +82,6 @@ void DesktopProcess::OnChannelConnected(int32_t peer_pid) {
 
 void DesktopProcess::OnChannelError() {
   // Shutdown the desktop process.
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker())
-    broker->DeregisterBrokerCommunicationChannel(daemon_channel_.get());
   daemon_channel_.reset();
   if (desktop_agent_.get()) {
     desktop_agent_->Stop();
@@ -144,10 +141,14 @@ bool DesktopProcess::Start(
       IPC::ChannelProxy::Create(daemon_channel_name_, IPC::Channel::MODE_CLIENT,
                                 this, io_task_runner.get());
 
-  IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
-  IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
-  if (broker && !broker->IsPrivilegedBroker())
-    broker->RegisterBrokerCommunicationChannel(daemon_channel_.get());
+  // Attachment broker may be already created in tests.
+  if (!IPC::AttachmentBroker::GetGlobal())
+    attachment_broker_ = IPC::AttachmentBrokerUnprivileged::CreateBroker();
+
+  if (attachment_broker_) {
+    attachment_broker_->DesignateBrokerCommunicationChannel(
+        daemon_channel_.get());
+  }
 
   // Pass |desktop_pipe| to the daemon.
   daemon_channel_->Send(