From 76b70f91d71f6e36489dd5c9876a54e65af46f7c Mon Sep 17 00:00:00 2001 From: "jam@chromium.org" Date: Mon, 21 Nov 2011 19:31:14 +0000 Subject: Fix race in PluginDataRemoverImpl going away while it's still being used on the IO thread, which was introduced in 110530. This also fix the incorrect usage of PluginService::OpenChannelToNpapiPlugin on the UI thread which existed before. BUG=104553,cros:23179 Review URL: http://codereview.chromium.org/8603012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@110979 0039d316-1c4b-4281-b951-d872f2087c98 --- content/browser/plugin_data_remover_impl.cc | 263 +++++++++++++++++----------- 1 file changed, 156 insertions(+), 107 deletions(-) (limited to 'content/browser/plugin_data_remover_impl.cc') diff --git a/content/browser/plugin_data_remover_impl.cc b/content/browser/plugin_data_remover_impl.cc index e6cec77..21a6f9e 100644 --- a/content/browser/plugin_data_remover_impl.cc +++ b/content/browser/plugin_data_remover_impl.cc @@ -8,6 +8,7 @@ #include "base/metrics/histogram.h" #include "base/synchronization/waitable_event.h" #include "base/version.h" +#include "content/browser/plugin_process_host.h" #include "content/browser/plugin_service.h" #include "content/common/plugin_messages.h" #include "content/public/browser/browser_thread.h" @@ -54,137 +55,185 @@ bool PluginDataRemover::IsSupported(webkit::WebPluginInfo* plugin) { } -PluginDataRemoverImpl::PluginDataRemoverImpl( - const content::ResourceContext& resource_context) - : mime_type_(kFlashMimeType), - is_starting_process_(false), - is_removing_(false), - context_(resource_context), - event_(new base::WaitableEvent(true, false)), - channel_(NULL), - ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) { -} +class PluginDataRemoverImpl::Context + : public PluginProcessHost::Client, + public IPC::Channel::Listener, + public base::RefCountedThreadSafe { + public: + Context(const std::string& mime_type, + base::Time begin_time, + const content::ResourceContext& resource_context) + : event_(new base::WaitableEvent(true, false)), + begin_time_(begin_time), + is_removing_(false), + resource_context_(resource_context), + channel_(NULL) { + // Balanced in OnChannelOpened or OnError. Exactly one them will eventually + // be called, so we need to keep this object around until then. + AddRef(); + remove_start_time_ = base::Time::Now(); + BrowserThread::PostTask( + BrowserThread::IO, + FROM_HERE, + base::Bind(&Context::Init, this, mime_type)); + + BrowserThread::PostDelayedTask( + BrowserThread::IO, + FROM_HERE, + base::Bind(&Context::OnTimeout, this), + kRemovalTimeoutMs); + } -PluginDataRemoverImpl::~PluginDataRemoverImpl() { - if (is_starting_process_) - PluginService::GetInstance()->CancelOpenChannelToNpapiPlugin(this); - DCHECK(!is_removing_); - if (channel_) - BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_); -} + virtual ~Context() { + if (channel_) + BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_); + } -base::WaitableEvent* PluginDataRemoverImpl::StartRemoving( - base::Time begin_time) { - DCHECK(!is_removing_); - remove_start_time_ = base::Time::Now(); - begin_time_ = begin_time; - - is_starting_process_ = true; - is_removing_ = true; - PluginService::GetInstance()->OpenChannelToNpapiPlugin( - 0, 0, GURL(), GURL(), mime_type_, this); - - BrowserThread::PostDelayedTask( - BrowserThread::IO, - FROM_HERE, - base::Bind(&PluginDataRemoverImpl::OnTimeout, weak_factory_.GetWeakPtr()), - kRemovalTimeoutMs); - - return event_.get(); -} + // PluginProcessHost::Client methods. + virtual int ID() OVERRIDE { + // Generate a unique identifier for this PluginProcessHostClient. + return ChildProcessInfo::GenerateChildProcessUniqueId(); + } -int PluginDataRemoverImpl::ID() { - // Generate a unique identifier for this PluginProcessHostClient. - return ChildProcessInfo::GenerateChildProcessUniqueId(); -} + virtual bool OffTheRecord() OVERRIDE { + return false; + } -bool PluginDataRemoverImpl::OffTheRecord() { - return false; -} + virtual const content::ResourceContext& GetResourceContext() OVERRIDE { + return resource_context_; + } -const content::ResourceContext& PluginDataRemoverImpl::GetResourceContext() { - return context_; -} + virtual void SetPluginInfo(const webkit::WebPluginInfo& info) OVERRIDE { + } -void PluginDataRemoverImpl::SetPluginInfo( - const webkit::WebPluginInfo& info) { -} + virtual void OnFoundPluginProcessHost(PluginProcessHost* host) OVERRIDE { + } -void PluginDataRemoverImpl::OnFoundPluginProcessHost( - PluginProcessHost* host) { -} + virtual void OnSentPluginChannelRequest() OVERRIDE { + } -void PluginDataRemoverImpl::OnSentPluginChannelRequest() { -} + virtual void OnChannelOpened(const IPC::ChannelHandle& handle) OVERRIDE { + ConnectToChannel(handle); + // Balancing the AddRef call. + Release(); + } -void PluginDataRemoverImpl::OnChannelOpened(const IPC::ChannelHandle& handle) { - is_starting_process_ = false; - ConnectToChannel(handle); -} + virtual void OnError() OVERRIDE { + LOG(DFATAL) << "Couldn't open plugin channel"; + SignalDone(); + // Balancing the AddRef call. + Release(); + } + + // IPC::Channel::Listener methods. + virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE { + IPC_BEGIN_MESSAGE_MAP(Context, message) + IPC_MESSAGE_HANDLER(PluginHostMsg_ClearSiteDataResult, + OnClearSiteDataResult) + IPC_MESSAGE_UNHANDLED_ERROR() + IPC_END_MESSAGE_MAP() + + return true; + } + + virtual void OnChannelError() OVERRIDE { + if (is_removing_) { + NOTREACHED() << "Channel error"; + SignalDone(); + } + } -void PluginDataRemoverImpl::ConnectToChannel(const IPC::ChannelHandle& handle) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - // If we timed out, don't bother connecting. - if (!is_removing_) - return; + base::WaitableEvent* event() { return event_.get(); } - DCHECK(!channel_); - channel_ = new IPC::Channel(handle, IPC::Channel::MODE_CLIENT, this); - if (!channel_->Connect()) { - NOTREACHED() << "Couldn't connect to plugin"; + private: + // Initialize on the IO thread. + void Init(const std::string& mime_type) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + is_removing_ = true; + PluginService::GetInstance()->OpenChannelToNpapiPlugin( + 0, 0, GURL(), GURL(), mime_type, this); + } + + // Connects the client side of a newly opened plug-in channel. + void ConnectToChannel(const IPC::ChannelHandle& handle) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + + // If we timed out, don't bother connecting. + if (!is_removing_) + return; + + DCHECK(!channel_); + channel_ = new IPC::Channel(handle, IPC::Channel::MODE_CLIENT, this); + if (!channel_->Connect()) { + NOTREACHED() << "Couldn't connect to plugin"; + SignalDone(); + return; + } + + if (!channel_->Send(new PluginMsg_ClearSiteData(std::string(), + kClearAllData, + begin_time_))) { + NOTREACHED() << "Couldn't send ClearSiteData message"; + SignalDone(); + return; + } + } + + // Handles the PluginHostMsg_ClearSiteDataResult message. + void OnClearSiteDataResult(bool success) { + LOG_IF(ERROR, !success) << "ClearSiteData returned error"; + UMA_HISTOGRAM_TIMES("ClearPluginData.time", + base::Time::Now() - remove_start_time_); SignalDone(); - return; } - if (!channel_->Send(new PluginMsg_ClearSiteData(std::string(), - kClearAllData, - begin_time_))) { - NOTREACHED() << "Couldn't send ClearSiteData message"; + // Called when a timeout happens in order not to block the client + // indefinitely. + void OnTimeout() { + LOG_IF(ERROR, is_removing_) << "Timed out"; SignalDone(); - return; } -} -void PluginDataRemoverImpl::OnError() { - LOG(DFATAL) << "Couldn't open plugin channel"; - SignalDone(); -} + // Signals that we are finished with removing data (successful or not). This + // method is safe to call multiple times. + void SignalDone() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); + if (!is_removing_) + return; + is_removing_ = false; + event_->Signal(); + } -void PluginDataRemoverImpl::OnClearSiteDataResult(bool success) { - LOG_IF(ERROR, !success) << "ClearSiteData returned error"; - UMA_HISTOGRAM_TIMES("ClearPluginData.time", - base::Time::Now() - remove_start_time_); - SignalDone(); -} + scoped_ptr event_; + // The point in time when we start removing data. + base::Time remove_start_time_; + // The point in time from which on we remove data. + base::Time begin_time_; + bool is_removing_; -void PluginDataRemoverImpl::OnTimeout() { - LOG_IF(ERROR, is_removing_) << "Timed out"; - SignalDone(); -} + // The resource context for the profile. + const content::ResourceContext& resource_context_; -bool PluginDataRemoverImpl::OnMessageReceived(const IPC::Message& msg) { - IPC_BEGIN_MESSAGE_MAP(PluginDataRemoverImpl, msg) - IPC_MESSAGE_HANDLER(PluginHostMsg_ClearSiteDataResult, - OnClearSiteDataResult) - IPC_MESSAGE_UNHANDLED_ERROR() - IPC_END_MESSAGE_MAP() + // We own the channel, but it's used on the IO thread, so it needs to be + // deleted there. It's NULL until we have opened a connection to the plug-in + // process. + IPC::Channel* channel_; +}; - return true; + +PluginDataRemoverImpl::PluginDataRemoverImpl( + const content::ResourceContext& resource_context) + : mime_type_(kFlashMimeType), + resource_context_(resource_context) { } -void PluginDataRemoverImpl::OnChannelError() { - is_starting_process_ = false; - if (is_removing_) { - NOTREACHED() << "Channel error"; - SignalDone(); - } +PluginDataRemoverImpl::~PluginDataRemoverImpl() { } -void PluginDataRemoverImpl::SignalDone() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - if (!is_removing_) - return; - is_removing_ = false; - event_->Signal(); +base::WaitableEvent* PluginDataRemoverImpl::StartRemoving( + base::Time begin_time) { + DCHECK(!context_.get()); + context_ = new Context(mime_type_, begin_time, resource_context_); + return context_->event(); } -- cgit v1.1