Lacros: Simplify version checks in AccountManagerFacadeImpl

Simplify version checks in AccountManagerFacadeImpl by using the
synchronous version accessor in LacrosChromeServiceImpl

Change-Id: I7a83a352f10977e0d3ce1d84528d76726688016c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640436
Commit-Queue: Kush Sinha <sinhak@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845656}

chrome/browser/ash/account_manager/account_manager_facade_factory_ash.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/account_manager_facade_factory.h"
 
+#include <limits>
 #include <map>
 #include <memory>
 #include <utility>
@@ -42,9 +43,14 @@ account_manager::AccountManagerFacade* GetAccountManagerFacade(
     mojo::Remote<crosapi::mojom::AccountManager> remote;
     GetAccountManagerAsh(profile_path)
         ->BindReceiver(remote.BindNewPipeAndPassReceiver());
+    // This is set to a sentinel value which will pass all minimum version
+    // checks.
+    // Calls within Ash are in the same process and don't need to check version
+    // compatibility with itself.
+    constexpr uint32_t remote_version = std::numeric_limits<uint32_t>::max();
     it = account_manager_facade_map
              ->emplace(profile_path, std::make_unique<AccountManagerFacadeImpl>(
-                                         std::move(remote)))
+                                         std::move(remote), remote_version))
              .first;
   }
 

chrome/browser/lacros/account_manager_facade_factory_lacros.cc

@@ -10,21 +10,33 @@
 #include "components/account_manager_core/account_manager_facade_impl.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
+namespace {
+
+mojo::Remote<crosapi::mojom::AccountManager> GetAccountManagerRemote() {
+  mojo::Remote<crosapi::mojom::AccountManager> remote;
+
+  auto* lacros_chrome_service_impl = chromeos::LacrosChromeServiceImpl::Get();
+  DCHECK(lacros_chrome_service_impl);
+  if (!lacros_chrome_service_impl->IsAccountManagerAvailable()) {
+    LOG(WARNING) << "Connected to an older version of ash. Account "
+                    "consistency will not be available";
+    return remote;
+  }
+
+  lacros_chrome_service_impl->BindAccountManagerReceiver(
+      remote.BindNewPipeAndPassReceiver());
+
+  return remote;
+}
+
+}  // namespace
+
 account_manager::AccountManagerFacade* GetAccountManagerFacade(
     const std::string& profile_path) {
   // Multi-Login is disabled with Lacros. Always return the same instance.
-  static base::NoDestructor<AccountManagerFacadeImpl> facade([] {
-    auto* lacros_chrome_service_impl = chromeos::LacrosChromeServiceImpl::Get();
-    DCHECK(lacros_chrome_service_impl);
-    if (!lacros_chrome_service_impl->IsAccountManagerAvailable()) {
-      LOG(WARNING) << "Connected to an older version of ash. Account "
-                      "consistency will not be available";
-      return mojo::Remote<crosapi::mojom::AccountManager>();
-    }
-    mojo::Remote<crosapi::mojom::AccountManager> remote;
-    lacros_chrome_service_impl->BindAccountManagerReceiver(
-        remote.BindNewPipeAndPassReceiver());
-    return remote;
-  }());
+  static base::NoDestructor<AccountManagerFacadeImpl> facade(
+      GetAccountManagerRemote(),
+      chromeos::LacrosChromeServiceImpl::Get()->GetInterfaceVersion(
+          crosapi::mojom::AccountManager::Uuid_));
   return facade.get();
 }

