ChannelMojo: Ensure that it always has ScopedIPCSupport

ChannelMojo has ScopedIPCSupport, but it is instantiated only in in-process mode. This CL lets it always instantiate to make it clear that ChannelInfo is protected by the ScopedIPCSupport. It simplifies the relationship between the support object and the channel, and makes the lifecycle invariant reasonable. With this change, we no longer need to protect ChannelMojo with ScopedIPCSupport on its client side. Now it's built-in. Note that this is a speculative fix of fuzzer generated crash, where Mojo channel related globals are gone before when channel mojo is being destroyed. BUG=473438 R=viettrungluu@chromium.org, rockot@chromium.org Committed: https://crrev.com/808706e71c213c916815e967e3156cae64d12c00 Cr-Commit-Position: refs/heads/master@{#324308} Review URL: https://codereview.chromium.org/1054253005 Cr-Commit-Position: refs/heads/master@{#324488}
cbchan · Apr 9, 2015 · 00194e7 · 00194e7
1 parent a5d60fa
commit 00194e7
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 128 deletions.
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
@@ -703,11 +703,10 @@ scoped_ptr<IPC::ChannelProxy> RenderProcessHostImpl::CreateChannelProxy(
       channel_mojo_host_.reset(new IPC::ChannelMojoHost(mojo_task_runner));
     }
 
-    return IPC::ChannelProxy::Create(
-        IPC::ChannelMojo::CreateServerFactory(
-            channel_mojo_host_->channel_delegate(), channel_id),
-        this,
-        runner.get());
+    return IPC::ChannelProxy::Create(IPC::ChannelMojo::CreateServerFactory(
+                                         channel_mojo_host_->channel_delegate(),
+                                         mojo_task_runner, channel_id),
+                                     this, runner.get());
   }
 
   return IPC::ChannelProxy::Create(

diff --git a/content/child/child_thread_impl.cc b/content/child/child_thread_impl.cc
@@ -56,7 +56,6 @@
 #include "ipc/ipc_sync_channel.h"
 #include "ipc/ipc_sync_message_filter.h"
 #include "ipc/mojo/ipc_channel_mojo.h"
-#include "ipc/mojo/scoped_ipc_support.h"
 
 #if defined(OS_WIN)
 #include "content/common/handle_enumerator_win.h"
@@ -205,45 +204,6 @@ ChildThread* ChildThread::Get() {
   return ChildThreadImpl::current();
 }
 
-// Mojo client channel delegate to be used in single process mode.
-class ChildThreadImpl::SingleProcessChannelDelegate
-    : public IPC::ChannelMojo::Delegate {
- public:
-  explicit SingleProcessChannelDelegate(
-      scoped_refptr<base::SequencedTaskRunner> io_runner)
-      : io_runner_(io_runner), weak_factory_(this) {}
-
-  ~SingleProcessChannelDelegate() override {}
-
-  base::WeakPtr<IPC::ChannelMojo::Delegate> ToWeakPtr() override {
-    return weak_factory_.GetWeakPtr();
-  }
-
-  scoped_refptr<base::TaskRunner> GetIOTaskRunner() override {
-    return io_runner_;
-  }
-
-  void OnChannelCreated(base::WeakPtr<IPC::ChannelMojo> channel) override {}
-
-  void DeleteSoon() {
-    io_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&base::DeletePointer<SingleProcessChannelDelegate>,
-                   base::Unretained(this)));
-  }
-
- private:
-  scoped_refptr<base::SequencedTaskRunner> io_runner_;
-  base::WeakPtrFactory<IPC::ChannelMojo::Delegate> weak_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(SingleProcessChannelDelegate);
-};
-
-void ChildThreadImpl::SingleProcessChannelDelegateDeleter::operator()(
-    SingleProcessChannelDelegate* delegate) const {
-  delegate->DeleteSoon();
-}
-
 ChildThreadImpl::Options::Options()
     : channel_name(base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
           switches::kProcessChannelID)),
