Monitor the ShellClient binding for pipe closure as a signal to shut down the ShellConnection, rather than the first Connector InterfacePtr.

R=sky@chromium.org
BUG=

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

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

11 files changed, 25 insertions, 35 deletions

diff --git a/mojo/services/network/network_service_delegate.cc b/mojo/services/network/network_service_delegate.cc
index 3637167..806261d 100644
--- a/mojo/services/network/network_service_delegate.cc
+++ b/mojo/services/network/network_service_delegate.cc
@@ -78,10 +78,6 @@ bool NetworkServiceDelegate::AcceptConnection(Connection* connection) {
   return true;
 }
 
-bool NetworkServiceDelegate::ShellConnectionLost() {
-  return true;
-}
-
 void NetworkServiceDelegate::Create(Connection* connection,
                                     InterfaceRequest<NetworkService> request) {
   new NetworkServiceImpl(ref_factory_.CreateRef(), std::move(request));
diff --git a/mojo/services/network/network_service_delegate.h b/mojo/services/network/network_service_delegate.h
index 4159f2a..0c444ad 100644
--- a/mojo/services/network/network_service_delegate.h
+++ b/mojo/services/network/network_service_delegate.h
@@ -38,7 +38,6 @@ class NetworkServiceDelegate : public ShellClient,
   void Initialize(Connector* connector, const std::string& url,
                   uint32_t id, uint32_t user_id) override;
   bool AcceptConnection(Connection* connection) override;
-  bool ShellConnectionLost() override;
 
   // InterfaceFactory<NetworkService> implementation.
   void Create(Connection* connection,
diff --git a/mojo/services/tracing/tracing_app.cc b/mojo/services/tracing/tracing_app.cc
index f7885eb..65b4421 100644
--- a/mojo/services/tracing/tracing_app.cc
+++ b/mojo/services/tracing/tracing_app.cc
@@ -42,9 +42,8 @@ bool TracingApp::AcceptConnection(mojo::Connection* connection) {
   return true;
 }
 
-bool TracingApp::ShellConnectionLost() {
+void TracingApp::ShellConnectionLost() {
   base::MessageLoop::current()->QuitWhenIdle();
-  return true;
 }
 
 void TracingApp::Create(mojo::Connection* connection,
diff --git a/mojo/services/tracing/tracing_app.h b/mojo/services/tracing/tracing_app.h
index eb5934a..17f4132 100644
--- a/mojo/services/tracing/tracing_app.h
+++ b/mojo/services/tracing/tracing_app.h
@@ -34,7 +34,7 @@ class TracingApp
  private:
   // mojo::ShellClient implementation.
   bool AcceptConnection(mojo::Connection* connection) override;
-  bool ShellConnectionLost() override;
+  void ShellConnectionLost() override;
 
   // mojo::InterfaceFactory<TraceCollector> implementation.
   void Create(mojo::Connection* connection,
diff --git a/mojo/shell/public/cpp/lib/connector_impl.cc b/mojo/shell/public/cpp/lib/connector_impl.cc
index 2244fef..675017d 100644
--- a/mojo/shell/public/cpp/lib/connector_impl.cc
+++ b/mojo/shell/public/cpp/lib/connector_impl.cc
@@ -16,10 +16,8 @@ Connector::ConnectParams::~ConnectParams() {}
 
 ConnectorImpl::ConnectorImpl(shell::mojom::ConnectorPtrInfo unbound_state)
     : unbound_state_(std::move(unbound_state)) {}
-ConnectorImpl::ConnectorImpl(shell::mojom::ConnectorPtr connector,
-                             const base::Closure& connection_error_closure)
+ConnectorImpl::ConnectorImpl(shell::mojom::ConnectorPtr connector)
     : connector_(std::move(connector)) {
-  connector_.set_connection_error_handler(connection_error_closure);
   thread_checker_.reset(new base::ThreadChecker);
 }
 ConnectorImpl::~ConnectorImpl() {}
@@ -33,8 +31,12 @@ scoped_ptr<Connection> ConnectorImpl::Connect(ConnectParams* params) {
   // Bind this object to the current thread the first time it is used to
   // connect.
   if (!connector_.is_bound()) {
-    if (!unbound_state_.is_valid())
+    if (!unbound_state_.is_valid()) {
+      // It's possible to get here when the link to the shell has been severed
+      // (and so the connector pipe has been closed) but the app has chosen not
+      // to quit.
       return nullptr;
+    }
     connector_.Bind(std::move(unbound_state_));
     thread_checker_.reset(new base::ThreadChecker);
   }
diff --git a/mojo/shell/public/cpp/lib/connector_impl.h b/mojo/shell/public/cpp/lib/connector_impl.h
index 0e0a348..44f37b1 100644
--- a/mojo/shell/public/cpp/lib/connector_impl.h
+++ b/mojo/shell/public/cpp/lib/connector_impl.h
@@ -15,8 +15,7 @@ namespace mojo {
 class ConnectorImpl : public Connector {
  public:
   explicit ConnectorImpl(shell::mojom::ConnectorPtrInfo unbound_state);
-  ConnectorImpl(shell::mojom::ConnectorPtr connector,
-                const base::Closure& connection_error_closure);
+  explicit ConnectorImpl(shell::mojom::ConnectorPtr connector);
   ~ConnectorImpl() override;
 
  private:
diff --git a/mojo/shell/public/cpp/lib/shell_client.cc b/mojo/shell/public/cpp/lib/shell_client.cc
index 6c20b6d..a06c875 100644
--- a/mojo/shell/public/cpp/lib/shell_client.cc
+++ b/mojo/shell/public/cpp/lib/shell_client.cc
@@ -17,8 +17,6 @@ bool ShellClient::AcceptConnection(Connection* connection) {
   return false;
 }
 
-bool ShellClient::ShellConnectionLost() {
-  return true;
-}
+void ShellClient::ShellConnectionLost() {}
 
 }  // namespace mojo
diff --git a/mojo/shell/public/cpp/lib/shell_connection.cc b/mojo/shell/public/cpp/lib/shell_connection.cc
index 5649a18..ca7dd1f 100644
--- a/mojo/shell/public/cpp/lib/shell_connection.cc
+++ b/mojo/shell/public/cpp/lib/shell_connection.cc
@@ -38,10 +38,10 @@ void ShellConnection::Initialize(shell::mojom::ConnectorPtr connector,
                                  const mojo::String& name,
                                  uint32_t id,
                                  uint32_t user_id) {
-  connector_.reset(new ConnectorImpl(
-      std::move(connector),
+  connector_.reset(new ConnectorImpl(std::move(connector)));
+  binding_.set_connection_error_handler(
       base::Bind(&ShellConnection::OnConnectionError,
-                 weak_factory_.GetWeakPtr())));
+                 weak_factory_.GetWeakPtr()));
   client_->Initialize(connector_.get(), name, id, user_id);
 }
 
@@ -69,12 +69,12 @@ void ShellConnection::AcceptConnection(
 // ShellConnection, private:
 
 void ShellConnection::OnConnectionError() {
-  // We give the client notice first, since it might want to do something on
-  // shell connection errors other than immediate termination of the run
-  // loop. The application might want to continue servicing connections other
-  // than the one to the shell.
-  if (client_->ShellConnectionLost())
-    connector_.reset();
+  // Note that the ShellClient doesn't technically have to quit now, it may live
+  // on to service existing connections. All existing Connectors however are
+  // invalid.
+  client_->ShellConnectionLost();
+  // We don't reset the connector as clients may have taken a raw pointer to it.
+  // Connect() will return nullptr if they try to connect to anything.
 }
 
 }  // namespace mojo
diff --git a/mojo/shell/public/cpp/shell_client.h b/mojo/shell/public/cpp/shell_client.h
index 2eb9079..36b93874 100644
--- a/mojo/shell/public/cpp/shell_client.h
+++ b/mojo/shell/public/cpp/shell_client.h
@@ -42,11 +42,10 @@ class ShellClient {
   // underlying pipe closed. The default implementation returns false.
   virtual bool AcceptConnection(Connection* connection);
 
-  // Called when ShellConnection's pipe to the Mojo Shell is closed.
-  //
-  // Returning true from this method will cause ...
-  // The default implementation returns true.
-  virtual bool ShellConnectionLost();
+  // Called when ShellConnection's ShellClient binding (i.e. the pipe the
+  // Mojo Shell has to talk to us over) is closed. A shell client may use this
+  // as a signal to terminate.
+  virtual void ShellConnectionLost();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShellClient);
diff --git a/mojo/shell/tests/connect/connect_test_app.cc b/mojo/shell/tests/connect/connect_test_app.cc
index 1abdc08..5081c8b 100644
--- a/mojo/shell/tests/connect/connect_test_app.cc
+++ b/mojo/shell/tests/connect/connect_test_app.cc
@@ -66,12 +66,11 @@ class ConnectTestApp : public ShellClient,
 
     return true;
   }
-  bool ShellConnectionLost() override {
+  void ShellConnectionLost() override {
     if (base::MessageLoop::current() &&
         base::MessageLoop::current()->is_running()) {
       base::MessageLoop::current()->QuitWhenIdle();
     }
-    return true;
   }
 
   // InterfaceFactory<test::mojom::ConnectTestService>:
diff --git a/mojo/shell/tests/connect/connect_test_package.cc b/mojo/shell/tests/connect/connect_test_package.cc
index 0e435d1..91b6aa2 100644
--- a/mojo/shell/tests/connect/connect_test_package.cc
+++ b/mojo/shell/tests/connect/connect_test_package.cc
@@ -148,12 +148,11 @@ class ConnectTestShellClient
     connection->AddInterface<test::mojom::ConnectTestService>(this);
     return true;
   }
-  bool ShellConnectionLost() override {
+  void ShellConnectionLost() override {
     if (base::MessageLoop::current() &&
         base::MessageLoop::current()->is_running()) {
       base::MessageLoop::current()->QuitWhenIdle();
     }
-    return true;
   }
 
   // InterfaceFactory<mojom::ShellClientFactory>: