lacros: Add availability checks to methods on AshChromeService.

This CL adds the basic plumbing necessary for Lacros to avoid calling
unavailable methods on ash due to ash being too old.

This requires modifying browser tests to disable all crosapi
functionality, and to modify production logic to avoid using crosapi
functionality if it's unavailable.

Bug: 1130810
Change-Id: Iedf7faae93372e42c59a61186df49d683d859f50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424544Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Jenny Zhang <jennyz@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810762}

chrome/browser/notifications/notification_platform_bridge_chromeos.cc

@@ -38,8 +38,11 @@ bool NotificationPlatformBridge::CanHandleType(
 
 NotificationPlatformBridgeChromeOs::NotificationPlatformBridgeChromeOs() {
 #if BUILDFLAG(IS_LACROS)
-  impl_ = std::make_unique<NotificationPlatformBridgeLacros>(
-      this, &chromeos::LacrosChromeServiceImpl::Get()->message_center_remote());
+  mojo::Remote<crosapi::mojom::MessageCenter>* remote = nullptr;
+  auto* service = chromeos::LacrosChromeServiceImpl::Get();
+  if (service->IsMessageCenterAvailable())
+    remote = &service->message_center_remote();
+  impl_ = std::make_unique<NotificationPlatformBridgeLacros>(this, remote);
 #else
   impl_ = std::make_unique<ChromeAshMessageCenterClient>(this);
 #endif

chrome/browser/notifications/notification_platform_bridge_lacros.cc

@@ -172,7 +172,6 @@ NotificationPlatformBridgeLacros::NotificationPlatformBridgeLacros(
     : bridge_delegate_(delegate),
       message_center_remote_(message_center_remote) {
   DCHECK(bridge_delegate_);
-  DCHECK(message_center_remote_);
 }
 
 NotificationPlatformBridgeLacros::~NotificationPlatformBridgeLacros() = default;
@@ -182,6 +181,9 @@ void NotificationPlatformBridgeLacros::Display(
     Profile* profile,
     const message_center::Notification& notification,
     std::unique_ptr<NotificationCommon::Metadata> metadata) {
+  if (!message_center_remote_)
+    return;
+
   // |profile| is ignored because Profile management is handled in
   // NotificationPlatformBridgeChromeOs, which includes a profile ID as part of
   // the notification ID. Lacros does not support Chrome OS multi-signin, so we
@@ -201,6 +203,9 @@ void NotificationPlatformBridgeLacros::Display(
 void NotificationPlatformBridgeLacros::Close(
     Profile* profile,
     const std::string& notification_id) {
+  if (!message_center_remote_)
+    return;
+
   (*message_center_remote_)->CloseNotification(notification_id);
   // |remote_notifications_| is cleaned up after the remote notification closes
   // and notifies us via the delegate.
@@ -215,9 +220,7 @@ void NotificationPlatformBridgeLacros::GetDisplayed(
 
 void NotificationPlatformBridgeLacros::SetReadyCallback(
     NotificationBridgeReadyCallback callback) {
-  // We don't handle the absence of Ash or a failure to open a Mojo connection,
-  // so just assume the client is ready.
-  std::move(callback).Run(true);
+  std::move(callback).Run(!!message_center_remote_);
 }
 
 void NotificationPlatformBridgeLacros::DisplayServiceShutDown(

chrome/browser/notifications/notification_platform_bridge_lacros.h

@@ -49,6 +49,8 @@ class NotificationPlatformBridgeLacros : public NotificationPlatformBridge {
   void OnRemoteNotificationClosed(const std::string& id);
 
   NotificationPlatformBridgeDelegate* const bridge_delegate_;
+
+  // May be nullptr if the message center is unavailable.
   mojo::Remote<crosapi::mojom::MessageCenter>* const message_center_remote_;
 
   // Map key is notification ID.

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -15,6 +15,10 @@
 namespace chromeos {
 namespace {
 
+// Tests will set this to |true| which will make all crosapi functionality
+// unavailable.
+bool g_disable_all_crosapi_for_tests = false;
+
 // We use a std::atomic here rather than a base::NoDestructor because we want to
 // allow instances of LacrosChromeServiceImpl to be destroyed to facilitate
 // testing.
@@ -189,44 +193,6 @@ LacrosChromeServiceImpl::LacrosChromeServiceImpl(
           &LacrosChromeServiceNeverBlockingState::BindAshChromeServiceRemote,
           weak_sequenced_state_));
 
-  // Bind the remote for SelectFile on the current thread, and then pass the
-  // receiver to the never_blocking_sequence_.
-  never_blocking_sequence_->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &LacrosChromeServiceNeverBlockingState::BindMessageCenterReceiver,
-          weak_sequenced_state_,
-          message_center_remote_.BindNewPipeAndPassReceiver()));
-
-  // Bind the remote for SelectFile on the current thread, and then pass the
-  // receiver to the never_blocking_sequence_.
-  mojo::PendingReceiver<crosapi::mojom::SelectFile>
-      select_file_pending_receiver =
-          select_file_remote_.BindNewPipeAndPassReceiver();
-  never_blocking_sequence_->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &LacrosChromeServiceNeverBlockingState::BindSelectFileReceiver,
-          weak_sequenced_state_, std::move(select_file_pending_receiver)));
-
-  mojo::PendingReceiver<crosapi::mojom::KeystoreService>
-      keystore_service_pending_receiver =
-          keystore_service_remote_.BindNewPipeAndPassReceiver();
-  never_blocking_sequence_->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &LacrosChromeServiceNeverBlockingState::BindKeystoreServiceReceiver,
-          weak_sequenced_state_, std::move(keystore_service_pending_receiver)));
-
-  mojo::PendingReceiver<device::mojom::HidManager>
-      hid_manager_pending_receiver =
-          hid_manager_remote_.BindNewPipeAndPassReceiver();
-  never_blocking_sequence_->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &LacrosChromeServiceNeverBlockingState::BindHidManagerReceiver,
-          weak_sequenced_state_, std::move(hid_manager_pending_receiver)));
-
   DCHECK(!g_instance);
   g_instance = this;
 }
@@ -244,10 +210,83 @@ void LacrosChromeServiceImpl::BindReceiver(
                                     BindLacrosChromeServiceReceiver,
                                 weak_sequenced_state_, std::move(receiver)));
   sequenced_state_->WaitForInit();
+  did_bind_receiver_ = true;
+
+  // Bind the remote for MessageCenter on the current thread, and then pass the
+  // receiver to the never_blocking_sequence_.
+  if (IsMessageCenterAvailable()) {
+    never_blocking_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LacrosChromeServiceNeverBlockingState::BindMessageCenterReceiver,
+            weak_sequenced_state_,
+            message_center_remote_.BindNewPipeAndPassReceiver()));
+  }
+
+  // Bind the remote for SelectFile on the current thread, and then pass the
+  // receiver to the never_blocking_sequence_.
+  if (IsSelectFileAvailable()) {
+    mojo::PendingReceiver<crosapi::mojom::SelectFile>
+        select_file_pending_receiver =
+            select_file_remote_.BindNewPipeAndPassReceiver();
+    never_blocking_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LacrosChromeServiceNeverBlockingState::BindSelectFileReceiver,
+            weak_sequenced_state_, std::move(select_file_pending_receiver)));
+  }
+
+  if (IsKeystoreServiceAvailable()) {
+    mojo::PendingReceiver<crosapi::mojom::KeystoreService>
+        keystore_service_pending_receiver =
+            keystore_service_remote_.BindNewPipeAndPassReceiver();
+    never_blocking_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LacrosChromeServiceNeverBlockingState::BindKeystoreServiceReceiver,
+            weak_sequenced_state_,
+            std::move(keystore_service_pending_receiver)));
+  }
+
+  if (IsHidManagerAvailable()) {
+    mojo::PendingReceiver<device::mojom::HidManager>
+        hid_manager_pending_receiver =
+            hid_manager_remote_.BindNewPipeAndPassReceiver();
+    never_blocking_sequence_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &LacrosChromeServiceNeverBlockingState::BindHidManagerReceiver,
+            weak_sequenced_state_, std::move(hid_manager_pending_receiver)));
+  }
+}
+
+void LacrosChromeServiceImpl::DisableCrosapiForTests() {
+  g_disable_all_crosapi_for_tests = true;
+}
+
+bool LacrosChromeServiceImpl::IsMessageCenterAvailable() {
+  return AshChromeServiceVersion() >= 0;
+}
+
+bool LacrosChromeServiceImpl::IsSelectFileAvailable() {
+  return AshChromeServiceVersion() >= 0;
+}
+
+bool LacrosChromeServiceImpl::IsKeystoreServiceAvailable() {
+  return AshChromeServiceVersion() >= 0;
+}
+
+bool LacrosChromeServiceImpl::IsHidManagerAvailable() {
+  return AshChromeServiceVersion() >= 0;
+}
+
+bool LacrosChromeServiceImpl::IsScreenManagerAvailable() {
+  return AshChromeServiceVersion() >= 0;
 }
 
 void LacrosChromeServiceImpl::BindScreenManagerReceiver(
     mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver) {
+  DCHECK(IsScreenManagerAvailable());
   never_blocking_sequence_->PostTask(
       FROM_HERE,
       base::BindOnce(
@@ -260,4 +299,11 @@ void LacrosChromeServiceImpl::NewWindowAffineSequence() {
   delegate_->NewWindow();
 }
 
+int LacrosChromeServiceImpl::AshChromeServiceVersion() {
+  if (g_disable_all_crosapi_for_tests)
+    return -1;
+  DCHECK(did_bind_receiver_);
+  return init_params_->ash_chrome_service_version;
+}
+
 }  // namespace chromeos

chromeos/lacros/lacros_chrome_service_impl.h

@@ -66,36 +66,58 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   void BindReceiver(
       mojo::PendingReceiver<crosapi::mojom::LacrosChromeService> receiver);
 
+  // Called during tests on affine sequence to disable all crosapi
+  // functionality.
+  // TODO(https://crbug.com/1131722): Ideally we could stub this out or make
+  // this functional for tests without modifying production code
+  static void DisableCrosapiForTests();
+
   // --------------------------------------------------------------------------
   // mojo::Remote is sequence affine. The following methods are convenient
   // helpers that expose pre-established Remotes that can only be used from the
   // affine sequence (main thread).
   // --------------------------------------------------------------------------
 
+  // message_center_remote() can only be used if this method returns true.
+  bool IsMessageCenterAvailable();
+
   // This must be called on the affine sequence.
   mojo::Remote<crosapi::mojom::MessageCenter>& message_center_remote() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+    DCHECK(IsMessageCenterAvailable());
     return message_center_remote_;
   }
 
+  // select_file_remote() can only be used if this method returns true.
+  bool IsSelectFileAvailable();
+
   // This must be called on the affine sequence. It exposes a remote that can
   // be used to show a select-file dialog.
   mojo::Remote<crosapi::mojom::SelectFile>& select_file_remote() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+    DCHECK(IsSelectFileAvailable());
     return select_file_remote_;
   }
 
+  // keystore_service_remote() can only be used if this method returns true.
+  bool IsKeystoreServiceAvailable();
+
   // This must be called on the affine sequence. It exposes a remote that can
   // be used to query the system keystores.
   mojo::Remote<crosapi::mojom::KeystoreService>& keystore_service_remote() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+    DCHECK(IsKeystoreServiceAvailable());
     return keystore_service_remote_;
   }
 
+  // hid_manager_remote() can only be used if this method returns true.
+  bool IsHidManagerAvailable();
+
   // This must be called on the affine sequence. It exposes a remote that can
   // be used to support HID devices.
   mojo::Remote<device::mojom::HidManager>& hid_manager_remote() {
     DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
+    DCHECK(IsHidManagerAvailable());
     return hid_manager_remote_;
   }
 
@@ -106,6 +128,9 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // the Remote (mojo::PendingReceiver) to ash to set up the interface.
   // --------------------------------------------------------------------------
 
+  // BindScreenManagerReceiver() can only be used if this method returns true.
+  bool IsScreenManagerAvailable();
+
   // This may be called on any thread.
   void BindScreenManagerReceiver(
       mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver);
@@ -122,6 +147,11 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // Creates a new window on the affine sequence.
   void NewWindowAffineSequence();
 
+  // Returns ash's version of the AshChromeService mojo interface version. This
+  // determines which interface methods are available. This is safe to call from
+  // any sequence. This can only be called after BindReceiver().
+  int AshChromeServiceVersion();
+
   // Delegate instance to inject Chrome dependent code. Must only be used on the
   // affine sequence.
   std::unique_ptr<LacrosChromeServiceDelegate> delegate_;
@@ -155,6 +185,9 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   // A sequence that is guaranteed to never block.
   scoped_refptr<base::SequencedTaskRunner> never_blocking_sequence_;
 
+  // Set to true after BindReceiver() is called.
+  bool did_bind_receiver_ = false;
+
   // Checks that the method is called on the affine sequence.
   SEQUENCE_CHECKER(affine_sequence_checker_);
 

content/public/test/DEPS

 include_rules = [
   "-content",
+
+  # These are low-level system APIs on ChromeOS that need to be available
+  # everywhere.
+  "+chromeos/lacros/lacros_chrome_service_impl.h",
+
   "+content/public",
   "+components/discardable_memory/service",
   "+components/download/public/common",

content/public/test/browser_test_base.cc

@@ -118,6 +118,7 @@
 #if BUILDFLAG(IS_LACROS)
 #include "base/files/file_path.h"
 #include "base/files/scoped_file.h"
+#include "chromeos/lacros/lacros_chrome_service_impl.h"
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
 #include "mojo/public/cpp/platform/socket_utils_posix.h"
@@ -365,7 +366,9 @@ void BrowserTestBase::SetUp() {
     // http://crrev.com/c/2402580/2/content/public/test/browser_test_base.cc
     std::string socket_path =
         command_line->GetSwitchValueASCII("lacros-mojo-socket-for-testing");
-    if (!socket_path.empty()) {
+    if (socket_path.empty()) {
+      chromeos::LacrosChromeServiceImpl::Get()->DisableCrosapiForTests();
+    } else {
       auto channel = mojo::NamedPlatformChannel::ConnectToServer(socket_path);
       base::ScopedFD socket_fd = channel.TakePlatformHandle().TakeFD();
 

content/test/BUILD.gn

@@ -4,6 +4,7 @@
 
 import("//build/config/chrome_build.gni")
 import("//build/config/chromecast_build.gni")
+import("//build/config/chromeos/ui_mode.gni")
 import("//build/config/compiler/compiler.gni")
 import("//build/config/crypto.gni")
 import("//build/config/features.gni")
@@ -618,6 +619,10 @@ static_library("test_support") {
       "../public/test/xr_test_utils.h",
     ]
   }
+
+  if (chromeos_is_browser_only) {
+    deps += [ "//chromeos/lacros" ]
+  }
 }
 
 # Fuchsia gpu integration tests use web_engine and a browser like shell

services/device/device_service.cc

@@ -72,7 +72,10 @@ void BindLaCrOSHidManager(
   // D-Bus. Use the HidManager interface from ash-chrome instead.
   auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
   DCHECK(lacros_chrome_service);
-  lacros_chrome_service->hid_manager_remote()->AddReceiver(std::move(receiver));
+  // If the Hid manager is not available, then the pending receiver is deleted.
+  if (lacros_chrome_service->IsHidManagerAvailable())
+    lacros_chrome_service->hid_manager_remote()->AddReceiver(
+        std::move(receiver));
 #endif
 }
 #endif