lacros: Avoid login crash when user disables the LacrosSupport flag

CrOSComponentManager::Unload() calls into code in MetadataTable that
assumes that system salt is available. This isn't always true when
chrome restarts to apply non-owner flags.

Unfortunately the MetadataTable API is synchronous and non-trivial
to convert to async. Work around the crash by ensuring system salt
is available before attempting to unload the lacros component.

Filed crbug.com/1122753 to track refactoring MetadataTable so it
doesn't depend on data that isn't guaranteed to be available.

  in about:flags, restart and let Lacros download/install. Then
  explicitly set flag to "Disabled" and sign out (not restart).
  Sign in again. No crash.

Test: With non-owner account (secondary user), enable LacrosSupport
Bug: 1122674
Change-Id: Idf25ef8955315e92c6f823a0da8f8b65fd0a40c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2380320Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802427}

chrome/browser/chromeos/crosapi/browser_loader.cc

@@ -15,6 +15,7 @@
 #include "base/task/thread_pool.h"
 #include "chrome/browser/chromeos/crosapi/browser_util.h"
 #include "chromeos/constants/chromeos_switches.h"
+#include "chromeos/cryptohome/system_salt_getter.h"
 
 namespace crosapi {
 
@@ -82,7 +83,7 @@ void BrowserLoader::Unload() {
       FROM_HERE, {base::MayBlock()},
       base::BindOnce(&CheckInstalledAndMaybeRemoveUserDirectory,
                      component_manager_),
-      base::BindOnce(&BrowserLoader::UnloadAfterCleanUp,
+      base::BindOnce(&BrowserLoader::OnCheckInstalled,
                      weak_factory_.GetWeakPtr()));
 }
 
@@ -101,9 +102,22 @@ void BrowserLoader::OnLoadComplete(
   std::move(callback).Run(success ? path : base::FilePath());
 }
 
-void BrowserLoader::UnloadAfterCleanUp(bool was_installed) {
-  if (was_installed)
-    component_manager_->Unload(kLacrosComponentName);
+void BrowserLoader::OnCheckInstalled(bool was_installed) {
+  if (!was_installed)
+    return;
+
+  // Workaround for login crash when the user un-sets the LacrosSupport flag.
+  // CrOSComponentManager::Unload() calls into code in MetadataTable that
+  // assumes that system salt is available. This isn't always true when chrome
+  // restarts to apply non-owner flags. It's hard to make MetadataTable async.
+  // Ensure salt is available before unloading. https://crbug.com/1122674
+  chromeos::SystemSaltGetter::Get()->GetSystemSalt(base::BindOnce(
+      &BrowserLoader::UnloadAfterCleanUp, weak_factory_.GetWeakPtr()));
+}
+
+void BrowserLoader::UnloadAfterCleanUp(const std::string& ignored_salt) {
+  CHECK(chromeos::SystemSaltGetter::Get()->GetRawSalt());
+  component_manager_->Unload(kLacrosComponentName);
 }
 
 }  // namespace crosapi

chrome/browser/chromeos/crosapi/browser_loader.h

@@ -42,9 +42,12 @@ class BrowserLoader {
                       component_updater::CrOSComponentManager::Error error,
                       const base::FilePath& path);
 
-  // Unloading hops threads. This is called after possible user directory
-  // removal.
-  void UnloadAfterCleanUp(bool was_installed);
+  // Unloading hops threads. This is called after we check whether Lacros was
+  // installed and maybe clean up the user directory.
+  void OnCheckInstalled(bool was_installed);
+
+  // Unloads the component. Called after system salt is available.
+  void UnloadAfterCleanUp(const std::string& ignored_salt);
 
   // May be null in tests.
   scoped_refptr<component_updater::CrOSComponentManager> component_manager_;

chromeos/cryptohome/system_salt_getter.h

@@ -44,6 +44,8 @@ class COMPONENT_EXPORT(CHROMEOS_CRYPTOHOME) SystemSaltGetter {
 
   // Returns pointer to binary system salt if it is already known.
   // Returns nullptr if system salt is not known.
+  // WARNING: This pointer is null early in startup. Do not assume it is valid.
+  // Prefer GetSystemSalt() above. https://crbug.com/1122674
   const RawSalt* GetRawSalt() const;
 
   // This is for browser tests API.