Quit the message loop by default in ShellConnectionLost when ApplicationRunner is used

This was originally at https://codereview.chromium.org/1814223002/ , but Ben is out and
I need this for tests, so I'm taking over.

BUG=none
TEST=covered by tests
TBR=ben@chromium.org
R=ben@chromium.org

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

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

15 files changed, 34 insertions, 34 deletions

diff --git a/mojo/services/network/network_service_delegate.cc b/mojo/services/network/network_service_delegate.cc
index 1970928..b257331 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;
 }
 
-void NetworkServiceDelegate::ShellConnectionLost() {
-  base::MessageLoop::current()->QuitWhenIdle();
-}
-
 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 8997097..7098939 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 Identity& identity,
                   uint32_t id) override;
   bool AcceptConnection(Connection* connection) override;
-  void 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 65b4421..d921a6d 100644
--- a/mojo/services/tracing/tracing_app.cc
+++ b/mojo/services/tracing/tracing_app.cc
@@ -42,8 +42,12 @@ bool TracingApp::AcceptConnection(mojo::Connection* connection) {
   return true;
 }
 
-void TracingApp::ShellConnectionLost() {
+bool TracingApp::ShellConnectionLost() {
+  // TODO(beng): This is only required because TracingApp isn't run by
+  // ApplicationRunner - instead it's launched automatically by the standalone
+  // shell. It shouldn't be.
   base::MessageLoop::current()->QuitWhenIdle();
+  return false;
 }
 
 void TracingApp::Create(mojo::Connection* connection,
diff --git a/mojo/services/tracing/tracing_app.h b/mojo/services/tracing/tracing_app.h
index 17f4132..eb5934a 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;
-  void ShellConnectionLost() override;
+  bool ShellConnectionLost() override;
 
   // mojo::InterfaceFactory<TraceCollector> implementation.
   void Create(mojo::Connection* connection,
diff --git a/mojo/shell/background/tests/test_service.cc b/mojo/shell/background/tests/test_service.cc
index d030a81..e69640d 100644
--- a/mojo/shell/background/tests/test_service.cc
+++ b/mojo/shell/background/tests/test_service.cc
@@ -25,8 +25,8 @@ class TestClient : public ShellClient,
     connection->AddInterface(this);
     return true;
   }
-  void ShellConnectionLost() override {
-    base::MessageLoop::current()->QuitWhenIdle();
+  bool ShellConnectionLost() override {
+    return true;
   }
 
   // InterfaceFactory<mojom::TestService>:
diff --git a/mojo/shell/public/cpp/lib/application_runner.cc b/mojo/shell/public/cpp/lib/application_runner.cc
index 5253053..201aa62 100644
--- a/mojo/shell/public/cpp/lib/application_runner.cc
+++ b/mojo/shell/public/cpp/lib/application_runner.cc
@@ -5,10 +5,12 @@
 #include "mojo/shell/public/cpp/application_runner.h"
 
 #include "base/at_exit.h"
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/process/launch.h"
+#include "base/run_loop.h"
 #include "mojo/shell/public/cpp/shell_client.h"
 #include "mojo/shell/public/cpp/shell_connection.h"
 
@@ -54,7 +56,9 @@ MojoResult ApplicationRunner::Run(MojoHandle shell_client_request_handle,
         client_.get(),
         MakeRequest<shell::mojom::ShellClient>(MakeScopedHandle(
             MessagePipeHandle(shell_client_request_handle)))));
-    loop->Run();
+    base::RunLoop run_loop;
+    connection_->set_connection_lost_closure(run_loop.QuitClosure());
+    run_loop.Run();
     // It's very common for the client to cache the app and terminate on errors.
     // If we don't delete the client before the app we run the risk of the
     // client having a stale reference to the app and trying to use it.
diff --git a/mojo/shell/public/cpp/lib/shell_client.cc b/mojo/shell/public/cpp/lib/shell_client.cc
index c41999b..3ca8cd7 100644
--- a/mojo/shell/public/cpp/lib/shell_client.cc
+++ b/mojo/shell/public/cpp/lib/shell_client.cc
@@ -17,6 +17,6 @@ bool ShellClient::AcceptConnection(Connection* connection) {
   return false;
 }
 
-void ShellClient::ShellConnectionLost() {}
+bool ShellClient::ShellConnectionLost() { return true; }
 
 }  // namespace mojo
diff --git a/mojo/shell/public/cpp/lib/shell_connection.cc b/mojo/shell/public/cpp/lib/shell_connection.cc
index d1cba7e..62ae598 100644
--- a/mojo/shell/public/cpp/lib/shell_connection.cc
+++ b/mojo/shell/public/cpp/lib/shell_connection.cc
@@ -86,7 +86,8 @@ void ShellConnection::OnConnectionError() {
   // 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();
+  if (client_->ShellConnectionLost() && !connection_lost_closure_.is_null())
+    connection_lost_closure_.Run();
   // 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.
 }
diff --git a/mojo/shell/public/cpp/shell_client.h b/mojo/shell/public/cpp/shell_client.h
index d6fca12..aff1aac 100644
--- a/mojo/shell/public/cpp/shell_client.h
+++ b/mojo/shell/public/cpp/shell_client.h
@@ -43,8 +43,13 @@ class ShellClient {
 
   // 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();
+  // as a signal to terminate. Return true from this method to tell the
+  // ShellConnection to run its connection lost closure if it has one, false to
+  // prevent it from being run. The default implementation returns true.
+  // When used in conjunction with ApplicationRunner, returning true here quits
+  // the message loop created by ApplicationRunner, which results in the app
+  // quitting.
+  virtual bool ShellConnectionLost();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShellClient);
diff --git a/mojo/shell/public/cpp/shell_connection.h b/mojo/shell/public/cpp/shell_connection.h
index a05f6f1..3e00cd3 100644
--- a/mojo/shell/public/cpp/shell_connection.h
+++ b/mojo/shell/public/cpp/shell_connection.h
@@ -61,7 +61,12 @@ class ShellConnection : public shell::mojom::ShellClient {
   // TODO(rockot): Remove this once we get rid of app tests.
   void SetAppTestConnectorForTesting(shell::mojom::ConnectorPtr connector);
 
- private:
+  // Specify a function to be called when the connection to the shell is lost.
+  void set_connection_lost_closure(const base::Closure& closure) {
+    connection_lost_closure_ = closure;
+  }
+
+private:
   // shell::mojom::ShellClient:
   void Initialize(shell::mojom::IdentityPtr identity,
                   uint32_t id,
@@ -90,6 +95,8 @@ class ShellConnection : public shell::mojom::ShellClient {
   Binding<shell::mojom::ShellClient> binding_;
   scoped_ptr<Connector> connector_;
 
+  base::Closure connection_lost_closure_;
+
   DISALLOW_COPY_AND_ASSIGN(ShellConnection);
 };
 
diff --git a/mojo/shell/tests/connect/connect_test_app.cc b/mojo/shell/tests/connect/connect_test_app.cc
index 342e14d..4180763 100644
--- a/mojo/shell/tests/connect/connect_test_app.cc
+++ b/mojo/shell/tests/connect/connect_test_app.cc
@@ -79,12 +79,6 @@ class ConnectTestApp : public ShellClient,
 
     return true;
   }
-  void ShellConnectionLost() override {
-    if (base::MessageLoop::current() &&
-        base::MessageLoop::current()->is_running()) {
-      base::MessageLoop::current()->QuitWhenIdle();
-    }
-  }
 
   // InterfaceFactory<test::mojom::ConnectTestService>:
   void Create(Connection* connection,
diff --git a/mojo/shell/tests/connect/connect_test_driver.cc b/mojo/shell/tests/connect/connect_test_driver.cc
index ace64df..b1bdce4 100644
--- a/mojo/shell/tests/connect/connect_test_driver.cc
+++ b/mojo/shell/tests/connect/connect_test_driver.cc
@@ -37,8 +37,9 @@ class Driver : public mojo::ShellClient,
     connection->AddInterface<ClientProcessTest>(this);
     return true;
   }
-  void ShellConnectionLost() override {
-    // TODO: This should exit cleanly.
+  bool ShellConnectionLost() override {
+    // TODO(rockot): http://crbug.com/596621. Should be able to remove this
+    // override entirely.
     _exit(1);
   }
 
diff --git a/mojo/shell/tests/connect/connect_test_package.cc b/mojo/shell/tests/connect/connect_test_package.cc
index c5a1a07..1458e65 100644
--- a/mojo/shell/tests/connect/connect_test_package.cc
+++ b/mojo/shell/tests/connect/connect_test_package.cc
@@ -184,12 +184,6 @@ class ConnectTestShellClient
     connection->AddInterface<test::mojom::ConnectTestService>(this);
     return true;
   }
-  void ShellConnectionLost() override {
-    if (base::MessageLoop::current() &&
-        base::MessageLoop::current()->is_running()) {
-      base::MessageLoop::current()->QuitWhenIdle();
-    }
-  }
 
   // InterfaceFactory<mojom::ShellClientFactory>:
   void Create(Connection* connection,
diff --git a/mojo/shell/tests/lifecycle/app_client.cc b/mojo/shell/tests/lifecycle/app_client.cc
index a2400ec..657d967 100644
--- a/mojo/shell/tests/lifecycle/app_client.cc
+++ b/mojo/shell/tests/lifecycle/app_client.cc
@@ -20,10 +20,6 @@ bool AppClient::AcceptConnection(mojo::Connection* connection) {
   return true;
 }
 
-void AppClient::ShellConnectionLost() {
-  GracefulQuit();
-}
-
 void AppClient::Create(mojo::Connection* connection,
                        LifecycleControlRequest request) {
   bindings_.AddBinding(this, std::move(request));
diff --git a/mojo/shell/tests/lifecycle/app_client.h b/mojo/shell/tests/lifecycle/app_client.h
index d389f20..5a7d5e6 100644
--- a/mojo/shell/tests/lifecycle/app_client.h
+++ b/mojo/shell/tests/lifecycle/app_client.h
@@ -37,7 +37,6 @@ class AppClient : public ShellClient,
 
   // ShellClient:
   bool AcceptConnection(Connection* connection) override;
-  void ShellConnectionLost() override;
 
   // InterfaceFactory<LifecycleControl>:
   void Create(Connection* connection, LifecycleControlRequest request) override;