Updater: Poll launchd for control service record

Because chrome/common/mac/launchd does not wait for process exit, and
because the launchctl process itself may exit prior to setting up the new
service, we must wait at certain points during updater setup to ensure
that services are installed prior to their use.

This CL replaces the naive 3 second wait with a 500ms polling loop that
has a 3 second timeout.

Bug: 1122120
Change-Id: I1c67bba21f223ec0691b55b9fe72eb28a03e98e9
Fixed: 1122120
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429309
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812130}

chrome/updater/BUILD.gn

@@ -129,6 +129,8 @@ if (is_win || is_mac) {
         "app/server/mac/update_service_wrappers.mm",
         "external_constants_mac.mm",
         "installer_mac.cc",
+        "launchd_util.cc",
+        "launchd_util.h",
         "lib_util_mac.mm",
         "mac/control_service_out_of_process.h",
         "mac/control_service_out_of_process.mm",
@@ -136,7 +138,7 @@ if (is_win || is_mac) {
         "mac/update_service_out_of_process.mm",
         "prefs_mac.mm",
         "service_factory_mac.mm",
-        "setup_mac.cc",
+        "setup_mac.mm",
       ]
     }
 

chrome/updater/app/app_install.cc

@@ -90,19 +90,15 @@ void AppInstall::FirstTaskRun() {
   splash_screen_ = splash_screen_maker_.Run();
   splash_screen_->Show();
 
-  base::ThreadPool::PostTask(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+  InstallCandidate(
+      false,
       base::BindOnce(
-          &InstallCandidate, false, base::SequencedTaskRunnerHandle::Get(),
-          base::BindOnce(
-              [](SplashScreen* splash_screen,
-                 base::OnceCallback<void(int)> done, int result) {
-                splash_screen->Dismiss(base::BindOnce(std::move(done), result));
-              },
-              splash_screen_.get(),
-              base::BindOnce(&AppInstall::InstallCandidateDone, this))));
+          [](SplashScreen* splash_screen, base::OnceCallback<void(int)> done,
+             int result) {
+            splash_screen->Dismiss(base::BindOnce(std::move(done), result));
+          },
+          splash_screen_.get(),
+          base::BindOnce(&AppInstall::InstallCandidateDone, this)));
 }
 
 void AppInstall::InstallCandidateDone(int result) {

chrome/updater/app/app_update.cc

@@ -42,11 +42,7 @@ void AppUpdate::Uninitialize() {
 }
 
 void AppUpdate::FirstTaskRun() {
-  base::ThreadPool::PostTask(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&InstallCandidate, false,
-                     base::SequencedTaskRunnerHandle::Get(),
-                     base::BindOnce(&AppUpdate::SetupDone, this)));
+  InstallCandidate(false, base::BindOnce(&AppUpdate::SetupDone, this));
 }
 
 void AppUpdate::SetupDone(int result) {

chrome/updater/launchd_util.cc

+// Copyright 2020 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 "chrome/updater/launchd_util.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/process/launch.h"
+#include "base/process/process.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/time/time.h"
+
+namespace updater {
+
+namespace {
+
+void PollLaunchctlListImpl(const std::string& service,
+                           base::TimeTicks deadline,
+                           base::OnceCallback<void(bool)> callback) {
+  if (base::TimeTicks::Now() > deadline) {
+    std::move(callback).Run(false);
+    return;
+  }
+  base::CommandLine command_line(base::FilePath("/bin/launchctl"));
+  command_line.AppendArg("list");
+  command_line.AppendArg(service);
+  base::Process process = base::LaunchProcess(command_line, {});
+  if (!process.IsValid()) {
+    VLOG(1) << "Failed to launch launchctl process.";
+    std::move(callback).Run(false);
+    return;
+  }
+  int exit_code = 0;
+  if (!process.WaitForExitWithTimeout(deadline - base::TimeTicks::Now(),
+                                      &exit_code)) {
+    std::move(callback).Run(false);
+    return;
+  }
+  if (exit_code == 0) {
+    std::move(callback).Run(true);
+    return;
+  }
+  base::ThreadPool::PostDelayedTask(
+      FROM_HERE, {base::WithBaseSyncPrimitives()},
+      base::BindOnce(&PollLaunchctlListImpl, service, deadline,
+                     std::move(callback)),
+      base::TimeDelta::FromMilliseconds(500));
+}
+
+}  // namespace
+
+void PollLaunchctlList(const std::string& service,
+                       base::TimeDelta timeout,
+                       base::OnceCallback<void(bool)> callback) {
+  base::ThreadPool::PostTask(
+      FROM_HERE, {base::WithBaseSyncPrimitives()},
+      base::BindOnce(
+          &PollLaunchctlListImpl, service, base::TimeTicks::Now() + timeout,
+          base::BindOnce(
+              [](scoped_refptr<base::TaskRunner> runner,
+                 base::OnceCallback<void(bool)> callback, bool result) {
+                runner->PostTask(FROM_HERE,
+                                 base::BindOnce(std::move(callback), result));
+              },
+              base::SequencedTaskRunnerHandle::Get(), std::move(callback))));
+}
+
+}  // namespace updater

chrome/updater/launchd_util.h

+// Copyright 2020 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 CHROME_UPDATER_LAUNCHD_UTIL_H_
+#define CHROME_UPDATER_LAUNCHD_UTIL_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+
+namespace base {
+class TimeDelta;
+}  // namespace base
+
+namespace updater {
+
+// Query `launchctl list |service|` with retries, calling |callback| with
+// 'true' if launchctl has the service, and false if |timeout| is reached.
+// Must be called in a sequence. The callback is posted to the same sequence.
+void PollLaunchctlList(const std::string& service,
+                       base::TimeDelta timeout,
+                       base::OnceCallback<void(bool)> callback);
+
+}  // namespace updater
+
+#endif  // CHROME_UPDATER_LAUNCHD_UTIL_H_

chrome/updater/mac/setup/setup.h

@@ -57,12 +57,15 @@ constexpr int kFailedToStartLaunchdControlJob = 42;
 // Failed to start the wake job.
 constexpr int kFailedToStartLaunchdWakeJob = 43;
 
+// Timed out while awaiting launchctl to become aware of the control job.
+constexpr int kFailedAwaitingLaunchdControlJob = 44;
+
 }  // namespace setup_exit_codes
 
 // Sets up the candidate updater by copying the bundle, creating launchd plists
 // for administration service and XPC service tasks, and start the corresponding
 // launchd jobs.
