Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than a ConnectorPtr being passed in with the call.
This allows clients to create their own Connector
pipe and lazily connect it to the shell thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first
   Connector pipe error: instead an Instance is
   kept alive by the shell as long as either the
   ShellClient pipe is connected OR any Connector
   pipes are connected.

 - ShellClientFactory endpoints are now strongly
   typed instead of being raw message pipe
   handles.

 - Some uses of MessagePumpMojo near other changes
   in this CL have been opportunistically removed.

BUG=591742

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

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

14 files changed, 186 insertions, 284 deletions

diff --git a/mojo/edk/embedder/embedder.cc b/mojo/edk/embedder/embedder.cc
index 62702af..e9a426a 100644
--- a/mojo/edk/embedder/embedder.cc
+++ b/mojo/edk/embedder/embedder.cc
@@ -54,6 +54,14 @@ void SetParentPipeHandle(ScopedPlatformHandle pipe) {
   internal::g_core->InitChild(std::move(pipe));
 }
 
+void SetParentPipeHandleFromCommandLine() {
+  ScopedPlatformHandle platform_channel =
+      PlatformChannelPair::PassClientHandleFromParentProcess(
+          *base::CommandLine::ForCurrentProcess());
+  if (platform_channel.is_valid())
+    SetParentPipeHandle(std::move(platform_channel));
+}
+
 void Init() {
   internal::g_core = new Core();
 }
