Make ChannelMojoHost::ChannelDelegate a RefContedThreadSafe

ChannelDelegate::GetWeakPtr() was called from the UI thread but
it was racy as the weak ptr is also used in the IO thread.
This CL turns ChannelDelegate a ThreadSafeRefCounted so that
we can pass ChannelDelegate itself to the task runner, instead
of using its weak ptr on the UI thread.

This change also turns some TaskRunner declarations to
SequencedTaskRunner to access its DeleteSoon() API from
ChannelDelegate.

TBR=creis@chromium.org
R=viettrungluu@chrormium.org, agl@chromium.org
BUG=460243

Review URL: https://codereview.chromium.org/955813002

Cr-Commit-Position: refs/heads/master@{#318994}

build/sanitizers/tsan_suppressions.cc

@@ -325,9 +325,6 @@ char kTSanDefaultSuppressions[] =
 // https://crbug.com/459429
 "race:randomnessPid\n"
 
-// https://crbug.com/460243
-"race:IPC::ChannelMojoHost::OnClientLaunched\n"
-
 // https://crbug.com/454655
 "race:content::BrowserTestBase::PostTaskToInProcessRendererAndWait\n"
 

content/browser/renderer_host/render_process_host_impl.cc

@@ -665,7 +665,7 @@ scoped_ptr<IPC::ChannelProxy> RenderProcessHostImpl::CreateChannelProxy(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
   if (ShouldUseMojoChannel()) {
     VLOG(1) << "Mojo Channel is enabled on host";
-    scoped_refptr<base::TaskRunner> io_task_runner =
+    scoped_refptr<base::SequencedTaskRunner> io_task_runner =
         BrowserThread::UnsafeGetMessageLoopForThread(BrowserThread::IO)
               ->task_runner();
     if (!channel_mojo_host_) {

ipc/ipc_test_base.cc

@@ -153,7 +153,7 @@ IPC::ChannelHandle IPCTestBase::GetTestChannelHandle() {
   return GetChannelName(test_client_name_);
 }
 
-scoped_refptr<base::TaskRunner> IPCTestBase::task_runner() {
+scoped_refptr<base::SequencedTaskRunner> IPCTestBase::task_runner() {
   return message_loop_->message_loop_proxy();
 }
 

ipc/ipc_test_base.h

@@ -101,7 +101,7 @@ class IPCTestBase : public base::MultiProcessTest {
   IPC::ChannelProxy* channel_proxy() { return channel_proxy_.get(); }
 
   const base::Process& client_process() const { return client_process_; }
-  scoped_refptr<base::TaskRunner> task_runner();
+  scoped_refptr<base::SequencedTaskRunner> task_runner();
 
   virtual scoped_ptr<IPC::ChannelFactory> CreateChannelFactory(
       const IPC::ChannelHandle& handle, base::TaskRunner* runner);

ipc/mojo/ipc_channel_mojo_host.cc

@@ -10,14 +10,22 @@
 
 namespace IPC {
 
+class ChannelMojoHost::ChannelDelegateTraits {
+ public:
+  static void Destruct(const ChannelMojoHost::ChannelDelegate* ptr);
+};
+
 // The delete class lives on the IO thread to talk to ChannelMojo on
 // behalf of ChannelMojoHost.
 //
 // The object must be touched only on the IO thread.
-class ChannelMojoHost::ChannelDelegate : public ChannelMojo::Delegate {
+class ChannelMojoHost::ChannelDelegate
+    : public base::RefCountedThreadSafe<ChannelMojoHost::ChannelDelegate,
+                                        ChannelMojoHost::ChannelDelegateTraits>,
+      public ChannelMojo::Delegate {
  public:
-  explicit ChannelDelegate(scoped_refptr<base::TaskRunner> io_task_runner);
-  ~ChannelDelegate() override;
+  explicit ChannelDelegate(
+      scoped_refptr<base::SequencedTaskRunner> io_task_runner);
 
   // ChannelMojo::Delegate
   base::WeakPtr<Delegate> ToWeakPtr() override;
@@ -27,10 +35,14 @@ class ChannelMojoHost::ChannelDelegate : public ChannelMojo::Delegate {
   // Returns an weak ptr of ChannelDelegate instead of Delegate
   base::WeakPtr<ChannelDelegate> GetWeakPtr();
   void OnClientLaunched(base::ProcessHandle process);
-  void DeleteThisSoon();
+  void DeleteThisSoon() const;
 
  private:
-  scoped_refptr<base::TaskRunner> io_task_runner_;
+  friend class base::DeleteHelper<ChannelDelegate>;
+
+  ~ChannelDelegate() override;
+
+  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
   base::WeakPtr<ChannelMojo> channel_;
   base::WeakPtrFactory<ChannelDelegate> weak_factory_;
 
@@ -38,7 +50,7 @@ class ChannelMojoHost::ChannelDelegate : public ChannelMojo::Delegate {
 };
 
 ChannelMojoHost::ChannelDelegate::ChannelDelegate(
-    scoped_refptr<base::TaskRunner> io_task_runner)
+    scoped_refptr<base::SequencedTaskRunner> io_task_runner)
     : io_task_runner_(io_task_runner), weak_factory_(this) {
 }
 
@@ -72,18 +84,16 @@ void ChannelMojoHost::ChannelDelegate::OnClientLaunched(
     channel_->OnClientLaunched(process);
 }
 
-void ChannelMojoHost::ChannelDelegate::DeleteThisSoon() {
-  io_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&base::DeletePointer<ChannelMojoHost::ChannelDelegate>,
-                 base::Unretained(this)));
+void ChannelMojoHost::ChannelDelegate::DeleteThisSoon() const {
+  io_task_runner_->DeleteSoon(FROM_HERE, this);
 }
 
 //
 // ChannelMojoHost
 //
 
-ChannelMojoHost::ChannelMojoHost(scoped_refptr<base::TaskRunner> io_task_runner)
+ChannelMojoHost::ChannelMojoHost(
+    scoped_refptr<base::SequencedTaskRunner> io_task_runner)
     : io_task_runner_(io_task_runner),
       channel_delegate_(new ChannelDelegate(io_task_runner)),
       weak_factory_(this) {
@@ -98,8 +108,7 @@ void ChannelMojoHost::OnClientLaunched(base::ProcessHandle process) {
   } else {
     io_task_runner_->PostTask(FROM_HERE,
                               base::Bind(&ChannelDelegate::OnClientLaunched,
-                                         channel_delegate_->GetWeakPtr(),
-                                         process));
+                                         channel_delegate_, process));
   }
 }
 
@@ -107,8 +116,9 @@ ChannelMojo::Delegate* ChannelMojoHost::channel_delegate() const {
   return channel_delegate_.get();
 }
 
-void ChannelMojoHost::DelegateDeleter::operator()(
-    ChannelMojoHost::ChannelDelegate* ptr) const {
+// static
+void ChannelMojoHost::ChannelDelegateTraits::Destruct(
+    const ChannelMojoHost::ChannelDelegate* ptr) {
   ptr->DeleteThisSoon();
 }
 

ipc/mojo/ipc_channel_mojo_host.h

@@ -12,7 +12,7 @@
 #include "ipc/mojo/ipc_channel_mojo.h"
 
 namespace base {
-class TaskRunner;
+class SequencedTaskRunner;
 }
 
 namespace IPC {
@@ -23,7 +23,8 @@ namespace IPC {
 // instance and call OnClientLaunched().
 class IPC_MOJO_EXPORT ChannelMojoHost {
  public:
-  explicit ChannelMojoHost(scoped_refptr<base::TaskRunner> io_task_runner);
+  explicit ChannelMojoHost(
+      scoped_refptr<base::SequencedTaskRunner> io_task_runner);
   ~ChannelMojoHost();
 
   void OnClientLaunched(base::ProcessHandle process);
@@ -31,16 +32,10 @@ class IPC_MOJO_EXPORT ChannelMojoHost {
 
  private:
   class ChannelDelegate;
+  class ChannelDelegateTraits;
 
-  // Delegate talks to ChannelMojo, whch lives in IO thread, thus
-  // the Delegate should also live and dies in the IO thread as well.
-  class DelegateDeleter {
-   public:
-    void operator()(ChannelDelegate* ptr) const;
-  };
-
-  const scoped_refptr<base::TaskRunner> io_task_runner_;
-  scoped_ptr<ChannelDelegate, DelegateDeleter> channel_delegate_;
+  const scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
+  scoped_refptr<ChannelDelegate> channel_delegate_;
   base::WeakPtrFactory<ChannelMojoHost> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelMojoHost);