mojo: fix little bug in DynamicServiceLoader/Runner

If the Runner wasn't actually told to Start(), the DelegateSimpleThread would
DCHECK for not having been Started. This patch makes it so we don't allocate
a DST unless we need to.

Also adds basic unittest files for DynamicServiceLoader and Runner.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285650

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

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

mojo/mojo.gyp

@@ -690,6 +690,8 @@
       ],
       'sources': [
         'shell/child_process_host_unittest.cc',
+        'shell/dynamic_service_loader_unittest.cc',
+        'shell/in_process_dynamic_service_runner_unittest.cc',
         'shell/shell_test_base.cc',
         'shell/shell_test_base.h',
         'shell/shell_test_base_unittest.cc',

mojo/shell/dynamic_service_loader.cc

@@ -67,8 +67,6 @@ class LocalLoader : public Loader {
     base::FilePath path;
     net::FileURLToFilePath(url, &path);
 
-    // TODO(darin): Check if the given file path exists.
-
     // Complete asynchronously for consistency with NetworkServiceLoader.
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
@@ -76,7 +74,7 @@ class LocalLoader : public Loader {
                    base::Unretained(this),
                    path,
                    base::Passed(&service_handle),
-                   true));
+                   base::PathExists(path)));
   }
 };
 

mojo/shell/dynamic_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 "base/files/scoped_temp_dir.h"
+#include "mojo/shell/context.h"
+#include "mojo/shell/dynamic_service_loader.h"
+#include "mojo/shell/dynamic_service_runner.h"
+#include "net/base/filename_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace shell {
+
+namespace {
+
+struct TestState {
+  TestState() : runner_was_started(false), runner_was_destroyed(false) {}
+  bool runner_was_started;
+  bool runner_was_destroyed;
+};
+
+class TestDynamicServiceRunner : public DynamicServiceRunner {
+ public:
+  explicit TestDynamicServiceRunner(TestState* state) : state_(state) {}
+  virtual ~TestDynamicServiceRunner() {
+    state_->runner_was_destroyed = true;
+    base::MessageLoop::current()->Quit();
+  }
+  virtual void Start(const base::FilePath& app_path,
+                     ScopedMessagePipeHandle service_handle,
+                     const base::Closure& app_completed_callback) OVERRIDE {
+    state_->runner_was_started = true;
+  }
+ private:
+  TestState* state_;
+};
+
+class TestDynamicServiceRunnerFactory : public DynamicServiceRunnerFactory {
+ public:
+  explicit TestDynamicServiceRunnerFactory(TestState* state) : state_(state) {}
+  virtual ~TestDynamicServiceRunnerFactory() {}
+  virtual scoped_ptr<DynamicServiceRunner> Create(Context* context) OVERRIDE {
+    return scoped_ptr<DynamicServiceRunner>(
+        new TestDynamicServiceRunner(state_));
+  }
+ private:
+  TestState* state_;
+};
+
+}  // namespace
+
+class DynamicServiceLoaderTest : public testing::Test {
+ public:
+  DynamicServiceLoaderTest() {}
+  virtual ~DynamicServiceLoaderTest() {}
+  virtual void SetUp() OVERRIDE {
+    scoped_ptr<DynamicServiceRunnerFactory> factory(
+        new TestDynamicServiceRunnerFactory(&state_));
+    loader_.reset(new DynamicServiceLoader(&context_, factory.Pass()));
+  }
+ protected:
+  base::MessageLoop loop_;
+  Context context_;
+  scoped_ptr<DynamicServiceLoader> loader_;
+  TestState state_;
+};
+
+TEST_F(DynamicServiceLoaderTest, DoesNotExist) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  base::FilePath nonexistent_file(FILE_PATH_LITERAL("nonexistent.txt"));
+  GURL url(net::FilePathToFileURL(temp_dir.path().Append(nonexistent_file)));
+  MessagePipe pipe;
+  loader_->LoadService(context_.service_manager(), url, pipe.handle0.Pass());
+  loop_.Run();
+  EXPECT_FALSE(state_.runner_was_started);
+  EXPECT_TRUE(state_.runner_was_destroyed);
+}
+
+}  // namespace shell
+}  // namespace mojo

mojo/shell/in_process_dynamic_service_runner.cc

@@ -16,14 +16,14 @@ namespace shell {
 
 InProcessDynamicServiceRunner::InProcessDynamicServiceRunner(
     Context* context)
-    : keep_alive_(context),
-      thread_(this, "app_thread") {
+    : keep_alive_(context) {
 }
 
 InProcessDynamicServiceRunner::~InProcessDynamicServiceRunner() {
-  if (thread_.HasBeenStarted()) {
-    DCHECK(!thread_.HasBeenJoined());
-    thread_.Join();
+  if (thread_) {
+    DCHECK(thread_->HasBeenStarted());
+    DCHECK(!thread_->HasBeenJoined());
+    thread_->Join();
   }
 
   // It is important to let the thread exit before unloading the DSO because
@@ -47,8 +47,9 @@ void InProcessDynamicServiceRunner::Start(
                                               FROM_HERE,
                                               app_completed_callback);
 
-  DCHECK(!thread_.HasBeenStarted());
-  thread_.Start();
+  DCHECK(!thread_);
+  thread_.reset(new base::DelegateSimpleThread(this, "app_thread"));
+  thread_->Start();
 }
 
 void InProcessDynamicServiceRunner::Run() {

mojo/shell/in_process_dynamic_service_runner.h

@@ -40,7 +40,7 @@ class InProcessDynamicServiceRunner
   base::Callback<bool(void)> app_completed_callback_runner_;
 
   base::ScopedNativeLibrary app_library_;
-  base::DelegateSimpleThread thread_;
+  scoped_ptr<base::DelegateSimpleThread> thread_;
 
   DISALLOW_COPY_AND_ASSIGN(InProcessDynamicServiceRunner);
 };

mojo/shell/in_process_dynamic_service_runner_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/shell/context.h"
+#include "mojo/shell/in_process_dynamic_service_runner.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace shell {
+
+TEST(InProcessDynamicServiceRunnerTest, NotStarted) {
+  base::MessageLoop loop;
+  Context context;
+  InProcessDynamicServiceRunner runner(&context);
+  // Shouldn't crash or DCHECK on destruction.
+}
+
+}  // namespace shell
+}  // namespace mojo