components/account_manager_core/account_manager_facade_impl.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/logging.h"
 #include "components/account_manager_core/account_manager_util.h"
 
 namespace {
@@ -20,17 +21,23 @@ constexpr uint32_t kMinVersionWithObserver = 1;
 
 AccountManagerFacadeImpl::AccountManagerFacadeImpl(
     mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote,
+    uint32_t remote_version,
     base::OnceClosure init_finished)
-    : account_manager_remote_(std::move(account_manager_remote)),
+    : remote_version_(remote_version),
+      account_manager_remote_(std::move(account_manager_remote)),
       init_finished_(std::move(init_finished)) {
   DCHECK(init_finished_);
-  if (!account_manager_remote_) {
+
+  if (!account_manager_remote_ || remote_version_ < kMinVersionWithObserver) {
+    LOG(WARNING) << "Found remote at: " << remote_version_
+                 << ", expected: " << kMinVersionWithObserver
+                 << ". Account consistency will be disabled";
     std::move(init_finished_).Run();
     return;
   }
-
-  account_manager_remote_.QueryVersion(base::BindOnce(
-      &AccountManagerFacadeImpl::OnVersionCheck, weak_factory_.GetWeakPtr()));
+  account_manager_remote_->AddObserver(
+      base::BindOnce(&AccountManagerFacadeImpl::OnReceiverReceived,
+                     weak_factory_.GetWeakPtr()));
 }
 
 AccountManagerFacadeImpl::~AccountManagerFacadeImpl() = default;
@@ -59,16 +66,6 @@ void AccountManagerFacadeImpl::ShowReauthAccountDialog(
   // TODO(crbug.com/1140469): implement this.
 }
 
-void AccountManagerFacadeImpl::OnVersionCheck(uint32_t version) {
-  if (version < kMinVersionWithObserver) {
-    std::move(init_finished_).Run();
-    return;
-  }
-
-  account_manager_remote_->AddObserver(
-      base::BindOnce(&AccountManagerFacadeImpl::OnReceiverReceived,
-                     weak_factory_.GetWeakPtr()));
-}
 
 void AccountManagerFacadeImpl::OnReceiverReceived(
     mojo::PendingReceiver<AccountManagerObserver> receiver) {

components/account_manager_core/account_manager_facade_impl.h

@@ -21,8 +21,14 @@ class COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE) AccountManagerFacadeImpl
     : public account_manager::AccountManagerFacade,
       public crosapi::mojom::AccountManagerObserver {
  public:
+  // Constructs `AccountManagerFacadeImpl`.
+  // `account_manager_remote` is a Mojo `Remote` to Account Manager in Ash -
+  // either in-process or out-of-process.
+  // `remote_version` is the Mojo API version of the remote.
+  // `init_finished` is called after Account Manager has been fully initialized.
   AccountManagerFacadeImpl(
       mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote,
+      uint32_t remote_version,
       base::OnceClosure init_finished = base::DoNothing());
   AccountManagerFacadeImpl(const AccountManagerFacadeImpl&) = delete;
   AccountManagerFacadeImpl& operator=(const AccountManagerFacadeImpl&) = delete;
@@ -44,11 +50,12 @@ class COMPONENT_EXPORT(ACCOUNT_MANAGER_CORE) AccountManagerFacadeImpl
   void OnAccountRemoved(crosapi::mojom::AccountPtr account) override;
 
  private:
-  void OnVersionCheck(uint32_t version);
   void OnReceiverReceived(
       mojo::PendingReceiver<AccountManagerObserver> receiver);
   void OnInitialized(bool is_initialized);
 
+  // Mojo API version on the remote (Ash) side.
+  const uint32_t remote_version_;
   mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote_;
   base::OnceClosure init_finished_;
   bool is_initialized_ = false;

components/account_manager_core/account_manager_facade_impl_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/account_manager_core/account_manager_facade_impl.h"
 
+#include <limits>
+
 #include "base/test/gmock_callback_support.h"
 #include "base/test/task_environment.h"
 #include "chromeos/crosapi/mojom/account_manager.mojom.h"
@@ -100,7 +102,9 @@ class AccountManagerFacadeImplTest : public testing::Test {
   std::unique_ptr<AccountManagerFacadeImpl> CreateFacade() {
     base::RunLoop run_loop;
     auto result = std::make_unique<AccountManagerFacadeImpl>(
-        account_manager().CreateRemote(), run_loop.QuitClosure());
+        account_manager().CreateRemote(),
+        /* remote_version= */ std::numeric_limits<uint32_t>::max(),
+        run_loop.QuitClosure());
     run_loop.Run();
     return result;
   }