Revert of Clean up public interface of AttachmentBrokerUnprivileged. (patchset #4 id:80001 of https://codereview.chromium.org/1679763002/ )
Reason for revert:
c:\b\build\slave\win\build\src\remoting\host\remoting_me2me_host.cc(937) : error C2065: 'attachment_broker_' : undeclared identifier
http://build.chromium.org/p/chromium/builders/Win/builds/40168
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
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/1688433005
Cr-Commit-Position: refs/heads/master@{#374771}
diff --git a/components/nacl/broker/nacl_broker_listener.cc b/components/nacl/broker/nacl_broker_listener.cc
index 78da409..cb104058 100644
--- a/components/nacl/broker/nacl_broker_listener.cc
+++ b/components/nacl/broker/nacl_broker_listener.cc
@@ -32,7 +32,8 @@
} // namespace
NaClBrokerListener::NaClBrokerListener() {
- IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
+ attachment_broker_.reset(
+ IPC::AttachmentBrokerUnprivileged::CreateBroker().release());
}
NaClBrokerListener::~NaClBrokerListener() {
@@ -43,9 +44,8 @@
base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
switches::kProcessChannelID);
channel_ = IPC::Channel::CreateClient(channel_name, this);
- IPC::AttachmentBroker* global = IPC::AttachmentBroker::GetGlobal();
- if (global && !global->IsPrivilegedBroker())
- global->DesignateBrokerCommunicationChannel(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
index 29829bb..12b550e 100644
--- 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 @@
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
index 377a1cb8..fae2536 100644
--- a/components/nacl/loader/nacl_listener.cc
+++ b/components/nacl/loader/nacl_listener.cc
@@ -196,7 +196,8 @@
#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 @@
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->DesignateBrokerCommunicationChannel(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
index 7c1efe3..a2d617c 100644
--- 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 @@
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
index c69e99b..dbf6c3b 100644
--- a/content/child/child_thread_impl.cc
+++ b/content/child/child_thread_impl.cc
@@ -367,7 +367,14 @@
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 @@
}
ConnectChannel(options.use_mojo_channel);
- IPC::AttachmentBroker* global = IPC::AttachmentBroker::GetGlobal();
- if (global && !global->IsPrivilegedBroker())
- global->DesignateBrokerCommunicationChannel(channel_.get());
+ if (attachment_broker_)
+ attachment_broker_->DesignateBrokerCommunicationChannel(channel_.get());
int connection_timeout = kConnectionTimeoutS;
std::string connection_override =
diff --git a/content/child/child_thread_impl.h b/content/child/child_thread_impl.h
index 2429e9e..d82af5c 100644
--- a/content/child/child_thread_impl.h
+++ b/content/child/child_thread_impl.h
@@ -31,6 +31,7 @@
} // namespace base
namespace IPC {
+class AttachmentBrokerUnprivileged;
class MessageFilter;
class ScopedIPCSupport;
class SyncChannel;
@@ -239,6 +240,7 @@
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
index b463356..50e3e2d9 100644
--- a/ipc/attachment_broker.cc
+++ b/ipc/attachment_broker.cc
@@ -82,15 +82,6 @@
NOTREACHED();
}
-void AttachmentBroker::DesignateBrokerCommunicationChannel(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
index e498bb2..4cf1ab5 100644
--- a/ipc/attachment_broker.h
+++ b/ipc/attachment_broker.h
@@ -97,13 +97,6 @@
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 DesignateBrokerCommunicationChannel(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_privileged.cc b/ipc/attachment_broker_privileged.cc
index 90067986..85c1ac9 100644
--- a/ipc/attachment_broker_privileged.cc
+++ b/ipc/attachment_broker_privileged.cc
@@ -67,7 +67,9 @@
// 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 @@
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
index 8b855996..686bb9d2 100644
--- a/ipc/attachment_broker_privileged.h
+++ b/ipc/attachment_broker_privileged.h
@@ -50,7 +50,6 @@
// 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_unprivileged.cc b/ipc/attachment_broker_unprivileged.cc
index a735bf8a..9286a89 100644
--- 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,15 +19,18 @@
namespace IPC {
-namespace {
+AttachmentBrokerUnprivileged::AttachmentBrokerUnprivileged()
+ : sender_(nullptr) {
+ IPC::AttachmentBroker::SetGlobal(this);
+}
-// 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() {
+AttachmentBrokerUnprivileged::~AttachmentBrokerUnprivileged() {
+ IPC::AttachmentBroker::SetGlobal(nullptr);
+}
+
+// static
+scoped_ptr<AttachmentBrokerUnprivileged>
+AttachmentBrokerUnprivileged::CreateBroker() {
#if defined(OS_WIN)
return scoped_ptr<AttachmentBrokerUnprivileged>(
new IPC::AttachmentBrokerUnprivilegedWin);
@@ -40,41 +42,6 @@
#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);
-}
-
-AttachmentBrokerUnprivileged::~AttachmentBrokerUnprivileged() {
- IPC::AttachmentBroker::SetGlobal(nullptr);
-}
-
-// static
-void AttachmentBrokerUnprivileged::CreateBrokerIfNeeded() {
- g_attachment_broker_make_once.Get();
-}
-
void AttachmentBrokerUnprivileged::DesignateBrokerCommunicationChannel(
Endpoint* endpoint) {
DCHECK(endpoint);
@@ -83,10 +50,6 @@
endpoint->SetAttachmentBrokerEndpoint(true);
}
-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
index 068bf8e2..b572ff8 100644
--- a/ipc/attachment_broker_unprivileged.h
+++ b/ipc/attachment_broker_unprivileged.h
@@ -22,14 +22,17 @@
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 DesignateBrokerCommunicationChannel(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
index bd0aa5a..749fd65 100644
--- a/remoting/host/desktop_process.cc
+++ b/remoting/host/desktop_process.cc
@@ -147,10 +147,14 @@
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->DesignateBrokerCommunicationChannel(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(
diff --git a/remoting/host/desktop_process.h b/remoting/host/desktop_process.h
index 4246724..e085c6a 100644
--- a/remoting/host/desktop_process.h
+++ b/remoting/host/desktop_process.h
@@ -19,6 +19,7 @@
#include "remoting/host/desktop_session_agent.h"
namespace IPC {
+class AttachmentBrokerUnprivileged;
class ChannelProxy;
} // namespace IPC
@@ -76,6 +77,9 @@
// process.
std::string daemon_channel_name_;
+ // Attachment broker for |daemon_channel_|.
+ scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
+
// IPC channel connecting the desktop process with the daemon process.
scoped_ptr<IPC::ChannelProxy> daemon_channel_;
diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index b01561c..7dbebc5f 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -460,6 +460,9 @@
// Accessed on the UI thread.
scoped_ptr<IPC::ChannelProxy> daemon_channel_;
+ // AttachmentBroker for |daemon_channel_|.
+ scoped_ptr<IPC::AttachmentBrokerUnprivileged> attachment_broker_;
+
// Owned as |desktop_environment_factory_|.
DesktopSessionConnector* desktop_session_connector_ = nullptr;
#endif // defined(REMOTING_MULTI_PROCESS)
@@ -541,10 +544,11 @@
this,
context_->network_task_runner());
- IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
- IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
- if (broker && !broker->IsPrivilegedBroker())
- broker->DesignateBrokerCommunicationChannel(daemon_channel_.get());
+ attachment_broker_ = IPC::AttachmentBrokerUnprivileged::CreateBroker();
+ if (attachment_broker_) {
+ attachment_broker_->DesignateBrokerCommunicationChannel(
+ daemon_channel_.get());
+ }
#else // !defined(REMOTING_MULTI_PROCESS)
if (cmd_line->HasSwitch(kHostConfigSwitchName)) {
