mojo: allow BackgroundServiceLoader-loaded apps to Quit themselves.

You can't Quit() the MessageLoop of a base::Thread. This CL makes
BackgroundServiceLoader use DelegateSimpleThread and run a MessageLoop
instead of using base::Thread, so that applications can safely Quit()
their current MessageLoop as usual to signify that they are done.

This shouldn't be necessary for the desktop shell once services are moved
out (network, NVS), but we'll likely continue to use this loader on android.

Note: the quit_on_shutdown() method is being removed in https://codereview.chromium.org/394903005/

BUG=

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287475 0039d316-1c4b-4281-b951-d872f2087c98

mojo/mojo.gyp

@@ -344,8 +344,8 @@
         '<(mojo_system_for_component)',
       ],
       'sources': [
-        'service_manager/background_service_loader.cc',
-        'service_manager/background_service_loader.h',
+        'service_manager/background_shell_service_loader.cc',
+        'service_manager/background_shell_service_loader.h',
         'service_manager/service_loader.h',
         'service_manager/service_manager.cc',
         'service_manager/service_manager.h',
@@ -372,6 +372,7 @@
       ],
       'includes': [ 'public/tools/bindings/mojom_bindings_generator.gypi' ],
       'sources': [
+        'service_manager/background_shell_service_loader_unittest.cc',
         'service_manager/service_manager_unittest.cc',
         'service_manager/test.mojom',
       ],

mojo/service_manager/BUILD.gn