-int InstallCandidate();
+int Setup();
 
 // Uninstalls this version of the updater.
 int UninstallCandidate();

chrome/updater/mac/setup/setup.mm

@@ -320,7 +320,7 @@ bool DeleteDataFolder() {
 
 }  // namespace
 
-int InstallCandidate() {
+int Setup() {
   const base::FilePath dest_path = GetVersionedUpdaterFolderPath();
 
   if (!CopyBundle(dest_path))

chrome/updater/mac/xpc_service_names.h

@@ -10,6 +10,8 @@
 
 namespace updater {
 
+extern const char kControlLaunchdName[];
+
 base::ScopedCFTypeRef<CFStringRef> CopyServiceLaunchdName();
 base::ScopedCFTypeRef<CFStringRef> CopyWakeLaunchdName();
 base::ScopedCFTypeRef<CFStringRef> CopyControlLaunchdName();

chrome/updater/mac/xpc_service_names.mm

@@ -12,6 +12,9 @@
 
 namespace updater {
 
+const char kControlLaunchdName[] =
+    MAC_BUNDLE_IDENTIFIER_STRING ".control." UPDATER_VERSION_STRING;
+
 base::ScopedCFTypeRef<CFStringRef> CopyServiceLaunchdName() {
   return base::SysUTF8ToCFStringRef(MAC_BUNDLE_IDENTIFIER_STRING ".service");
 }
@@ -22,8 +25,7 @@ base::ScopedCFTypeRef<CFStringRef> CopyWakeLaunchdName() {
 }
 
 base::ScopedCFTypeRef<CFStringRef> CopyControlLaunchdName() {
-  return base::SysUTF8ToCFStringRef(MAC_BUNDLE_IDENTIFIER_STRING
-                                    ".control." UPDATER_VERSION_STRING);
+  return base::SysUTF8ToCFStringRef(kControlLaunchdName);
 }
 
 base::scoped_nsobject<NSString> GetServiceLaunchdLabel() {

chrome/updater/setup.h

@@ -8,15 +8,11 @@
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 
-namespace base {
-class TaskRunner;
-}  // namespace base
-
 namespace updater {
 
-// Installs the candidate, then posts |callback| to |runner|.
+// Installs the candidate, then posts |callback| to the main sequence. Must
+// be called on the main sequence.
 void InstallCandidate(bool is_machine,
-                      scoped_refptr<base::TaskRunner> runner,
                       base::OnceCallback<void(int)> callback);
 
 }  // namespace updater

chrome/updater/setup_mac.cc

-// Copyright 2020 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 "chrome/updater/mac/setup/setup.h"
-#include "chrome/updater/setup.h"
-
-#include "base/bind.h"
-#include "base/callback.h"
-#include "base/task_runner.h"
-
-namespace updater {
-
-void InstallCandidate(bool is_machine,
-                      scoped_refptr<base::TaskRunner> runner,
-                      base::OnceCallback<void(int)> callback) {
-  runner->PostDelayedTask(
-      FROM_HERE, base::BindOnce(std::move(callback), InstallCandidate()),
-      base::TimeDelta::FromSeconds(3));
-}
-
-}  // namespace updater

chrome/updater/setup_mac.mm

+// Copyright 2020 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 "chrome/updater/mac/setup/setup.h"
+#include "chrome/updater/setup.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/time/time.h"
+#include "chrome/updater/launchd_util.h"
+#include "chrome/updater/mac/xpc_service_names.h"
+
+namespace updater {
+
+namespace {
+
+void SetupDone(base::OnceCallback<void(int)> callback, int result) {
+  if (result != setup_exit_codes::kSuccess) {
+    std::move(callback).Run(result);
+    return;
+  }
+  PollLaunchctlList(
+      kControlLaunchdName, base::TimeDelta::FromSeconds(3),
+      base::BindOnce(
+          [](base::OnceCallback<void(int)> callback, bool service_exists) {
+            std::move(callback).Run(
+                service_exists
+                    ? setup_exit_codes::kSuccess
+                    : setup_exit_codes::kFailedAwaitingLaunchdControlJob);
+          },
+          std::move(callback)));
+}
+
+}  // namespace
+
+void InstallCandidate(bool is_machine, base::OnceCallback<void(int)> callback) {
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::MayBlock()}, base::BindOnce(&Setup),
+      base::BindOnce(&SetupDone, std::move(callback)));
+}
+
+}  // namespace updater

chrome/updater/setup_win.cc

@@ -7,15 +7,16 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/task_runner.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 
 namespace updater {
 
 void InstallCandidate(bool is_machine,
-                      scoped_refptr<base::TaskRunner> runner,
                       base::OnceCallback<void(int)> callback) {
-  runner->PostTask(FROM_HERE,
-                   base::BindOnce(std::move(callback), Setup(is_machine)));
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::MayBlock()}, base::BindOnce(&Setup, is_machine),
+      std::move(callback));
 }
 
 }  // namespace updater