Fix race in PluginDataRemoverImpl going away while it's still being used on...

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

@@ -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,75 +55,108 @@ bool PluginDataRemover::IsSupported(webkit::WebPluginInfo* plugin) {
 
 }
 
-PluginDataRemoverImpl::PluginDataRemoverImpl(
+class PluginDataRemoverImpl::Context
+    : public PluginProcessHost::Client,
+      public IPC::Channel::Listener,
+      public base::RefCountedThreadSafe<Context> {
+ public:
+  Context(const std::string& mime_type,
+          base::Time begin_time,
           const content::ResourceContext& resource_context)
-    : mime_type_(kFlashMimeType),
-      is_starting_process_(false),
+      : event_(new base::WaitableEvent(true, false)),
+        begin_time_(begin_time),
         is_removing_(false),
-      context_(resource_context),
-      event_(new base::WaitableEvent(true, false)),
-      channel_(NULL),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
-}
-
-PluginDataRemoverImpl::~PluginDataRemoverImpl() {
-  if (is_starting_process_)
-    PluginService::GetInstance()->CancelOpenChannelToNpapiPlugin(this);
-  DCHECK(!is_removing_);
-  if (channel_)
-    BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_);
-}
-
-base::WaitableEvent* PluginDataRemoverImpl::StartRemoving(
-    base::Time begin_time) {
-  DCHECK(!is_removing_);
+        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();
-  begin_time_ = begin_time;
-
-  is_starting_process_ = true;
-  is_removing_ = true;
-  PluginService::GetInstance()->OpenChannelToNpapiPlugin(
-      0, 0, GURL(), GURL(), mime_type_, this);
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&Context::Init, this, mime_type));
 
     BrowserThread::PostDelayedTask(
         BrowserThread::IO,
         FROM_HERE,
-      base::Bind(&PluginDataRemoverImpl::OnTimeout, weak_factory_.GetWeakPtr()),
+        base::Bind(&Context::OnTimeout, this),
         kRemovalTimeoutMs);
+  }
 
-  return event_.get();
-}
+  virtual ~Context() {
+    if (channel_)
+      BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, channel_);
+  }
 
-int PluginDataRemoverImpl::ID() {
+  // PluginProcessHost::Client methods.
+  virtual int ID() OVERRIDE {
     // Generate a unique identifier for this PluginProcessHostClient.
     return ChildProcessInfo::GenerateChildProcessUniqueId();
-}
+  }
 
-bool PluginDataRemoverImpl::OffTheRecord() {
+  virtual bool OffTheRecord() OVERRIDE {
     return false;
-}
+  }
 
-const content::ResourceContext& PluginDataRemoverImpl::GetResourceContext() {
-  return context_;
-}
+  virtual const content::ResourceContext& GetResourceContext() OVERRIDE {
+    return resource_context_;
+  }
 
-void PluginDataRemoverImpl::SetPluginInfo(
-    const webkit::WebPluginInfo& info) {
-}
+  virtual void SetPluginInfo(const webkit::WebPluginInfo& info) OVERRIDE {
+  }
 
-void PluginDataRemoverImpl::OnFoundPluginProcessHost(
-    PluginProcessHost* host) {
-}
+  virtual void OnFoundPluginProcessHost(PluginProcessHost* host) OVERRIDE {
+  }
 
-void PluginDataRemoverImpl::OnSentPluginChannelRequest() {
-}
+  virtual void OnSentPluginChannelRequest() OVERRIDE {
+  }
 
-void PluginDataRemoverImpl::OnChannelOpened(const IPC::ChannelHandle& handle) {
-  is_starting_process_ = false;
+  virtual void OnChannelOpened(const IPC::ChannelHandle& handle) OVERRIDE {
     ConnectToChannel(handle);
-}
+    // Balancing the AddRef call.
+    Release();
+  }
+
+  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) {
+
+  base::WaitableEvent* event() { return event_.get(); }
+
+ 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.
@@ -144,47 +178,62 @@ void PluginDataRemoverImpl::ConnectToChannel(const IPC::ChannelHandle& handle) {
       SignalDone();
       return;
     }
-}
-
-void PluginDataRemoverImpl::OnError() {
-  LOG(DFATAL) << "Couldn't open plugin channel";
-  SignalDone();
-}
+  }
 
-void PluginDataRemoverImpl::OnClearSiteDataResult(bool success) {
+  // 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();
-}
+  }
 
-void PluginDataRemoverImpl::OnTimeout() {
+  // Called when a timeout happens in order not to block the client
+  // indefinitely.
+  void OnTimeout() {
     LOG_IF(ERROR, is_removing_) << "Timed out";
     SignalDone();
-}
-
-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()
-
-  return true;
-}
-
-void PluginDataRemoverImpl::OnChannelError() {
-  is_starting_process_ = false;
-  if (is_removing_) {
-    NOTREACHED() << "Channel error";
-    SignalDone();
   }
-}
 
-void PluginDataRemoverImpl::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();
+  }
+
+  scoped_ptr<base::WaitableEvent> 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_;
+
+  // The resource context for the profile.
+  const content::ResourceContext& resource_context_;
+
+  // 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_;
+};
+
+
+PluginDataRemoverImpl::PluginDataRemoverImpl(
+    const content::ResourceContext& resource_context)
+    : mime_type_(kFlashMimeType),
+      resource_context_(resource_context) {
+}
+
+PluginDataRemoverImpl::~PluginDataRemoverImpl() {
+}
+
+base::WaitableEvent* PluginDataRemoverImpl::StartRemoving(
+    base::Time begin_time) {
+  DCHECK(!context_.get());
+  context_ = new Context(mime_type_, begin_time, resource_context_);
+  return context_->event();
 }