diff --git a/mojo/edk/embedder/embedder.h b/mojo/edk/embedder/embedder.h
index 99191a9..b96d847 100644
--- a/mojo/edk/embedder/embedder.h
+++ b/mojo/edk/embedder/embedder.h
@@ -48,6 +48,10 @@ MOJO_SYSTEM_IMPL_EXPORT void ChildProcessLaunched(
 // that the parent received from ChildProcessLaunched.
 MOJO_SYSTEM_IMPL_EXPORT void SetParentPipeHandle(ScopedPlatformHandle pipe);
 
+// Same as above but extracts the pipe handle from the command line. See
+// PlatformChannelPair for details.
+MOJO_SYSTEM_IMPL_EXPORT void SetParentPipeHandleFromCommandLine();
+
 // Must be called first, or just after setting configuration parameters, to
 // initialize the (global, singleton) system.
 MOJO_SYSTEM_IMPL_EXPORT void Init();
diff --git a/mojo/shell/background/tests/background_shell_unittest.cc b/mojo/shell/background/tests/background_shell_unittest.cc
index 82b36b1..8901ab3 100644
--- a/mojo/shell/background/tests/background_shell_unittest.cc
+++ b/mojo/shell/background/tests/background_shell_unittest.cc
@@ -57,7 +57,6 @@ TEST(BackgroundShellTest, MAYBE_Basic) {
   ShellClientImpl shell_client;
   ShellConnection shell_connection(
       &shell_client, background_shell.CreateShellClientRequest(kTestName));
-  shell_connection.WaitForInitialize();
   mojom::TestServicePtr test_service;
   shell_connection.connector()->ConnectToInterface(
       "mojo:background_shell_test_app", &test_service);
diff --git a/mojo/shell/public/cpp/lib/application_test_base.cc b/mojo/shell/public/cpp/lib/application_test_base.cc
index c895a24..c03f7f72 100644
--- a/mojo/shell/public/cpp/lib/application_test_base.cc
+++ b/mojo/shell/public/cpp/lib/application_test_base.cc
@@ -4,6 +4,7 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ -18,6 +19,7 @@ namespace mojo {
 namespace test {
 
 namespace {
+
 // Share the application name with multiple application tests.
 shell::mojom::IdentityPtr g_identity;
 uint32_t g_id = shell::mojom::kInvalidInstanceID;
@@ -43,13 +45,14 @@ class ShellGrabber : public shell::mojom::ShellClient {
 
  private:
   // shell::mojom::ShellClient implementation.
-  void Initialize(shell::mojom::ConnectorPtr connector,
-                  shell::mojom::IdentityPtr identity,
-                  uint32_t id) override {
+  void Initialize(shell::mojom::IdentityPtr identity,
+                  uint32_t id,
+                  const InitializeCallback& callback) override {
+    callback.Run(GetProxy(&g_connector));
+
     g_identity = std::move(identity);
     g_id = id;
     g_shell_client_request = binding_.Unbind();
-    g_connector = std::move(connector);
   }
 
   void AcceptConnection(
@@ -65,6 +68,8 @@ class ShellGrabber : public shell::mojom::ShellClient {
   Binding<ShellClient> binding_;
 };
 
+void IgnoreConnectorRequest(shell::mojom::ConnectorRequest) {}
+
 }  // namespace
 
 MojoResult RunAllTests(MojoHandle shell_client_request_handle) {
@@ -115,9 +120,12 @@ TestHelper::TestHelper(ShellClient* client)
       name_(g_identity->name),
       userid_(g_identity->user_id),
       instance_id_(g_id) {
+  shell_connection_->SetAppTestConnectorForTesting(std::move(g_connector));
+
   // Fake ShellClient initialization.
   shell::mojom::ShellClient* shell_client = shell_connection_.get();
-  shell_client->Initialize(std::move(g_connector), std::move(g_identity), g_id);
+  shell_client->Initialize(std::move(g_identity), g_id,
+                           base::Bind(&IgnoreConnectorRequest));
 }
 
 TestHelper::~TestHelper() {
diff --git a/mojo/shell/public/cpp/lib/shell_connection.cc b/mojo/shell/public/cpp/lib/shell_connection.cc
index b169895..24210ad 100644
--- a/mojo/shell/public/cpp/lib/shell_connection.cc
+++ b/mojo/shell/public/cpp/lib/shell_connection.cc
@@ -6,6 +6,7 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "mojo/public/cpp/bindings/interface_ptr.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/shell/public/cpp/capabilities.h"
 #include "mojo/shell/public/cpp/connector.h"
 #include "mojo/shell/public/cpp/lib/connection_impl.h"
@@ -18,30 +19,44 @@ namespace mojo {
 ////////////////////////////////////////////////////////////////////////////////
 // ShellConnection, public:
 
-ShellConnection::ShellConnection(
-    mojo::ShellClient* client,
-    InterfaceRequest<shell::mojom::ShellClient> request)
-    : client_(client),
-      binding_(this, std::move(request)),
-      weak_factory_(this) {}
+ShellConnection::ShellConnection(mojo::ShellClient* client)
+    : ShellConnection(client, nullptr) {}
+
+ShellConnection::ShellConnection(mojo::ShellClient* client,
+                                 shell::mojom::ShellClientRequest request)
+    : client_(client), binding_(this) {
+  shell::mojom::ConnectorPtr connector;
+  pending_connector_request_ = GetProxy(&connector);
+  connector_.reset(new ConnectorImpl(std::move(connector)));
+
+  if (request.is_pending())
+    BindToRequest(std::move(request));
+}
 
 ShellConnection::~ShellConnection() {}
 
-void ShellConnection::WaitForInitialize() {
-  DCHECK(!connector_);
-  binding_.WaitForIncomingMethodCall();
+void ShellConnection::BindToRequest(shell::mojom::ShellClientRequest request) {
+  DCHECK(!binding_.is_bound());
+  binding_.Bind(std::move(request));
+}
+
+void ShellConnection::SetAppTestConnectorForTesting(
+    shell::mojom::ConnectorPtr connector) {
+  pending_connector_request_ = nullptr;
+  connector_.reset(new ConnectorImpl(std::move(connector)));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ShellConnection, shell::mojom::ShellClient implementation:
 
-void ShellConnection::Initialize(shell::mojom::ConnectorPtr connector,
-                                 shell::mojom::IdentityPtr identity,
-                                 uint32_t id) {
-  connector_.reset(new ConnectorImpl(std::move(connector)));
-  binding_.set_connection_error_handler(
-      base::Bind(&ShellConnection::OnConnectionError,
-                 weak_factory_.GetWeakPtr()));
+void ShellConnection::Initialize(shell::mojom::IdentityPtr identity,
+                                 uint32_t id,
+                                 const InitializeCallback& callback) {
+  callback.Run(std::move(pending_connector_request_));
+
+  DCHECK(binding_.is_bound());
+  binding_.set_connection_error_handler([this] { OnConnectionError(); });
+
   client_->Initialize(connector_.get(), identity.To<Identity>(), id);
 }
 
diff --git a/mojo/shell/public/cpp/lib/shell_test.cc b/mojo/shell/public/cpp/lib/shell_test.cc
index 624fdc5..402d750 100644
--- a/mojo/shell/public/cpp/lib/shell_test.cc
+++ b/mojo/shell/public/cpp/lib/shell_test.cc
@@ -36,7 +36,7 @@ void ShellTest::InitializeCalled(Connector* connector,
                                  const std::string& name,
                                  const std::string& user_id,
                                  uint32_t id) {
-  connector_ = connector;
+  DCHECK_EQ(connector_, connector);
   initialize_name_ = name;
   initialize_instance_id_ = id;
   initialize_userid_ = user_id;
@@ -50,7 +50,7 @@ void ShellTest::SetUp() {
   shell_connection_.reset(new ShellConnection(
       shell_client_.get(),
       background_shell_->CreateShellClientRequest(test_name_)));
-  shell_connection_->WaitForInitialize();
+  connector_ = shell_connection_->connector();
 }
 
 void ShellTest::TearDown() {
diff --git a/mojo/shell/public/cpp/shell_connection.h b/mojo/shell/public/cpp/shell_connection.h
index e0b1eb1..216ff93 100644
--- a/mojo/shell/public/cpp/shell_connection.h
+++ b/mojo/shell/public/cpp/shell_connection.h
@@ -10,7 +10,6 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_vector.h"
-#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/callback.h"
 #include "mojo/public/cpp/system/core.h"
@@ -44,23 +43,34 @@ class Connector;
 //
 class ShellConnection : public shell::mojom::ShellClient {
  public:
-  // Does not take ownership of |delegate|, which must remain valid for the
+  // Creates a new ShellConnection to eventually be bound to a
+  // ShellClientRequest (see BindToRequest()). This connection may be used
+  // immediately to begin making outgoing connections via connector().
+  //
+  // Does not take ownership of |client|, which must remain valid for the
   // lifetime of ShellConnection.
+  explicit ShellConnection(mojo::ShellClient* client);
+
+  // Like above but binds to |request| upon construction.
   ShellConnection(mojo::ShellClient* client,
-                  InterfaceRequest<shell::mojom::ShellClient> request);
-  ~ShellConnection() override;
+                  shell::mojom::ShellClientRequest request);
 
-  // Block the calling thread until the Initialize() method is called by the
-  // shell.
-  void WaitForInitialize();
+  ~ShellConnection() override;
 
   Connector* connector() { return connector_.get(); }
 
+  // Binds this ShellConnection to a client request if one was not available at
+  // construction time.
+  void BindToRequest(shell::mojom::ShellClientRequest request);
+
+  // TODO(rockot): Remove this once we get rid of app tests.
+  void SetAppTestConnectorForTesting(shell::mojom::ConnectorPtr connector);
+
  private:
   // shell::mojom::ShellClient:
-  void Initialize(shell::mojom::ConnectorPtr connector,
-                  shell::mojom::IdentityPtr identity,
-                  uint32_t id) override;
+  void Initialize(shell::mojom::IdentityPtr identity,
+                  uint32_t id,
+                  const InitializeCallback& callback) override;
   void AcceptConnection(
       shell::mojom::IdentityPtr source,
       uint32_t source_id,
@@ -74,10 +84,13 @@ class ShellConnection : public shell::mojom::ShellClient {
   // We track the lifetime of incoming connection registries as it more
   // convenient for the client.
   ScopedVector<Connection> incoming_connections_;
+
+  // A pending Connector request which will eventually be passed to the shell.
+  shell::mojom::ConnectorRequest pending_connector_request_;
+
   mojo::ShellClient* client_;
   Binding<shell::mojom::ShellClient> binding_;
   scoped_ptr<Connector> connector_;
-  base::WeakPtrFactory<ShellConnection> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ShellConnection);
 };
diff --git a/mojo/shell/public/interfaces/shell_client.mojom b/mojo/shell/public/interfaces/shell_client.mojom
index 260bb2c..b0c601cd 100644
--- a/mojo/shell/public/interfaces/shell_client.mojom
+++ b/mojo/shell/public/interfaces/shell_client.mojom
@@ -17,10 +17,6 @@ interface ShellClient {
   //
   // Parameters:
   //
-  //  connector
-  //    An interface back to the shell by which new connections may be
-  //    established.
-  //
   //  identity
   //    The identity of this instance in the shell. Includes:
   //    * The resolved name used in the connection request that resulted in this
@@ -32,7 +28,14 @@ interface ShellClient {
   //  id
   //    A unique identifier used by the shell to identify this instance.
   //
-  Initialize(Connector connector, Identity identity, uint32 id);
+  //
+  // Response parameters:
+  //
+  //  connector_request
+  //    An optional Connector request for the shell to bind, allowing the
+  //    initialized client to connect to others.
+  //
+  Initialize(Identity identity, uint32 id) => (Connector&? connector_request);
 
   // Called when another application attempts to open a connection to this
   // application. An application implements this method to complete the exchange
diff --git a/mojo/shell/runner/child/BUILD.gn b/mojo/shell/runner/child/BUILD.gn
index 6730d94..e8c63bd 100644
--- a/mojo/shell/runner/child/BUILD.gn
+++ b/mojo/shell/runner/child/BUILD.gn
@@ -25,6 +25,7 @@ source_set("lib") {
     "//mojo/edk/system",
     "//mojo/message_pump",
     "//mojo/platform_handle:platform_handle_impl",
+    "//mojo/shell/public/cpp:sources",
     "//mojo/shell/public/interfaces",
     "//mojo/shell/runner/common",
   ]
diff --git a/mojo/shell/runner/child/runner_connection.cc b/mojo/shell/runner/child/runner_connection.cc
index fc56f6a..f1478d5 100644
--- a/mojo/shell/runner/child/runner_connection.cc
+++ b/mojo/shell/runner/child/runner_connection.cc
@@ -14,13 +14,9 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/thread_task_runner_handle.h"
-#include "base/threading/thread.h"
-#include "base/threading/thread_checker.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/platform_channel_pair.h"
 #include "mojo/edk/embedder/scoped_platform_handle.h"
-#include "mojo/message_pump/message_pump_mojo.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/shell/public/interfaces/shell_client_factory.mojom.h"
 #include "mojo/shell/runner/common/switches.h"
@@ -29,137 +25,25 @@ namespace mojo {
 namespace shell {
 namespace {
 
-// Blocks a thread until another thread unblocks it, at which point it unblocks
-// and runs a closure provided by that thread.
-class Blocker {
+class RunnerConnectionImpl : public RunnerConnection,
+                             public mojom::ShellClientFactory {
  public:
-  // Unblocks a Blocker. Safe to copy around to any thread, but must only be
-  // used from a single thread.
-  class Unblocker {
-   public:
-    explicit Unblocker(Blocker* blocker = nullptr) : blocker_(blocker) {}
-    ~Unblocker() {}
-
-    bool IsBlocking() const { return blocker_ != nullptr; }
-
-    void Unblock(base::Closure run_after) {
-      DCHECK(blocker_);
-      DCHECK(blocker_->run_after_.is_null());
-      blocker_->run_after_ = run_after;
-      blocker_->event_.Signal();
-      blocker_ = nullptr;
-    }
-
-   private:
-    Blocker* blocker_;
-
-    // Copy and assign allowed.
-  };
-
-  Blocker() : event_(true, false) {}
-  ~Blocker() {}
-
-  void Block() {
-    DCHECK(run_after_.is_null());
-    event_.Wait();
-    if (!run_after_.is_null())
-      run_after_.Run();
-  }
-
-  Unblocker GetUnblocker() { return Unblocker(this); }
-
- private:
-  base::WaitableEvent event_;
-  base::Closure run_after_;
-
-  DISALLOW_COPY_AND_ASSIGN(Blocker);
-};
-
-using GotApplicationRequestCallback =
-    base::Callback<void(InterfaceRequest<mojom::ShellClient>)>;
-
-void OnGotApplicationRequest(InterfaceRequest<mojom::ShellClient>* out_request,
-                             InterfaceRequest<mojom::ShellClient> request) {
-  *out_request = std::move(request);
-}
-
-class ChildControllerImpl;
-
-class RunnerConnectionImpl : public RunnerConnection {
- public:
-  RunnerConnectionImpl() : controller_thread_("controller_thread") {
-    StartControllerThread();
-  }
-  ~RunnerConnectionImpl() override {
-    controller_runner_->PostTask(
-        FROM_HERE, base::Bind(&RunnerConnectionImpl::ShutdownOnControllerThread,
-                              base::Unretained(this)));
-    controller_thread_.Stop();
+  RunnerConnectionImpl(mojo::ShellConnection* shell_connetion,
+                       mojom::ShellClientFactoryRequest client_factory_request,
+                       bool exit_on_error)
+      : shell_connection_(shell_connetion),
+        binding_(this, std::move(client_factory_request)),
+        exit_on_error_(exit_on_error) {
+    binding_.set_connection_error_handler([this] { OnConnectionError(); });
   }
 
-  // Returns true if a connection to the runner has been established and
-  // |request| has been modified, false if no connection was established.
-  bool WaitForApplicationRequest(InterfaceRequest<mojom::ShellClient>* request,
-                                 ScopedMessagePipeHandle handle,
-                                 bool exit_on_error);
-
-  ChildControllerImpl* controller() const { return controller_.get(); }
-
-  void set_controller(scoped_ptr<ChildControllerImpl> controller) {
-    controller_ = std::move(controller);
-  }
+  ~RunnerConnectionImpl() override {}
 
  private:
-  void StartControllerThread() {
-    base::Thread::Options controller_thread_options;
-    controller_thread_options.message_loop_type =
-        base::MessageLoop::TYPE_CUSTOM;
-    controller_thread_options.message_pump_factory =
-        base::Bind(&common::MessagePumpMojo::Create);
-    CHECK(controller_thread_.StartWithOptions(controller_thread_options));
-    controller_runner_ = controller_thread_.task_runner().get();
-    CHECK(controller_runner_.get());
-  }
-
-  void ShutdownOnControllerThread() { controller_.reset(); }
-
-  base::Thread controller_thread_;
-  scoped_refptr<base::SingleThreadTaskRunner> controller_runner_;
-
-  // Accessed only on the controller thread.
-  scoped_ptr<ChildControllerImpl> controller_;
-
-  DISALLOW_COPY_AND_ASSIGN(RunnerConnectionImpl);
-};
-
-class ChildControllerImpl : public mojom::ShellClientFactory {
- public:
-  ~ChildControllerImpl() override {
-    DCHECK(thread_checker_.CalledOnValidThread());
-  }
-
-  // To be executed on the controller thread. Creates the |ChildController|,
-  // etc.
-  static void Create(RunnerConnectionImpl* connection,
-                     const GotApplicationRequestCallback& callback,
-                     ScopedMessagePipeHandle runner_handle,
-                     const Blocker::Unblocker& unblocker,
-                     bool exit_on_error) {
-    DCHECK(connection);
-    DCHECK(!connection->controller());
-
-    scoped_ptr<ChildControllerImpl> impl(
-        new ChildControllerImpl(connection, callback, unblocker,
-                                exit_on_error));
-
-    impl->Bind(std::move(runner_handle));
-
-    connection->set_controller(std::move(impl));
-  }
-
-  void Bind(ScopedMessagePipeHandle handle) {
-    binding_.Bind(std::move(handle));
-    binding_.set_connection_error_handler([this]() { OnConnectionError(); });
+  // mojom::ShellClientFactory:
+  void CreateShellClient(mojom::ShellClientRequest client_request,
+                         const String& name) override {
+    shell_connection_->BindToRequest(std::move(client_request));
   }
 
   void OnConnectionError() {
@@ -168,103 +52,32 @@ class ChildControllerImpl : public mojom::ShellClientFactory {
     DLOG(ERROR) << "Connection error to the shell.";
     if (exit_on_error_)
       _exit(1);
-    else
-      UnblockConnection(nullptr);
-  }
-
-  // |mojom::ShellClientFactory| methods:
-  void CreateShellClient(mojom::ShellClientRequest request,
-                         const String& name) override {
-    UnblockConnection(std::move(request));
   }
 
- private:
-  ChildControllerImpl(RunnerConnectionImpl* connection,
-                      const GotApplicationRequestCallback& callback,
-                      const Blocker::Unblocker& unblocker,
-                      bool exit_on_error)
-      : connection_(connection),
-        callback_(callback),
-        unblocker_(unblocker),
-        binding_(this),
-        exit_on_error_(exit_on_error) {}
-
-  static void ReturnApplicationRequestOnMainThread(
-      const GotApplicationRequestCallback& callback,
-      InterfaceRequest<mojom::ShellClient> request) {
-    callback.Run(std::move(request));
-  }
-
-  void UnblockConnection(mojom::ShellClientRequest request) {
-    DCHECK(thread_checker_.CalledOnValidThread());
-    if (unblocker_.IsBlocking()) {
-      unblocker_.Unblock(
-          base::Bind(&ChildControllerImpl::ReturnApplicationRequestOnMainThread,
-                     callback_, base::Passed(&request)));
-    }
-  }
-
-  base::ThreadChecker thread_checker_;
-  RunnerConnectionImpl* const connection_;
-  GotApplicationRequestCallback callback_;
-  Blocker::Unblocker unblocker_;
-
+  mojo::ShellConnection* const shell_connection_;
   Binding<mojom::ShellClientFactory> binding_;
+  const bool exit_on_error_;
 
-  bool exit_on_error_;
-
-  DISALLOW_COPY_AND_ASSIGN(ChildControllerImpl);
+  DISALLOW_COPY_AND_ASSIGN(RunnerConnectionImpl);
 };
 
-bool RunnerConnectionImpl::WaitForApplicationRequest(
-    InterfaceRequest<mojom::ShellClient>* request,
-    ScopedMessagePipeHandle handle,
-    bool exit_on_error) {
-  // If a valid message pipe to the runner was not provided, look for one on the
-  // command line.
-  if (!handle.is_valid()) {
-    edk::ScopedPlatformHandle platform_channel =
-        edk::PlatformChannelPair::PassClientHandleFromParentProcess(
-            *base::CommandLine::ForCurrentProcess());
-    if (!platform_channel.is_valid())
-      return false;
-    edk::SetParentPipeHandle(std::move(platform_channel));
-    std::string primordial_pipe_token =
-        base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-            switches::kPrimordialPipeToken);
-    handle = edk::CreateChildMessagePipe(primordial_pipe_token);
-  }
-
-  DCHECK(handle.is_valid());
-
-  Blocker blocker;
-  controller_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &ChildControllerImpl::Create, base::Unretained(this),
-          base::Bind(&OnGotApplicationRequest, request), base::Passed(&handle),
-          blocker.GetUnblocker(), exit_on_error));
-  blocker.Block();
-
-  return request->is_pending();
-}
-
 }  // namespace
 
 RunnerConnection::~RunnerConnection() {}
 
 // static
-RunnerConnection* RunnerConnection::ConnectToRunner(
-    InterfaceRequest<mojom::ShellClient>* request,
-    ScopedMessagePipeHandle handle,
+scoped_ptr<RunnerConnection> RunnerConnection::Create(
+    mojo::ShellConnection* shell_connection,
     bool exit_on_error) {
-  RunnerConnectionImpl* connection = new RunnerConnectionImpl;
-  if (!connection->WaitForApplicationRequest(
-      request, std::move(handle), exit_on_error)) {
-    delete connection;
-    return nullptr;
-  }
-  return connection;
+  std::string primordial_pipe_token =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kPrimordialPipeToken);
+  mojom::ShellClientFactoryRequest client_factory_request;
+  client_factory_request.Bind(
+      edk::CreateChildMessagePipe(primordial_pipe_token));
+
+  return make_scoped_ptr(new RunnerConnectionImpl(
+      shell_connection, std::move(client_factory_request), exit_on_error));
 }
 
 RunnerConnection::RunnerConnection() {}
diff --git a/mojo/shell/runner/child/runner_connection.h b/mojo/shell/runner/child/runner_connection.h
index e49f73e..e1829c1 100644
--- a/mojo/shell/runner/child/runner_connection.h
+++ b/mojo/shell/runner/child/runner_connection.h
@@ -5,35 +5,41 @@
 #ifndef MOJO_SHELL_RUNNER_CHILD_RUNNER_CONNECTION_H_
 #define MOJO_SHELL_RUNNER_CHILD_RUNNER_CONNECTION_H_
 
+#include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "mojo/shell/public/cpp/shell_connection.h"
+#include "mojo/shell/public/interfaces/connector.mojom.h"
 #include "mojo/shell/public/interfaces/shell_client.mojom.h"
+#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h"
 
 namespace mojo {
 namespace shell {
 
-// Encapsulates a connection to a runner process. The connection object starts a
-// background controller thread that is used to receive control messages from
-// the runner. When this object is destroyed the thread is joined.
+// Encapsulates a connection to a runner process.
 class RunnerConnection {
  public:
   virtual ~RunnerConnection();
 
-  // Establish a connection to the runner, blocking the calling thread until
-  // it is established. The Application request from the runner is returned via
-  // |request|.
+  // Establishes a new runner connection using a ShellClientFactory request pipe
+  // from the command line.
   //
-  // If a connection to the runner cannot be established, |request| will not be
-  // modified and this function will return null.
+  // |shell_connection| is a ShellConnection which will be used to service
+  // incoming connection requests from other clients. This may be null, in which
+  // case no incoming connection requests will be serviced. This is not owned
+  // and must out-live the RunnerConnection.
   //
   // If |exit_on_error| is true, the calling process will be terminated in the
-  // event of an error on |handle|.
+  // event of an error on the ShellClientFactory pipe.
   //
-  // TODO(rockot): Remove |exit_on_error| when it's safe for all clients to be
-  // terminated on such errors. For now we don't want this killing content's
-  // child processes.
-  static RunnerConnection* ConnectToRunner(
-      InterfaceRequest<mojom::ShellClient>* request,
-      ScopedMessagePipeHandle handle,
+  // The RunnerConnection returned by this call bounds the lifetime of the
+  // connection. If connection fails (which can happen if there is no
+  // ShellClientFactory request pipe on the command line) this returns null.
+  //
+  // TODO(rockot): Remove |exit_on_error|. We shouldn't be killing processes
+  // abruptly on pipe errors, but some tests currently depend on this behavior.
+  static scoped_ptr<RunnerConnection> Create(
+      mojo::ShellConnection* shell_connection,
       bool exit_on_error = true);
 
  protected:
diff --git a/mojo/shell/runner/child/test_native_main.cc b/mojo/shell/runner/child/test_native_main.cc
index f8e4532..4caddc3 100644
--- a/mojo/shell/runner/child/test_native_main.cc
+++ b/mojo/shell/runner/child/test_native_main.cc
@@ -14,7 +14,6 @@
 #include "build/build_config.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/process_delegate.h"
-#include "mojo/message_pump/message_pump_mojo.h"
 #include "mojo/shell/public/cpp/shell_client.h"
 #include "mojo/shell/public/cpp/shell_connection.h"
 #include "mojo/shell/runner/child/runner_connection.h"
@@ -56,13 +55,12 @@ int TestNativeMain(mojo::ShellClient* shell_client) {
     CHECK(io_thread.StartWithOptions(io_thread_options));
 
     mojo::edk::InitIPCSupport(&process_delegate, io_thread.task_runner());
+    mojo::edk::SetParentPipeHandleFromCommandLine();
 
-    mojom::ShellClientRequest request;
-    scoped_ptr<mojo::shell::RunnerConnection> connection(
-        mojo::shell::RunnerConnection::ConnectToRunner(
-            &request, ScopedMessagePipeHandle()));
-    base::MessageLoop loop(mojo::common::MessagePumpMojo::Create());
-    mojo::ShellConnection impl(shell_client, std::move(request));
+    base::MessageLoop loop;
+    mojo::ShellConnection impl(shell_client);;
+    scoped_ptr<mojo::shell::RunnerConnection> connection =
+        mojo::shell::RunnerConnection::Create(&impl);
     loop.Run();
 
     mojo::edk::ShutdownIPCSupport();
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index 0d833e1..9d24d0d 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -117,16 +117,39 @@ class Shell::Instance : public mojom::Connector,
       pid_ = base::Process::Current().Pid();
     }
     DCHECK_NE(mojom::kInvalidInstanceID, id_);
+
+    shell_client_.set_connection_error_handler(
+        base::Bind(&Instance::OnShellClientLost, base::Unretained(this)));
   }
 
   ~Instance() override {}
 
+  void OnShellClientLost() {
+    shell_client_.reset();
+    OnConnectionLost();
+  }
+
+  void OnConnectionLost() {
+    // Any time a Connector is lost or we lose the ShellClient connection, it
+    // may have been the last pipe using this Instance. If so, clean up.
+    if (connectors_.empty() && !shell_client_) {
+      // Deletes |this|.
+      shell_->OnInstanceError(this);
+    }
+  }
+
+  void OnInitializeResponse(mojom::ConnectorRequest connector_request) {
+    if (connector_request.is_pending()) {
+      connectors_.AddBinding(this, std::move(connector_request));
+      connectors_.set_connection_error_handler(
+          base::Bind(&Instance::OnConnectionLost, base::Unretained(this)));
+    }
+  }
+
   void InitializeClient() {
-    shell_client_->Initialize(connectors_.CreateInterfacePtrAndBind(this),
-                              mojom::Identity::From(identity_), id_);
-    connectors_.set_connection_error_handler(
-        base::Bind(&mojo::shell::Shell::OnInstanceError,
-                   base::Unretained(shell_), base::Unretained(this)));
+    shell_client_->Initialize(mojom::Identity::From(identity_), id_,
+                              base::Bind(&Instance::OnInitializeResponse,
+                                         base::Unretained(this)));
   }
 
   void ConnectToClient(scoped_ptr<ConnectParams> params) {
@@ -217,6 +240,7 @@ class Shell::Instance : public mojom::Connector,
     params->set_connect_callback(callback);
     shell_->Connect(std::move(params));
   }
+
   void Clone(mojom::ConnectorRequest request) override {
     connectors_.AddBinding(this, std::move(request));
   }
@@ -353,6 +377,7 @@ class Shell::Instance : public mojom::Connector,
   }
 
   mojo::shell::Shell* const shell_;
+
   // An id that identifies this instance. Distinct from pid, as a single process
   // may vend multiple application instances, and this object may exist before a
   // process is launched.
@@ -396,7 +421,6 @@ Shell::Shell(scoped_ptr<NativeRunnerFactory> native_runner_factory,
   CreateInstance(CreateShellIdentity(), GetPermissiveCapabilities(),
                  std::move(client));
   shell_connection_.reset(new ShellConnection(this, std::move(request)));
-  shell_connection_->WaitForInitialize();
 
   if (catalog)
     InitCatalog(std::move(catalog));
diff --git a/mojo/shell/tests/shell/shell_unittest.cc b/mojo/shell/tests/shell/shell_unittest.cc
index 4a0debf..04af796 100644
--- a/mojo/shell/tests/shell/shell_unittest.cc
+++ b/mojo/shell/tests/shell/shell_unittest.cc
@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/process/process_handle.h"
+#include "base/run_loop.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/shell/public/cpp/interface_factory.h"
 #include "mojo/shell/public/cpp/shell_client.h"
@@ -92,7 +93,9 @@ class ShellTest : public mojo::test::ShellTest,
     connector()->ConnectToInterface("mojo:shell", &shell);
 
     shell->AddInstanceListener(binding_.CreateInterfacePtrAndBind());
-    binding_.WaitForIncomingMethodCall();
+
+    wait_for_instances_loop_.reset(new base::RunLoop);
+    wait_for_instances_loop_->Run();
   }
 
   bool ContainsInstanceWithName(const std::string& name) const {
@@ -129,10 +132,15 @@ class ShellTest : public mojo::test::ShellTest,
       initial_instances_.push_back(InstanceInfo(instances[i]->id,
                                                 instances[i]->identity->name));
     }
+
+    DCHECK(wait_for_instances_loop_);
+    wait_for_instances_loop_->Quit();
   }
+
   void InstanceCreated(mojom::InstanceInfoPtr instance) override {
     instances_.push_back(InstanceInfo(instance->id, instance->identity->name));
   }
+
   void InstanceDestroyed(uint32_t id) override {
     for (auto it = instances_.begin(); it != instances_.end(); ++it) {
       auto& instance = *it;
@@ -142,6 +150,7 @@ class ShellTest : public mojo::test::ShellTest,
       }
     }
   }
+
   void InstancePIDAvailable(uint32_t id, uint32_t pid) override {
     for (auto& instance : instances_) {
       if (instance.id == id) {
@@ -155,6 +164,7 @@ class ShellTest : public mojo::test::ShellTest,
   Binding<mojom::InstanceListener> binding_;
   std::vector<InstanceInfo> instances_;
   std::vector<InstanceInfo> initial_instances_;
+  scoped_ptr<base::RunLoop> wait_for_instances_loop_;
 
   DISALLOW_COPY_AND_ASSIGN(ShellTest);
 };