chromeos: Introduce LacrosManager.

Currently LacrosLoader had two responsibilities:
- Loading Lacros via ComponentUpdater.
- Launching Lacros executable.

This CL splits the responsibility. The latter one is
now LacrosManager's responsibility, and it delegates the
loading to LacrosLoader.

      Flipped the flag, and made sure loading works.

Bug: 1094106
Test: Ran lacros-chrome locally.
Change-Id: Ic18dc38641d1a28149fda3fce8619aefd87bc406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2241203
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#778632}

chrome/browser/apps/app_service/app_service_proxy.cc

@@ -175,11 +175,10 @@ void AppServiceProxy::Initialize() {
       plugin_vm_apps_ = std::make_unique<PluginVmApps>(app_service_, profile_);
     }
     if (chromeos::features::IsLacrosSupportEnabled()) {
-      // LacrosApps uses LacrosLoader, which is a singleton. Don't create an
+      // LacrosApps uses LacrosManager, which is a singleton. Don't create an
       // instance of LacrosApps for the lock screen app profile, as we want to
       // maintain a single instance of LacrosApps.
-      // TODO(jamescook): Multiprofile support. Consider adding a list of
-      // "ready" callbacks to LacrosLoader.
+      // TODO(jamescook): Multiprofile support. Consider switching to observers.
       if (!chromeos::ProfileHelper::IsLockScreenAppProfile(profile_)) {
         lacros_apps_ = std::make_unique<LacrosApps>(app_service_);
       }

chrome/browser/apps/app_service/lacros_apps.cc

@@ -9,7 +9,7 @@
 #include "base/bind.h"
 #include "build/branding_buildflags.h"
 #include "chrome/browser/apps/app_service/app_icon_factory.h"
-#include "chrome/browser/chromeos/lacros/lacros_loader.h"
+#include "chrome/browser/chromeos/lacros/lacros_manager.h"
 #include "chrome/grit/chrome_unscaled_resources.h"
 #include "chromeos/constants/chromeos_features.h"
 #include "components/services/app_service/public/mojom/types.mojom.h"
@@ -68,9 +68,9 @@ apps::mojom::IconKeyPtr LacrosApps::NewIconKey(State state) {
 void LacrosApps::Connect(
     mojo::PendingRemote<apps::mojom::Subscriber> subscriber_remote,
     apps::mojom::ConnectOptionsPtr opts) {
-  bool is_ready = LacrosLoader::Get()->IsReady();
+  bool is_ready = chromeos::LacrosManager::Get()->IsReady();
   if (!is_ready) {
-    LacrosLoader::Get()->SetLoadCompleteCallback(base::BindOnce(
+    chromeos::LacrosManager::Get()->SetLoadCompleteCallback(base::BindOnce(
         &LacrosApps::OnLoadComplete, weak_factory_.GetWeakPtr()));
   }
   std::vector<apps::mojom::AppPtr> apps;
@@ -105,7 +105,7 @@ void LacrosApps::Launch(const std::string& app_id,
                         apps::mojom::LaunchSource launch_source,
                         int64_t display_id) {
   DCHECK_EQ(extension_misc::kLacrosAppId, app_id);
-  LacrosLoader::Get()->Start();
+  chromeos::LacrosManager::Get()->Start();
 }
 
 void LacrosApps::GetMenuModel(const std::string& app_id,

chrome/browser/chromeos/BUILD.gn

@@ -1341,6 +1341,8 @@ source_set("chromeos") {
     "kerberos/kerberos_ticket_expiry_notification.h",
     "lacros/lacros_loader.cc",
     "lacros/lacros_loader.h",
+    "lacros/lacros_manager.cc",
+    "lacros/lacros_manager.h",
     "lacros/lacros_util.cc",
     "lacros/lacros_util.h",
     "language_preferences.cc",

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -69,7 +69,7 @@
 #include "chrome/browser/chromeos/extensions/login_screen/login_screen_ui/ui_handler.h"
 #include "chrome/browser/chromeos/external_metrics.h"
 #include "chrome/browser/chromeos/input_method/input_method_configuration.h"
-#include "chrome/browser/chromeos/lacros/lacros_loader.h"
+#include "chrome/browser/chromeos/lacros/lacros_manager.h"
 #include "chrome/browser/chromeos/language_preferences.h"
 #include "chrome/browser/chromeos/lock_screen_apps/state_controller.h"
 #include "chrome/browser/chromeos/logging.h"
@@ -739,10 +739,10 @@ void ChromeBrowserMainPartsChromeos::PreProfileInit() {
       std::make_unique<lock_screen_apps::StateController>();
   lock_screen_apps_state_controller_->Initialize();
 
-  // Always construct LacrosLoader, even if the lacros flag is disabled, so
+  // Always construct LacrosManager, even if the lacros flag is disabled, so
   // it can do cleanup work if needed. Initialized in PreProfileInit because the
   // profile-keyed service AppService can call into it.
-  lacros_loader_ = std::make_unique<LacrosLoader>(
+  lacros_manager_ = std::make_unique<LacrosManager>(
       g_browser_process->platform_part()->cros_component_manager());
 
   if (immediate_login) {
@@ -1029,7 +1029,7 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
 
   BootTimesRecorder::Get()->AddLogoutTimeMarker("UIMessageLoopEnded", true);
 
-  lacros_loader_.reset();
+  lacros_manager_.reset();
 
   if (lock_screen_apps_state_controller_)
     lock_screen_apps_state_controller_->Shutdown();

chrome/browser/chromeos/chrome_browser_main_chromeos.h

@@ -17,7 +17,6 @@ class AssistantClientImpl;
 class AssistantStateClient;
 class ChromeKeyboardControllerClient;
 class ImageDownloaderImpl;
-class LacrosLoader;
 class SpokenFeedbackEventRewriterDelegate;
 
 namespace lock_screen_apps {
@@ -49,6 +48,7 @@ class EventRewriterDelegateImpl;
 class FastTransitionObserver;
 class GnubbyNotification;
 class IdleActionWarningObserver;
+class LacrosManager;
 class LoginScreenExtensionsLifetimeManager;
 class LoginScreenExtensionsStorageCleaner;
 class LowDiskNotification;
@@ -156,7 +156,7 @@ class ChromeBrowserMainPartsChromeos : public ChromeBrowserMainPartsLinux {
 
   std::unique_ptr<lock_screen_apps::StateController>
       lock_screen_apps_state_controller_;
-  std::unique_ptr<LacrosLoader> lacros_loader_;
+  std::unique_ptr<LacrosManager> lacros_manager_;
 
   std::unique_ptr<power::SmartChargingManager> smart_charging_manager_;
 

chrome/browser/chromeos/lacros/lacros_loader.h

@@ -7,73 +7,50 @@
 
 #include "base/callback.h"
 #include "base/files/file_path.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "base/process/process.h"
 #include "chrome/browser/component_updater/cros_component_manager.h"
-#include "components/session_manager/core/session_manager_observer.h"
 
-// Manages download and launch of the lacros-chrome binary.
-class LacrosLoader : public session_manager::SessionManagerObserver {
- public:
-  // Direct getter because there are no accessors to the owning object.
-  static LacrosLoader* Get();
+namespace chromeos {
 
+// Manages download of the lacros-chrome binary.
+class LacrosLoader {
+ public:
   explicit LacrosLoader(
       scoped_refptr<component_updater::CrOSComponentManager> manager);
+
   LacrosLoader(const LacrosLoader&) = delete;
   LacrosLoader& operator=(const LacrosLoader&) = delete;
-  ~LacrosLoader() override;
 
-  // Returns true if the binary is ready to launch. Typical usage is to check
-  // IsReady(), then if it returns false, call SetLoadCompleteCallback() to be
-  // notified when the download completes.
-  bool IsReady() const;
+  ~LacrosLoader();
 
-  // Sets a callback to be called when the binary download completes. The
-  // download may not be successful.
-  using LoadCompleteCallback = base::OnceCallback<void(bool success)>;
-  void SetLoadCompleteCallback(LoadCompleteCallback callback);
+  // Starts to load lacros-chrome binary.
+  // |callback| is called on completion with the path to the lacros-chrome on
+  // success, or an empty filepath on failure.
+  using LoadCompletionCallback =
+      base::OnceCallback<void(const base::FilePath&)>;
+  void Load(LoadCompletionCallback callback);
 
-  // Starts the lacros-chrome binary.
-  void Start();
-
-  // session_manager::SessionManagerObserver:
-  void OnUserSessionStarted(bool is_primary_user) override;
+  // Starts to unload lacros-chrome binary.
+  // Note that this triggers to remove the user directory for lacros-chrome.
+  void Unload();
 
  private:
-  // Starting Lacros requires a hop to a background thread. The flow is
-  // Start(), then StartBackground() in (the anonymous namespace),
-  // then StartForeground().
-  // The parameter |already_running| refers to whether the Lacros binary is
-  // already launched and running.
-  void StartForeground(bool already_running);
-
-  void OnLoadComplete(component_updater::CrOSComponentManager::Error error,
+  // Called on the completion of loading.
+  void OnLoadComplete(LoadCompletionCallback callback,
+                      component_updater::CrOSComponentManager::Error error,
                       const base::FilePath& path);
 
-  // Removes any state that Lacros left behind.
-  void CleanUp(bool previously_installed);
-
-  // Checks whether Lacros is already running.
-  bool IsLacrosRunning();
+  // Unloading hops threads. This is called after possible user directory
+  // removal.
+  void UnloadAfterCleanUp(bool was_installed);
 
   // May be null in tests.
-  scoped_refptr<component_updater::CrOSComponentManager>
-      cros_component_manager_;
-
-  // Path to the lacros-chrome disk image directory.
-  base::FilePath lacros_path_;
-
-  // Called when the binary download completes.
-  LoadCompleteCallback load_complete_callback_;
-
-  // Process handle for the lacros-chrome process.
-  // TODO(https://crbug.com/1091863): There is currently no notification for
-  // when lacros-chrome is killed, so the underlying pid may be pointing at a
-  // non-existent process, or a new, unrelated process with the same pid.
-  base::Process lacros_process_;
+  scoped_refptr<component_updater::CrOSComponentManager> component_manager_;
 
   base::WeakPtrFactory<LacrosLoader> weak_factory_{this};
 };
 
+}  // namespace chromeos
+
 #endif  // CHROME_BROWSER_CHROMEOS_LACROS_LACROS_LOADER_H_

chrome/browser/chromeos/lacros/lacros_manager.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/browser/chromeos/lacros/lacros_manager.h"
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/files/file.h"
+#include "base/files/file_util.h"
+#include "base/logging.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
+#include "base/process/launch.h"
+#include "base/process/process_handle.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "chrome/browser/chromeos/lacros/lacros_loader.h"
+#include "chrome/browser/chromeos/lacros/lacros_util.h"
+#include "chrome/browser/component_updater/cros_component_manager.h"
+#include "chromeos/constants/chromeos_features.h"
+#include "chromeos/constants/chromeos_switches.h"
+#include "components/session_manager/core/session_manager.h"
+#include "google_apis/google_api_keys.h"
+
+namespace chromeos {
+namespace {
+
+// Pointer to the global instance of LacrosManager.
+LacrosManager* g_instance = nullptr;
+
+base::FilePath LacrosLogPath() {
+  return base::FilePath(lacros_util::kUserDataDir).Append("lacros.log");
+}
+
+// TODO(https://crbug.com/1091863): This logic is not robust against the
+// situation where Lacros has been killed, but another process was spawned
+// with the same pid. This logic also relies on I/O, which we'd like to avoid
+// if possible.
+bool IsLacrosChromeInProc(base::ProcessId pid,
+                          const base::FilePath& lacros_path) {
+  // We avoid using WaitForExitWithTimeout() since that can block for up to
+  // 256ms. Instead, we check existence of /proc/<pid>/cmdline and check for a
+  // match against lacros_path_. This logic assumes that lacros_path_ is a fully
+  // qualified path.
+  base::FilePath cmdline_filepath("/proc");
+  cmdline_filepath = cmdline_filepath.Append(base::NumberToString(pid));
+  cmdline_filepath = cmdline_filepath.Append("cmdline");
+  base::File cmdline_file = base::File(
+      cmdline_filepath, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!cmdline_file.IsValid())
+    return false;
+  std::string expected_cmdline = lacros_path.value();
+  size_t expected_length = expected_cmdline.size();
+  char data[1000];
+  int size_read = cmdline_file.Read(0, data, 1000);
+  if (static_cast<size_t>(size_read) < expected_length)
+    return false;
+  return expected_cmdline.compare(0, expected_length, data, expected_length) ==
+         0;
+}
+
+bool StartBackground(base::ProcessId pid, const base::FilePath& lacros_path) {
+  bool already_running =
+      pid != base::kNullProcessId && IsLacrosChromeInProc(pid, lacros_path);
+
+  if (!already_running) {
+    // Only delete the old log file if lacros is not running. If it's already
+    // running, then the subsequent call to base::LaunchProcess opens a new
+    // window, and we do not want to delete the existing log file.
+    // TODO(erikchen): Currently, launching a second instance of chrome deletes
+    // the existing log file, even though the new instance quickly exits.
+    base::DeleteFile(LacrosLogPath(), /*recursive=*/false);
+  }
+
+  return already_running;
+}
+
+}  // namespace
+
+// static
+LacrosManager* LacrosManager::Get() {
+  return g_instance;
+}
+
+LacrosManager::LacrosManager(
+    scoped_refptr<component_updater::CrOSComponentManager> manager)
+    : component_manager_(manager) {
+  DCHECK(!g_instance);
+  g_instance = this;
+
+  // Wait to query the flag until the user has entered the session. Enterprise
+  // devices restart Chrome during login to apply flags. We don't want to run
+  // the flag-off cleanup logic until we know we have the final flag state.
+  session_manager::SessionManager::Get()->AddObserver(this);
+}
+
+LacrosManager::~LacrosManager() {
+  // Unregister, just in case the manager is destroyed before
+  // OnUserSessionStarted() is called.
+  session_manager::SessionManager::Get()->RemoveObserver(this);
+
+  // Try to kill the lacros-chrome binary.
+  if (lacros_process_.IsValid())
+    lacros_process_.Terminate(/*ignored=*/0, /*wait=*/false);
+
+  DCHECK_EQ(g_instance, this);
+  g_instance = nullptr;
+}
+
+bool LacrosManager::IsReady() const {
+  return !lacros_path_.empty();
+}
+
+void LacrosManager::SetLoadCompleteCallback(LoadCompleteCallback callback) {
+  load_complete_callback_ = std::move(callback);
+}
+
+void LacrosManager::Start() {
+  if (!lacros_util::IsLacrosAllowed())
+    return;
+
+  if (lacros_path_.empty()) {
+    LOG(WARNING) << "lacros component image not yet available";
+    return;
+  }
+
+  // Because we haven't yet handled process termination of lacros-chrome,
+  // lacros_process_ may point to a stale process. Check it by looking at
+  // procfs in a background task runner in addition.
+  // TODO(hidehiko): Handle the process termination correctly after mojo
+  // connection available.
+  scoped_refptr<base::SequencedTaskRunner> task_runner =
+      base::ThreadPool::CreateSequencedTaskRunner(
+          {base::MayBlock(), base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
+  task_runner->PostTaskAndReplyWithResult(
+      FROM_HERE,
+      base::BindOnce(&StartBackground, lacros_process_.Pid(), lacros_path_),
+      base::BindOnce(&LacrosManager::StartForeground,
+                     weak_factory_.GetWeakPtr()));
+}
+
+void LacrosManager::StartForeground(bool already_running) {
+  DCHECK(!lacros_path_.empty());
+
+  std::string chrome_path = lacros_path_.MaybeAsASCII() + "/chrome";
+  LOG(WARNING) << "Launching lacros-chrome at " << chrome_path;
+
+  base::LaunchOptions options;
+  options.environment["EGL_PLATFORM"] = "surfaceless";
+  options.environment["XDG_RUNTIME_DIR"] = "/run/chrome";
+
+  std::string api_key;
+  if (google_apis::HasAPIKeyConfigured())
+    api_key = google_apis::GetAPIKey();
+  else
+    api_key = google_apis::GetNonStableAPIKey();
+  options.environment["GOOGLE_API_KEY"] = api_key;
+  options.environment["GOOGLE_DEFAULT_CLIENT_ID"] =
+      google_apis::GetOAuth2ClientID(google_apis::CLIENT_MAIN);
+  options.environment["GOOGLE_DEFAULT_CLIENT_SECRET"] =
+      google_apis::GetOAuth2ClientSecret(google_apis::CLIENT_MAIN);
+
+  options.kill_on_parent_death = true;
+
+  std::vector<std::string> argv = {
+      chrome_path,
+      "--ozone-platform=wayland",
+      std::string("--user-data-dir=") + lacros_util::kUserDataDir,
+      "--enable-gpu-rasterization",
+      "--enable-oop-rasterization",
+      "--lang=en-US",
+      "--breakpad-dump-location=/tmp"};
+
+  // We assume that if there's a custom chrome path, that this is a developer
+  // and they want to enable logging.
+  bool custom_chrome_path = base::CommandLine::ForCurrentProcess()->HasSwitch(
+      chromeos::switches::kLacrosChromePath);
+  if (custom_chrome_path) {
+    argv.push_back("--enable-logging");
+    argv.push_back("--log-file=" + LacrosLogPath().value());
+  }
+
+  // TODO(hidehiko): Support Mojo connection.
+
+  if (already_running) {
+    // If Lacros is already running, then the new call to launch process spawns
+    // a new window but does not create a lasting process.
+    // TODO(erikchen): we should send a mojo signal to open a new tab rather
+    // than going through the start flow again.
+    base::LaunchProcess(argv, options);
+  } else {
+    base::RecordAction(base::UserMetricsAction("Lacros.Launch"));
+    // If lacros_process_ already exists, because it does not call waitpid(2),
+    // the process will never be collected.
+    // TODO(hidehiko): Fix the case by collecting the processes.
+    lacros_process_ = base::LaunchProcess(argv, options);
+  }
+  LOG(WARNING) << "Launched lacros-chrome with pid " << lacros_process_.Pid();
+}
+
+void LacrosManager::OnUserSessionStarted(bool is_primary_user) {
+  // Ensure this isn't called multiple times.
+  session_manager::SessionManager::Get()->RemoveObserver(this);
+
+  // Must be checked after user session start because it depends on user type.
+  if (!lacros_util::IsLacrosAllowed())
+    return;
+
+  // May be null in tests.
+  if (!component_manager_)
+    return;
+
+  DCHECK(!lacros_loader_);
+  lacros_loader_ = std::make_unique<LacrosLoader>(component_manager_);
+  if (chromeos::features::IsLacrosSupportEnabled()) {
+    lacros_loader_->Load(base::BindOnce(&LacrosManager::OnLoadComplete,
+                                        weak_factory_.GetWeakPtr()));
+  } else {
+    lacros_loader_->Unload();
+  }
+}
+
+void LacrosManager::OnLoadComplete(const base::FilePath& path) {
+  lacros_path_ = path;
+  if (load_complete_callback_) {
+    const bool success = !path.empty();
+    std::move(load_complete_callback_).Run(success);
+  }
+}
+
+}  // namespace chromeos

chrome/browser/chromeos/lacros/lacros_manager.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_BROWSER_CHROMEOS_LACROS_LACROS_MANAGER_H_
+#define CHROME_BROWSER_CHROMEOS_LACROS_LACROS_MANAGER_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/process/process.h"
+#include "components/session_manager/core/session_manager_observer.h"
+
+namespace component_updater {
+class CrOSComponentManager;
+}  // namespace component_updater
+
+namespace chromeos {
+
+class LacrosLoader;
+
+// Manages the lifetime of lacros-chrome, and its loading status.
+class LacrosManager : public session_manager::SessionManagerObserver {
+ public:
+  // Static getter of LacrosManager instance. In real use cases,
+  // LacrosManager instance should be unique in the process.
+  static LacrosManager* Get();
+
+  explicit LacrosManager(
+      scoped_refptr<component_updater::CrOSComponentManager> manager);
+
+  LacrosManager(const LacrosManager&) = delete;
+  LacrosManager& operator=(const LacrosManager&) = delete;
+
+  ~LacrosManager() override;
+
+  // Returns true if the binary is ready to launch. Typical usage is to check
+  // IsReady(), then if it returns false, call SetLoadCompleteCallback() to be
+  // notified when the download completes.
+  bool IsReady() const;
+
+  // Sets a callback to be called when the binary download completes. The
+  // download may not be successful.
+  using LoadCompleteCallback = base::OnceCallback<void(bool success)>;
+  void SetLoadCompleteCallback(LoadCompleteCallback callback);
+
+  // Starts the lacros-chrome binary.
+  // This needs to be called after loading. The condition can be checked
+  // IsReady(), and if not yet, SetLoadCompletionCallback can be used
+  // to wait for the loading.
+  void Start();
+
+ private:
+  // Starting Lacros requires a hop to a background thread. The flow is
+  // Start(), then StartBackground() in (the anonymous namespace),
+  // then StartForeground().
+  // The parameter |already_running| refers to whether the Lacros binary is
+  // already launched and running.
+  void StartForeground(bool already_running);
+
+  // session_manager::SessionManagerObserver:
+  // Starts to load the lacros-chrome executable.
+  void OnUserSessionStarted(bool is_primary_user) override;
+
+  // Called on load completion.
+  void OnLoadComplete(const base::FilePath& path);
+
+  // May be null in tests.
+  scoped_refptr<component_updater::CrOSComponentManager> component_manager_;
+
+  std::unique_ptr<LacrosLoader> lacros_loader_;
+
+  // Path to the lacros-chrome disk image directory.
+  base::FilePath lacros_path_;
+
+  // Called when the binary download completes.
+  LoadCompleteCallback load_complete_callback_;
+
+  // Process handle for the lacros-chrome process.
+  // TODO(https://crbug.com/1091863): There is currently no notification for
+  // when lacros-chrome is killed, so the underlying pid may be pointing at a
+  // non-existent process, or a new, unrelated process with the same pid.
+  base::Process lacros_process_;
+
+  base::WeakPtrFactory<LacrosManager> weak_factory_{this};
+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_LACROS_LACROS_MANAGER_H_

chrome/browser/chromeos/lacros/lacros_util.cc

@@ -38,6 +38,8 @@ bool IsUserTypeAllowed(const User* user) {
 
 }  // namespace
 
+constexpr char kUserDataDir[] = "/home/chronos/user/lacros";
+
 bool IsLacrosAllowed() {
   return IsLacrosAllowed(chrome::GetChannel());
 }

chrome/browser/chromeos/lacros/lacros_util.h

@@ -11,6 +11,9 @@ enum class Channel;
 
 namespace lacros_util {
 
+// Path of the user directory for lacros-chrome.
+extern const char kUserDataDir[];
+
 // Returns true if lacros is allowed for the current user type, chrome channel,
 // etc.
 bool IsLacrosAllowed();