Support early associated interface binding on ChannelMojo

Changes the associated bindings implementation for ChannelMojo
such that remote interfaces can be acquired immediately upon
ChannelMojo construction rather than having to wait for connection
on the IO thread.

Simplifies the Channel bootstrapping process, removing a round-trip
Init message (and in fact the entire IPC::mojom::Boostrap interface)
since there's no need to actually exchange associated interface handles
over the pipe. Instead both sides can assume the other will use a fixed,
reserved endpoint ID for their IPC::mojom::Channel interface.

This also removes the restriction that associated interfaces must be
added to a Channel after Init. Instead the same constraints apply as
with AddFilter: an associated interface, like a filter, may be added
at any time as long as either Init hasn't been called OR the remote
process hasn't been launched.

The result of this CL is that any place it's safe to AddFilter,
it's also safe to AddAssociatedInterface; and any place it's safe to
Send, it's also safe to GetRemoteAssociatedInterface and begin using
any such remote interface immediately.

Remote interface requests as well as all messages to remote interfaces
retain FIFO with respect to any Send calls on the same thread. Local
interface request dispatch as well as all messages on locally bound
associated interfaces retain FIFO with respect to any OnMessageReceived
calls on the same thread.

BUG=612500,619202

Committed: https://crrev.com/e1037f997da9e1d44ca3b09d4ff32f0465673091
Committed: https://crrev.com/508da24622f957a01b076ccd058bfdccc79068a4
Review-Url: https://codereview.chromium.org/2163633003
Cr-Original-Original-Commit-Position: refs/heads/master@{#406720}
Cr-Original-Commit-Position: refs/heads/master@{#407050}
Cr-Commit-Position: refs/heads/master@{#407264}

ipc/ipc.mojom

@@ -22,17 +22,13 @@ struct SerializedHandle {
 interface GenericInterface {};
 
 interface Channel {
+  // Informs the remote end of this client's PID. Must be called exactly once,
+  // before any calls to Receive() below.
+  SetPeerPid(int32 pid);
+
+  // Transmits a classical Chrome IPC message.
   Receive(array<uint8> data, array<SerializedHandle>? handles);
 
+  // Requests a Channel-associated interface.
   GetAssociatedInterface(string name, associated GenericInterface& request);
 };
-
-// An interface for connecting a pair of Channel interfaces representing a
-// bidirectional IPC channel.
-interface Bootstrap {
-  // Initializes a Chrome IPC channel over |to_client_channel| and
-  // |to_server_channel|. Each side also sends its PID to the other side.
-  Init(associated Channel& to_client_channel,
-       associated Channel to_server_channel,
-       int32 pid) => (int32 pid);
-};

ipc/ipc_channel.h

@@ -103,10 +103,11 @@ class IPC_EXPORT Channel : public Endpoint {
     virtual ~AssociatedInterfaceSupport() {}
 
     // Accesses the AssociatedGroup used to associate new interface endpoints
-    // with this Channel.
+    // with this Channel. Must be safe to call from any thread.
     virtual mojo::AssociatedGroup* GetAssociatedGroup() = 0;
 
-    // Adds an interface factory to this channel for interface |name|.
+    // Adds an interface factory to this channel for interface |name|. Must be
+    // safe to call from any thread.
     virtual void AddGenericAssociatedInterface(
         const std::string& name,
         const GenericAssociatedInterfaceFactory& factory) = 0;
@@ -116,11 +117,6 @@ class IPC_EXPORT Channel : public Endpoint {
         const std::string& name,
         mojo::ScopedInterfaceEndpointHandle handle) = 0;
 
-    // Sets the TaskRunner on which to support proxied dispatch for associated
-    // interfaces.
-    virtual void SetProxyTaskRunner(
-        scoped_refptr<base::SingleThreadTaskRunner> task_runner) = 0;
-
     // Template helper to add an interface factory to this channel.
     template <typename Interface>
     using AssociatedInterfaceFactory =

ipc/ipc_channel_mojo.cc

@@ -16,6 +16,7 @@
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/process/process_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "ipc/ipc_listener.h"
@@ -269,13 +270,11 @@ ChannelMojo::ChannelMojo(
     Mode mode,
     Listener* listener,
     const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner)
-    : pipe_(handle.get()),
-      listener_(listener),
-      waiting_connect_(true),
-      weak_factory_(this) {
+    : pipe_(handle.get()), listener_(listener), weak_factory_(this) {
   // Create MojoBootstrap after all members are set as it touches
   // ChannelMojo from a different thread.
-  bootstrap_ = MojoBootstrap::Create(std::move(handle), mode, this);
+  bootstrap_ =
+      MojoBootstrap::Create(std::move(handle), mode, this, ipc_task_runner);
 }
 
 ChannelMojo::~ChannelMojo() {
@@ -284,89 +283,32 @@ ChannelMojo::~ChannelMojo() {
 
 bool ChannelMojo::Connect() {
   WillConnect();
-  {
-    base::AutoLock lock(lock_);
-    DCHECK(!task_runner_);
-    task_runner_ = base::ThreadTaskRunnerHandle::Get();
-    DCHECK(!message_reader_);
-  }
+
+  DCHECK(!task_runner_);
+  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DCHECK(!message_reader_);
+
   bootstrap_->Connect();
   return true;
 }
 
 void ChannelMojo::Close() {
-  std::unique_ptr<internal::MessagePipeReader, ReaderDeleter> reader;
-  {
-    base::AutoLock lock(lock_);
-    if (!message_reader_)
-      return;
-    // The reader's destructor may re-enter Close, so we swap it out first to
-    // avoid deadlock when freeing it below.
-    std::swap(message_reader_, reader);
-
-    // We might Close() before we Connect().
-    waiting_connect_ = false;
-  }
-
+  // NOTE: The MessagePipeReader's destructor may re-enter this function. Use
+  // caution when changing this method.
+  std::unique_ptr<internal::MessagePipeReader> reader =
+      std::move(message_reader_);
   reader.reset();
-}
 
-// MojoBootstrap::Delegate implementation
-void ChannelMojo::OnPipesAvailable(
-    mojom::ChannelAssociatedPtrInfo send_channel,
-    mojom::ChannelAssociatedRequest receive_channel,
-    int32_t peer_pid) {
-  InitMessageReader(std::move(send_channel), std::move(receive_channel),
-                    peer_pid);
+  base::AutoLock lock(associated_interface_lock_);
+  associated_interfaces_.clear();
 }
 
-void ChannelMojo::OnBootstrapError() {
-  listener_->OnChannelError();
-}
-
-void ChannelMojo::OnAssociatedInterfaceRequest(
-    const std::string& name,
-    mojo::ScopedInterfaceEndpointHandle handle) {
-  auto iter = associated_interfaces_.find(name);
-  if (iter != associated_interfaces_.end())
-    iter->second.Run(std::move(handle));
-}
-
-void ChannelMojo::InitMessageReader(mojom::ChannelAssociatedPtrInfo sender,
-                                    mojom::ChannelAssociatedRequest receiver,
-                                    base::ProcessId peer_pid) {
-  mojom::ChannelAssociatedPtr sender_ptr;
-  sender_ptr.Bind(std::move(sender));
-  std::unique_ptr<internal::MessagePipeReader, ChannelMojo::ReaderDeleter>
-      reader(new internal::MessagePipeReader(
-          pipe_, std::move(sender_ptr), std::move(receiver), peer_pid, this));
-
-  bool connected = true;
-  {
-    base::AutoLock lock(lock_);
-    for (size_t i = 0; i < pending_messages_.size(); ++i) {
-      if (!reader->Send(std::move(pending_messages_[i]))) {
-        LOG(ERROR) << "Failed to flush pending messages";
-        pending_messages_.clear();
-        connected = false;
-        break;
-      }
-    }
-
-    if (connected) {
-      // We set |message_reader_| here and won't get any |pending_messages_|
-      // hereafter. Although we might have some if there is an error, we don't
-      // care. They cannot be sent anyway.
-      message_reader_ = std::move(reader);
-      pending_messages_.clear();
-      waiting_connect_ = false;
-    }
-  }
-
-  if (connected)
-    listener_->OnChannelConnected(static_cast<int32_t>(GetPeerPID()));
-  else
-    OnPipeError();
+// MojoBootstrap::Delegate implementation
+void ChannelMojo::OnPipesAvailable(mojom::ChannelAssociatedPtr sender,
+                                   mojom::ChannelAssociatedRequest receiver) {
+  sender->SetPeerPid(GetSelfPID());
+  message_reader_.reset(new internal::MessagePipeReader(
+      pipe_, std::move(sender), std::move(receiver), this));
 }
 
 void ChannelMojo::OnPipeError() {
@@ -380,15 +322,26 @@ void ChannelMojo::OnPipeError() {
   }
 }
 
-bool ChannelMojo::Send(Message* message) {
-  base::AutoLock lock(lock_);
-  if (!message_reader_) {
-    pending_messages_.push_back(base::WrapUnique(message));
-    // Counts as OK before the connection is established, but it's an
-    // error otherwise.
-    return waiting_connect_;
+void ChannelMojo::OnAssociatedInterfaceRequest(
+    const std::string& name,
+    mojo::ScopedInterfaceEndpointHandle handle) {
+  GenericAssociatedInterfaceFactory factory;
+  {
+    base::AutoLock locker(associated_interface_lock_);
+    auto iter = associated_interfaces_.find(name);
+    if (iter != associated_interfaces_.end())
+      factory = iter->second;
   }
 
+  if (!factory.is_null())
+    factory.Run(std::move(handle));
+}
+
+bool ChannelMojo::Send(Message* message) {
+  std::unique_ptr<Message> scoped_message = base::WrapUnique(message);
+  if (!message_reader_)
+    return false;
+
   // Comment copied from ipc_channel_posix.cc:
   // We can't close the pipe here, because calling OnChannelError may destroy
   // this object, and that would be bad if we are called from Send(). Instead,
@@ -398,7 +351,7 @@ bool ChannelMojo::Send(Message* message) {
   //
   // With Mojo, there's no OnFileCanReadWithoutBlocking, but we expect the
   // pipe's connection error handler will be invoked in its place.
-  return message_reader_->Send(base::WrapUnique(message));
+  return message_reader_->Send(std::move(scoped_message));
 }
 
 bool ChannelMojo::IsSendThreadSafe() const {
@@ -406,20 +359,30 @@ bool ChannelMojo::IsSendThreadSafe() const {
 }
 
 base::ProcessId ChannelMojo::GetPeerPID() const {
-  base::AutoLock lock(lock_);
   if (!message_reader_)
     return base::kNullProcessId;
-
   return message_reader_->GetPeerPid();
 }
 
 base::ProcessId ChannelMojo::GetSelfPID() const {
-  return bootstrap_->GetSelfPID();
+#if defined(OS_LINUX)
+  if (int global_pid = GetGlobalPid())
+    return global_pid;
+#endif  // OS_LINUX
+#if defined(OS_NACL)
+  return -1;
+#else
+  return base::GetCurrentProcId();
+#endif  // defined(OS_NACL)
 }
 
 Channel::AssociatedInterfaceSupport*
 ChannelMojo::GetAssociatedInterfaceSupport() { return this; }
 
+void ChannelMojo::OnPeerPidReceived() {
+  listener_->OnChannelConnected(static_cast<int32_t>(GetPeerPID()));
+}
+
 void ChannelMojo::OnMessageReceived(const Message& message) {
   TRACE_EVENT2("ipc,toplevel", "ChannelMojo::OnMessageReceived",
                "class", IPC_MESSAGE_ID_CLASS(message.type()),
@@ -504,6 +467,7 @@ mojo::AssociatedGroup* ChannelMojo::GetAssociatedGroup() {
 void ChannelMojo::AddGenericAssociatedInterface(
     const std::string& name,
     const GenericAssociatedInterfaceFactory& factory) {
+  base::AutoLock locker(associated_interface_lock_);
   auto result = associated_interfaces_.insert({ name, factory });
   DCHECK(result.second);
 }
@@ -511,14 +475,8 @@ void ChannelMojo::AddGenericAssociatedInterface(
 void ChannelMojo::GetGenericRemoteAssociatedInterface(
     const std::string& name,
     mojo::ScopedInterfaceEndpointHandle handle) {
-  DCHECK(message_reader_);
-  message_reader_->GetRemoteInterface(name, std::move(handle));
-}
-
-void ChannelMojo::SetProxyTaskRunner(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  DCHECK(bootstrap_);
-  bootstrap_->SetProxyTaskRunner(task_runner);
+  if (message_reader_)
+    message_reader_->GetRemoteInterface(name, std::move(handle));
 }
 
 }  // namespace IPC

ipc/ipc_channel_mojo.h

@@ -42,7 +42,7 @@ namespace IPC {
 class IPC_EXPORT ChannelMojo
     : public Channel,
       public Channel::AssociatedInterfaceSupport,
-      public MojoBootstrap::Delegate,
+      public NON_EXPORTED_BASE(MojoBootstrap::Delegate),
       public NON_EXPORTED_BASE(internal::MessagePipeReader::Delegate) {
  public:
   // Creates a ChannelMojo.
@@ -90,17 +90,16 @@ class IPC_EXPORT ChannelMojo
       mojo::Array<mojom::SerializedHandlePtr>* handles);
 
   // MojoBootstrapDelegate implementation
-  void OnPipesAvailable(mojom::ChannelAssociatedPtrInfo send_channel,
-                        mojom::ChannelAssociatedRequest receive_channel,
-                        int32_t peer_pid) override;
-  void OnBootstrapError() override;
-  void OnAssociatedInterfaceRequest(
-      const std::string& name,
-      mojo::ScopedInterfaceEndpointHandle handle) override;
+  void OnPipesAvailable(mojom::ChannelAssociatedPtr sender,
+                        mojom::ChannelAssociatedRequest receiver) override;
 
   // MessagePipeReader::Delegate
+  void OnPeerPidReceived() override;
   void OnMessageReceived(const Message& message) override;
   void OnPipeError() override;
+  void OnAssociatedInterfaceRequest(
+      const std::string& name,
+      mojo::ScopedInterfaceEndpointHandle handle) override;
 
  private:
   ChannelMojo(
@@ -109,10 +108,6 @@ class IPC_EXPORT ChannelMojo
       Listener* listener,
       const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner);
 
-  void InitMessageReader(mojom::ChannelAssociatedPtrInfo sender,
-                         mojom::ChannelAssociatedRequest receiver,
-                         base::ProcessId peer_pid);
-
   // Channel::AssociatedInterfaceSupport:
   mojo::AssociatedGroup* GetAssociatedGroup() override;
   void AddGenericAssociatedInterface(
@@ -121,13 +116,6 @@ class IPC_EXPORT ChannelMojo
   void GetGenericRemoteAssociatedInterface(
       const std::string& name,
       mojo::ScopedInterfaceEndpointHandle handle) override;
-  void SetProxyTaskRunner(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner) override;
-
-  // ChannelMojo needs to kill its MessagePipeReader in delayed manner
-  // because the channel wants to kill these readers during the
-  // notifications invoked by them.
-  typedef internal::MessagePipeReader::DelayedDeleter ReaderDeleter;
 
   // A TaskRunner which runs tasks on the ChannelMojo's owning thread.
   scoped_refptr<base::TaskRunner> task_runner_;
@@ -136,15 +124,12 @@ class IPC_EXPORT ChannelMojo
   std::unique_ptr<MojoBootstrap> bootstrap_;
   Listener* listener_;
 
+  std::unique_ptr<internal::MessagePipeReader> message_reader_;
+
+  base::Lock associated_interface_lock_;
   std::map<std::string, GenericAssociatedInterfaceFactory>
       associated_interfaces_;
 
-  // Guards access to the fields below.
-  mutable base::Lock lock_;
-  std::unique_ptr<internal::MessagePipeReader, ReaderDeleter> message_reader_;
-  std::vector<std::unique_ptr<Message>> pending_messages_;
-  bool waiting_connect_;
-
   base::WeakPtrFactory<ChannelMojo> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelMojo);

ipc/ipc_channel_mojo_unittest.cc

@@ -758,6 +758,10 @@ class IPCChannelProxyMojoTest : public IPCChannelMojoTestBase {
   }
   void CreateProxy(IPC::Listener* listener) { runner_->CreateProxy(listener); }
   void RunProxy() { runner_->RunProxy(); }
+  void DestroyProxy() {
+    runner_.reset();
+    base::RunLoop().RunUntilIdle();
+  }
 
   IPC::ChannelProxy* proxy() { return runner_->proxy(); }
 
@@ -808,6 +812,7 @@ class ListenerWithSimpleProxyAssociatedInterface
   void RequestQuit(const RequestQuitCallback& callback) override {
     received_quit_ = true;
     callback.Run();
+    binding_.Close();
     base::MessageLoop::current()->QuitWhenIdle();
   }
 
@@ -838,7 +843,7 @@ TEST_F(IPCChannelProxyMojoTest, ProxyThreadAssociatedInterface) {
   EXPECT_TRUE(WaitForClientShutdown());
   EXPECT_TRUE(listener.received_all_messages());
 
-  base::RunLoop().RunUntilIdle();
+  DestroyProxy();
 }
 
 class ChannelProxyClient {
@@ -848,6 +853,10 @@ class ChannelProxyClient {
   }
   void CreateProxy(IPC::Listener* listener) { runner_->CreateProxy(listener); }
   void RunProxy() { runner_->RunProxy(); }
+  void DestroyProxy() {
+    runner_.reset();
+    base::RunLoop().RunUntilIdle();
+  }
 
   IPC::ChannelProxy* proxy() { return runner_->proxy(); }
 
@@ -856,26 +865,17 @@ class ChannelProxyClient {
   std::unique_ptr<ChannelProxyRunner> runner_;
 };
 
-class ListenerThatWaitsForConnect : public IPC::Listener {
+class DummyListener : public IPC::Listener {
  public:
-  explicit ListenerThatWaitsForConnect(const base::Closure& connect_handler)
-      : connect_handler_(connect_handler) {}
-
   // IPC::Listener
   bool OnMessageReceived(const IPC::Message& message) override { return true; }
-  void OnChannelConnected(int32_t) override { connect_handler_.Run(); }
-
- private:
-  base::Closure connect_handler_;
 };
 
 DEFINE_IPC_CHANNEL_MOJO_TEST_CLIENT(ProxyThreadAssociatedInterfaceClient,
                                     ChannelProxyClient) {
-  base::RunLoop connect_loop;
-  ListenerThatWaitsForConnect listener(connect_loop.QuitClosure());
+  DummyListener listener;
   CreateProxy(&listener);
   RunProxy();
-  connect_loop.Run();
 
   // Send a bunch of interleaved messages, alternating between the associated
   // interface and a legacy IPC::Message.
@@ -889,6 +889,8 @@ DEFINE_IPC_CHANNEL_MOJO_TEST_CLIENT(ProxyThreadAssociatedInterfaceClient,
   }
   driver->RequestQuit(base::MessageLoop::QuitWhenIdleClosure());
   base::RunLoop().Run();
+
+  DestroyProxy();
 }
 
 #if defined(OS_POSIX)

ipc/ipc_channel_proxy.cc

@@ -79,6 +79,17 @@ void ChannelProxy::Context::CreateChannel(
   channel_ = factory->BuildChannel(this);
   channel_send_thread_safe_ = channel_->IsSendThreadSafe();
   channel_->SetAttachmentBrokerEndpoint(attachment_broker_endpoint_);
+
+  Channel::AssociatedInterfaceSupport* support =
+      channel_->GetAssociatedInterfaceSupport();
+  if (support) {
+    associated_group_ = *support->GetAssociatedGroup();
+
+    base::AutoLock l(pending_filters_lock_);
+    for (auto& entry : pending_interfaces_)
+      support->AddGenericAssociatedInterface(entry.first, entry.second);
+    pending_interfaces_.clear();
+  }
 }
 
 bool ChannelProxy::Context::TryFilters(const Message& message) {
@@ -162,23 +173,6 @@ void ChannelProxy::Context::OnChannelOpened() {
 
   for (size_t i = 0; i < filters_.size(); ++i)
     filters_[i]->OnFilterAdded(channel_.get());
-
-  Channel::AssociatedInterfaceSupport* support =
-      channel_->GetAssociatedInterfaceSupport();
-  if (support) {
-    support->SetProxyTaskRunner(listener_task_runner_);
-    for (auto& entry : io_thread_interfaces_)
-      support->AddGenericAssociatedInterface(entry.first, entry.second);
-    for (auto& entry : proxy_thread_interfaces_) {
-      support->AddGenericAssociatedInterface(
-          entry.first, base::Bind(&BindAssociatedInterfaceOnTaskRunner,
-                                  listener_task_runner_, entry.second));
-    }
-  } else {
-    // Sanity check to ensure nobody's expecting to use associated interfaces on
-    // a Channel that doesn't support them.
-    DCHECK(io_thread_interfaces_.empty() && proxy_thread_interfaces_.empty());
-  }
 }
 
 // Called on the IPC::Channel thread
@@ -332,18 +326,6 @@ void ChannelProxy::Context::OnDispatchConnected() {
   if (channel_connected_called_)
     return;
 
-  {
-    base::AutoLock l(channel_lifetime_lock_);
-    if (channel_) {
-      Channel::AssociatedInterfaceSupport* associated_interface_support =
-          channel_->GetAssociatedInterfaceSupport();
-      if (associated_interface_support) {
-        channel_associated_group_.reset(new mojo::AssociatedGroup(
-            *associated_interface_support->GetAssociatedGroup()));
-      }
-    }
-  }
-
   base::ProcessId peer_pid;
   {
     base::AutoLock l(peer_pid_lock_);
@@ -369,6 +351,30 @@ void ChannelProxy::Context::OnDispatchBadMessage(const Message& message) {
 void ChannelProxy::Context::ClearChannel() {
   base::AutoLock l(channel_lifetime_lock_);
   channel_.reset();
+  associated_group_ = mojo::AssociatedGroup();
+}
+
+void ChannelProxy::Context::AddGenericAssociatedInterface(
+    const std::string& name,
+    const GenericAssociatedInterfaceFactory& factory) {
+  AddGenericAssociatedInterfaceForIOThread(
+      name, base::Bind(&BindAssociatedInterfaceOnTaskRunner,
+                       listener_task_runner_, factory));
+}
+
+void ChannelProxy::Context::AddGenericAssociatedInterfaceForIOThread(
+    const std::string& name,
+    const GenericAssociatedInterfaceFactory& factory) {
+  base::AutoLock l(channel_lifetime_lock_);
+  if (!channel_) {
+    base::AutoLock l(pending_filters_lock_);
+    pending_interfaces_.emplace_back(name, factory);
+    return;
+  }
+  Channel::AssociatedInterfaceSupport* support =
+      channel_->GetAssociatedInterfaceSupport();
+  DCHECK(support);
+  support->AddGenericAssociatedInterface(name, factory);
 }
 
 void ChannelProxy::Context::SendFromThisThread(Message* message) {
@@ -546,24 +552,20 @@ void ChannelProxy::RemoveFilter(MessageFilter* filter) {
                             base::RetainedRef(filter)));
 }
 
-void ChannelProxy::AddGenericAssociatedInterfaceForIOThread(
+void ChannelProxy::AddGenericAssociatedInterface(
     const std::string& name,
     const GenericAssociatedInterfaceFactory& factory) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!did_init_);
-  context_->io_thread_interfaces_.insert({ name, factory });
+  context()->AddGenericAssociatedInterface(name, factory);
 }
 
-void ChannelProxy::AddGenericAssociatedInterface(
+void ChannelProxy::AddGenericAssociatedInterfaceForIOThread(
     const std::string& name,
     const GenericAssociatedInterfaceFactory& factory) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!did_init_);
-  context_->proxy_thread_interfaces_.insert({ name, factory });
+  context()->AddGenericAssociatedInterfaceForIOThread(name, factory);
 }
 
 mojo::AssociatedGroup* ChannelProxy::GetAssociatedGroup() {
-  return context_->channel_associated_group_.get();
+  return context()->associated_group();
 }
 
 void ChannelProxy::GetGenericRemoteAssociatedInterface(

ipc/ipc_channel_proxy.h

@@ -189,8 +189,7 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
   }
 
   // Gets the AssociatedGroup used to create new associated endpoints on this
-  // ChannelProxy. This must only be called after the listener's
-  // OnChannelConnected is called.
+  // ChannelProxy.
   mojo::AssociatedGroup* GetAssociatedGroup();
 
   // Requests an associated interface from the remote endpoint.
@@ -199,7 +198,6 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
       mojo::ScopedInterfaceEndpointHandle handle);
 
   // Template helper to request associated interfaces from the remote endpoint.
-  // Must only be called after the listener's OnChannelConnected is called.
   template <typename Interface>
   void GetRemoteAssociatedInterface(
       mojo::AssociatedInterfacePtr<Interface>* proxy) {
@@ -231,7 +229,7 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
   class Context;
   // A subclass uses this constructor if it needs to add more information
   // to the internal state.
-  ChannelProxy(Context* context);
+  explicit ChannelProxy(Context* context);
 
 
   // Used internally to hold state that is referenced on the IPC thread.
@@ -312,6 +310,15 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
     void SendFromThisThread(Message* message);
     void ClearChannel();
 
+    mojo::AssociatedGroup* associated_group() { return &associated_group_; }
+
+    void AddGenericAssociatedInterface(
+        const std::string& name,
+        const GenericAssociatedInterfaceFactory& factory);
+    void AddGenericAssociatedInterfaceForIOThread(
+        const std::string& name,
+        const GenericAssociatedInterfaceFactory& factory);
+
     scoped_refptr<base::SingleThreadTaskRunner> listener_task_runner_;
     Listener* listener_;
 
@@ -353,15 +360,14 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
     // brokerable attachment messages to/from the broker process.
     bool attachment_broker_endpoint_;
 
-    // Modified only on the listener's thread before Init() is called.
-    std::map<std::string, GenericAssociatedInterfaceFactory>
-        io_thread_interfaces_;
-    std::map<std::string, GenericAssociatedInterfaceFactory>
-        proxy_thread_interfaces_;
+    mojo::AssociatedGroup associated_group_;
 
-    // Valid and constant any time after the ChannelProxy's Listener receives
-    // OnChannelConnected on its own thread.
-    std::unique_ptr<mojo::AssociatedGroup> channel_associated_group_;
+    // Holds associated interface binders added by AddGenericAssociatedInterface
+    // or AddGenericAssociatedInterfaceForIOThread until the underlying channel
+    // has been initialized.
+    base::Lock pending_interfaces_lock_;
+    std::vector<std::pair<std::string, GenericAssociatedInterfaceFactory>>
+        pending_interfaces_;
   };
 
   Context* context() { return context_.get(); }

ipc/ipc_message_pipe_reader.cc

@@ -55,10 +55,8 @@ MessagePipeReader::MessagePipeReader(
     mojo::MessagePipeHandle pipe,
     mojom::ChannelAssociatedPtr sender,
     mojo::AssociatedInterfaceRequest<mojom::Channel> receiver,
-    base::ProcessId peer_pid,
     MessagePipeReader::Delegate* delegate)
     : delegate_(delegate),
-      peer_pid_(peer_pid),
       sender_(std::move(sender)),
       binding_(this, std::move(receiver)),
       sender_interface_id_(sender_.interface_id()),
@@ -123,9 +121,15 @@ void MessagePipeReader::GetRemoteInterface(
   sender_->GetAssociatedInterface(name, std::move(request));
 }
 
+void MessagePipeReader::SetPeerPid(int32_t peer_pid) {
+  peer_pid_ = peer_pid;
+  delegate_->OnPeerPidReceived();
+}
+
 void MessagePipeReader::Receive(
     mojo::Array<uint8_t> data,
     mojo::Array<mojom::SerializedHandlePtr> handles) {
+  DCHECK_NE(peer_pid_, base::kNullProcessId);
   Message message(
       data.size() == 0 ? "" : reinterpret_cast<const char*>(&data[0]),
       static_cast<uint32_t>(data.size()));
@@ -156,16 +160,12 @@ void MessagePipeReader::GetAssociatedInterface(
 
 void MessagePipeReader::OnPipeError(MojoResult error) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (delegate_)
-    delegate_->OnPipeError();
+
   Close();
-}
 
-void MessagePipeReader::DelayedDeleter::operator()(
-    MessagePipeReader* ptr) const {
-  ptr->Close();
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                base::Bind(&DeleteNow, ptr));
+  // NOTE: The delegate call below may delete |this|.
+  if (delegate_)
+    delegate_->OnPipeError();
 }
 
 }  // namespace internal

ipc/ipc_message_pipe_reader.h

@@ -13,6 +13,7 @@
 #include "base/atomicops.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/process/process_handle.h"
 #include "base/threading/thread_checker.h"
 #include "ipc/ipc.mojom.h"
 #include "ipc/ipc_export.h"
@@ -47,6 +48,7 @@ class IPC_EXPORT MessagePipeReader : public NON_EXPORTED_BASE(mojom::Channel) {
  public:
   class Delegate {
    public:
+    virtual void OnPeerPidReceived() = 0;
     virtual void OnMessageReceived(const Message& message) = 0;
     virtual void OnPipeError() = 0;
     virtual void OnAssociatedInterfaceRequest(
@@ -54,21 +56,6 @@ class IPC_EXPORT MessagePipeReader : public NON_EXPORTED_BASE(mojom::Channel) {
         mojo::ScopedInterfaceEndpointHandle handle) = 0;
   };
 
-  // Delay the object deletion using the current message loop.
-  // This is intended to used by MessagePipeReader owners.
-  class DelayedDeleter {
-   public:
-    typedef std::default_delete<MessagePipeReader> DefaultType;
-
-    static void DeleteNow(MessagePipeReader* ptr) { delete ptr; }
-
-    DelayedDeleter() {}
-    explicit DelayedDeleter(const DefaultType&) {}
-    DelayedDeleter& operator=(const DefaultType&) { return *this; }
-
-    void operator()(MessagePipeReader* ptr) const;
-  };
-
   // Builds a reader that reads messages from |receive_handle| and lets
   // |delegate| know.
   //
@@ -82,7 +69,6 @@ class IPC_EXPORT MessagePipeReader : public NON_EXPORTED_BASE(mojom::Channel) {
   MessagePipeReader(mojo::MessagePipeHandle pipe,
                     mojom::ChannelAssociatedPtr sender,
                     mojo::AssociatedInterfaceRequest<mojom::Channel> receiver,
-                    base::ProcessId peer_pid,
                     Delegate* delegate);
   ~MessagePipeReader() override;
 
@@ -108,6 +94,7 @@ class IPC_EXPORT MessagePipeReader : public NON_EXPORTED_BASE(mojom::Channel) {
 
  private:
   // mojom::Channel:
+  void SetPeerPid(int32_t peer_pid) override;
   void Receive(mojo::Array<uint8_t> data,
                mojo::Array<mojom::SerializedHandlePtr> handles) override;
   void GetAssociatedInterface(
@@ -116,7 +103,7 @@ class IPC_EXPORT MessagePipeReader : public NON_EXPORTED_BASE(mojom::Channel) {
 
   // |delegate_| is null once the message pipe is closed.
   Delegate* delegate_;
-  base::ProcessId peer_pid_;
+  base::ProcessId peer_pid_ = base::kNullProcessId;
   mojom::ChannelAssociatedPtr sender_;
   mojo::AssociatedBinding<mojom::Channel> binding_;
 

ipc/ipc_mojo_bootstrap.h

@@ -11,7 +11,6 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/process/process_handle.h"
 #include "base/single_thread_task_runner.h"
 #include "build/build_config.h"
 #include "ipc/ipc.mojom.h"
@@ -34,57 +33,26 @@ class IPC_EXPORT MojoBootstrap {
  public:
   class Delegate {
    public:
-    virtual void OnPipesAvailable(
-        mojom::ChannelAssociatedPtrInfo send_channel,
-        mojom::ChannelAssociatedRequest receive_channel,
-        int32_t peer_pid) = 0;
-    virtual void OnBootstrapError() = 0;
+    virtual ~Delegate() {}
+
+    virtual void OnPipesAvailable(mojom::ChannelAssociatedPtr sender,
+                                  mojom::ChannelAssociatedRequest receiver) = 0;
   };
 
+  virtual ~MojoBootstrap() {}
+
   // Create the MojoBootstrap instance, using |handle| as the message pipe, in
   // mode as specified by |mode|. The result is passed to |delegate|.
   static std::unique_ptr<MojoBootstrap> Create(
       mojo::ScopedMessagePipeHandle handle,
       Channel::Mode mode,
-      Delegate* delegate);
-
-  MojoBootstrap();
-  virtual ~MojoBootstrap();
+      Delegate* delegate,
+      const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner);
 
   // Start the handshake over the underlying message pipe.
   virtual void Connect() = 0;
 
   virtual mojo::AssociatedGroup* GetAssociatedGroup() = 0;
-
-  virtual void SetProxyTaskRunner(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner) = 0;
-
-  // GetSelfPID returns our PID.
-  base::ProcessId GetSelfPID() const;
-
- protected:
-  // On MojoServerBootstrap: INITIALIZED -> WAITING_ACK -> READY
-  // On MojoClientBootstrap: INITIALIZED -> READY
-  // STATE_ERROR is a catch-all state that captures any observed error.
-  enum State { STATE_INITIALIZED, STATE_WAITING_ACK, STATE_READY, STATE_ERROR };
-
-  Delegate* delegate() const { return delegate_; }
-  void Fail();
-  bool HasFailed() const;
-
-  State state() const { return state_; }
-  void set_state(State state) { state_ = state; }
-
-  mojo::ScopedMessagePipeHandle TakeHandle();
-
- private:
-  void Init(mojo::ScopedMessagePipeHandle, Delegate* delegate);
-
-  mojo::ScopedMessagePipeHandle handle_;
-  Delegate* delegate_;
-  State state_;
-
-  DISALLOW_COPY_AND_ASSIGN(MojoBootstrap);
 };
 
 }  // namespace IPC

ipc/ipc_mojo_bootstrap_unittest.cc

@@ -35,10 +35,9 @@ class TestingDelegate : public IPC::MojoBootstrap::Delegate {
   explicit TestingDelegate(const base::Closure& quit_callback)
       : passed_(false), quit_callback_(quit_callback) {}
 
-  void OnPipesAvailable(IPC::mojom::ChannelAssociatedPtrInfo send_channel,
-                        IPC::mojom::ChannelAssociatedRequest receive_channel,
-                        int32_t peer_pid) override;
-  void OnBootstrapError() override;
+  void OnPipesAvailable(
+      IPC::mojom::ChannelAssociatedPtr sender,
+      IPC::mojom::ChannelAssociatedRequest receiver) override;
 
   bool passed() const { return passed_; }
 
@@ -48,24 +47,20 @@ class TestingDelegate : public IPC::MojoBootstrap::Delegate {
 };
 
 void TestingDelegate::OnPipesAvailable(
-    IPC::mojom::ChannelAssociatedPtrInfo send_channel,
-    IPC::mojom::ChannelAssociatedRequest receive_channel,
-    int32_t peer_pid) {
+    IPC::mojom::ChannelAssociatedPtr sender,
+    IPC::mojom::ChannelAssociatedRequest receiver) {
   passed_ = true;
   quit_callback_.Run();
 }
 
-void TestingDelegate::OnBootstrapError() {
-  quit_callback_.Run();
-}
-
 TEST_F(IPCMojoBootstrapTest, Connect) {
   base::MessageLoop message_loop;
   base::RunLoop run_loop;
   TestingDelegate delegate(run_loop.QuitClosure());
   std::unique_ptr<IPC::MojoBootstrap> bootstrap = IPC::MojoBootstrap::Create(
       helper_.StartChild("IPCMojoBootstrapTestClient"),
-      IPC::Channel::MODE_SERVER, &delegate);
+      IPC::Channel::MODE_SERVER, &delegate,
+      base::ThreadTaskRunnerHandle::Get());
 
   bootstrap->Connect();
   run_loop.Run();
@@ -84,7 +79,8 @@ MULTIPROCESS_TEST_MAIN_WITH_SETUP(
   std::unique_ptr<IPC::MojoBootstrap> bootstrap = IPC::MojoBootstrap::Create(
       mojo::edk::CreateChildMessagePipe(
           mojo::edk::test::MultiprocessTestHelper::primordial_pipe_token),
-      IPC::Channel::MODE_CLIENT, &delegate);
+      IPC::Channel::MODE_CLIENT, &delegate,
+      base::ThreadTaskRunnerHandle::Get());
 
   bootstrap->Connect();
 

mojo/public/cpp/bindings/lib/interface_endpoint_client.cc

@@ -159,7 +159,8 @@ InterfaceEndpointClient::InterfaceEndpointClient(
 InterfaceEndpointClient::~InterfaceEndpointClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  handle_.group_controller()->DetachEndpointClient(handle_);
+  if (handle_.is_valid())
+    handle_.group_controller()->DetachEndpointClient(handle_);
 }
 
 AssociatedGroup* InterfaceEndpointClient::associated_group() {