From d5669c5cce5e182f29a4387807cda0c74e8265eb Mon Sep 17 00:00:00 2001 From: rockot Date: Mon, 14 Mar 2016 16:38:28 -0700 Subject: Change primordial pipes to ShellClient Changes shell client startup pipes to be ShellClient instead of ShellClientFactory. This simplifies a lot of startup code. Due to this change, apps must now take responsibility for shutting themselves down when they feel it's appropriate. For now, all relevant apps are force-exited to mimic the old behavior. Once apptests are deleted, we should expose a clean quit closure to apps so they have something to run when they want to quit. Hard-exiting the process is definitely not the right thing to do. BUG= Review URL: https://codereview.chromium.org/1801963002 Cr-Commit-Position: refs/heads/master@{#381114} --- mojo/mojo_shell.gyp | 27 +-- mojo/shell/native_runner.h | 7 +- mojo/shell/public/cpp/application_runner.h | 6 +- mojo/shell/public/cpp/connector.h | 12 +- mojo/shell/public/cpp/lib/application_runner.cc | 4 + mojo/shell/public/cpp/lib/application_test_base.cc | 4 +- mojo/shell/public/cpp/lib/connector_impl.cc | 18 +- mojo/shell/public/cpp/lib/shell_connection.cc | 12 +- mojo/shell/public/cpp/names.h | 6 +- mojo/shell/public/cpp/shell_connection.h | 17 +- mojo/shell/public/interfaces/connector.mojom | 10 +- mojo/shell/runner/child/BUILD.gn | 21 +- mojo/shell/runner/child/runner_connection.cc | 86 -------- mojo/shell/runner/child/runner_connection.h | 55 ----- mojo/shell/runner/child/test_native_main.cc | 7 +- mojo/shell/runner/common/BUILD.gn | 13 ++ mojo/shell/runner/common/client_util.cc | 38 ++++ mojo/shell/runner/common/client_util.h | 32 +++ mojo/shell/runner/host/child_process_base.cc | 234 ++------------------- mojo/shell/runner/host/child_process_base.h | 1 - mojo/shell/runner/host/child_process_host.cc | 117 +++++------ mojo/shell/runner/host/child_process_host.h | 23 +- .../runner/host/child_process_host_unittest.cc | 2 +- mojo/shell/runner/host/in_process_native_runner.cc | 9 +- mojo/shell/runner/host/in_process_native_runner.h | 3 +- .../runner/host/out_of_process_native_runner.cc | 12 +- .../runner/host/out_of_process_native_runner.h | 6 +- mojo/shell/shell.cc | 138 ++++++------ mojo/shell/shell.h | 21 +- mojo/shell/tests/connect/connect_test_driver.cc | 4 + mojo/shell/tests/shell/driver.cc | 18 +- mojo/shell/tests/util.cc | 6 +- 32 files changed, 323 insertions(+), 646 deletions(-) delete mode 100644 mojo/shell/runner/child/runner_connection.cc delete mode 100644 mojo/shell/runner/child/runner_connection.h create mode 100644 mojo/shell/runner/common/client_util.cc create mode 100644 mojo/shell/runner/common/client_util.h (limited to 'mojo') diff --git a/mojo/mojo_shell.gyp b/mojo/mojo_shell.gyp index 9e2e5ee..0b95228 100644 --- a/mojo/mojo_shell.gyp +++ b/mojo/mojo_shell.gyp @@ -74,35 +74,28 @@ 'mojom_bindings_generator_explicit.gypi', ], }, { - 'target_name': 'mojo_runner_connection_lib', + 'target_name': 'mojo_runner_common_lib', 'type': 'static_library', 'sources': [ - 'shell/runner/child/runner_connection.cc', - 'shell/runner/child/runner_connection.h', + 'shell/runner/common/client_util.cc', + 'shell/runner/common/client_util.h', + 'shell/runner/common/switches.cc', + 'shell/runner/common/switches.h', + ], + 'include_dirs': [ + '..', ], 'dependencies': [ - 'mojo_runner_common_lib', '<(DEPTH)/base/base.gyp:base', - '<(DEPTH)/mojo/mojo_base.gyp:mojo_application_base', '<(DEPTH)/mojo/mojo_base.gyp:mojo_application_bindings', '<(DEPTH)/mojo/mojo_edk.gyp:mojo_system_impl', - '<(DEPTH)/mojo/mojo_platform_handle.gyp:platform_handle', - '<(DEPTH)/mojo/mojo_public.gyp:mojo_message_pump_lib', + '<(DEPTH)/mojo/mojo_public.gyp:mojo_cpp_bindings', + '<(DEPTH)/mojo/mojo_public.gyp:mojo_cpp_system', ], 'export_dependent_settings': [ '<(DEPTH)/mojo/mojo_base.gyp:mojo_application_bindings', ], }, { - 'target_name': 'mojo_runner_common_lib', - 'type': 'static_library', - 'sources': [ - 'shell/runner/common/switches.cc', - 'shell/runner/common/switches.h', - ], - 'include_dirs': [ - '..', - ], - }, { 'target_name': 'mojo_runner_host_lib', 'type': 'static_library', 'sources': [ diff --git a/mojo/shell/native_runner.h b/mojo/shell/native_runner.h index 92c1adc..bf28755 100644 --- a/mojo/shell/native_runner.h +++ b/mojo/shell/native_runner.h @@ -10,7 +10,6 @@ #include "base/process/process_handle.h" #include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/shell/public/interfaces/shell_client.mojom.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" namespace base { class FilePath; @@ -28,12 +27,12 @@ class NativeRunner { virtual ~NativeRunner() {} // Loads the app in the file at |app_path| and runs it on some other - // thread/process. - virtual void Start( + // thread/process. Returns a ShellClient handle the shell can use to connect + // to the the app. + virtual mojom::ShellClientPtr Start( const base::FilePath& app_path, const Identity& target, bool start_sandboxed, - InterfaceRequest request, const base::Callback& pid_available_callback, const base::Closure& app_completed_callback) = 0; }; diff --git a/mojo/shell/public/cpp/application_runner.h b/mojo/shell/public/cpp/application_runner.h index 3863923..2fb28a2 100644 --- a/mojo/shell/public/cpp/application_runner.h +++ b/mojo/shell/public/cpp/application_runner.h @@ -27,7 +27,7 @@ class ShellConnection; // ultimately Quit(). class ApplicationRunner { public: - // Takes ownership of |delegate|. + // Takes ownership of |client|. explicit ApplicationRunner(ShellClient* client); ~ApplicationRunner(); @@ -53,6 +53,10 @@ class ApplicationRunner { // requests from others. void DestroyShellConnection(); + // Allows the caller to explicitly quit the application. Must be called from + // the thread which created the ApplicationRunner. + void Quit(); + private: scoped_ptr connection_; scoped_ptr client_; diff --git a/mojo/shell/public/cpp/connector.h b/mojo/shell/public/cpp/connector.h index 73a6ef4..b615535 100644 --- a/mojo/shell/public/cpp/connector.h +++ b/mojo/shell/public/cpp/connector.h @@ -9,7 +9,7 @@ #include "mojo/shell/public/cpp/identity.h" #include "mojo/shell/public/interfaces/connector.mojom.h" #include "mojo/shell/public/interfaces/shell.mojom.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" +#include "mojo/shell/public/interfaces/shell_client.mojom.h" namespace mojo { @@ -40,21 +40,21 @@ class Connector { const Identity& target() { return target_; } void set_target(const Identity& target) { target_ = target; } void set_client_process_connection( - shell::mojom::ShellClientFactoryPtr shell_client_factory, + shell::mojom::ShellClientPtr shell_client, shell::mojom::PIDReceiverRequest pid_receiver_request) { - shell_client_factory_ = std::move(shell_client_factory); + shell_client_ = std::move(shell_client); pid_receiver_request_ = std::move(pid_receiver_request); } void TakeClientProcessConnection( - shell::mojom::ShellClientFactoryPtr* shell_client_factory, + shell::mojom::ShellClientPtr* shell_client, shell::mojom::PIDReceiverRequest* pid_receiver_request) { - *shell_client_factory = std::move(shell_client_factory_); + *shell_client = std::move(shell_client_); *pid_receiver_request = std::move(pid_receiver_request_); } private: Identity target_; - shell::mojom::ShellClientFactoryPtr shell_client_factory_; + shell::mojom::ShellClientPtr shell_client_; shell::mojom::PIDReceiverRequest pid_receiver_request_; DISALLOW_COPY_AND_ASSIGN(ConnectParams); diff --git a/mojo/shell/public/cpp/lib/application_runner.cc b/mojo/shell/public/cpp/lib/application_runner.cc index dd9be48..5253053 100644 --- a/mojo/shell/public/cpp/lib/application_runner.cc +++ b/mojo/shell/public/cpp/lib/application_runner.cc @@ -81,4 +81,8 @@ void ApplicationRunner::DestroyShellConnection() { connection_.reset(); } +void ApplicationRunner::Quit() { + base::MessageLoop::current()->QuitWhenIdle(); +} + } // namespace mojo diff --git a/mojo/shell/public/cpp/lib/application_test_base.cc b/mojo/shell/public/cpp/lib/application_test_base.cc index c03f7f72..a52a193 100644 --- a/mojo/shell/public/cpp/lib/application_test_base.cc +++ b/mojo/shell/public/cpp/lib/application_test_base.cc @@ -36,7 +36,9 @@ shell::mojom::ConnectorPtr g_connector; class ShellGrabber : public shell::mojom::ShellClient { public: explicit ShellGrabber(InterfaceRequest request) - : binding_(this, std::move(request)) {} + : binding_(this, std::move(request)) { + binding_.set_connection_error_handler([] { _exit(1); }); + } void WaitForInitialize() { // Initialize is always the first call made on ShellClient. diff --git a/mojo/shell/public/cpp/lib/connector_impl.cc b/mojo/shell/public/cpp/lib/connector_impl.cc index 7ff56d4..3b16a0c 100644 --- a/mojo/shell/public/cpp/lib/connector_impl.cc +++ b/mojo/shell/public/cpp/lib/connector_impl.cc @@ -59,21 +59,19 @@ scoped_ptr ConnectorImpl::Connect(ConnectParams* params) { shell::mojom::kInvalidInstanceID, std::move(remote_interfaces), std::move(local_request), request, Connection::State::PENDING)); - shell::mojom::ShellClientFactoryPtr shell_client_factory; + shell::mojom::ShellClientPtr shell_client; shell::mojom::PIDReceiverRequest pid_receiver_request; - params->TakeClientProcessConnection(&shell_client_factory, - &pid_receiver_request); + params->TakeClientProcessConnection(&shell_client, &pid_receiver_request); shell::mojom::ClientProcessConnectionPtr client_process_connection; - if (shell_client_factory.is_bound() && pid_receiver_request.is_pending()) { + if (shell_client.is_bound() && pid_receiver_request.is_pending()) { client_process_connection = shell::mojom::ClientProcessConnection::New(); - client_process_connection->shell_client_factory = - shell_client_factory.PassInterface().PassHandle(); + client_process_connection->shell_client = + shell_client.PassInterface().PassHandle(); client_process_connection->pid_receiver_request = pid_receiver_request.PassMessagePipe(); - } else if (shell_client_factory.is_bound() || - pid_receiver_request.is_pending()) { - NOTREACHED() << "If one of shell_client_factory or pid_receiver_request is" - << "valid, both must be valid."; + } else if (shell_client.is_bound() || pid_receiver_request.is_pending()) { + NOTREACHED() << "If one of shell_client or pid_receiver_request is valid, " + << "both must be valid."; return std::move(registry); } connector_->Connect( diff --git a/mojo/shell/public/cpp/lib/shell_connection.cc b/mojo/shell/public/cpp/lib/shell_connection.cc index 24210ad..7c8b032 100644 --- a/mojo/shell/public/cpp/lib/shell_connection.cc +++ b/mojo/shell/public/cpp/lib/shell_connection.cc @@ -19,9 +19,6 @@ namespace mojo { //////////////////////////////////////////////////////////////////////////////// // ShellConnection, public: -ShellConnection::ShellConnection(mojo::ShellClient* client) - : ShellConnection(client, nullptr) {} - ShellConnection::ShellConnection(mojo::ShellClient* client, shell::mojom::ShellClientRequest request) : client_(client), binding_(this) { @@ -29,17 +26,12 @@ ShellConnection::ShellConnection(mojo::ShellClient* client, pending_connector_request_ = GetProxy(&connector); connector_.reset(new ConnectorImpl(std::move(connector))); - if (request.is_pending()) - BindToRequest(std::move(request)); + DCHECK(request.is_pending()); + binding_.Bind(std::move(request)); } ShellConnection::~ShellConnection() {} -void ShellConnection::BindToRequest(shell::mojom::ShellClientRequest request) { - DCHECK(!binding_.is_bound()); - binding_.Bind(std::move(request)); -} - void ShellConnection::SetAppTestConnectorForTesting( shell::mojom::ConnectorPtr connector) { pending_connector_request_ = nullptr; diff --git a/mojo/shell/public/cpp/names.h b/mojo/shell/public/cpp/names.h index 9815c87..9fbabf2 100644 --- a/mojo/shell/public/cpp/names.h +++ b/mojo/shell/public/cpp/names.h @@ -28,9 +28,9 @@ namespace mojo { // Represents a native executable on the host platform, expected to live // alongside the shell executable. Executables launched via this mechanism are // passed a handle to the shell on the command line and are expected to bind -// a ShellClientFactory via which they will receive a ShellClientRequest -// enabling further communication with the shell. The path component contains -// the executable name, minus any platform-specific extension. +// a ShellClientRequest enabling further communication with the shell. The +// path component contains the executable name, minus any platform-specific +// extension. // // Other types may be supplied but are not recognized by any of the // NativeRunners, and as such custom loaders must be specified for such names. diff --git a/mojo/shell/public/cpp/shell_connection.h b/mojo/shell/public/cpp/shell_connection.h index 216ff93..8c90ec7 100644 --- a/mojo/shell/public/cpp/shell_connection.h +++ b/mojo/shell/public/cpp/shell_connection.h @@ -43,15 +43,10 @@ class Connector; // class ShellConnection : public shell::mojom::ShellClient { public: - // Creates a new ShellConnection to eventually be bound to a - // ShellClientRequest (see BindToRequest()). This connection may be used - // immediately to begin making outgoing connections via connector(). - // - // Does not take ownership of |client|, which must remain valid for the - // lifetime of ShellConnection. - explicit ShellConnection(mojo::ShellClient* client); - - // Like above but binds to |request| upon construction. + // Creates a new ShellConnection bound to |request|. This connection may be + // used immediately to make outgoing connections via connector(). Does not + // take ownership of |client|, which must remain valid for the lifetime of + // ShellConnection. ShellConnection(mojo::ShellClient* client, shell::mojom::ShellClientRequest request); @@ -59,10 +54,6 @@ class ShellConnection : public shell::mojom::ShellClient { Connector* connector() { return connector_.get(); } - // Binds this ShellConnection to a client request if one was not available at - // construction time. - void BindToRequest(shell::mojom::ShellClientRequest request); - // TODO(rockot): Remove this once we get rid of app tests. void SetAppTestConnectorForTesting(shell::mojom::ConnectorPtr connector); diff --git a/mojo/shell/public/interfaces/connector.mojom b/mojo/shell/public/interfaces/connector.mojom index 4dff63da..b48440a 100644 --- a/mojo/shell/public/interfaces/connector.mojom +++ b/mojo/shell/public/interfaces/connector.mojom @@ -18,7 +18,7 @@ enum ConnectResult { // The name or user id supplied was malformed, or the application specified // by |name| could not be loaded. INVALID_ARGUMENT, - + // The connection was blocked by policy. Either connections to |name| are // forbidden from this app by the CapabilityFilter, or the application // attempted to connect using a user id other than its own, @@ -69,12 +69,12 @@ interface PIDReceiver { // the process itself and provide the shell the pipes it needs to communicate // with it. When an instance of this struct is supplied to Connect(), the client // owns the lifetime of the child process, not the shell. The shell binds the -// |shell_client_factory| pipe, and when it closes destroys the associated -// instance but the process stays alive. +// |shell_client| pipe, and when it closes destroys the associated instance but +// the process stays alive. struct ClientProcessConnection { - // Provides the shell the ability to bind a ShellClientRequest from the client + // Provides the shell the ability to bind a ShellClient from the client // process to the instance it creates. - handle shell_client_factory; + handle shell_client; // Allows the client process launcher to tell the shell the PID of the process // it created (the pid isn't supplied directly here as the process may not diff --git a/mojo/shell/runner/child/BUILD.gn b/mojo/shell/runner/child/BUILD.gn index e8c63bd..0276b3d 100644 --- a/mojo/shell/runner/child/BUILD.gn +++ b/mojo/shell/runner/child/BUILD.gn @@ -10,24 +10,6 @@ group("child") { testonly = true deps = [ ":apptests", - ":lib", - ] -} - -source_set("lib") { - sources = [ - "runner_connection.cc", - "runner_connection.h", - ] - - deps = [ - "//base", - "//mojo/edk/system", - "//mojo/message_pump", - "//mojo/platform_handle:platform_handle_impl", - "//mojo/shell/public/cpp:sources", - "//mojo/shell/public/interfaces", - "//mojo/shell/runner/common", ] } @@ -46,7 +28,7 @@ source_set("test_native_main") { "//mojo/edk/system", "//mojo/message_pump", "//mojo/shell/public/cpp", - "//mojo/shell/runner/child:lib", + "//mojo/shell/runner/common", ] } @@ -100,7 +82,6 @@ executable("native_target") { deps = [ ":apptest_interfaces", - ":lib", ":test_native_main", "//base", "//build/config/sanitizers:deps", diff --git a/mojo/shell/runner/child/runner_connection.cc b/mojo/shell/runner/child/runner_connection.cc deleted file mode 100644 index f1478d5..0000000 --- a/mojo/shell/runner/child/runner_connection.cc +++ /dev/null @@ -1,86 +0,0 @@ -// Copyright 2015 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/runner/child/runner_connection.h" - -#include - -#include - -#include "base/bind.h" -#include "base/callback.h" -#include "base/command_line.h" -#include "base/macros.h" -#include "base/memory/ref_counted.h" -#include "base/memory/scoped_ptr.h" -#include "mojo/edk/embedder/embedder.h" -#include "mojo/edk/embedder/platform_channel_pair.h" -#include "mojo/edk/embedder/scoped_platform_handle.h" -#include "mojo/public/cpp/bindings/binding.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" -#include "mojo/shell/runner/common/switches.h" - -namespace mojo { -namespace shell { -namespace { - -class RunnerConnectionImpl : public RunnerConnection, - public mojom::ShellClientFactory { - public: - RunnerConnectionImpl(mojo::ShellConnection* shell_connetion, - mojom::ShellClientFactoryRequest client_factory_request, - bool exit_on_error) - : shell_connection_(shell_connetion), - binding_(this, std::move(client_factory_request)), - exit_on_error_(exit_on_error) { - binding_.set_connection_error_handler([this] { OnConnectionError(); }); - } - - ~RunnerConnectionImpl() override {} - - private: - // mojom::ShellClientFactory: - void CreateShellClient(mojom::ShellClientRequest client_request, - const String& name) override { - shell_connection_->BindToRequest(std::move(client_request)); - } - - void OnConnectionError() { - // A connection error means the connection to the shell is lost. This is not - // recoverable. - DLOG(ERROR) << "Connection error to the shell."; - if (exit_on_error_) - _exit(1); - } - - mojo::ShellConnection* const shell_connection_; - Binding binding_; - const bool exit_on_error_; - - DISALLOW_COPY_AND_ASSIGN(RunnerConnectionImpl); -}; - -} // namespace - -RunnerConnection::~RunnerConnection() {} - -// static -scoped_ptr RunnerConnection::Create( - mojo::ShellConnection* shell_connection, - bool exit_on_error) { - std::string primordial_pipe_token = - base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( - switches::kPrimordialPipeToken); - mojom::ShellClientFactoryRequest client_factory_request; - client_factory_request.Bind( - edk::CreateChildMessagePipe(primordial_pipe_token)); - - return make_scoped_ptr(new RunnerConnectionImpl( - shell_connection, std::move(client_factory_request), exit_on_error)); -} - -RunnerConnection::RunnerConnection() {} - -} // namespace shell -} // namespace mojo diff --git a/mojo/shell/runner/child/runner_connection.h b/mojo/shell/runner/child/runner_connection.h deleted file mode 100644 index e1829c1..0000000 --- a/mojo/shell/runner/child/runner_connection.h +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright 2015 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_SHELL_RUNNER_CHILD_RUNNER_CONNECTION_H_ -#define MOJO_SHELL_RUNNER_CHILD_RUNNER_CONNECTION_H_ - -#include "base/callback.h" -#include "base/macros.h" -#include "base/memory/scoped_ptr.h" -#include "mojo/shell/public/cpp/shell_connection.h" -#include "mojo/shell/public/interfaces/connector.mojom.h" -#include "mojo/shell/public/interfaces/shell_client.mojom.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" - -namespace mojo { -namespace shell { - -// Encapsulates a connection to a runner process. -class RunnerConnection { - public: - virtual ~RunnerConnection(); - - // Establishes a new runner connection using a ShellClientFactory request pipe - // from the command line. - // - // |shell_connection| is a ShellConnection which will be used to service - // incoming connection requests from other clients. This may be null, in which - // case no incoming connection requests will be serviced. This is not owned - // and must out-live the RunnerConnection. - // - // If |exit_on_error| is true, the calling process will be terminated in the - // event of an error on the ShellClientFactory pipe. - // - // The RunnerConnection returned by this call bounds the lifetime of the - // connection. If connection fails (which can happen if there is no - // ShellClientFactory request pipe on the command line) this returns null. - // - // TODO(rockot): Remove |exit_on_error|. We shouldn't be killing processes - // abruptly on pipe errors, but some tests currently depend on this behavior. - static scoped_ptr Create( - mojo::ShellConnection* shell_connection, - bool exit_on_error = true); - - protected: - RunnerConnection(); - - private: - DISALLOW_COPY_AND_ASSIGN(RunnerConnection); -}; - -} // namespace shell -} // namespace mojo - -#endif // MOJO_SHELL_RUNNER_CHILD_RUNNER_CONNECTION_H_ diff --git a/mojo/shell/runner/child/test_native_main.cc b/mojo/shell/runner/child/test_native_main.cc index 4caddc3..cc54215 100644 --- a/mojo/shell/runner/child/test_native_main.cc +++ b/mojo/shell/runner/child/test_native_main.cc @@ -16,7 +16,7 @@ #include "mojo/edk/embedder/process_delegate.h" #include "mojo/shell/public/cpp/shell_client.h" #include "mojo/shell/public/cpp/shell_connection.h" -#include "mojo/shell/runner/child/runner_connection.h" +#include "mojo/shell/runner/common/client_util.h" #include "mojo/shell/runner/init.h" namespace mojo { @@ -58,9 +58,8 @@ int TestNativeMain(mojo::ShellClient* shell_client) { mojo::edk::SetParentPipeHandleFromCommandLine(); base::MessageLoop loop; - mojo::ShellConnection impl(shell_client);; - scoped_ptr connection = - mojo::shell::RunnerConnection::Create(&impl); + mojo::ShellConnection impl( + shell_client, mojo::shell::GetShellClientRequestFromCommandLine()); loop.Run(); mojo::edk::ShutdownIPCSupport(); diff --git a/mojo/shell/runner/common/BUILD.gn b/mojo/shell/runner/common/BUILD.gn index 2fe8f65..f780070 100644 --- a/mojo/shell/runner/common/BUILD.gn +++ b/mojo/shell/runner/common/BUILD.gn @@ -4,7 +4,20 @@ source_set("common") { sources = [ + "client_util.cc", + "client_util.h", "switches.cc", "switches.h", ] + + deps = [ + "//base", + "//mojo/edk/system", + "//mojo/public/cpp/bindings", + "//mojo/public/cpp/system", + ] + + public_deps = [ + "//mojo/shell/public/interfaces", + ] } diff --git a/mojo/shell/runner/common/client_util.cc b/mojo/shell/runner/common/client_util.cc new file mode 100644 index 0000000..2c264bd --- /dev/null +++ b/mojo/shell/runner/common/client_util.cc @@ -0,0 +1,38 @@ +// 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/shell/runner/common/client_util.h" + +#include + +#include "base/command_line.h" +#include "mojo/edk/embedder/embedder.h" +#include "mojo/shell/runner/common/switches.h" + +namespace mojo { +namespace shell { + +mojom::ShellClientPtr PassShellClientRequestOnCommandLine( + base::CommandLine* command_line) { + std::string token = edk::GenerateRandomToken(); + command_line->AppendSwitchASCII(switches::kPrimordialPipeToken, token); + + mojom::ShellClientPtr client; + client.Bind( + mojom::ShellClientPtrInfo(edk::CreateParentMessagePipe(token), 0)); + return client; +} + +mojom::ShellClientRequest GetShellClientRequestFromCommandLine() { + std::string token = + base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( + switches::kPrimordialPipeToken); + mojom::ShellClientRequest request; + if (!token.empty()) + request.Bind(edk::CreateChildMessagePipe(token)); + return request; +} + +} // namespace shell +} // namespace mojo diff --git a/mojo/shell/runner/common/client_util.h b/mojo/shell/runner/common/client_util.h new file mode 100644 index 0000000..ac61c77 --- /dev/null +++ b/mojo/shell/runner/common/client_util.h @@ -0,0 +1,32 @@ +// 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_SHELL_RUNNER_COMMON_CLIENT_UTIL_H_ +#define MOJO_SHELL_RUNNER_COMMON_CLIENT_UTIL_H_ + +#include "mojo/shell/public/interfaces/shell_client.mojom.h" + +namespace base { +class CommandLine; +} + +namespace mojo { +namespace shell { + +// Creates a new ShellClient pipe and returns one end of it. The other end is +// passed via a token in |command_line|. A child of the calling process may +// extract a ShellClientRequest from this by calling +// GetShellClientRequestFromCommandLine(). +mojom::ShellClientPtr PassShellClientRequestOnCommandLine( + base::CommandLine* command_line); + +// Extracts a ShellClientRequest from the command line of the current process. +// The parent of this process should have passed a request using +// PassShellClientRequestOnCommandLine(). +mojom::ShellClientRequest GetShellClientRequestFromCommandLine(); + +} // namespace shell +} // namespace mojo + +#endif // MOJO_SHELL_RUNNER_COMMON_CLIENT_UTIL_H_ diff --git a/mojo/shell/runner/host/child_process_base.cc b/mojo/shell/runner/host/child_process_base.cc index 538cf7d..e6b9a25 100644 --- a/mojo/shell/runner/host/child_process_base.cc +++ b/mojo/shell/runner/host/child_process_base.cc @@ -4,99 +4,28 @@ #include "mojo/shell/runner/host/child_process_base.h" -#include - -#include - -#include "base/base_switches.h" -#include "base/bind.h" -#include "base/callback_helpers.h" -#include "base/command_line.h" -#include "base/debug/stack_trace.h" -#include "base/files/file_path.h" -#include "base/location.h" #include "base/logging.h" #include "base/macros.h" #include "base/memory/ref_counted.h" -#include "base/memory/scoped_ptr.h" #include "base/message_loop/message_loop.h" #include "base/single_thread_task_runner.h" #include "base/synchronization/waitable_event.h" -#include "base/thread_task_runner_handle.h" #include "base/threading/thread.h" -#include "base/threading/thread_checker.h" #include "mojo/edk/embedder/embedder.h" -#include "mojo/edk/embedder/platform_channel_pair.h" #include "mojo/edk/embedder/process_delegate.h" -#include "mojo/edk/embedder/scoped_platform_handle.h" -#include "mojo/message_pump/message_pump_mojo.h" -#include "mojo/public/cpp/bindings/binding.h" -#include "mojo/public/cpp/system/core.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" -#include "mojo/shell/runner/common/switches.h" -#include "mojo/shell/runner/init.h" +#include "mojo/shell/runner/common/client_util.h" namespace mojo { namespace shell { namespace { -// Blocker --------------------------------------------------------------------- - -// Blocks a thread until another thread unblocks it, at which point it unblocks -// and runs a closure provided by that thread. -class Blocker { +// Should be created and initialized on the main thread and kept alive as long +// a Mojo application is running in the current process. +class ScopedAppContext : public edk::ProcessDelegate { public: - class Unblocker { - public: - explicit Unblocker(Blocker* blocker = nullptr) : blocker_(blocker) {} - ~Unblocker() {} - - void Unblock(base::Closure run_after) { - DCHECK(blocker_); - DCHECK(blocker_->run_after_.is_null()); - blocker_->run_after_ = run_after; - blocker_->event_.Signal(); - blocker_ = nullptr; - } - - private: - Blocker* blocker_; - - // Copy and assign allowed. - }; - - Blocker() : event_(true, false) {} - ~Blocker() {} - - void Block() { - DCHECK(run_after_.is_null()); - event_.Wait(); - if (!run_after_.is_null()) - run_after_.Run(); - } - - Unblocker GetUnblocker() { return Unblocker(this); } - - private: - base::WaitableEvent event_; - base::Closure run_after_; - - DISALLOW_COPY_AND_ASSIGN(Blocker); -}; - -// AppContext ------------------------------------------------------------------ - -class ChildControllerImpl; - -// Should be created and initialized on the main thread. -class AppContext : public edk::ProcessDelegate { - public: - AppContext() - : io_thread_("io_thread"), controller_thread_("controller_thread") {} - ~AppContext() override {} - - void Init() { + ScopedAppContext() + : io_thread_("io_thread"), wait_for_shutdown_event_(true, false) { // Initialize Mojo before starting any threads. edk::Init(); @@ -106,170 +35,37 @@ class AppContext : public edk::ProcessDelegate { io_runner_ = io_thread_.task_runner().get(); CHECK(io_runner_.get()); - // 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_); + edk::SetParentPipeHandleFromCommandLine(); } - void StartControllerThread() { - // Create and start our controller thread. - base::Thread::Options controller_thread_options; - controller_thread_options.message_loop_type = - base::MessageLoop::TYPE_CUSTOM; - controller_thread_options.message_pump_factory = - base::Bind(&common::MessagePumpMojo::Create); - CHECK(controller_thread_.StartWithOptions(controller_thread_options)); - controller_runner_ = controller_thread_.task_runner().get(); - CHECK(controller_runner_.get()); - } - - void Shutdown() { - Blocker blocker; - shutdown_unblocker_ = blocker.GetUnblocker(); - controller_runner_->PostTask( - FROM_HERE, base::Bind(&AppContext::ShutdownOnControllerThread, - base::Unretained(this))); - blocker.Block(); - } - - base::SingleThreadTaskRunner* io_runner() const { return io_runner_.get(); } - - base::SingleThreadTaskRunner* controller_runner() const { - return controller_runner_.get(); - } - - ChildControllerImpl* controller() const { return controller_.get(); } - - void set_controller(scoped_ptr controller) { - controller_ = std::move(controller); - } - - private: - void ShutdownOnControllerThread() { - // First, destroy the controller. - controller_.reset(); - - // Next shutdown IPC. We'll unblock the main thread in OnShutdownComplete(). + ~ScopedAppContext() override { edk::ShutdownIPCSupport(); + wait_for_shutdown_event_.Wait(); } + private: // ProcessDelegate implementation. void OnShutdownComplete() override { - shutdown_unblocker_.Unblock(base::Closure()); + wait_for_shutdown_event_.Signal(); } base::Thread io_thread_; scoped_refptr io_runner_; - base::Thread controller_thread_; - scoped_refptr controller_runner_; - - // Accessed only on the controller thread. - scoped_ptr controller_; - // Used to unblock the main thread on shutdown. - Blocker::Unblocker shutdown_unblocker_; + base::WaitableEvent wait_for_shutdown_event_; - DISALLOW_COPY_AND_ASSIGN(AppContext); + DISALLOW_COPY_AND_ASSIGN(ScopedAppContext); }; -// ChildControllerImpl ------------------------------------------------------ - -class ChildControllerImpl : public mojom::ShellClientFactory { - public: - ~ChildControllerImpl() override { - DCHECK(thread_checker_.CalledOnValidThread()); - } - - // To be executed on the controller thread. Creates the |ChildController|, - // etc. - static void Init(AppContext* app_context, - const RunCallback& run_callback, - ScopedMessagePipeHandle host_message_pipe, - const Blocker::Unblocker& unblocker) { - DCHECK(app_context); - DCHECK(host_message_pipe.is_valid()); - - DCHECK(!app_context->controller()); - - scoped_ptr impl( - new ChildControllerImpl(app_context, run_callback, unblocker)); - - impl->Bind(std::move(host_message_pipe)); - - app_context->set_controller(std::move(impl)); - } - - void Bind(ScopedMessagePipeHandle handle) { - binding_.Bind(std::move(handle)); - binding_.set_connection_error_handler([this]() { OnConnectionError(); }); - } - - void OnConnectionError() { - // A connection error means the connection to the shell is lost. This is not - // recoverable. - LOG(ERROR) << "Connection error to the shell."; - _exit(1); - } - - // |mojom::ShellClientFactory| methods: - void CreateShellClient(mojom::ShellClientRequest request, - const String& name) override { - DCHECK(thread_checker_.CalledOnValidThread()); - unblocker_.Unblock(base::Bind(run_callback_, base::Passed(&request))); - } - - private: - ChildControllerImpl(AppContext* app_context, - const RunCallback& run_callback, - const Blocker::Unblocker& unblocker) - : run_callback_(run_callback), unblocker_(unblocker), binding_(this) {} - - base::ThreadChecker thread_checker_; - RunCallback run_callback_; - Blocker::Unblocker unblocker_; - - Binding binding_; - - DISALLOW_COPY_AND_ASSIGN(ChildControllerImpl); -}; - -ScopedMessagePipeHandle InitializeHostMessagePipe( - edk::ScopedPlatformHandle platform_channel, - scoped_refptr io_task_runner) { - edk::SetParentPipeHandle(std::move(platform_channel)); - std::string primordial_pipe_token = - base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII( - switches::kPrimordialPipeToken); - return edk::CreateChildMessagePipe(primordial_pipe_token); -} - } // namespace void ChildProcessMain(const RunCallback& callback) { - const base::CommandLine& command_line = - *base::CommandLine::ForCurrentProcess(); - edk::ScopedPlatformHandle platform_channel = - edk::PlatformChannelPair::PassClientHandleFromParentProcess(command_line); - CHECK(platform_channel.is_valid()); - DCHECK(!base::MessageLoop::current()); - Blocker blocker; - AppContext app_context; - app_context.Init(); - app_context.StartControllerThread(); - - ScopedMessagePipeHandle host_pipe = InitializeHostMessagePipe( - std::move(platform_channel), app_context.io_runner()); - app_context.controller_runner()->PostTask( - FROM_HERE, base::Bind(&ChildControllerImpl::Init, &app_context, callback, - base::Passed(&host_pipe), blocker.GetUnblocker())); - - // This will block, then run whatever the controller wants. - blocker.Block(); - - app_context.Shutdown(); + ScopedAppContext app_context; + callback.Run(GetShellClientRequestFromCommandLine()); } } // namespace shell diff --git a/mojo/shell/runner/host/child_process_base.h b/mojo/shell/runner/host/child_process_base.h index 49c5465..f2d23e5 100644 --- a/mojo/shell/runner/host/child_process_base.h +++ b/mojo/shell/runner/host/child_process_base.h @@ -6,7 +6,6 @@ #define MOJO_SHELL_RUNNER_HOST_CHILD_PROCESS_BASE_H_ #include "base/callback.h" -#include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/shell/public/interfaces/shell_client.mojom.h" namespace mojo { diff --git a/mojo/shell/runner/host/child_process_host.cc b/mojo/shell/runner/host/child_process_host.cc index 60a9947..1083128 100644 --- a/mojo/shell/runner/host/child_process_host.cc +++ b/mojo/shell/runner/host/child_process_host.cc @@ -22,6 +22,7 @@ #include "mojo/public/cpp/bindings/interface_ptr_info.h" #include "mojo/public/cpp/system/core.h" #include "mojo/shell/native_runner_delegate.h" +#include "mojo/shell/runner/common/client_util.h" #include "mojo/shell/runner/common/switches.h" #if defined(OS_LINUX) && !defined(OS_ANDROID) @@ -47,47 +48,64 @@ ChildProcessHost::ChildProcessHost(base::TaskRunner* launch_process_runner, app_path_(app_path), start_child_process_event_(false, false), weak_factory_(this) { - node_channel_.reset(new edk::PlatformChannelPair); - primordial_pipe_token_ = edk::GenerateRandomToken(); - factory_.Bind(InterfacePtrInfo( - edk::CreateParentMessagePipe(primordial_pipe_token_), 0u)); } ChildProcessHost::~ChildProcessHost() { - if (!app_path_.empty()) - CHECK(!factory_) << "Destroying ChildProcessHost before calling Join"; + if (!app_path_.empty()) { + CHECK(!mojo_ipc_channel_) + << "Destroying ChildProcessHost before calling Join"; + } } -void ChildProcessHost::Start(mojom::ShellClientRequest request, - const String& name, - const ProcessReadyCallback& callback, - const base::Closure& quit_closure) { +mojom::ShellClientPtr ChildProcessHost::Start( + const String& name, + const ProcessReadyCallback& callback, + const base::Closure& quit_closure) { DCHECK(!child_process_.IsValid()); - // Request is invalid in child_process_host_unittest. - if (request.is_pending()) { - factory_->CreateShellClient(std::move(request), name); - factory_.set_connection_error_handler(quit_closure); + + const base::CommandLine* parent_command_line = + base::CommandLine::ForCurrentProcess(); + base::FilePath target_path = parent_command_line->GetProgram(); + // |app_path_| can be empty in tests. + if (!app_path_.MatchesExtension(FILE_PATH_LITERAL(".mojo")) && + !app_path_.empty()) { + target_path = app_path_; } + + scoped_ptr child_command_line( + new base::CommandLine(target_path)); + + child_command_line->AppendArguments(*parent_command_line, false); + + if (target_path != app_path_) + child_command_line->AppendSwitchPath(switches::kChildProcess, app_path_); + + if (start_sandboxed_) + child_command_line->AppendSwitch(switches::kEnableSandbox); + + mojo_ipc_channel_.reset(new edk::PlatformChannelPair); + mojo_ipc_channel_->PrepareToPassClientHandleToChildProcess( + child_command_line.get(), &handle_passing_info_); + + mojom::ShellClientPtr client = + PassShellClientRequestOnCommandLine(child_command_line.get()); launch_process_runner_->PostTaskAndReply( FROM_HERE, - base::Bind(&ChildProcessHost::DoLaunch, base::Unretained(this)), + base::Bind(&ChildProcessHost::DoLaunch, base::Unretained(this), + base::Passed(&child_command_line)), base::Bind(&ChildProcessHost::DidStart, weak_factory_.GetWeakPtr(), callback)); + return client; } void ChildProcessHost::Join() { - if (factory_) + if (mojo_ipc_channel_) start_child_process_event_.Wait(); - - factory_.reset(); - - // This host may be hosting a child process whose lifetime is controlled - // elsewhere. In this case we have no known process handle to wait on. + mojo_ipc_channel_.reset(); if (child_process_.IsValid()) { int rv = -1; LOG_IF(ERROR, !child_process_.WaitForExit(&rv)) << "Failed to wait for child process"; - child_process_.Close(); } } @@ -97,52 +115,24 @@ void ChildProcessHost::DidStart(const ProcessReadyCallback& callback) { callback.Run(child_process_.Pid()); } else { LOG(ERROR) << "Failed to start child process"; - factory_.reset(); + mojo_ipc_channel_.reset(); } } -void ChildProcessHost::DoLaunch() { - const base::CommandLine* parent_command_line = - base::CommandLine::ForCurrentProcess(); - base::FilePath target_path = parent_command_line->GetProgram(); - // |app_path_| can be empty in tests. - if (!app_path_.MatchesExtension(FILE_PATH_LITERAL(".mojo")) && - !app_path_.empty()) { - target_path = app_path_; - } - - base::CommandLine child_command_line(target_path); - child_command_line.AppendArguments(*parent_command_line, false); - - if (target_path != app_path_) - child_command_line.AppendSwitchPath(switches::kChildProcess, app_path_); - - if (start_sandboxed_) - child_command_line.AppendSwitch(switches::kEnableSandbox); - - if (node_channel_.get()) { - node_channel_->PrepareToPassClientHandleToChildProcess( - &child_command_line, &handle_passing_info_); - } - - child_command_line.AppendSwitchASCII(switches::kPrimordialPipeToken, - primordial_pipe_token_); - +void ChildProcessHost::DoLaunch( + scoped_ptr child_command_line) { if (delegate_) { delegate_->AdjustCommandLineArgumentsForTarget(target_, - &child_command_line); + child_command_line.get()); } base::LaunchOptions options; #if defined(OS_WIN) - if (base::win::GetVersion() >= base::win::VERSION_VISTA) { - options.handles_to_inherit = &handle_passing_info_; - } else { + options.handles_to_inherit = &handle_passing_info_; #if defined(OFFICIAL_BUILD) - CHECK(false) << "Launching mojo process with inherit_handles is insecure!"; + CHECK(false) << "Launching mojo process with inherit_handles is insecure!"; #endif - options.inherit_handles = true; - } + options.inherit_handles = true; options.stdin_handle = INVALID_HANDLE_VALUE; options.stdout_handle = GetStdHandle(STD_OUTPUT_HANDLE); options.stderr_handle = GetStdHandle(STD_ERROR_HANDLE); @@ -173,27 +163,26 @@ void ChildProcessHost::DoLaunch() { options.fds_to_remap = &handle_passing_info_; #endif DVLOG(2) << "Launching child with command line: " - << child_command_line.GetCommandLineString(); + << child_command_line->GetCommandLineString(); #if defined(OS_LINUX) && !defined(OS_ANDROID) if (start_sandboxed_) { child_process_ = - sandbox::NamespaceSandbox::LaunchProcess(child_command_line, options); + sandbox::NamespaceSandbox::LaunchProcess(*child_command_line, options); if (!child_process_.IsValid()) { LOG(ERROR) << "Starting the process with a sandbox failed. Missing kernel" << " support."; } } else #endif - child_process_ = base::LaunchProcess(child_command_line, options); + child_process_ = base::LaunchProcess(*child_command_line, options); if (child_process_.IsValid()) { - platform_channel_pair_.ChildProcessLaunched(); - if (node_channel_.get()) { - node_channel_->ChildProcessLaunched(); + if (mojo_ipc_channel_.get()) { + mojo_ipc_channel_->ChildProcessLaunched(); mojo::edk::ChildProcessLaunched( child_process_.Handle(), mojo::edk::ScopedPlatformHandle(mojo::edk::PlatformHandle( - node_channel_->PassServerHandle().release().handle))); + mojo_ipc_channel_->PassServerHandle().release().handle))); } } start_child_process_event_.Signal(); diff --git a/mojo/shell/runner/host/child_process_host.h b/mojo/shell/runner/host/child_process_host.h index 6c6411d..c13251a 100644 --- a/mojo/shell/runner/host/child_process_host.h +++ b/mojo/shell/runner/host/child_process_host.h @@ -10,9 +10,11 @@ #include #include "base/callback.h" +#include "base/command_line.h" #include "base/files/file_path.h" #include "base/macros.h" #include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/process/process.h" #include "base/synchronization/lock.h" @@ -59,10 +61,9 @@ class ChildProcessHost { // |Start()|s the child process; calls |DidStart()| (on the thread on which // |Start()| was called) when the child has been started (or failed to start). - void Start(mojom::ShellClientRequest request, - const String& name, - const ProcessReadyCallback& callback, - const base::Closure& quit_closure); + mojom::ShellClientPtr Start(const String& name, + const ProcessReadyCallback& callback, + const base::Closure& quit_closure); // Waits for the child process to terminate. void Join(); @@ -71,7 +72,7 @@ class ChildProcessHost { void DidStart(const ProcessReadyCallback& callback); private: - void DoLaunch(); + void DoLaunch(scoped_ptr child_command_line); scoped_refptr launch_process_runner_; NativeRunnerDelegate* delegate_ = nullptr; @@ -79,21 +80,15 @@ class ChildProcessHost { Identity target_; const base::FilePath app_path_; base::Process child_process_; - // Used for the ShellClientFactory binding. - edk::PlatformChannelPair platform_channel_pair_; - mojom::ShellClientFactoryPtr factory_; - edk::HandlePassingInformation handle_passing_info_; - // Used to back the NodeChannel between the parent and child node. - scoped_ptr node_channel_; + // Used to initialize the Mojo IPC channel between parent and child. + scoped_ptr mojo_ipc_channel_; + edk::HandlePassingInformation handle_passing_info_; // Since Start() calls a method on another thread, we use an event to block // the main thread if it tries to destruct |this| while launching the process. base::WaitableEvent start_child_process_event_; - // A token the child can use to connect a primordial pipe to the host. - std::string primordial_pipe_token_; - base::WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(ChildProcessHost); diff --git a/mojo/shell/runner/host/child_process_host_unittest.cc b/mojo/shell/runner/host/child_process_host_unittest.cc index 2f57bf6..593360d 100644 --- a/mojo/shell/runner/host/child_process_host_unittest.cc +++ b/mojo/shell/runner/host/child_process_host_unittest.cc @@ -96,7 +96,7 @@ TEST(ChildProcessHostTest, MAYBE_StartJoin) { Identity(), base::FilePath()); base::RunLoop run_loop; child_process_host.Start( - mojom::ShellClientRequest(), String(), + String(), base::Bind(&ProcessReadyCallbackAdapater, run_loop.QuitClosure()), base::Bind(&base::DoNothing)); run_loop.Run(); diff --git a/mojo/shell/runner/host/in_process_native_runner.cc b/mojo/shell/runner/host/in_process_native_runner.cc index c006a79..823cf5f 100644 --- a/mojo/shell/runner/host/in_process_native_runner.cc +++ b/mojo/shell/runner/host/in_process_native_runner.cc @@ -13,6 +13,7 @@ #include "base/task_runner.h" #include "base/thread_task_runner_handle.h" #include "base/threading/platform_thread.h" +#include "mojo/public/cpp/bindings/interface_request.h" #include "mojo/shell/runner/host/native_application_support.h" #include "mojo/shell/runner/host/out_of_process_native_runner.h" #include "mojo/shell/runner/init.h" @@ -33,17 +34,17 @@ InProcessNativeRunner::~InProcessNativeRunner() { } } -void InProcessNativeRunner::Start( +mojom::ShellClientPtr InProcessNativeRunner::Start( const base::FilePath& app_path, const Identity& target, bool start_sandboxed, - mojom::ShellClientRequest request, const base::Callback& pid_available_callback, const base::Closure& app_completed_callback) { app_path_ = app_path; DCHECK(!request_.is_pending()); - request_ = std::move(request); + mojom::ShellClientPtr client; + request_ = GetProxy(&client); DCHECK(app_completed_callback_runner_.is_null()); app_completed_callback_runner_ = base::Bind( @@ -58,6 +59,8 @@ void InProcessNativeRunner::Start( thread_.reset(new base::DelegateSimpleThread(this, thread_name)); thread_->Start(); pid_available_callback.Run(base::kNullProcessId); + + return client; } void InProcessNativeRunner::Run() { diff --git a/mojo/shell/runner/host/in_process_native_runner.h b/mojo/shell/runner/host/in_process_native_runner.h index 9f63396..675ea8d 100644 --- a/mojo/shell/runner/host/in_process_native_runner.h +++ b/mojo/shell/runner/host/in_process_native_runner.h @@ -30,11 +30,10 @@ class InProcessNativeRunner : public NativeRunner, ~InProcessNativeRunner() override; // NativeRunner: - void Start( + mojom::ShellClientPtr Start( const base::FilePath& app_path, const Identity& target, bool start_sandboxed, - InterfaceRequest request, const base::Callback& pid_available_callback, const base::Closure& app_completed_callback) override; diff --git a/mojo/shell/runner/host/out_of_process_native_runner.cc b/mojo/shell/runner/host/out_of_process_native_runner.cc index 619bb1d..0bf38d5 100644 --- a/mojo/shell/runner/host/out_of_process_native_runner.cc +++ b/mojo/shell/runner/host/out_of_process_native_runner.cc @@ -13,6 +13,7 @@ #include "base/files/file_util.h" #include "base/logging.h" #include "base/task_runner.h" +#include "mojo/shell/runner/common/client_util.h" #include "mojo/shell/runner/host/child_process_host.h" #include "mojo/shell/runner/host/in_process_native_runner.h" @@ -29,11 +30,10 @@ OutOfProcessNativeRunner::~OutOfProcessNativeRunner() { child_process_host_->Join(); } -void OutOfProcessNativeRunner::Start( +mojom::ShellClientPtr OutOfProcessNativeRunner::Start( const base::FilePath& app_path, const Identity& target, bool start_sandboxed, - mojom::ShellClientRequest request, const base::Callback& pid_available_callback, const base::Closure& app_completed_callback) { app_path_ = app_path; @@ -43,10 +43,10 @@ void OutOfProcessNativeRunner::Start( child_process_host_.reset(new ChildProcessHost( launch_process_runner_, delegate_, start_sandboxed, target, app_path)); - child_process_host_->Start(std::move(request), target.name(), - pid_available_callback, - base::Bind(&OutOfProcessNativeRunner::AppCompleted, - base::Unretained(this))); + return child_process_host_->Start( + target.name(), pid_available_callback, + base::Bind(&OutOfProcessNativeRunner::AppCompleted, + base::Unretained(this))); } void OutOfProcessNativeRunner::AppCompleted() { diff --git a/mojo/shell/runner/host/out_of_process_native_runner.h b/mojo/shell/runner/host/out_of_process_native_runner.h index d56fd6c..45d7cf8 100644 --- a/mojo/shell/runner/host/out_of_process_native_runner.h +++ b/mojo/shell/runner/host/out_of_process_native_runner.h @@ -7,12 +7,13 @@ #include +#include + #include "base/callback.h" #include "base/files/file_path.h" #include "base/macros.h" #include "base/memory/scoped_ptr.h" #include "mojo/shell/native_runner.h" -#include "mojo/shell/public/interfaces/shell_client_factory.mojom.h" namespace base { class TaskRunner; @@ -33,11 +34,10 @@ class OutOfProcessNativeRunner : public NativeRunner { ~OutOfProcessNativeRunner() override; // NativeRunner: - void Start( + mojom::ShellClientPtr Start( const base::FilePath& app_path, const Identity& identity, bool start_sandboxed, - mojom::ShellClientRequest request, const base::Callback& pid_available_callback, const base::Closure& app_completed_callback) override; diff --git a/mojo/shell/shell.cc b/mojo/shell/shell.cc index 48370a3..beb0fb7b 100644 --- a/mojo/shell/shell.cc +++ b/mojo/shell/shell.cc @@ -104,8 +104,7 @@ class Shell::Instance : public mojom::Connector, public InterfaceFactory, public mojom::Shell { public: - Instance(mojom::ShellClientPtr shell_client, - mojo::shell::Shell* shell, + Instance(mojo::shell::Shell* shell, const Identity& identity, const CapabilitySpec& capability_spec) : shell_(shell), @@ -113,7 +112,6 @@ class Shell::Instance : public mojom::Connector, identity_(identity), capability_spec_(capability_spec), allow_any_application_(capability_spec.required.count("*") == 1), - shell_client_(std::move(shell_client)), pid_receiver_binding_(this), weak_factory_(this) { if (identity_.name() == kShellName || @@ -121,9 +119,6 @@ class Shell::Instance : public mojom::Connector, pid_ = base::Process::Current().Pid(); } DCHECK_NE(mojom::kInvalidInstanceID, id_); - - shell_client_.set_connection_error_handler( - base::Bind(&Instance::OnShellClientLost, base::Unretained(this))); } ~Instance() override {} @@ -150,13 +145,8 @@ class Shell::Instance : public mojom::Connector, } } - void InitializeClient() { - shell_client_->Initialize(mojom::Identity::From(identity_), id_, - base::Bind(&Instance::OnInitializeResponse, - base::Unretained(this))); - } - void ConnectToClient(scoped_ptr params) { + CHECK(shell_client_.is_bound()); params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED, identity_.user_id(), id_); uint32_t source_id = mojom::kInvalidInstanceID; @@ -174,28 +164,38 @@ class Shell::Instance : public mojom::Connector, mojom::CapabilityRequest::From(spec), params->target().name()); } + void StartWithClient(mojom::ShellClientPtr client) { + CHECK(!shell_client_); + shell_client_ = std::move(client); + shell_client_.set_connection_error_handler( + base::Bind(&Instance::OnShellClientLost, base::Unretained(this))); + shell_client_->Initialize(mojom::Identity::From(identity_), id_, + base::Bind(&Instance::OnInitializeResponse, + base::Unretained(this))); + } + void StartWithClientProcessConnection( - mojom::ShellClientRequest request, mojom::ClientProcessConnectionPtr client_process_connection) { - factory_.Bind(mojom::ShellClientFactoryPtrInfo( - std::move(client_process_connection->shell_client_factory), 0u)); + mojom::ShellClientPtr client; + client.Bind(mojom::ShellClientPtrInfo( + std::move(client_process_connection->shell_client), 0)); pid_receiver_binding_.Bind( std::move(client_process_connection->pid_receiver_request)); - factory_->CreateShellClient(std::move(request), identity_.name()); + StartWithClient(std::move(client)); } - void StartWithFilePath(mojom::ShellClientRequest request, - const base::FilePath& path) { + void StartWithFilePath(const base::FilePath& path) { + CHECK(!shell_client_); scoped_ptr runner = shell_->native_runner_factory_->Create(path); bool start_sandboxed = false; - runner->Start(path, identity_, start_sandboxed, std::move(request), - base::Bind(&Instance::PIDAvailable, - weak_factory_.GetWeakPtr()), - base::Bind(&mojo::shell::Shell::CleanupRunner, - shell_->weak_ptr_factory_.GetWeakPtr(), - runner.get())); + mojom::ShellClientPtr client = runner->Start( + path, identity_, start_sandboxed, + base::Bind(&Instance::PIDAvailable, weak_factory_.GetWeakPtr()), + base::Bind(&mojo::shell::Shell::CleanupRunner, + shell_->weak_ptr_factory_.GetWeakPtr(), runner.get())); shell_->native_runners_.push_back(std::move(runner)); + StartWithClient(std::move(client)); } mojom::InstanceInfoPtr CreateInstanceInfo() const { @@ -299,9 +299,9 @@ class Shell::Instance : public mojom::Connector, return false; } - if (!(*client_process_connection)->shell_client_factory.is_valid() || + if (!(*client_process_connection)->shell_client.is_valid() || !(*client_process_connection)->pid_receiver_request.is_valid()) { - LOG(ERROR) << "Error: must supply both shell_client_factory AND " + LOG(ERROR) << "Error: must supply both shell_client AND " << "pid_receiver_request when sending " << "client_process_connection."; callback.Run(mojom::ConnectResult::INVALID_ARGUMENT, @@ -393,7 +393,6 @@ class Shell::Instance : public mojom::Connector, Binding pid_receiver_binding_; BindingSet connectors_; BindingSet shell_bindings_; - mojom::ShellClientFactoryPtr factory_; NativeRunner* runner_ = nullptr; base::ProcessId pid_ = base::kNullProcessId; base::WeakPtrFactory weak_factory_; @@ -422,8 +421,9 @@ Shell::Shell(scoped_ptr native_runner_factory, weak_ptr_factory_(this) { mojom::ShellClientPtr client; mojom::ShellClientRequest request = GetProxy(&client); - CreateInstance(CreateShellIdentity(), GetPermissiveCapabilities(), - std::move(client)); + Instance* instance = CreateInstance(CreateShellIdentity(), + GetPermissiveCapabilities()); + instance->StartWithClient(std::move(client)); shell_connection_.reset(new ShellConnection(this, std::move(request))); if (catalog) @@ -492,7 +492,9 @@ bool Shell::AcceptConnection(Connection* connection) { // Shell, private: void Shell::InitCatalog(mojom::ShellClientPtr catalog) { - CreateInstance(CreateCatalogIdentity(), CapabilitySpec(), std::move(catalog)); + Instance* instance = + CreateInstance(CreateCatalogIdentity(), CapabilitySpec()); + instance->StartWithClient(std::move(catalog)); // TODO(beng): this doesn't work anymore. // Seed the catalog with manifest info for the shell & catalog. @@ -582,10 +584,9 @@ bool Shell::ConnectToExistingInstance(scoped_ptr* params) { } Shell::Instance* Shell::CreateInstance(const Identity& target, - const CapabilitySpec& spec, - mojom::ShellClientPtr client) { + const CapabilitySpec& spec) { CHECK(target.user_id() != mojom::kInheritUserID); - Instance* instance = new Instance(std::move(client), this, target, spec); + Instance* instance = new Instance(this, target, spec); DCHECK(identity_to_instance_.find(target) == identity_to_instance_.end()); identity_to_instance_[target] = instance; @@ -594,7 +595,6 @@ Shell::Instance* Shell::CreateInstance(const Identity& target, [this, &info](mojom::InstanceListener* listener) { listener->InstanceCreated(info.Clone()); }); - instance->InitializeClient(); return instance; } @@ -608,10 +608,10 @@ void Shell::AddInstanceListener(mojom::InstanceListenerPtr listener) { instance_listeners_.AddInterfacePtr(std::move(listener)); } -void Shell::CreateShellClient(const Identity& source, - const Identity& shell_client_factory, - const std::string& name, - mojom::ShellClientRequest request) { +void Shell::CreateShellClientWithFactory(const Identity& source, + const Identity& shell_client_factory, + const std::string& name, + mojom::ShellClientRequest request) { mojom::ShellClientFactory* factory = GetShellClientFactory(shell_client_factory, source); factory->CreateShellClient(std::move(request), name); @@ -674,42 +674,40 @@ void Shell::OnGotResolvedName(mojom::ShellResolverPtr resolver, mojom::ClientProcessConnectionPtr client_process_connection = params->TakeClientProcessConnection(); - - mojom::ShellClientRequest request; - if (!client.is_bound()) - request = GetProxy(&client); - - Instance* instance = CreateInstance(target, capabilities, std::move(client)); - instance->ConnectToClient(std::move(params)); - - // If a ShellClientPtr was provided, there's no more work to do: someone - // is already holding a corresponding ShellClientRequest. - if (!request.is_pending()) - return; - - if (client_process_connection.is_null() && LoadWithLoader(target, &request)) - return; - - CHECK(!file_url.is_null() && !capabilities_ptr.is_null()); - - if (target.name() != resolved_name) { - // In cases where a package alias is resolved, we have to use the instance - // from the original request rather than for the package itself, which will - // always be the same. - CreateShellClient( - source, Identity(resolved_name, target.user_id(), instance_name), - target.name(), std::move(request)); + Instance* instance = CreateInstance(target, capabilities); + + // Below are various paths through which a new Instance can be bound to a + // ShellClient proxy. + if (client.is_bound()) { + // If a ShellClientPtr was provided, there's no more work to do: someone + // is already holding a corresponding ShellClientRequest. + instance->StartWithClient(std::move(client)); + } else if (!client_process_connection.is_null()) { + // Likewise if a ClientProcessConnection was given via Connect(), it + // provides the ShellClient proxy to use. + instance->StartWithClientProcessConnection( + std::move(client_process_connection)); } else { - if (!client_process_connection.is_null()) { - // The client already started a process for this instance, use it. - instance->StartWithClientProcessConnection( - std::move(request), std::move(client_process_connection)); + // Otherwise we create a new ShellClient pipe. + mojom::ShellClientRequest request = GetProxy(&client); + if (LoadWithLoader(target, &request)) { + instance->StartWithClient(std::move(client)); } else { - // Otherwise we make our own process. - instance->StartWithFilePath(std::move(request), - util::UrlToFilePath(file_url.To())); + CHECK(!file_url.is_null() && !capabilities_ptr.is_null()); + + if (target.name() != resolved_name) { + instance->StartWithClient(std::move(client)); + CreateShellClientWithFactory( + source, Identity(resolved_name, target.user_id(), instance_name), + target.name(), std::move(request)); + } else { + instance->StartWithFilePath(util::UrlToFilePath(file_url.To())); + } } } + + // Now that the instance has a ShellClient, we can connect to it. + instance->ConnectToClient(std::move(params)); } bool Shell::LoadWithLoader(const Identity& target, diff --git a/mojo/shell/shell.h b/mojo/shell/shell.h index 03e9bfd..ca5e211 100644 --- a/mojo/shell/shell.h +++ b/mojo/shell/shell.h @@ -111,7 +111,7 @@ class Shell : public ShellClient { // // If |client| is not null, there must not be an instance of the target // application already running. The shell will create a new instance and use - // |client| to control. + // |client| to control it. void Connect(scoped_ptr params, mojom::ShellClientPtr client); // Returns a running instance matching |identity|. @@ -128,22 +128,21 @@ class Shell : public ShellClient { bool ConnectToExistingInstance(scoped_ptr* params); Instance* CreateInstance(const Identity& target, - const CapabilitySpec& spec, - mojom::ShellClientPtr client); + const CapabilitySpec& spec); // Called from the instance implementing mojom::Shell. void AddInstanceListener(mojom::InstanceListenerPtr listener); - void CreateShellClient(const Identity& source, - const Identity& shell_client_factory, - const std::string& name, - mojom::ShellClientRequest request); - // Returns a running ShellClientFactory for |shell_client_factory_identity|, - // if there is not one running one is started for |source_identity|. + void CreateShellClientWithFactory(const Identity& source, + const Identity& shell_client_factory, + const std::string& name, + mojom::ShellClientRequest request); + // Returns a running ShellClientFactory for |shell_client_factory_identity|. + // If there is not one running one is started for |source_identity|. mojom::ShellClientFactory* GetShellClientFactory( const Identity& shell_client_factory_identity, const Identity& source_identity); - void OnShellClientFactoryLost(const Identity& which);; + void OnShellClientFactoryLost(const Identity& which); // Callback when remote Catalog resolves mojo:foo to mojo:bar. // |params| are the params passed to Connect(). @@ -181,7 +180,7 @@ class Shell : public ShellClient { IdentityToInstanceMap identity_to_instance_; IdentityToShellClientFactoryMap shell_client_factories_; - // Counter used to assign ids to content handlers. + // Counter used to assign ids to client factories. uint32_t shell_client_factory_id_counter_; InterfacePtrSet instance_listeners_; diff --git a/mojo/shell/tests/connect/connect_test_driver.cc b/mojo/shell/tests/connect/connect_test_driver.cc index 9e5a4cc..ace64df 100644 --- a/mojo/shell/tests/connect/connect_test_driver.cc +++ b/mojo/shell/tests/connect/connect_test_driver.cc @@ -37,6 +37,10 @@ class Driver : public mojo::ShellClient, connection->AddInterface(this); return true; } + void ShellConnectionLost() override { + // TODO: This should exit cleanly. + _exit(1); + } // mojo::InterfaceFactory: void Create(mojo::Connection* connection, diff --git a/mojo/shell/tests/shell/driver.cc b/mojo/shell/tests/shell/driver.cc index 159920d..9d8af6b 100644 --- a/mojo/shell/tests/shell/driver.cc +++ b/mojo/shell/tests/shell/driver.cc @@ -29,6 +29,7 @@ #include "mojo/shell/public/interfaces/connector.mojom.h" #include "mojo/shell/public/interfaces/shell.mojom.h" #include "mojo/shell/runner/child/test_native_main.h" +#include "mojo/shell/runner/common/client_util.h" #include "mojo/shell/runner/common/switches.h" #include "mojo/shell/runner/init.h" #include "mojo/shell/tests/shell/shell_unittest.mojom.h" @@ -71,25 +72,14 @@ class Driver : public mojo::ShellClient, platform_channel_pair.PrepareToPassClientHandleToChildProcess( &child_command_line, &handle_passing_info); - // Generate a token for the child to find and connect to a primordial pipe - // and pass that as well. - std::string primordial_pipe_token = mojo::edk::GenerateRandomToken(); - child_command_line.AppendSwitchASCII(switches::kPrimordialPipeToken, - primordial_pipe_token); - - // Allocate the pipe locally. - mojo::ScopedMessagePipeHandle pipe = - mojo::edk::CreateParentMessagePipe(primordial_pipe_token); - - mojo::shell::mojom::ShellClientFactoryPtr factory; - factory.Bind(mojo::InterfacePtrInfo( - std::move(pipe), 0u)); + mojo::shell::mojom::ShellClientPtr client = + mojo::shell::PassShellClientRequestOnCommandLine(&child_command_line); mojo::shell::mojom::PIDReceiverPtr receiver; mojo::Identity target("exe:shell_unittest_target", mojo::shell::mojom::kInheritUserID); mojo::Connector::ConnectParams params(target); - params.set_client_process_connection(std::move(factory), + params.set_client_process_connection(std::move(client), GetProxy(&receiver)); scoped_ptr connection = connector->Connect(¶ms); connection->AddConnectionCompletedClosure( diff --git a/mojo/shell/tests/util.cc b/mojo/shell/tests/util.cc index d57d311..8326ebe 100644 --- a/mojo/shell/tests/util.cc +++ b/mojo/shell/tests/util.cc @@ -66,13 +66,13 @@ scoped_ptr LaunchAndConnectToProcess( mojo::ScopedMessagePipeHandle pipe = mojo::edk::CreateParentMessagePipe(primordial_pipe_token); - mojo::shell::mojom::ShellClientFactoryPtr factory; - factory.Bind(mojo::InterfacePtrInfo( + mojo::shell::mojom::ShellClientPtr client; + client.Bind(mojo::InterfacePtrInfo( std::move(pipe), 0u)); mojo::shell::mojom::PIDReceiverPtr receiver; mojo::Connector::ConnectParams params(target); - params.set_client_process_connection(std::move(factory), GetProxy(&receiver)); + params.set_client_process_connection(std::move(client), GetProxy(&receiver)); scoped_ptr connection = connector->Connect(¶ms); { base::RunLoop loop; -- cgit v1.1