[Mojo] Channel::RemoteProcessLaunched->RemoteProcessLaunchAttempted.

Replaces the Channel::RemoteProcessLaunched API, which callers had to
invoke only if they successfully launched a process with the Channel
handle, with RemoteProcessLaunchAttempted, which must always be called
regardless of whether process-launch was successful, so long as it was
attempted with base::LaunchProcess.

This is required under Fuchsia, where the remote endpoint handle is
transferred, rather than cloned, and the LaunchProcess API consumes the
handle even on failure, for consistency.

On POSIX and Windows the effect is just to close the remote endpoint
handle earlier in the case of failure to launch.

Bug: 848028, 754449
Change-Id: Ia79efebdc8b629f3d579a76c941283d461980e4c
Reviewed-on: https://chromium-review.googlesource.com/1086385Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564620}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc

@@ -167,9 +167,9 @@ ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread() {
           ? g_test_delegate->LaunchTestProcess(cleaner_command_line_,
                                                launch_options)
           : base::LaunchProcess(cleaner_command_line_, launch_options);
+  channel.RemoteProcessLaunchAttempted();
   if (!cleaner_process.IsValid())
     return ProcessStatus(LaunchStatus::kLaunchFailed);
-  channel.RemoteProcessLaunched();
 
   // ChromePromptImpl tasks will need to run on the IO thread. There is no
   // need to synchronize its creation, since the client end will wait for this

chrome/utility/importer/firefox_importer_unittest_utils_mac.cc

@@ -200,8 +200,8 @@ bool FFUnitTestDecryptorProxy::Setup(const base::FilePath& nss_path) {
   child_process_ = LaunchNSSDecrypterChildProcess(
       nss_path, channel.TakeRemoteEndpoint().TakePlatformHandle().TakeFD(),
       token);
+  channel.RemoteProcessLaunchAttempted();
   if (child_process_.IsValid()) {
-    channel.RemoteProcessLaunched();
     mojo::OutgoingInvitation::Send(std::move(invitation),
                                    child_process_.Handle(),
                                    channel.TakeLocalEndpoint());

content/browser/child_process_launcher_helper.cc

@@ -126,7 +126,7 @@ void ChildProcessLauncherHelper::PostLaunchOnLauncherThread(
     ChildProcessLauncherHelper::Process process,
     int launch_result) {
   if (mojo_channel_)
-    mojo_channel_->RemoteProcessLaunched();
+    mojo_channel_->RemoteProcessLaunchAttempted();
 
   if (process.process.IsValid()) {
     RecordHistogramsOnLauncherThread(base::TimeTicks::Now() -

mojo/edk/system/invitation_unittest.cc

@@ -303,7 +303,7 @@ base::Process InvitationTest::LaunchChildTestClient(
   base::Process child_process = base::SpawnMultiProcessTestChild(
       test_client_name, command_line, launch_options);
   if (channel)
-    channel->RemoteProcessLaunched();
+    channel->RemoteProcessLaunchAttempted();
 
   MojoPlatformProcessHandle process_handle;
   process_handle.struct_size = sizeof(process_handle);

mojo/edk/test/multiprocess_test_helper.cc

@@ -218,7 +218,7 @@ ScopedMessagePipeHandle MultiprocessTestHelper::StartChildWithExtraSwitch(
   test_child_ =
       base::SpawnMultiProcessTestChild(test_child_main, command_line, options);
   if (launch_type == LaunchType::CHILD || launch_type == LaunchType::PEER)
-    channel.RemoteProcessLaunched();
+    channel.RemoteProcessLaunchAttempted();
 
   if (launch_type == LaunchType::CHILD) {
     DCHECK(local_channel_endpoint.is_valid());

mojo/public/cpp/platform/platform_channel.cc

@@ -207,10 +207,11 @@ void PlatformChannel::PrepareToPassRemoteEndpoint(
     command_line->AppendSwitchASCII(kHandleSwitch, value);
 }
 
-void PlatformChannel::RemoteProcessLaunched() {
+void PlatformChannel::RemoteProcessLaunchAttempted() {
 #if defined(OS_FUCHSIA)
-  // Unlike other platforms, Fuchsia just transfers handle ownership to the
-  // launcher process rather than duplicating it.
+  // Unlike other platforms, Fuchsia transfers handle ownership to the new
+  // process, rather than duplicating it. For consistency the process-launch
+  // call will have consumed the handle regardless of whether launch succeeded.
   DCHECK(remote_endpoint_.platform_handle().is_valid_handle());
   ignore_result(remote_endpoint_.TakePlatformHandle().ReleaseHandle());
 #else

mojo/public/cpp/platform/platform_channel.h

@@ -74,8 +74,9 @@ class COMPONENT_EXPORT(MOJO_CPP_PLATFORM) PlatformChannel {
   // passed on the new process's command line.
   //
   // **NOTE**: If this method is called it is important to also call
-  // |RemoteProcessLaunched()| on this PlatformChanenl *after* the process has
-  // launched. Failing to do so can result in leaked handles.
+  // |RemoteProcessLaunchAttempted()| on this PlatformChannel *after* attempting
+  // to launch the new process, regardless of whether the attempt succeeded.
+  // Failing to do so can result in leaked handles.
   void PrepareToPassRemoteEndpoint(HandlePassingInfo* info, std::string* value);
 
   // Like above but modifies |*command_line| to include the endpoint string
@@ -83,11 +84,11 @@ class COMPONENT_EXPORT(MOJO_CPP_PLATFORM) PlatformChannel {
   void PrepareToPassRemoteEndpoint(HandlePassingInfo* info,
                                    base::CommandLine* command_line);
 
-  // Must be called after the corresponding process launch if
+  // Must be called after the corresponding process launch attempt if
   // |PrepareToPassRemoteEndpoint()| was used.
-  void RemoteProcessLaunched();
+  void RemoteProcessLaunchAttempted();
 
-  // Recovers and endpoint handle which was passed to the calling process by
+  // Recovers an endpoint handle which was passed to the calling process by
   // its creator. |value| is a string returned by
   // |PrepareToPassRemoteEndpoint()| in the creator's process.
   static PlatformChannelEndpoint RecoverPassedEndpointFromString(

mojo/public/cpp/system/tests/invitation_unittest.cc

@@ -97,7 +97,7 @@ class InvitationCppTest : public testing::Test,
     child_process_ = base::SpawnMultiProcessTestChild(
         test_client_name, command_line, launch_options);
     if (channel)
-      channel->RemoteProcessLaunched();
+      channel->RemoteProcessLaunchAttempted();
 
     OutgoingInvitation invitation;
     for (uint64_t name = 0; name < num_primordial_pipes; ++name)

services/service_manager/runner/host/service_process_launcher.cc

@@ -125,6 +125,8 @@ void ServiceProcessLauncher::DidStart(ProcessReadyCallback callback) {
 
 void ServiceProcessLauncher::DoLaunch(
     std::unique_ptr<base::CommandLine> child_command_line) {
+  DCHECK(channel_);
+
   if (delegate_) {
     delegate_->AdjustCommandLineArgumentsForTarget(target_,
                                                    child_command_line.get());
@@ -188,6 +190,8 @@ void ServiceProcessLauncher::DoLaunch(
 #endif
   }
 
+  channel_->RemoteProcessLaunchAttempted();
+
   if (child_process_.IsValid()) {
 #if defined(OS_CHROMEOS)
     // Always log instead of DVLOG because knowing which pid maps to which
@@ -201,12 +205,9 @@ void ServiceProcessLauncher::DoLaunch(
         << ", instance=" << target_.instance() << ", name=" << target_.name()
         << ", user_id=" << target_.user_id();
 
-    if (channel_) {
-      channel_->RemoteProcessLaunched();
-      mojo::OutgoingInvitation::Send(std::move(invitation_),
-                                     child_process_.Handle(),
-                                     channel_->TakeLocalEndpoint());
-    }
+    mojo::OutgoingInvitation::Send(std::move(invitation_),
+                                   child_process_.Handle(),
+                                   channel_->TakeLocalEndpoint());
   }
   start_child_process_event_.Signal();
 }

services/service_manager/runner/host/service_process_launcher_unittest.cc

@@ -31,10 +31,11 @@ const base::FilePath::CharType kServiceExtension[] =
     FILE_PATH_LITERAL(".service");
 #endif
 
-void ProcessReadyCallbackAdapater(const base::Closure& callback,
-                                  base::ProcessId process_id) {
-  EXPECT_NE(process_id, base::kNullProcessId);
-  callback.Run();
+void ProcessReadyCallbackAdapter(bool expect_process_id_valid,
+                                 base::OnceClosure callback,
+                                 base::ProcessId process_id) {
+  EXPECT_EQ(expect_process_id_valid, process_id != base::kNullProcessId);
+  std::move(callback).Run();
 }
 
 class ServiceProcessLauncherDelegateImpl
@@ -87,7 +88,8 @@ TEST(ServiceProcessLauncherTest, MAYBE_StartJoin) {
   base::RunLoop run_loop;
   launcher.Start(
       Identity(), SANDBOX_TYPE_NO_SANDBOX,
-      base::Bind(&ProcessReadyCallbackAdapater, run_loop.QuitClosure()));
+      base::BindOnce(&ProcessReadyCallbackAdapter,
+                     true /*expect_process_id_valid*/, run_loop.QuitClosure()));
   run_loop.Run();
 
   launcher.Join();
@@ -96,5 +98,33 @@ TEST(ServiceProcessLauncherTest, MAYBE_StartJoin) {
   EXPECT_EQ(1u, service_process_launcher_delegate.get_and_clear_adjust_count());
 }
 
+#if !defined(OS_POSIX) || defined(OS_MACOSX)
+// Verify that if ServiceProcessLauncher cannot launch a process running the
+// service from the specified path, then we are able to clean up without e.g.
+// double-freeing the platform-channel handle reserved for the peer.
+// This test won't work as-is on POSIX platforms, where we use fork()+exec() to
+// launch child processes, since we won't fail until exec(), therefore the test
+// will see a valid child process-Id. We use posix_spawn() on Mac OS X.
+TEST(ServiceProcessLauncherTest, FailToLaunchProcess) {
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+
+  // Pick a service path that could not possibly ever exist.
+  base::FilePath test_service_path(FILE_PATH_LITERAL("rockot@_rules.service"));
+
+  ServiceProcessLauncherDelegateImpl service_process_launcher_delegate;
+  ServiceProcessLauncher launcher(&service_process_launcher_delegate,
+                                  test_service_path);
+  base::RunLoop run_loop;
+  launcher.Start(Identity(), SANDBOX_TYPE_NO_SANDBOX,
+                 base::BindOnce(&ProcessReadyCallbackAdapter,
+                                false /*expect_process_id_valid*/,
+                                run_loop.QuitClosure()));
+  run_loop.Run();
+
+  launcher.Join();
+  scoped_task_environment.RunUntilIdle();
+}
+#endif  //  !defined(OS_POSIX) || defined(OS_MACOSX)
+
 }  // namespace
 }  // namespace service_manager

services/service_manager/tests/service_manager/service_manager_unittest.cc

@@ -292,7 +292,7 @@ class ServiceManagerTest : public test::ServiceTest,
 
     target_ = base::LaunchProcess(child_command_line, options);
     DCHECK(target_.IsValid());
-    channel.RemoteProcessLaunched();
+    channel.RemoteProcessLaunchAttempted();
     receiver->SetPID(target_.Pid());
     mojo::OutgoingInvitation::Send(std::move(invitation), target_.Handle(),
                                    channel.TakeLocalEndpoint());

services/service_manager/tests/util.cc

@@ -93,7 +93,7 @@ mojom::ConnectResult LaunchAndConnectToProcess(
 #endif
   *process = base::LaunchProcess(child_command_line, options);
   DCHECK(process->IsValid());
-  channel.RemoteProcessLaunched();
+  channel.RemoteProcessLaunchAttempted();
   receiver->SetPID(process->Pid());
   mojo::OutgoingInvitation::Send(std::move(invitation), process->Handle(),
                                  channel.TakeLocalEndpoint());