Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Unified Diff: mojo/shell/runner/host/child_process_host.cc

Issue 1678333003: Revert of [mojo-edk] Simplify multiprocess pipe bootstrap (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Created 4 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « mojo/shell/runner/host/child_process_host.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: mojo/shell/runner/host/child_process_host.cc
diff --git a/mojo/shell/runner/host/child_process_host.cc b/mojo/shell/runner/host/child_process_host.cc
index 2b3cfc943c7d5893ecb6ab3e6835d4355c37d0fb..9a5b439fa2d8ecbbe6d4eb866f361b093d185bcc 100644
--- a/mojo/shell/runner/host/child_process_host.cc
+++ b/mojo/shell/runner/host/child_process_host.cc
@@ -35,24 +35,55 @@
namespace mojo {
namespace shell {
+ChildProcessHost::PipeHolder::PipeHolder() {}
+
+void ChildProcessHost::PipeHolder::Reject() {
+ base::AutoLock lock(lock_);
+ reject_pipe_ = true;
+ pipe_.reset();
+}
+
+void ChildProcessHost::PipeHolder::SetPipe(ScopedMessagePipeHandle pipe) {
+ base::AutoLock lock(lock_);
+ DCHECK(!pipe_.is_valid());
+ if (!reject_pipe_)
+ pipe_ = std::move(pipe);
+}
+
+ScopedMessagePipeHandle ChildProcessHost::PipeHolder::PassPipe() {
+ base::AutoLock lock(lock_);
+ DCHECK(pipe_.is_valid());
+ return std::move(pipe_);
+}
+
+ChildProcessHost::PipeHolder::~PipeHolder() {}
+
ChildProcessHost::ChildProcessHost(base::TaskRunner* launch_process_runner,
bool start_sandboxed,
const base::FilePath& app_path)
: launch_process_runner_(launch_process_runner),
start_sandboxed_(start_sandboxed),
app_path_(app_path),
+ channel_info_(nullptr),
start_child_process_event_(false, false),
weak_factory_(this) {
- node_channel_.reset(new edk::PlatformChannelPair);
- primordial_pipe_token_ = edk::GenerateRandomToken();
- controller_.Bind(
- InterfacePtrInfo<mojom::ChildController>(
- edk::CreateParentMessagePipe(primordial_pipe_token_), 0u));
+ pipe_holder_ = new PipeHolder();
+ if (base::CommandLine::ForCurrentProcess()->HasSwitch("use-new-edk")) {
+ node_channel_.reset(new edk::PlatformChannelPair);
+ primordial_pipe_token_ = edk::GenerateRandomToken();
+ } else {
+ pipe_holder_->SetPipe(embedder::CreateChannel(
+ platform_channel_pair_.PassServerHandle(),
+ base::Bind(&ChildProcessHost::DidCreateChannel, base::Unretained(this)),
+ base::ThreadTaskRunnerHandle::Get()));
+ OnMessagePipeCreated();
+ }
}
ChildProcessHost::ChildProcessHost(ScopedHandle channel)
: launch_process_runner_(nullptr),
start_sandboxed_(false),
+ channel_info_(nullptr),
start_child_process_event_(false, false),
weak_factory_(this) {
CHECK(channel.is_valid());
@@ -68,11 +99,40 @@
void ChildProcessHost::Start(const ProcessReadyCallback& callback) {
DCHECK(!child_process_.IsValid());
+ DCHECK(process_ready_callback_.is_null());
+
+ process_ready_callback_ = callback;
+ if (base::CommandLine::ForCurrentProcess()->HasSwitch("use-new-edk")) {
+ // With the new EDK, bootstrap message pipes are created asynchronously.
+ // We recieve the bound pipe (if successful) on an arbitrary thread,
+ // stash it in the thread-safe |pipe_holder_|, and then try to call
+ // OnMessagePipeCreated() on the host's main thread.
+ //
+ // Because of the way the launcher process shuts down, it's possible for
+ // the main thread's MessageLoop to stop running (but not yet be destroyed!)
+ // while this boostrap is pending, resulting in OnMessagePipeCreated() never
+ // being called.
+ //
+ // A typical child process (i.e. one using ShellConnection to bind the other
+ // end of this pipe) may hang forever waiting for an Initialize() message
+ // unless the pipe is closed. This in turn means that Join() could hang
+ // waiting for the process to exit. Deadlock!
+ //
+ // |pipe_holder_| exists for this reason. If it's still holding onto the
+ // pipe when Join() is called, the pipe will be closed.
+ DCHECK(!primordial_pipe_token_.empty());
+ edk::CreateParentMessagePipe(
+ primordial_pipe_token_,
+ base::Bind(&OnParentMessagePipeCreated, pipe_holder_,
+ base::ThreadTaskRunnerHandle::Get(),
+ base::Bind(&ChildProcessHost::OnMessagePipeCreated,
+ weak_factory_.GetWeakPtr())));
+ }
+
launch_process_runner_->PostTaskAndReply(
FROM_HERE,
base::Bind(&ChildProcessHost::DoLaunch, base::Unretained(this)),
- base::Bind(&ChildProcessHost::DidStart,
- weak_factory_.GetWeakPtr(), callback));
+ base::Bind(&ChildProcessHost::DidStart, weak_factory_.GetWeakPtr()));
}
int ChildProcessHost::Join() {
@@ -81,6 +141,10 @@
controller_ = mojom::ChildControllerPtr();
DCHECK(child_process_.IsValid());
+
+ // Ensure the child pipe is closed even if it wasn't yet connected to the
+ // controller.
+ pipe_holder_->Reject();
int rv = -1;
LOG_IF(ERROR, !child_process_.WaitForExit(&rv))
@@ -108,11 +172,11 @@
controller_->ExitNow(exit_code);
}
-void ChildProcessHost::DidStart(const ProcessReadyCallback& callback) {
+void ChildProcessHost::DidStart() {
DVLOG(2) << "ChildProcessHost::DidStart()";
if (child_process_.IsValid()) {
- callback.Run(child_process_.Pid());
+ MaybeNotifyProcessReady();
} else {
LOG(ERROR) << "Failed to start child process";
AppCompleted(MOJO_RESULT_UNKNOWN);
@@ -214,5 +278,34 @@
}
}
+void ChildProcessHost::DidCreateChannel(embedder::ChannelInfo* channel_info) {
+ DVLOG(2) << "AppChildProcessHost::DidCreateChannel()";
+
+ DCHECK(channel_info ||
+ base::CommandLine::ForCurrentProcess()->HasSwitch("use-new-edk"));
+ channel_info_ = channel_info;
+}
+
+void ChildProcessHost::OnMessagePipeCreated() {
+ controller_.Bind(
+ InterfacePtrInfo<mojom::ChildController>(pipe_holder_->PassPipe(), 0u));
+ MaybeNotifyProcessReady();
+}
+
+void ChildProcessHost::MaybeNotifyProcessReady() {
+ if (controller_.is_bound() && child_process_.IsValid())
+ process_ready_callback_.Run(child_process_.Pid());
+}
+
+// static
+void ChildProcessHost::OnParentMessagePipeCreated(
+ scoped_refptr<PipeHolder> holder,
+ scoped_refptr<base::TaskRunner> callback_task_runner,
+ const base::Closure& callback,
+ ScopedMessagePipeHandle pipe) {
+ holder->SetPipe(std::move(pipe));
+ callback_task_runner->PostTask(FROM_HERE, callback);
+}
+
} // namespace shell
} // namespace mojo
« no previous file with comments | « mojo/shell/runner/host/child_process_host.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698