content/browser/plugin_data_remover_impl.h

@@ -9,14 +9,10 @@
 #include <string>
 
 #include "base/compiler_specific.h"
-#include "base/memory/weak_ptr.h"
-#include "content/browser/plugin_process_host.h"
+#include "base/memory/ref_counted.h"
 #include "content/public/browser/plugin_data_remover.h"
 
-class CONTENT_EXPORT PluginDataRemoverImpl
-    : public content::PluginDataRemover,
-      public NON_EXPORTED_BASE(PluginProcessHost::Client),
-      public IPC::Channel::Listener {
+class CONTENT_EXPORT PluginDataRemoverImpl : public content::PluginDataRemover {
  public:
   explicit PluginDataRemoverImpl(
       const content::ResourceContext& resource_context);
@@ -30,48 +26,16 @@ class CONTENT_EXPORT PluginDataRemoverImpl
   // different plug-in (for example in tests).
   void set_mime_type(const std::string& mime_type) { mime_type_ = mime_type; }
 
-  // PluginProcessHost::Client methods.
-  virtual int ID() OVERRIDE;
-  virtual bool OffTheRecord() OVERRIDE;
-  virtual const content::ResourceContext& GetResourceContext() OVERRIDE;
-  virtual void SetPluginInfo(const webkit::WebPluginInfo& info) OVERRIDE;
-  virtual void OnFoundPluginProcessHost(PluginProcessHost* host) OVERRIDE;
-  virtual void OnSentPluginChannelRequest() OVERRIDE;
-  virtual void OnChannelOpened(const IPC::ChannelHandle& handle) OVERRIDE;
-  virtual void OnError() OVERRIDE;
-
-  // IPC::Channel::Listener methods.
-  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
-  virtual void OnChannelError() OVERRIDE;
-
  private:
-  // Signals that we are finished with removing data (successful or not). This
-  // method is safe to call multiple times.
-  void SignalDone();
-  // Connects the client side of a newly opened plug-in channel.
-  void ConnectToChannel(const IPC::ChannelHandle& handle);
-  // Handles the PluginHostMsg_ClearSiteDataResult message.
-  void OnClearSiteDataResult(bool success);
-  // Called when a timeout happens in order not to block the client
-  // indefinitely.
-  void OnTimeout();
+  class Context;
 
   std::string mime_type_;
-  bool is_starting_process_;
-  bool is_removing_;
-  // 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_;
   // The resource context for the profile.
-  const content::ResourceContext& context_;
-  scoped_ptr<base::WaitableEvent> event_;
-  // 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_;
+  const content::ResourceContext& resource_context_;
 
-  base::WeakPtrFactory<PluginDataRemoverImpl> weak_factory_;
+  // This allows this object to be deleted on the UI thread while it's still
+  // being used on the IO thread.
+  scoped_refptr<Context> context_;
 
   DISALLOW_COPY_AND_ASSIGN(PluginDataRemoverImpl);
 };

content/browser/plugin_service.cc

@@ -332,6 +332,7 @@ void PluginService::OpenChannelToNpapiPlugin(
     const GURL& page_url,
     const std::string& mime_type,
     PluginProcessHost::Client* client) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!ContainsKey(pending_plugin_clients_, client));
   pending_plugin_clients_.insert(client);
 

content/browser/plugin_service_browsertest.cc

@@ -23,6 +23,13 @@ namespace {
 
 const char kNPAPITestPluginMimeType[] = "application/vnd.npapi-test";
 
+void OpenChannel(PluginProcessHost::Client* client) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Start opening the channel
+  PluginService::GetInstance()->OpenChannelToNpapiPlugin(
+      0, 0, GURL(), GURL(), kNPAPITestPluginMimeType, client);
+}
+
 // Mock up of the Client and the Listener classes that would supply the
 // communication channel with the plugin.
 class MockPluginProcessHostClient : public PluginProcessHost::Client,
@@ -100,8 +107,9 @@ class PluginServiceTest : public InProcessBrowserTest {
 IN_PROC_BROWSER_TEST_F(PluginServiceTest, OpenChannelToPlugin) {
   ::testing::StrictMock<MockPluginProcessHostClient> mock_client(
       browser()->profile()->GetResourceContext());
-  PluginService::GetInstance()->OpenChannelToNpapiPlugin(
-      0, 0, GURL(), GURL(), kNPAPITestPluginMimeType, &mock_client);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&OpenChannel, &mock_client));
   ui_test_utils::RunMessageLoop();
 }
 
@@ -234,13 +242,6 @@ class MockCanceledBeforeSentPluginProcessHostClient
   DISALLOW_COPY_AND_ASSIGN(MockCanceledBeforeSentPluginProcessHostClient);
 };
 
-void OpenChannel(PluginProcessHost::Client* client) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  // Start opening the channel
-  PluginService::GetInstance()->OpenChannelToNpapiPlugin(
-      0, 0, GURL(), GURL(), kNPAPITestPluginMimeType, client);
-}
-
 IN_PROC_BROWSER_TEST_F(
     PluginServiceTest, CancelBeforeSentOpenChannelToPluginProcessHost) {
   ::testing::StrictMock<MockCanceledBeforeSentPluginProcessHostClient>