@@ -321,15 +281,9 @@ void ChildThreadImpl::ConnectChannel(bool use_mojo_channel) {
     VLOG(1) << "Mojo is enabled on child";
     scoped_refptr<base::SequencedTaskRunner> io_task_runner = GetIOTaskRunner();
     DCHECK(io_task_runner);
-    if (IsInBrowserProcess())
-      single_process_channel_delegate_.reset(
-          new SingleProcessChannelDelegate(io_task_runner));
-    ipc_support_.reset(new IPC::ScopedIPCSupport(io_task_runner));
-    channel_->Init(
-        IPC::ChannelMojo::CreateClientFactory(
-            single_process_channel_delegate_.get(),
-            channel_name_),
-        create_pipe_now);
+    channel_->Init(IPC::ChannelMojo::CreateClientFactory(
+                       nullptr, io_task_runner, channel_name_),
+                   create_pipe_now);
     return;
   }
 

diff --git a/content/child/child_thread_impl.h b/content/child/child_thread_impl.h
@@ -235,15 +235,6 @@ class CONTENT_EXPORT ChildThreadImpl
 
   void EnsureConnected();
 
-  class SingleProcessChannelDelegate;
-  class SingleProcessChannelDelegateDeleter {
-   public:
-    void operator()(SingleProcessChannelDelegate* delegate) const;
-  };
-
-  scoped_ptr<IPC::ScopedIPCSupport> ipc_support_;
-  scoped_ptr<SingleProcessChannelDelegate, SingleProcessChannelDelegateDeleter>
-      single_process_channel_delegate_;
   scoped_ptr<MojoApplication> mojo_application_;
 
   std::string channel_name_;