@@ -6,8 +6,8 @@
 component("service_manager") {
   output_name = "mojo_service_manager"
   sources = [
-    "background_service_loader.cc",
-    "background_service_loader.h",
+    "background_shell_service_loader.cc",
+    "background_shell_service_loader.h",
     "service_loader.h",
     "service_manager.cc",
     "service_manager.h",

mojo/service_manager/background_service_loader.cc → mojo/service_manager/background_shell_service_loader.cc

@@ -2,14 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "mojo/service_manager/background_service_loader.h"
+#include "mojo/service_manager/background_shell_service_loader.h"
 
 #include "base/bind.h"
+#include "base/run_loop.h"
 #include "mojo/service_manager/service_manager.h"
 
 namespace mojo {
 
-class BackgroundServiceLoader::BackgroundLoader {
+class BackgroundShellServiceLoader::BackgroundLoader {
  public:
   explicit BackgroundLoader(ServiceLoader* loader) : loader_(loader) {}
   ~BackgroundLoader() {}
@@ -25,81 +26,93 @@ class BackgroundServiceLoader::BackgroundLoader {
   }
 
  private:
-  base::MessageLoop::Type message_loop_type_;
-  ServiceLoader* loader_;  // Owned by BackgroundServiceLoader
+  ServiceLoader* loader_;  // Owned by BackgroundShellServiceLoader
 
   DISALLOW_COPY_AND_ASSIGN(BackgroundLoader);
 };
 
-BackgroundServiceLoader::BackgroundServiceLoader(
+BackgroundShellServiceLoader::BackgroundShellServiceLoader(
     scoped_ptr<ServiceLoader> real_loader,
-    const char* thread_name,
+    const std::string& thread_name,
     base::MessageLoop::Type message_loop_type)
-    : loader_(real_loader.Pass()),
-      thread_(thread_name),
+    : quit_on_shutdown_(false),
+      loader_(real_loader.Pass()),
       message_loop_type_(message_loop_type),
+      thread_name_(thread_name),
+      message_loop_created_(true, false),
       background_loader_(NULL) {
 }
 
-BackgroundServiceLoader::~BackgroundServiceLoader() {
-  if (thread_.IsRunning()) {
-    thread_.message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&BackgroundServiceLoader::ShutdownOnBackgroundThread,
-                   base::Unretained(this)));
+BackgroundShellServiceLoader::~BackgroundShellServiceLoader() {
+  if (thread_) {
+    if (quit_on_shutdown_)
+      task_runner_->PostTask(FROM_HERE, quit_closure_);
+    thread_->Join();
   }
-  thread_.Stop();
 }
 
-void BackgroundServiceLoader::LoadService(
+void BackgroundShellServiceLoader::LoadService(
     ServiceManager* manager,
     const GURL& url,
     ScopedMessagePipeHandle shell_handle) {
-  const int kDefaultStackSize = 0;
-  if (!thread_.IsRunning()) {
-    thread_.StartWithOptions(
-        base::Thread::Options(message_loop_type_, kDefaultStackSize));
+  if (!thread_) {
+    // TODO(tim): It'd be nice if we could just have each LoadService call
+    // result in a new thread like DynamicService{Loader, Runner}. But some
+    // loaders are creating multiple ApplicationImpls (NetworkServiceLoader)
+    // sharing a delegate (etc). So we have to keep it single threaded, wait
+    // for the thread to initialize, and post to the TaskRunner for subsequent
+    // LoadService calls for now.
+    thread_.reset(new base::DelegateSimpleThread(this, thread_name_));
+    thread_->Start();
+    message_loop_created_.Wait();
+    DCHECK(task_runner_);
   }
-  thread_.message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&BackgroundServiceLoader::LoadServiceOnBackgroundThread,
+
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(&BackgroundShellServiceLoader::LoadServiceOnBackgroundThread,
                  base::Unretained(this), manager, url,
                  base::Owned(
                     new ScopedMessagePipeHandle(shell_handle.Pass()))));
 }
 
-void BackgroundServiceLoader::OnServiceError(ServiceManager* manager,
-                                             const GURL& url) {
-  if (!thread_.IsRunning())
-    thread_.Start();
-  thread_.message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&BackgroundServiceLoader::OnServiceErrorOnBackgroundThread,
-                 base::Unretained(this), manager, url));
+void BackgroundShellServiceLoader::OnServiceError(
+    ServiceManager* manager, const GURL& url) {
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(
+          &BackgroundShellServiceLoader::OnServiceErrorOnBackgroundThread,
+          base::Unretained(this), manager, url));
 }
 
-void BackgroundServiceLoader::LoadServiceOnBackgroundThread(
-    ServiceManager* manager,
-    const GURL& url,
-    ScopedMessagePipeHandle* shell_handle) {
+void BackgroundShellServiceLoader::Run() {
+  base::MessageLoop message_loop(message_loop_type_);
+  base::RunLoop loop;
+  task_runner_ = message_loop.task_runner();
+  quit_closure_ = loop.QuitClosure();
+  message_loop_created_.Signal();
+  loop.Run();
+
+  delete background_loader_;
+  background_loader_ = NULL;
+  // Destroy |loader_| on the thread it's actually used on.
+  loader_.reset();
+}
+
+void BackgroundShellServiceLoader::LoadServiceOnBackgroundThread(
+      ServiceManager* manager,
+      const GURL& url,
+      ScopedMessagePipeHandle* shell_handle) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
   if (!background_loader_)
     background_loader_ = new BackgroundLoader(loader_.get());
-  background_loader_->LoadService(
-      manager, url, shell_handle->Pass());
+  background_loader_->LoadService(manager, url, shell_handle->Pass());
 }
 
-void BackgroundServiceLoader::OnServiceErrorOnBackgroundThread(
-    ServiceManager* manager,
-    const GURL& url) {
+void BackgroundShellServiceLoader::OnServiceErrorOnBackgroundThread(
+    ServiceManager* manager, const GURL& url) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
   if (!background_loader_)
     background_loader_ = new BackgroundLoader(loader_.get());
   background_loader_->OnServiceError(manager, url);
 }
 
-void BackgroundServiceLoader::ShutdownOnBackgroundThread() {
-  delete background_loader_;
-  // Destroy |loader_| on the thread it's actually used on.
-  loader_.reset();
-}
-
 }  // namespace mojo

mojo/service_manager/background_service_loader.h → mojo/service_manager/background_shell_service_loader.h

@@ -2,27 +2,28 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef MOJO_SERVICE_MANAGER_BACKGROUND_SERVICE_LOADER_H_
-#define MOJO_SERVICE_MANAGER_BACKGROUND_SERVICE_LOADER_H_
+#ifndef MOJO_SERVICE_MANAGER_BACKGROUND_SHELL_SERVICE_LOADER_H_
+#define MOJO_SERVICE_MANAGER_BACKGROUND_SHELL_SERVICE_LOADER_H_
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
-#include "base/threading/thread.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/threading/simple_thread.h"
 #include "mojo/service_manager/service_loader.h"
 
 namespace mojo {
 
-class ServiceManager;
-
-// ServiceLoader implementation that creates a background thread and issues load
-// requests there.
-class MOJO_SERVICE_MANAGER_EXPORT BackgroundServiceLoader
-    : public ServiceLoader {
+// TODO(tim): Eventually this should be Android-only to support services
+// that we need to bundle with the shell (such as NetworkService). Perhaps
+// we should move it to shell/ as well.
+class MOJO_SERVICE_MANAGER_EXPORT BackgroundShellServiceLoader :
+    public ServiceLoader,
+    public base::DelegateSimpleThread::Delegate {
  public:
-  BackgroundServiceLoader(scoped_ptr<ServiceLoader> real_loader,
-                          const char* thread_name,
-                          base::MessageLoop::Type message_loop_type);
-  virtual ~BackgroundServiceLoader();
+  BackgroundShellServiceLoader(scoped_ptr<ServiceLoader> real_loader,
+                               const std::string& thread_name,
+                               base::MessageLoop::Type message_loop_type);
+  virtual ~BackgroundShellServiceLoader();
 
   // ServiceLoader overrides:
   virtual void LoadService(ServiceManager* manager,
@@ -31,9 +32,18 @@ class MOJO_SERVICE_MANAGER_EXPORT BackgroundServiceLoader
   virtual void OnServiceError(ServiceManager* manager,
                               const GURL& url) OVERRIDE;
 
+  // Post a task to quit the MessageLoop we are running for the app when we
+  // are destroyed. Most apps should quit themselves and not need this.
+  // TODO(tim): Remove this method once applications are smart enough
+  // to quit themselves. Bug 394477.
+  void set_quit_on_shutdown() { quit_on_shutdown_ = true; }
+
  private:
   class BackgroundLoader;
 
+  // |base::DelegateSimpleThread::Delegate| method:
+  virtual void Run() OVERRIDE;
+
   // These functions are exected on the background thread. They call through
   // to |background_loader_| to do the actual loading.
   // TODO: having this code take a |manager| is fragile (as ServiceManager isn't
@@ -44,18 +54,30 @@ class MOJO_SERVICE_MANAGER_EXPORT BackgroundServiceLoader
       ScopedMessagePipeHandle* shell_handle);
   void OnServiceErrorOnBackgroundThread(ServiceManager* manager,
                                         const GURL& url);
-  void ShutdownOnBackgroundThread();
-
+  bool quit_on_shutdown_;
   scoped_ptr<ServiceLoader> loader_;
-  base::Thread thread_;
-  base::MessageLoop::Type message_loop_type_;
+
+  const base::MessageLoop::Type message_loop_type_;
+  const std::string thread_name_;
+
+  // Created on |thread_| during construction of |this|. Protected against
+  // uninitialized use by |message_loop_created_|, and protected against
+  // use-after-free by holding a reference to the thread-safe object. Note
+  // that holding a reference won't hold |thread_| from exiting.
+  scoped_refptr<base::TaskRunner> task_runner_;
+  base::WaitableEvent message_loop_created_;
+
+  // Lives on |thread_|.
+  base::Closure quit_closure_;
+
+  scoped_ptr<base::DelegateSimpleThread> thread_;
 
   // Lives on |thread_|. Trivial interface that calls through to |loader_|.
   BackgroundLoader* background_loader_;
 
-  DISALLOW_COPY_AND_ASSIGN(BackgroundServiceLoader);
+  DISALLOW_COPY_AND_ASSIGN(BackgroundShellServiceLoader);
 };
 
 }  // namespace mojo
 
-#endif  // MOJO_SERVICE_MANAGER_BACKGROUND_SERVICE_LOADER_H_
+#endif  // MOJO_SERVICE_MANAGER_BACKGROUND_SHELL_SERVICE_LOADER_H_

mojo/service_manager/background_shell_service_loader_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "mojo/service_manager/background_shell_service_loader.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+
+namespace {
+
+class DummyLoader : public ServiceLoader {
+ public:
+  DummyLoader() : simulate_app_quit_(true) {}
+  virtual ~DummyLoader() {}
+
+  // ServiceLoader overrides:
+  virtual void LoadService(ServiceManager* manager,
+                           const GURL& url,
+                           ScopedMessagePipeHandle shell_handle) OVERRIDE {
+    if (simulate_app_quit_)
+      base::MessageLoop::current()->Quit();
+  }
+
+  virtual void OnServiceError(ServiceManager* manager,
+                              const GURL& url) OVERRIDE {
+  }
+
+  void DontSimulateAppQuit() { simulate_app_quit_ = false; }
+
+ private:
+  bool simulate_app_quit_;
+};
+
+}  // namespace
+
+// Tests that the loader can start and stop gracefully.
+TEST(BackgroundShellServiceLoaderTest, StartStop) {
+  scoped_ptr<ServiceLoader> real_loader(new DummyLoader());
+  BackgroundShellServiceLoader loader(real_loader.Pass(), "test",
+                                      base::MessageLoop::TYPE_DEFAULT);
+}
+
+// Tests that the loader can load a service that is well behaved (quits
+// itself).
+TEST(BackgroundShellServiceLoaderTest, Load) {
+  scoped_ptr<ServiceLoader> real_loader(new DummyLoader());
+  BackgroundShellServiceLoader loader(real_loader.Pass(), "test",
+                                      base::MessageLoop::TYPE_DEFAULT);
+  MessagePipe dummy;
+  loader.LoadService(NULL, GURL(), dummy.handle0.Pass());
+}
+
+// Test that an app that doesn't quit itself can still be handled.
+// TODO(tim): Remove this.
+TEST(BackgroundShellServiceLoaderTest, LoadMisbehavedService) {
+  scoped_ptr<DummyLoader> real_loader(new DummyLoader());
+  real_loader->DontSimulateAppQuit();
+  BackgroundShellServiceLoader loader(
+      real_loader.PassAs<ServiceLoader>(), "test",
+      base::MessageLoop::TYPE_DEFAULT);
+  // Because this app is mis-behaved (doesn't quit itself), we need to
+  // explicitly kill the thread.
+  loader.set_quit_on_shutdown();
+  MessagePipe dummy;
+  loader.LoadService(NULL, GURL(), dummy.handle0.Pass());
+}
+
+}  // namespace mojo

mojo/service_manager/service_manager_unittest.cc

@@ -10,7 +10,7 @@
 #include "mojo/public/cpp/application/application_impl.h"
 #include "mojo/public/cpp/application/interface_factory.h"
 #include "mojo/public/interfaces/application/service_provider.mojom.h"
-#include "mojo/service_manager/background_service_loader.h"
+#include "mojo/service_manager/background_shell_service_loader.h"
 #include "mojo/service_manager/service_loader.h"
 #include "mojo/service_manager/service_manager.h"
 #include "mojo/service_manager/test.mojom.h"
@@ -251,7 +251,11 @@ class TestAImpl : public InterfaceImpl<TestA> {
       : test_context_(test_context) {
     connection->ConnectToApplication(kTestBURLString)->ConnectToService(&b_);
   }
-  virtual ~TestAImpl() { test_context_->IncrementNumADeletes(); }
+  virtual ~TestAImpl() {
+    test_context_->IncrementNumADeletes();
+    if (base::MessageLoop::current()->is_running())
+      Quit();
+  }
 
  private:
   virtual void CallB() OVERRIDE {
@@ -263,6 +267,7 @@ class TestAImpl : public InterfaceImpl<TestA> {
   }
 
   void Quit() {
+    base::MessageLoop::current()->Quit();
     test_context_->set_a_called_quit();
     test_context_->QuitSoon();
   }
@@ -280,6 +285,8 @@ class TestBImpl : public InterfaceImpl<TestB> {
 
   virtual ~TestBImpl() {
     test_context_->IncrementNumBDeletes();
+    if (base::MessageLoop::current()->is_running())
+      base::MessageLoop::current()->Quit();
     test_context_->QuitSoon();
   }
 
@@ -341,6 +348,7 @@ class Tester : public ApplicationDelegate,
           requestor_url_ != connection->GetRemoteApplicationURL()) {
       context_->set_tester_called_quit();
       context_->QuitSoon();
+      base::MessageLoop::current()->Quit();
       return false;
     }
     // If we're coming from A, then add B, otherwise A.
@@ -431,13 +439,19 @@ class ServiceManagerTest : public testing::Test {
     service_manager_.reset(NULL);
   }
 
-  void AddLoaderForURL(const GURL& url, const std::string& requestor_url) {
+  scoped_ptr<BackgroundShellServiceLoader> MakeLoader(
+      const std::string& requestor_url) {
     scoped_ptr<ServiceLoader> real_loader(
         new Tester(&tester_context_, requestor_url));
+    scoped_ptr<BackgroundShellServiceLoader> loader(
+        new BackgroundShellServiceLoader(real_loader.Pass(), std::string(),
+                                         base::MessageLoop::TYPE_DEFAULT));
+    return loader.Pass();
+  }
+
+  void AddLoaderForURL(const GURL& url, const std::string& requestor_url) {
     service_manager_->SetLoaderForURL(
-        scoped_ptr<ServiceLoader>(new BackgroundServiceLoader(
-            real_loader.Pass(), "", base::MessageLoop::TYPE_DEFAULT)),
-        url);
+        MakeLoader(requestor_url).PassAs<ServiceLoader>(), url);
   }
 
   bool HasFactoryForTestURL() {
@@ -598,11 +612,20 @@ TEST_F(ServiceManagerTest, ANoLoadB) {
   a->CallB();
   loop_.Run();
   EXPECT_EQ(0, tester_context_.num_b_calls());
+
+  EXPECT_FALSE(tester_context_.a_called_quit());
   EXPECT_TRUE(tester_context_.tester_called_quit());
 }
 
 TEST_F(ServiceManagerTest, NoServiceNoLoad) {
-  AddLoaderForURL(GURL(kTestAURLString), std::string());
+  // Because we'll never successfully connect to anything here and apps are not
+  // capable of noticing zero incoming connections and quitting, we need to use
+  // a quittable loader.
+  scoped_ptr<BackgroundShellServiceLoader> loader(MakeLoader(std::string()));
+  loader->set_quit_on_shutdown();
+  service_manager_->SetLoaderForURL(loader.PassAs<ServiceLoader>(),
+                                    GURL(kTestAURLString));
+
 
   // There is no TestC service implementation registered with ServiceManager,
   // so this cannot succeed (but also shouldn't crash).

mojo/shell/context.cc

@@ -11,7 +11,7 @@
 #include "mojo/embedder/embedder.h"
 #include "mojo/gles2/gles2_support_impl.h"
 #include "mojo/public/cpp/application/application_impl.h"
-#include "mojo/service_manager/background_service_loader.h"
+#include "mojo/service_manager/background_shell_service_loader.h"
 #include "mojo/service_manager/service_loader.h"
 #include "mojo/service_manager/service_manager.h"
 #include "mojo/services/native_viewport/native_viewport_service.h"
@@ -116,13 +116,18 @@ void Context::Init() {
               this)),
       GURL("mojo:mojo_native_viewport_service"));
 #else
-  service_manager_.SetLoaderForURL(
-      scoped_ptr<ServiceLoader>(
-          new BackgroundServiceLoader(
-              scoped_ptr<ServiceLoader>(new NativeViewportServiceLoader()),
-              "native_viewport",
-              base::MessageLoop::TYPE_UI)),
-      GURL("mojo:mojo_native_viewport_service"));
+  {
+    scoped_ptr<BackgroundShellServiceLoader> loader(
+        new BackgroundShellServiceLoader(
+            scoped_ptr<ServiceLoader>(new NativeViewportServiceLoader()),
+            "native_viewport",
+             base::MessageLoop::TYPE_UI));
+    // TODO(tim): NativeViewportService doesn't quit itself yet.
+    loader->set_quit_on_shutdown();
+    service_manager_.SetLoaderForURL(
+        loader.PassAs<ServiceLoader>(),
+        GURL("mojo:mojo_native_viewport_service"));
+  }
 #endif
 #if defined(USE_AURA)
   // TODO(sky): need a better way to find this. It shouldn't be linked in.
@@ -145,13 +150,17 @@ void Context::Init() {
 #if defined(OS_ANDROID)
   // On android, the network service is bundled with the shell because the
   // network stack depends on the android runtime.
-  service_manager_.SetLoaderForURL(
-      scoped_ptr<ServiceLoader>(
-          new BackgroundServiceLoader(
-              scoped_ptr<ServiceLoader>(new NetworkServiceLoader()),
-              "network_service",
-              base::MessageLoop::TYPE_IO)),
-      GURL("mojo:mojo_network_service"));
+  {
+    scoped_ptr<BackgroundShellServiceLoader> loader(
+        new BackgroundShellServiceLoader(
+            scoped_ptr<ServiceLoader>(new NetworkServiceLoader()),
+            "network_service",
+             base::MessageLoop::TYPE_IO));
+    // TODO(tim): NetworkService doesn't quit itself yet.
+    loader->set_quit_on_shutdown();
+    service_manager_.SetLoaderForURL(loader.PassAs<ServiceLoader>(),
+                                     GURL("mojo:mojo_network_service"));
+  }
 #endif
 }