Revert of Remove loader for catalog & some other cleanup (patchset #4 id:60001 of https://codereview.chromium.org/1785743002/ )

Reason for revert:
Based on https://codereview.chromium.org/1781913003 which broke mojo_shell_unittests on Linux.

Original issue's description:
> Remove loader for catalog & some other cleanup:
>
> - instantiate the catalog shell client directly in the shell
> - remove some obsolete comments
> - move userid inheritance handling into the instance
>
> R=sky@chromium.org
>
> Committed: https://crrev.com/ecae0ae5f672a04579ab563462531889f6c0fab9
> Cr-Commit-Position: refs/heads/master@{#380575}

TBR=sky@chromium.org,ben@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

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

11 files changed, 114 insertions, 27 deletions

diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp
index 5474664..213857b 100644
--- a/mojo/mojo_shell.gyp
+++ b/mojo/mojo_shell.gyp
@@ -13,6 +13,8 @@
       'services/catalog/catalog.h',
       'services/catalog/entry.cc',
       'services/catalog/entry.h',
+      'services/catalog/loader.cc',
+      'services/catalog/loader.h',
       'services/catalog/store.cc',
       'services/catalog/store.h',
       'shell/loader.h',
diff --git a/mojo/services/catalog/BUILD.gn b/mojo/services/catalog/BUILD.gn
index f1eb63d..633f6d1 100644
--- a/mojo/services/catalog/BUILD.gn
+++ b/mojo/services/catalog/BUILD.gn
@@ -21,6 +21,8 @@ source_set("lib") {
     "catalog.h",
     "entry.cc",
     "entry.h",
+    "loader.cc",
+    "loader.h",
     "store.cc",
     "store.h",
   ]
diff --git a/mojo/services/catalog/loader.cc b/mojo/services/catalog/loader.cc
new file mode 100644
index 0000000..7f1fccb
--- /dev/null
+++ b/mojo/services/catalog/loader.cc
@@ -0,0 +1,25 @@
+// Copyright 2016 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/services/catalog/loader.h"
+
+#include "mojo/services/catalog/catalog.h"
+#include "mojo/shell/public/cpp/shell_client.h"
+#include "mojo/shell/public/cpp/shell_connection.h"
+
+namespace catalog {
+
+Loader::Loader(base::TaskRunner* blocking_pool, scoped_ptr<Store> store)
+    : blocking_pool_(blocking_pool),
+      store_(std::move(store)) {}
+Loader::~Loader() {}
+
+void Loader::Load(const std::string& name,
+                  mojo::shell::mojom::ShellClientRequest request) {
+  client_.reset(new catalog::Catalog(blocking_pool_, std::move(store_)));
+  connection_.reset(new mojo::ShellConnection(client_.get(),
+                                              std::move(request)));
+}
+
+}  // namespace catalog
diff --git a/mojo/services/catalog/loader.h b/mojo/services/catalog/loader.h
new file mode 100644
index 0000000..ad02411
--- /dev/null
+++ b/mojo/services/catalog/loader.h
@@ -0,0 +1,45 @@
+// Copyright 2016 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.
+
+#ifndef MOJO_SERVICES_CATALOG_LOADER_H_
+#define MOJO_SERVICES_CATALOG_LOADER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "mojo/services/catalog/store.h"
+#include "mojo/shell/loader.h"
+
+namespace base {
+class BlockingPool;
+}
+
+namespace mojo {
+class ShellClient;
+class ShellConnection;
+}
+
+namespace catalog {
+
+class Store;
+
+class Loader : public mojo::shell::Loader {
+ public:
+  Loader(base::TaskRunner* blocking_pool, scoped_ptr<Store> store);
+  ~Loader() override;
+
+  // mojo::shell::Loader:
+  void Load(const std::string& name,
+            mojo::shell::mojom::ShellClientRequest request) override;
+
+ private:
+  base::TaskRunner* blocking_pool_;
+  scoped_ptr<Store> store_;
+  scoped_ptr<mojo::ShellClient> client_;
+  scoped_ptr<mojo::ShellConnection> connection_;
+
+  DISALLOW_COPY_AND_ASSIGN(Loader);
+};
+
+}  // namespace catalog
+
+#endif  // MOJO_SERVICES_CATALOG_LOADER_H_
diff --git a/mojo/shell/public/cpp/lib/connector_impl.cc b/mojo/shell/public/cpp/lib/connector_impl.cc
index 7ff56d4..98a7ae5 100644
--- a/mojo/shell/public/cpp/lib/connector_impl.cc
+++ b/mojo/shell/public/cpp/lib/connector_impl.cc
@@ -46,6 +46,9 @@ scoped_ptr<Connection> ConnectorImpl::Connect(ConnectParams* params) {
   DCHECK(params);
   // We allow all interfaces on outgoing connections since we are presumably in
   // a position to know who we're talking to.
+  // TODO(beng): We should filter outgoing interfaces also. The shell must pass
+  //             the manifest CapabilityFilter to the ShellConnection via
+  //             Initialize(), it can be used here.
   CapabilityRequest request;
   request.interfaces.insert("*");
   shell::mojom::InterfaceProviderPtr local_interfaces;
diff --git a/mojo/shell/runner/host/child_process_base.cc b/mojo/shell/runner/host/child_process_base.cc
index 538cf7d..d4d6870 100644
--- a/mojo/shell/runner/host/child_process_base.cc
+++ b/mojo/shell/runner/host/child_process_base.cc
@@ -106,6 +106,7 @@ class AppContext : public edk::ProcessDelegate {
     io_runner_ = io_thread_.task_runner().get();
     CHECK(io_runner_.get());
 
+    // TODO(vtl): This should be SLAVE, not NONE.
     // This must be created before controller_thread_ since MessagePumpMojo will
     // create a message pipe which requires this code to be run first.
     edk::InitIPCSupport(this, io_runner_);
diff --git a/mojo/shell/runner/host/native_application_support.h b/mojo/shell/runner/host/native_application_support.h
index d7ce8a2..2371f65 100644
--- a/mojo/shell/runner/host/native_application_support.h
+++ b/mojo/shell/runner/host/native_application_support.h
@@ -32,6 +32,7 @@ base::NativeLibrary LoadNativeApplication(const base::FilePath& app_path);
 // should be called on the same thread as |LoadNativeApplication()|. Returns
 // true if |MojoMain()| was called (even if it returns an error), and false
 // otherwise.
+// TODO(vtl): Maybe this should also have a |MojoResult| as an out parameter?
 bool RunNativeApplication(base::NativeLibrary app_library,
                           InterfaceRequest<mojom::ShellClient> request);
 
diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc
index ceb25e6..e5c46b3 100644
--- a/mojo/shell/shell.cc
+++ b/mojo/shell/shell.cc
@@ -21,9 +21,9 @@
 #include "mojo/common/url_type_converters.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
-#include "mojo/services/catalog/catalog.h"
+#include "mojo/services/catalog/loader.h"
 #include "mojo/shell/connect_util.h"
-#include "mojo/shell/public/cpp/connector.h"
+#include "mojo/shell/public/cpp/connect.h"
 #include "mojo/shell/public/cpp/names.h"
 #include "mojo/shell/public/cpp/shell_connection.h"
 #include "mojo/shell/public/interfaces/connector.mojom.h"
@@ -35,7 +35,6 @@ namespace mojo {
 namespace shell {
 namespace {
 const char kCatalogName[] = "mojo:catalog";
-const char kShellName[] = "mojo:shell";
 
 void EmptyResolverCallback(const String& resolved_name,
                            const String& resolved_instance,
@@ -45,7 +44,7 @@ void EmptyResolverCallback(const String& resolved_name,
 }
 
 Identity CreateShellIdentity() {
-  return Identity(kShellName, mojom::kRootUserID);
+  return Identity("mojo:shell", mojom::kRootUserID);
 }
 
 CapabilitySpec GetPermissiveCapabilities() {
@@ -110,7 +109,7 @@ class Shell::Instance : public mojom::Connector,
       shell_client_(std::move(shell_client)),
       pid_receiver_binding_(this),
       weak_factory_(this) {
-    if (identity_.name() == kShellName ||
+    if (identity_.name() == "mojo:shell" ||
         shell_->GetLoaderForName(identity_.name())) {
       pid_ = base::Process::Current().Pid();
     }
@@ -208,9 +207,6 @@ class Shell::Instance : public mojom::Connector,
     if (!ValidateCapabilities(target, callback))
       return;
 
-    if (target.user_id() == mojom::kInheritUserID)
-      target.set_user_id(identity_.user_id());
-
     scoped_ptr<ConnectParams> params(new ConnectParams);
     params->set_source(identity_);
     params->set_target(target);
@@ -356,7 +352,6 @@ Shell::Shell(scoped_ptr<NativeRunnerFactory> native_runner_factory,
   mojom::ShellClientRequest request;
   CreateInstance(CreateShellIdentity(), GetPermissiveCapabilities(), &request);
   shell_connection_.reset(new ShellConnection(this, std::move(request)));
-  shell_connection_->WaitForInitialize();
 
   InitCatalog(std::move(catalog_store));
 }
@@ -378,8 +373,17 @@ void Shell::Connect(scoped_ptr<ConnectParams> params) {
                        TRACE_EVENT_SCOPE_THREAD, "original_name",
                        params->target().name());
   DCHECK(IsValidName(params->target().name()));
-  DCHECK(base::IsValidGUID(params->target().user_id()));
-  DCHECK_NE(mojom::kInheritUserID, params->target().user_id());
+
+  if (params->target().user_id() == mojom::kInheritUserID) {
+    Instance* source = GetExistingInstance(params->source());
+    Identity target = params->target();
+    // TODO(beng): we should CHECK source.
+    target.set_user_id(source ? source->identity().user_id()
+                              : mojom::kRootUserID);
+    params->set_target(target);
+  }
+
+  CHECK(params->target().user_id() != mojom::kInheritUserID);
 
   // Connect to an existing matching instance, if possible.
   if (ConnectToExistingInstance(&params))
@@ -438,23 +442,26 @@ bool Shell::AcceptConnection(Connection* connection) {
 // Shell, private:
 
 void Shell::InitCatalog(scoped_ptr<catalog::Store> store) {
+  scoped_ptr<Loader> loader(
+      new catalog::Loader(file_task_runner_, std::move(store)));
+  Loader* loader_raw = loader.get();
+  std::string name = kCatalogName;
+  SetLoaderForName(std::move(loader), name);
+
   mojom::ShellClientRequest request;
-  Identity identity(kCatalogName, mojom::kRootUserID);
-  CreateInstance(identity, CapabilitySpec(), &request);
+  // TODO(beng): Does the catalog actually have to be run with a permissive
+  //             filter?
+  Identity identity(name, mojom::kRootUserID);
+  CreateInstance(identity, GetPermissiveCapabilities(), &request);
+  loader_raw->Load(name, std::move(request));
 
-  catalog_shell_client_.reset(
-      new catalog::Catalog(file_task_runner_, std::move(store)));
-  catalog_connection_.reset(
-      new ShellConnection(catalog_shell_client_.get(), std::move(request)));
-  shell_connection_->connector()->ConnectToInterface(
-      kCatalogName, &shell_resolver_);
+  ConnectToInterface(this, CreateShellIdentity(), name, &shell_resolver_);
 
   // Seed the catalog with manifest info for the shell & catalog.
   if (file_task_runner_) {
-    shell_resolver_->ResolveMojoName(
-        kCatalogName, base::Bind(&EmptyResolverCallback));
-    shell_resolver_->ResolveMojoName(
-        kShellName, base::Bind(&EmptyResolverCallback));
+    shell_resolver_->ResolveMojoName(name, base::Bind(&EmptyResolverCallback));
+    shell_resolver_->ResolveMojoName("mojo:shell",
+                                     base::Bind(&EmptyResolverCallback));
   }
 }
 
@@ -553,6 +560,7 @@ mojom::ShellClientFactory* Shell::GetShellClientFactory(
     return it->second.get();
 
   mojom::ShellClientFactoryPtr factory;
+  // TODO(beng): we should forward the original source identity!
   ConnectToInterface(this, source_identity, shell_client_factory_identity,
                      &factory);
   mojom::ShellClientFactory* factory_interface = factory.get();
diff --git a/mojo/shell/shell.h b/mojo/shell/shell.h
index e8f18fb..2d66593 100644
--- a/mojo/shell/shell.h
+++ b/mojo/shell/shell.h
@@ -34,7 +34,6 @@ class SequencedWorkerPool;
 }
 
 namespace mojo {
-class ShellClient;
 class ShellConnection;
 namespace shell {
 
@@ -192,8 +191,6 @@ class Shell : public ShellClient {
   scoped_ptr<NativeRunnerFactory> native_runner_factory_;
   std::vector<scoped_ptr<NativeRunner>> native_runners_;
   scoped_ptr<ShellConnection> shell_connection_;
-  scoped_ptr<ShellConnection> catalog_connection_;
-  scoped_ptr<ShellClient> catalog_shell_client_;
   base::WeakPtrFactory<Shell> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Shell);
diff --git a/mojo/shell/standalone/context.cc b/mojo/shell/standalone/context.cc
index 4e847fb..23dc96b 100644
--- a/mojo/shell/standalone/context.cc
+++ b/mojo/shell/standalone/context.cc
@@ -134,6 +134,7 @@ void Context::Init(scoped_ptr<InitParams> init_params) {
   blocking_pool_ =
       new base::SequencedWorkerPool(kMaxBlockingPoolThreads, "blocking_pool");
 
+  // TODO(vtl): This should be MASTER, not NONE.
   edk::InitIPCSupport(this, io_thread_->task_runner().get());
 
   scoped_ptr<NativeRunnerFactory> runner_factory;
@@ -163,7 +164,7 @@ void Context::Init(scoped_ptr<InitParams> init_params) {
 
   scoped_ptr<ConnectParams> params(new ConnectParams);
   params->set_source(CreateShellIdentity());
-  params->set_target(Identity("mojo:tracing", mojom::kRootUserID));
+  params->set_target(Identity("mojo:tracing", mojom::kInheritUserID));
   params->set_remote_interfaces(GetProxy(&tracing_remote_interfaces));
   params->set_local_interfaces(std::move(tracing_local_interfaces));
   shell_->Connect(std::move(params));
diff --git a/mojo/shell/tests/connect/connect_unittest.cc b/mojo/shell/tests/connect/connect_unittest.cc
index df6c6fa..0c65d9d 100644
--- a/mojo/shell/tests/connect/connect_unittest.cc
+++ b/mojo/shell/tests/connect/connect_unittest.cc
@@ -287,6 +287,8 @@ TEST_F(ConnectTest, CapabilityClasses) {
 }
 
 // Tests that we can expose an interface to targets on outbound connections.
+// TODO(beng): Currently all interfaces on outbound connections are exposed.
+//             See ConnectorImpl::Connect().
 TEST_F(ConnectTest, LocalInterface) {
   // Connect to a standalone application.
   {