diff --git a/content/test/render_thread_impl_browser_test_ipc_helper.cc b/content/test/render_thread_impl_browser_test_ipc_helper.cc
@@ -54,6 +54,7 @@ void RenderThreadImplBrowserIPCTestHelper::SetupMojo() {
 
   channel_ = IPC::ChannelProxy::Create(
       IPC::ChannelMojo::CreateServerFactory(mojo_host_->channel_delegate(),
+                                            ipc_thread_->task_runner(),
                                             channel_id_),
       dummy_listener_.get(), ipc_thread_->task_runner());
 

diff --git a/ipc/mojo/ipc_channel_mojo.cc b/ipc/mojo/ipc_channel_mojo.cc
@@ -28,20 +28,26 @@ namespace {
 class MojoChannelFactory : public ChannelFactory {
  public:
   MojoChannelFactory(ChannelMojo::Delegate* delegate,
+                     scoped_refptr<base::TaskRunner> io_runner,
                      ChannelHandle channel_handle,
                      Channel::Mode mode)
-      : delegate_(delegate), channel_handle_(channel_handle), mode_(mode) {}
+      : delegate_(delegate),
+        io_runner_(io_runner),
+        channel_handle_(channel_handle),
+        mode_(mode) {}
 
   std::string GetName() const override {
     return channel_handle_.name;
   }
 
   scoped_ptr<Channel> BuildChannel(Listener* listener) override {
-    return ChannelMojo::Create(delegate_, channel_handle_, mode_, listener);
+    return ChannelMojo::Create(delegate_, io_runner_, channel_handle_, mode_,
+                               listener);
   }
 
  private:
   ChannelMojo::Delegate* delegate_;
+  scoped_refptr<base::TaskRunner> io_runner_;
   ChannelHandle channel_handle_;
   Channel::Mode mode_;
 };
@@ -53,6 +59,7 @@ class ClientChannelMojo : public ChannelMojo,
                           public mojo::ErrorHandler {
  public:
   ClientChannelMojo(ChannelMojo::Delegate* delegate,
+                    scoped_refptr<base::TaskRunner> io_runner,
                     const ChannelHandle& handle,
                     Listener* listener);
   ~ClientChannelMojo() override;
@@ -73,9 +80,10 @@ class ClientChannelMojo : public ChannelMojo,
 };
 
 ClientChannelMojo::ClientChannelMojo(ChannelMojo::Delegate* delegate,
+                                     scoped_refptr<base::TaskRunner> io_runner,
                                      const ChannelHandle& handle,
                                      Listener* listener)
-    : ChannelMojo(delegate, handle, Channel::MODE_CLIENT, listener),
+    : ChannelMojo(delegate, io_runner, handle, Channel::MODE_CLIENT, listener),
       binding_(this) {
 }
 
@@ -104,6 +112,7 @@ void ClientChannelMojo::Init(
 class ServerChannelMojo : public ChannelMojo, public mojo::ErrorHandler {
  public:
   ServerChannelMojo(ChannelMojo::Delegate* delegate,
+                    scoped_refptr<base::TaskRunner> io_runner,
                     const ChannelHandle& handle,
                     Listener* listener);
   ~ServerChannelMojo() override;
@@ -126,9 +135,10 @@ class ServerChannelMojo : public ChannelMojo, public mojo::ErrorHandler {
 };
 
 ServerChannelMojo::ServerChannelMojo(ChannelMojo::Delegate* delegate,
+                                     scoped_refptr<base::TaskRunner> io_runner,
                                      const ChannelHandle& handle,
                                      Listener* listener)
-    : ChannelMojo(delegate, handle, Channel::MODE_SERVER, listener) {
+    : ChannelMojo(delegate, io_runner, handle, Channel::MODE_SERVER, listener) {
 }
 
 ServerChannelMojo::~ServerChannelMojo() {
@@ -196,17 +206,19 @@ bool ChannelMojo::ShouldBeUsed() {
 }
 
 // static
-scoped_ptr<ChannelMojo> ChannelMojo::Create(ChannelMojo::Delegate* delegate,
-                                            const ChannelHandle& channel_handle,
-                                            Mode mode,
-                                            Listener* listener) {
+scoped_ptr<ChannelMojo> ChannelMojo::Create(
+    ChannelMojo::Delegate* delegate,
+    scoped_refptr<base::TaskRunner> io_runner,
+    const ChannelHandle& channel_handle,
+    Mode mode,
+    Listener* listener) {
   switch (mode) {
     case Channel::MODE_CLIENT:
       return make_scoped_ptr(
-          new ClientChannelMojo(delegate, channel_handle, listener));
+          new ClientChannelMojo(delegate, io_runner, channel_handle, listener));
     case Channel::MODE_SERVER:
       return make_scoped_ptr(
-          new ServerChannelMojo(delegate, channel_handle, listener));
+          new ServerChannelMojo(delegate, io_runner, channel_handle, listener));
     default:
       NOTREACHED();
       return nullptr;
@@ -216,20 +228,23 @@ scoped_ptr<ChannelMojo> ChannelMojo::Create(ChannelMojo::Delegate* delegate,
 // static
 scoped_ptr<ChannelFactory> ChannelMojo::CreateServerFactory(
     ChannelMojo::Delegate* delegate,
+    scoped_refptr<base::TaskRunner> io_runner,
     const ChannelHandle& channel_handle) {
-  return make_scoped_ptr(
-      new MojoChannelFactory(delegate, channel_handle, Channel::MODE_SERVER));
+  return make_scoped_ptr(new MojoChannelFactory(
+      delegate, io_runner, channel_handle, Channel::MODE_SERVER));
 }
 
 // static
 scoped_ptr<ChannelFactory> ChannelMojo::CreateClientFactory(
     ChannelMojo::Delegate* delegate,
+    scoped_refptr<base::TaskRunner> io_runner,
     const ChannelHandle& channel_handle) {
-  return make_scoped_ptr(
-      new MojoChannelFactory(delegate, channel_handle, Channel::MODE_CLIENT));
+  return make_scoped_ptr(new MojoChannelFactory(
+      delegate, io_runner, channel_handle, Channel::MODE_CLIENT));
 }
 
 ChannelMojo::ChannelMojo(ChannelMojo::Delegate* delegate,
+                         scoped_refptr<base::TaskRunner> io_runner,
                          const ChannelHandle& handle,
                          Mode mode,
                          Listener* listener)
@@ -240,26 +255,24 @@ ChannelMojo::ChannelMojo(ChannelMojo::Delegate* delegate,
   // Create MojoBootstrap after all members are set as it touches
   // ChannelMojo from a different thread.
   bootstrap_ = MojoBootstrap::Create(handle, mode, 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));
-    }
+  if (io_runner == base::MessageLoop::current()->message_loop_proxy()) {
+    InitOnIOThread(delegate);
+  } else {
+    io_runner->PostTask(FROM_HERE,
+                        base::Bind(&ChannelMojo::InitOnIOThread,
+                                   base::Unretained(this), delegate));
   }
 }
 
 ChannelMojo::~ChannelMojo() {
   Close();
 }
 
-void ChannelMojo::InitDelegate(ChannelMojo::Delegate* delegate) {
+void ChannelMojo::InitOnIOThread(ChannelMojo::Delegate* delegate) {
   ipc_support_.reset(
       new ScopedIPCSupport(base::MessageLoop::current()->task_runner()));
+  if (!delegate)
+    return;
   delegate_ = delegate->ToWeakPtr();
   delegate_->OnChannelCreated(weak_factory_.GetWeakPtr());
 }

diff --git a/ipc/mojo/ipc_channel_mojo.h b/ipc/mojo/ipc_channel_mojo.h
@@ -54,7 +54,6 @@ class IPC_MOJO_EXPORT ChannelMojo
    public:
     virtual ~Delegate() {}
     virtual base::WeakPtr<Delegate> ToWeakPtr() = 0;
-    virtual scoped_refptr<base::TaskRunner> GetIOTaskRunner() = 0;
     virtual void OnChannelCreated(base::WeakPtr<ChannelMojo> channel) = 0;
   };
 
@@ -63,20 +62,24 @@ class IPC_MOJO_EXPORT ChannelMojo
 
   // Create ChannelMojo. A bootstrap channel is created as well.
   // |host| must not be null for server channels.
-  static scoped_ptr<ChannelMojo> Create(Delegate* delegate,
-                                        const ChannelHandle& channel_handle,
-                                        Mode mode,
-                                        Listener* listener);
+  static scoped_ptr<ChannelMojo> Create(
+      Delegate* delegate,
+      scoped_refptr<base::TaskRunner> io_runner,
+      const ChannelHandle& channel_handle,
+      Mode mode,
+      Listener* listener);
 
   // Create a factory object for ChannelMojo.
   // The factory is used to create Mojo-based ChannelProxy family.
   // |host| must not be null.
   static scoped_ptr<ChannelFactory> CreateServerFactory(
       Delegate* delegate,
+      scoped_refptr<base::TaskRunner> io_runner,
       const ChannelHandle& channel_handle);
 
   static scoped_ptr<ChannelFactory> CreateClientFactory(
       Delegate* delegate,
+      scoped_refptr<base::TaskRunner> io_runner,
       const ChannelHandle& channel_handle);
 
   ~ChannelMojo() override;
@@ -115,6 +118,7 @@ class IPC_MOJO_EXPORT ChannelMojo
 
  protected:
   ChannelMojo(Delegate* delegate,
+              scoped_refptr<base::TaskRunner> io_runner,
               const ChannelHandle& channel_handle,
               Mode mode,
               Listener* listener);
@@ -136,7 +140,7 @@ class IPC_MOJO_EXPORT ChannelMojo
   // notifications invoked by them.
   typedef internal::MessagePipeReader::DelayedDeleter ReaderDeleter;
 
-  void InitDelegate(ChannelMojo::Delegate* delegate);
+  void InitOnIOThread(ChannelMojo::Delegate* delegate);
 
   scoped_ptr<MojoBootstrap> bootstrap_;
   base::WeakPtr<Delegate> delegate_;

diff --git a/ipc/mojo/ipc_channel_mojo_host.cc b/ipc/mojo/ipc_channel_mojo_host.cc
@@ -30,7 +30,6 @@ class ChannelMojoHost::ChannelDelegate
   // ChannelMojo::Delegate
   base::WeakPtr<Delegate> ToWeakPtr() override;
   void OnChannelCreated(base::WeakPtr<ChannelMojo> channel) override;
-  scoped_refptr<base::TaskRunner> GetIOTaskRunner() override;
 
   // Returns an weak ptr of ChannelDelegate instead of Delegate
   base::WeakPtr<ChannelDelegate> GetWeakPtr();
@@ -73,11 +72,6 @@ void ChannelMojoHost::ChannelDelegate::OnChannelCreated(
   channel_ = channel;
 }
 
-scoped_refptr<base::TaskRunner>
-ChannelMojoHost::ChannelDelegate::GetIOTaskRunner() {
-  return io_task_runner_;
-}
-
 void ChannelMojoHost::ChannelDelegate::OnClientLaunched(
     base::ProcessHandle process) {
   if (channel_)