ChannelMojo: Handle when ChannelMojo outlives ChannelMojoHost

In some case ChannelMojo outlives ChannelMojoHost because
two objects are living in diffent thread.
Instead of using lifecycle callbacks, this CL relies on
WeakPtr. See comment on ipc_channel_mojo_host.h for
more details.

This CL also fixes a crash on --single-process mode.

R=viettrungluu@chromium.org
TBR=jam@chromium.org
TEST=content_browsertests (with --enable-renderer-mojo-channel on)
BUG=377980

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

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

6 files changed, 147 insertions, 57 deletions

diff --git a/ipc/mojo/ipc_channel_mojo.cc b/ipc/mojo/ipc_channel_mojo.cc
index fadc0d2..00f5452 100644
--- a/ipc/mojo/ipc_channel_mojo.cc
+++ b/ipc/mojo/ipc_channel_mojo.cc
@@ -8,7 +8,6 @@
 #include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
 #include "ipc/ipc_listener.h"
-#include "ipc/mojo/ipc_channel_mojo_host.h"
 #include "ipc/mojo/ipc_channel_mojo_readers.h"
 #include "ipc/mojo/ipc_mojo_bootstrap.h"
 #include "mojo/embedder/embedder.h"
@@ -23,22 +22,22 @@ namespace {
 
 class MojoChannelFactory : public ChannelFactory {
  public:
-  MojoChannelFactory(ChannelMojoHost* host,
+  MojoChannelFactory(ChannelMojo::Delegate* delegate,
                      ChannelHandle channel_handle,
                      Channel::Mode mode)
-      : host_(host), channel_handle_(channel_handle), mode_(mode) {}
+      : delegate_(delegate), channel_handle_(channel_handle), mode_(mode) {}
 
   virtual std::string GetName() const OVERRIDE {
     return channel_handle_.name;
   }
 
   virtual scoped_ptr<Channel> BuildChannel(Listener* listener) OVERRIDE {
-    return ChannelMojo::Create(host_, channel_handle_, mode_, listener)
+    return ChannelMojo::Create(delegate_, channel_handle_, mode_, listener)
         .PassAs<Channel>();
   }
 
  private:
-  ChannelMojoHost* host_;
+  ChannelMojo::Delegate* delegate_;
   ChannelHandle channel_handle_;
   Channel::Mode mode_;
 };
@@ -55,19 +54,20 @@ void ChannelMojo::ChannelInfoDeleter::operator()(
 //------------------------------------------------------------------------------
 
 // static
-scoped_ptr<ChannelMojo> ChannelMojo::Create(ChannelMojoHost* host,
+scoped_ptr<ChannelMojo> ChannelMojo::Create(ChannelMojo::Delegate* delegate,
                                             const ChannelHandle& channel_handle,
                                             Mode mode,
                                             Listener* listener) {
-  return make_scoped_ptr(new ChannelMojo(host, channel_handle, mode, listener));
+  return make_scoped_ptr(
+      new ChannelMojo(delegate, channel_handle, mode, listener));
 }
 
 // static
 scoped_ptr<ChannelFactory> ChannelMojo::CreateServerFactory(
-    ChannelMojoHost* host,
+    ChannelMojo::Delegate* delegate,
     const ChannelHandle& channel_handle) {
-  return make_scoped_ptr(
-             new MojoChannelFactory(host, channel_handle, Channel::MODE_SERVER))
+  return make_scoped_ptr(new MojoChannelFactory(
+                             delegate, channel_handle, Channel::MODE_SERVER))
       .PassAs<ChannelFactory>();
 }
 
@@ -79,27 +79,37 @@ scoped_ptr<ChannelFactory> ChannelMojo::CreateClientFactory(
       .PassAs<ChannelFactory>();
 }
 
-ChannelMojo::ChannelMojo(ChannelMojoHost* host,
+ChannelMojo::ChannelMojo(ChannelMojo::Delegate* delegate,
                          const ChannelHandle& handle,
                          Mode mode,
                          Listener* listener)
-    : host_(host),
-      mode_(mode),
+    : mode_(mode),
       listener_(listener),
       peer_pid_(base::kNullProcessId),
       weak_factory_(this) {
   // Create MojoBootstrap after all members are set as it touches
   // ChannelMojo from a different thread.
   bootstrap_ = MojoBootstrap::Create(handle, mode, this);
-  if (host_)
-    host_->OnChannelCreated(this);
+  if (delegate) {
+    if (delegate->GetIOTaskRunner() ==
+        base::MessageLoop::current()->message_loop_proxy()) {
+      InitDelegate(delegate);
+    } else {
+      delegate->GetIOTaskRunner()->PostTask(
+          FROM_HERE,
+          base::Bind(
+              &ChannelMojo::InitDelegate, base::Unretained(this), delegate));
+    }
+  }
 }
 
 ChannelMojo::~ChannelMojo() {
   Close();
+}
 
-  if (host_)
-    host_->OnChannelDestroyed();
+void ChannelMojo::InitDelegate(ChannelMojo::Delegate* delegate) {
+  delegate_ = delegate->ToWeakPtr();
+  delegate_->OnChannelCreated(weak_factory_.GetWeakPtr());
 }
 
 void ChannelMojo::InitControlReader(
diff --git a/ipc/mojo/ipc_channel_mojo.h b/ipc/mojo/ipc_channel_mojo.h
index c356906..5867c08 100644
--- a/ipc/mojo/ipc_channel_mojo.h
+++ b/ipc/mojo/ipc_channel_mojo.h
@@ -32,8 +32,6 @@ class ClientControlReader;
 class MessageReader;
 }
 
-class ChannelMojoHost;
-
 // Mojo-based IPC::Channel implementation over a platform handle.
 //
 // ChannelMojo builds Mojo MessagePipe using underlying pipe given by
@@ -61,9 +59,17 @@ class ChannelMojoHost;
 class IPC_MOJO_EXPORT ChannelMojo : public Channel,
                                     public MojoBootstrap::Delegate {
  public:
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+    virtual base::WeakPtr<Delegate> ToWeakPtr() = 0;
+    virtual scoped_refptr<base::TaskRunner> GetIOTaskRunner() = 0;
+    virtual void OnChannelCreated(base::WeakPtr<ChannelMojo> channel) = 0;
+  };
+
   // Create ChannelMojo. A bootstrap channel is created as well.
-  // |host| must not be null.
-  static scoped_ptr<ChannelMojo> Create(ChannelMojoHost* host,
+  // |host| must not be null for server channels.
+  static scoped_ptr<ChannelMojo> Create(Delegate* delegate,
                                         const ChannelHandle& channel_handle,
                                         Mode mode,
                                         Listener* listener);
@@ -72,7 +78,7 @@ class IPC_MOJO_EXPORT ChannelMojo : public Channel,
   // The factory is used to create Mojo-based ChannelProxy family.
   // |host| must not be null.
   static scoped_ptr<ChannelFactory> CreateServerFactory(
-      ChannelMojoHost* host,
+      Delegate* delegate,
       const ChannelHandle& channel_handle);
 
   static scoped_ptr<ChannelFactory> CreateClientFactory(
@@ -117,7 +123,7 @@ class IPC_MOJO_EXPORT ChannelMojo : public Channel,
   void set_peer_pid(base::ProcessId pid) { peer_pid_ = pid; }
 
  protected:
-  ChannelMojo(ChannelMojoHost* host,
+  ChannelMojo(Delegate* delegate,
               const ChannelHandle& channel_handle,
               Mode mode,
               Listener* listener);
@@ -132,10 +138,11 @@ class IPC_MOJO_EXPORT ChannelMojo : public Channel,
   // notifications invoked by them.
   typedef internal::MessagePipeReader::DelayedDeleter ReaderDeleter;
 
+  void InitDelegate(ChannelMojo::Delegate* delegate);
   void InitControlReader(mojo::embedder::ScopedPlatformHandle handle);
 
   scoped_ptr<MojoBootstrap> bootstrap_;
-  ChannelMojoHost* const host_;
+  base::WeakPtr<Delegate> delegate_;
   Mode mode_;
   Listener* listener_;
   base::ProcessId peer_pid_;
diff --git a/ipc/mojo/ipc_channel_mojo_host.cc b/ipc/mojo/ipc_channel_mojo_host.cc
index 4318a15..87290ce 100644
--- a/ipc/mojo/ipc_channel_mojo_host.cc
+++ b/ipc/mojo/ipc_channel_mojo_host.cc
@@ -10,38 +10,106 @@
 
 namespace IPC {
 
-ChannelMojoHost::ChannelMojoHost(scoped_refptr<base::TaskRunner> task_runner)
-    : weak_factory_(this), task_runner_(task_runner), channel_(NULL) {
+// 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 {
+ public:
+  explicit ChannelDelegate(scoped_refptr<base::TaskRunner> io_task_runner);
+  virtual ~ChannelDelegate();
+
+  // ChannelMojo::Delegate
+  virtual base::WeakPtr<Delegate> ToWeakPtr() OVERRIDE;
+  virtual void OnChannelCreated(base::WeakPtr<ChannelMojo> channel) OVERRIDE;
+  virtual scoped_refptr<base::TaskRunner> GetIOTaskRunner() OVERRIDE;
+
+  // Returns an weak ptr of ChannelDelegate instead of Delegate
+  base::WeakPtr<ChannelDelegate> GetWeakPtr();
+  void OnClientLaunched(base::ProcessHandle process);
+  void DeleteThisSoon();
+
+ private:
+  scoped_refptr<base::TaskRunner> io_task_runner_;
+  base::WeakPtrFactory<ChannelDelegate> weak_factory_;
+  base::WeakPtr<ChannelMojo> channel_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChannelDelegate);
+};
+
+ChannelMojoHost::ChannelDelegate::ChannelDelegate(
+    scoped_refptr<base::TaskRunner> io_task_runner)
+    : io_task_runner_(io_task_runner), weak_factory_(this) {
+}
+
+ChannelMojoHost::ChannelDelegate::~ChannelDelegate() {
+}
+
+base::WeakPtr<ChannelMojo::Delegate>
+ChannelMojoHost::ChannelDelegate::ToWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+base::WeakPtr<ChannelMojoHost::ChannelDelegate>
+ChannelMojoHost::ChannelDelegate::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void ChannelMojoHost::ChannelDelegate::OnChannelCreated(
+    base::WeakPtr<ChannelMojo> channel) {
+  DCHECK(!channel_);
+  channel_ = channel;
+}
+
+scoped_refptr<base::TaskRunner>
+ChannelMojoHost::ChannelDelegate::GetIOTaskRunner() {
+  return io_task_runner_;
+}
+
+void ChannelMojoHost::ChannelDelegate::OnClientLaunched(
+    base::ProcessHandle process) {
+  if (channel_)
+    channel_->OnClientLaunched(process);
+}
+
+void ChannelMojoHost::ChannelDelegate::DeleteThisSoon() {
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&base::DeletePointer<ChannelMojoHost::ChannelDelegate>,
+                 base::Unretained(this)));
+}
+
+//
+// ChannelMojoHost
+//
+
+ChannelMojoHost::ChannelMojoHost(scoped_refptr<base::TaskRunner> io_task_runner)
+    : weak_factory_(this),
+      io_task_runner_(io_task_runner),
+      channel_delegate_(new ChannelDelegate(io_task_runner)) {
 }
 
 ChannelMojoHost::~ChannelMojoHost() {
 }
 
 void ChannelMojoHost::OnClientLaunched(base::ProcessHandle process) {
-  if (task_runner_ == base::MessageLoop::current()->message_loop_proxy()) {
-    InvokeOnClientLaunched(process);
+  if (io_task_runner_ == base::MessageLoop::current()->message_loop_proxy()) {
+    channel_delegate_->OnClientLaunched(process);
   } else {
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&ChannelMojoHost::InvokeOnClientLaunched,
-                                      weak_factory_.GetWeakPtr(),
-                                      process));
+    io_task_runner_->PostTask(FROM_HERE,
+                              base::Bind(&ChannelDelegate::OnClientLaunched,
+                                         channel_delegate_->GetWeakPtr(),
+                                         process));
   }
 }
 
-void ChannelMojoHost::InvokeOnClientLaunched(base::ProcessHandle process) {
-  if (!channel_)
-    return;
-  channel_->OnClientLaunched(process);
-}
-
-void ChannelMojoHost::OnChannelCreated(ChannelMojo* channel) {
-  DCHECK(channel_ == NULL);
-  channel_ = channel;
+ChannelMojo::Delegate* ChannelMojoHost::channel_delegate() const {
+  return channel_delegate_.get();
 }
 
-void ChannelMojoHost::OnChannelDestroyed() {
-  DCHECK(channel_ != NULL);
-  channel_ = NULL;
+void ChannelMojoHost::DelegateDeleter::operator()(
+    ChannelMojoHost::ChannelDelegate* ptr) const {
+  ptr->DeleteThisSoon();
 }
 
 }  // namespace IPC
diff --git a/ipc/mojo/ipc_channel_mojo_host.h b/ipc/mojo/ipc_channel_mojo_host.h
index e0d94be..dd67897e 100644
--- a/ipc/mojo/ipc_channel_mojo_host.h
+++ b/ipc/mojo/ipc_channel_mojo_host.h
@@ -9,6 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/process/process_handle.h"
 #include "ipc/ipc_export.h"
+#include "ipc/mojo/ipc_channel_mojo.h"
 
 namespace base {
 class TaskRunner;
@@ -16,30 +17,31 @@ class TaskRunner;
 
 namespace IPC {
 
-class ChannelMojo;
-
 // Through ChannelMojoHost, ChannelMojo gets extra information that
 // its client provides, including the child process's process handle. Every
 // server process that uses ChannelMojo must have a ChannelMojoHost
 // instance and call OnClientLaunched().
 class IPC_MOJO_EXPORT ChannelMojoHost {
  public:
-  explicit ChannelMojoHost(scoped_refptr<base::TaskRunner> task_runner);
+  explicit ChannelMojoHost(scoped_refptr<base::TaskRunner> io_task_runner);
   ~ChannelMojoHost();
 
   void OnClientLaunched(base::ProcessHandle process);
+  ChannelMojo::Delegate* channel_delegate() const;
 
  private:
-  friend class ChannelMojo;
-
-  void OnChannelCreated(ChannelMojo* channel);
-  void OnChannelDestroyed();
+  class ChannelDelegate;
 
-  void InvokeOnClientLaunched(base::ProcessHandle process);
+  // 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;
+  };
 
   base::WeakPtrFactory<ChannelMojoHost> weak_factory_;
-  scoped_refptr<base::TaskRunner> task_runner_;
-  ChannelMojo* channel_;
+  const scoped_refptr<base::TaskRunner> io_task_runner_;
+  scoped_ptr<ChannelDelegate, DelegateDeleter> channel_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelMojoHost);
 };
diff --git a/ipc/mojo/ipc_channel_mojo_unittest.cc b/ipc/mojo/ipc_channel_mojo_unittest.cc
index e1adecd..ac1efd1 100644
--- a/ipc/mojo/ipc_channel_mojo_unittest.cc
+++ b/ipc/mojo/ipc_channel_mojo_unittest.cc
@@ -83,7 +83,8 @@ class IPCChannelMojoTest : public IPCTestBase {
       const IPC::ChannelHandle& handle,
       base::TaskRunner* runner) OVERRIDE {
     host_.reset(new IPC::ChannelMojoHost(task_runner()));
-    return IPC::ChannelMojo::CreateServerFactory(host_.get(), handle);
+    return IPC::ChannelMojo::CreateServerFactory(host_->channel_delegate(),
+                                                 handle);
   }
 
   virtual bool DidStartClient() OVERRIDE {
@@ -190,7 +191,8 @@ class IPCChannelMojoErrorTest : public IPCTestBase {
       const IPC::ChannelHandle& handle,
       base::TaskRunner* runner) OVERRIDE {
     host_.reset(new IPC::ChannelMojoHost(task_runner()));
-    return IPC::ChannelMojo::CreateServerFactory(host_.get(), handle);
+    return IPC::ChannelMojo::CreateServerFactory(host_->channel_delegate(),
+                                                 handle);
   }
 
   virtual bool DidStartClient() OVERRIDE {
diff --git a/ipc/mojo/ipc_mojo_perftest.cc b/ipc/mojo/ipc_mojo_perftest.cc
index 576f13e..81dcd70 100644
--- a/ipc/mojo/ipc_mojo_perftest.cc
+++ b/ipc/mojo/ipc_mojo_perftest.cc
@@ -33,7 +33,8 @@ public:
       const IPC::ChannelHandle& handle,
       base::TaskRunner* runner) OVERRIDE {
     host_.reset(new IPC::ChannelMojoHost(task_runner()));
-    return IPC::ChannelMojo::CreateServerFactory(host_.get(), handle);
+    return IPC::ChannelMojo::CreateServerFactory(host_->channel_delegate(),
+                                                 handle);
   }
 
   virtual bool DidStartClient() OVERRIDE {