mojo: allow BackgroundServiceLoader-loaded apps to Quit themselves.

mojo/service_manager/background_shell_service_loader.cc

Index: mojo/service_manager/background_shell_service_loader.cc
diff --git a/mojo/service_manager/background_service_loader.cc b/mojo/service_manager/background_shell_service_loader.cc
similarity index 36%
rename from mojo/service_manager/background_service_loader.cc
rename to mojo/service_manager/background_shell_service_loader.cc
index 8c60a2cc00696df313e989933dcf63d6875d480f..a136a9510c90c99b65302944cef293ece4c0bbd6 100644
--- a/mojo/service_manager/background_service_loader.cc
+++ b/mojo/service_manager/background_shell_service_loader.cc
@@ -2,14 +2,22 @@
 // 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 "mojo/service_manager/service_manager.h"
 
 namespace mojo {
 
-class BackgroundServiceLoader::BackgroundLoader {
+namespace {
+
+void QuitLoop() {
+  base::MessageLoop::current()->Quit();

[sky, 2014/07/31 20:59:33]

Use a RunLoop.

[tim (not reviewing), 2014/07/31 22:17:28]

Done, assuming you meant Run() a RunLoop in BackgroundShellServiceLoader::Run()
from the spawned thread, stash a QuitClosure on the main thread that we Post to
the task_runner in the dtor...

[sky, 2014/08/04 18:56:25]

I was actually think you use the base::RunLoop you have on the stack at line 88
a member and Quit() it.

[tim (not reviewing), 2014/08/04 19:51:29]

I see. It'd have to be heap allocated pointer in that case, since ServiceLoader
is constructed on the shell thread.  I was trying to keep as many vars as
possible that live on the background thread scoped to Run(). In particular the
MessageLoop and RunLoop so we don't have to manage and poke at them from
multiple threads...

+}
+
+}  // namespace
+
+class BackgroundShellServiceLoader::BackgroundLoader {
  public:
   explicit BackgroundLoader(ServiceLoader* loader) : loader_(loader) {}
   ~BackgroundLoader() {}
@@ -25,81 +33,100 @@ 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),
+      loaded_service_(false),
+      loader_(real_loader.Pass()),
       message_loop_type_(message_loop_type),
+      message_loop_created_(true, false),
+      thread_(this, thread_name),
       background_loader_(NULL) {
+  // 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_.Start();
+  message_loop_created_.Wait();

[sky, 2014/07/31 20:59:33]

Why do we need to block here? Or for that matter start the thread?

[tim (not reviewing), 2014/07/31 22:17:28]

DelegateSimpleThread requires that it is both Started and Joined -- you can't
allocate one without Start()ing it. Once we start it, if we don't Wait then we
have to worry about the semi-initialized case all over the place, where we've
started the thread but haven't initialized task_runner_ and can't PostTask (for
another LoadService call, or in dtor). Blocking ensures anything can be safely
called and handled in a single way after construction.

I could make the DST member a scoped_ptr member, only allocate in LoadService,
and do the Wait() there. I could do-away with |loaded_service_| too.  Done.

+  DCHECK(task_runner_);
 }
 
-BackgroundServiceLoader::~BackgroundServiceLoader() {
-  if (thread_.IsRunning()) {
-    thread_.message_loop()->PostTask(
-        FROM_HERE,
-        base::Bind(&BackgroundServiceLoader::ShutdownOnBackgroundThread,
-                   base::Unretained(this)));
-  }
-  thread_.Stop();
+BackgroundShellServiceLoader::~BackgroundShellServiceLoader() {
+  if (quit_on_shutdown_ || !loaded_service_)
+    task_runner_->PostTask(FROM_HERE, base::Bind(&QuitLoop));
+  thread_.Join();
 }
 
-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));
-  }
-  thread_.message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&BackgroundServiceLoader::LoadServiceOnBackgroundThread,
+  loaded_service_ = true;
+  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() {
+  scoped_ptr<base::MessageLoop> message_loop;

[sky, 2014/07/31 20:59:33]

You should be able to use MessageLoop(type) here. All the subclasses are the
same size (there are compiler asserts to this effect).

[tim (not reviewing), 2014/07/31 22:17:28]

Ah.. forgot about that.  Thanks!

+  switch (message_loop_type_) {
+    case base::MessageLoop::TYPE_UI:
+      message_loop.reset(new base::MessageLoopForUI());
+      break;
+    case base::MessageLoop::TYPE_IO:
+      message_loop.reset(new base::MessageLoopForIO());
+      break;
+    case base::MessageLoop::TYPE_DEFAULT:
+      message_loop.reset(new base::MessageLoop());
+      break;
+    default:
+      NOTREACHED();
+  }
+  task_runner_ = message_loop->task_runner();
+  message_loop_created_.Signal();
+  message_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