Notify CrosUsbDetector on ARCVM startup

Previously devices would only be visible in ARCVM after a USB connect
event which didn't allow already connected devices to be visible on
startup without disconnecting and reconnecting them again.

Bug: b:123374026
Test: unit_tests --gtest_filter='*CrosUsbDetector*'
Test: Login with USB device already connected and run lsusb in Android
Change-Id: Ifc4e708f9244fb7e2fae06f2086232a282f9d8e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1686930
Commit-Queue: Philippe Liard <pliard@google.com>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678560}

chrome/browser/chromeos/BUILD.gn

@@ -620,6 +620,8 @@ source_set("chromeos") {
     "arc/tracing/arc_value_event_trimmer.h",
     "arc/tts/arc_tts_service.cc",
     "arc/tts/arc_tts_service.h",
+    "arc/usb/arc_usb_host_bridge_delegate.cc",
+    "arc/usb/arc_usb_host_bridge_delegate.h",
     "arc/user_session/arc_user_session_service.cc",
     "arc/user_session/arc_user_session_service.h",
     "arc/video/gpu_arc_video_service_host.cc",

chrome/browser/chromeos/arc/arc_service_launcher.cc

@@ -42,6 +42,7 @@
 #include "chrome/browser/chromeos/arc/tracing/arc_app_performance_tracing.h"
 #include "chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.h"
 #include "chrome/browser/chromeos/arc/tts/arc_tts_service.h"
+#include "chrome/browser/chromeos/arc/usb/arc_usb_host_bridge_delegate.h"
 #include "chrome/browser/chromeos/arc/user_session/arc_user_session_service.h"
 #include "chrome/browser/chromeos/arc/video/gpu_arc_video_service_host.h"
 #include "chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.h"
@@ -207,7 +208,8 @@ void ArcServiceLauncher::OnPrimaryUserProfilePrepared(Profile* profile) {
   ArcTracingBridge::GetForBrowserContext(profile);
   ArcAppPerformanceTracing::GetForBrowserContext(profile);
   ArcTtsService::GetForBrowserContext(profile);
-  ArcUsbHostBridge::GetForBrowserContext(profile);
+  ArcUsbHostBridge::GetForBrowserContext(profile)->SetDelegate(
+      std::make_unique<ArcUsbHostBridgeDelegate>());
   ArcUsbHostPermissionManager::GetForBrowserContext(profile);
   ArcUserSessionService::GetForBrowserContext(profile);
   ArcVolumeMounterBridge::GetForBrowserContext(profile);

chrome/browser/chromeos/arc/usb/arc_usb_host_bridge_delegate.cc

+// Copyright 2019 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/arc/usb/arc_usb_host_bridge_delegate.h"
+
+#include "chrome/browser/chromeos/usb/cros_usb_detector.h"
+#include "components/arc/arc_util.h"
+
+namespace arc {
+
+void ArcUsbHostBridgeDelegate::AttachDevicesToArcVm() {
+  auto* const usb_detector = chromeos::CrosUsbDetector::Get();
+  if (usb_detector && IsArcVmEnabled())
+    usb_detector->ConnectSharedDevicesOnVmStartup(kArcVmName);
+}
+
+}  // namespace arc

chrome/browser/chromeos/arc/usb/arc_usb_host_bridge_delegate.h

+// Copyright 2019 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_ARC_USB_ARC_USB_HOST_BRIDGE_DELEGATE_H_
+#define CHROME_BROWSER_CHROMEOS_ARC_USB_ARC_USB_HOST_BRIDGE_DELEGATE_H_
+
+#include "components/arc/usb/usb_host_bridge.h"
+
+namespace arc {
+
+// Implementation of the ArcUsbHostBridge::Delegate interface.
+class ArcUsbHostBridgeDelegate : public ArcUsbHostBridge::Delegate {
+ public:
+  // ArcUsbHostBridge::Delegate:
+  void AttachDevicesToArcVm() override;
+};
+
+}  // namespace arc
+
+#endif  // CHROME_BROWSER_CHROMEOS_ARC_USB_ARC_USB_HOST_BRIDGE_DELEGATE_H_

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -2350,7 +2350,8 @@ void CrostiniManager::FinishRestart(CrostiniRestarter* restarter,
 
   if (chromeos::CrosUsbDetector::Get()) {
     // Mount shared devices
-    chromeos::CrosUsbDetector::Get()->ConnectSharedDevicesOnVmStartup();
+    chromeos::CrosUsbDetector::Get()->ConnectSharedDevicesOnVmStartup(
+        restarter->vm_name());
   }
 }
 

chrome/browser/chromeos/usb/cros_usb_detector.cc

@@ -413,18 +413,23 @@ void CrosUsbDetector::OnDeviceManagerConnectionError() {
   ConnectToDeviceManager();
 }
 
-void CrosUsbDetector::ConnectSharedDevicesOnVmStartup() {
+void CrosUsbDetector::ConnectSharedDevicesOnVmStartup(
+    const std::string& vm_name) {
   // Reattach shared devices when the VM becomes available.
   for (auto& device : usb_devices_) {
-    bool attached_device = false;
+    bool is_shared = false;
+    bool is_shared_with_this_vm = false;
     for (const auto& sharing_pair : device.vm_sharing_info) {
       if (sharing_pair.second.shared) {
-        attached_device = true;
-        AttachUsbDeviceToVm(sharing_pair.first, device.guid, base::DoNothing());
+        is_shared = true;
+        if (sharing_pair.first == vm_name) {
+          is_shared_with_this_vm = true;
+          break;
+        }
       }
     }
-    if (!attached_device) {
-      AttachUsbDeviceToVm(kDefaultVmName, device.guid, base::DoNothing());
+    if (is_shared_with_this_vm || (vm_name == kDefaultVmName && !is_shared)) {
+      AttachUsbDeviceToVm(vm_name, device.guid, base::DoNothing());
     }
   }
 }

chrome/browser/chromeos/usb/cros_usb_detector.h

@@ -96,9 +96,8 @@ class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
   // device manager during testing.
   void ConnectToDeviceManager();
 
-  // Called by CrostiniManager when a VM starts, to attach USB devices marked as
-  // shared to the VM.
-  void ConnectSharedDevicesOnVmStartup();
+  // Called when a VM starts, to attach USB devices marked as shared to the VM.
+  void ConnectSharedDevicesOnVmStartup(const std::string& vm_name);
 
   // device::mojom::UsbDeviceManagerClient
   void OnDeviceAdded(device::mojom::UsbDeviceInfoPtr device) override;

chrome/browser/chromeos/usb/cros_usb_detector_unittest.cc

@@ -171,6 +171,12 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
     base::RunLoop().RunUntilIdle();
   }
 
+  void DetachDeviceFromVm(const std::string& vm_name, const std::string& guid) {
+    cros_usb_detector_->DetachUsbDeviceFromVm(
+        vm_name, guid, base::Bind([](bool result) { EXPECT_TRUE(result); }));
+    base::RunLoop().RunUntilIdle();
+  }
+
   chromeos::CrosUsbDeviceInfo GetSingleDeviceInfo() const {
     auto devices = cros_usb_detector_->GetConnectedDevices();
     EXPECT_EQ(1U, devices.size());
@@ -738,9 +744,7 @@ TEST_F(CrosUsbDetectorTest, DetachingDeviceFromCrostiniAttachesItToArcVm) {
   auto device_info = GetSingleDeviceInfo();
   AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
   cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
-  cros_usb_detector_->DetachUsbDeviceFromVm(
-      crostini::kCrostiniDefaultVmName, device_info.guid,
-      base::Bind([](bool result) { EXPECT_TRUE(result); }));
+  DetachDeviceFromVm(crostini::kCrostiniDefaultVmName, device_info.guid);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(2, usb_device_observer_.notify_count());
@@ -783,3 +787,35 @@ TEST_F(CrosUsbDetectorTest, DeviceCanBeAttachedToArcVmWhenCrostiniIsDisabled) {
 
   EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
 }
+
+TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
+  ConnectToDeviceManager();
+  base::RunLoop().RunUntilIdle();
+
+  auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
+      0, 1, kManufacturerName, kProductName_1, "002");
+  device_manager_.AddDevice(device_1);
+  base::RunLoop().RunUntilIdle();
+
+  const auto device = GetSingleDeviceInfo();
+  AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device.guid);
+  base::RunLoop().RunUntilIdle();
+
+  cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
+  cros_usb_detector_->ConnectSharedDevicesOnVmStartup(arc::kArcVmName);
+  base::RunLoop().RunUntilIdle();
+  // Starting ARCVM should be a no-op here as the device was already shared with
+  // Crostini.
+  EXPECT_EQ(0, usb_device_observer_.notify_count());
+
+  cros_usb_detector_->ConnectSharedDevicesOnVmStartup(
+      crostini::kCrostiniDefaultVmName);
+  base::RunLoop().RunUntilIdle();
+  // The device was already attached to Crostini.
+  EXPECT_EQ(0, usb_device_observer_.notify_count());
+
+  DetachDeviceFromVm(crostini::kCrostiniDefaultVmName, device.guid);
+  cros_usb_detector_->ConnectSharedDevicesOnVmStartup(arc::kArcVmName);
+  EXPECT_EQ(2, usb_device_observer_.notify_count());
+  EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
+}

components/arc/usb/usb_host_bridge.cc

@@ -211,6 +211,9 @@ void ArcUsbHostBridge::GetDeviceInfo(const std::string& guid,
 }
 
 void ArcUsbHostBridge::OnConnectionReady() {
+  if (delegate_)
+    delegate_->AttachDevicesToArcVm();
+
   // Request UsbDeviceManagerPtr from DeviceService.
   content::GetSystemConnector()->BindInterface(
       device::mojom::kServiceName, mojo::MakeRequest(&usb_manager_));
@@ -241,6 +244,10 @@ void ArcUsbHostBridge::SetUiDelegate(ArcUsbHostUiDelegate* ui_delegate) {
   ui_delegate_ = ui_delegate;
 }
 
+void ArcUsbHostBridge::SetDelegate(std::unique_ptr<Delegate> delegate) {
+  delegate_ = std::move(delegate);
+}
+
 void ArcUsbHostBridge::InitDeviceList(
     std::vector<device::mojom::UsbDeviceInfoPtr> devices) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);

components/arc/usb/usb_host_bridge.h

@@ -37,6 +37,17 @@ class ArcUsbHostBridge : public KeyedService,
                          public device::mojom::UsbDeviceManagerClient,
                          public mojom::UsbHostHost {
  public:
+  class Delegate {
+   public:
+    virtual ~Delegate() = default;
+
+    // Attaches the unclaimed USB devices to the ARCVM instance if ARCVM is
+    // enabled. Called by ArcUsbHostBridge once it successfully established the
+    // Mojo connection to the ARC instance. This may be called multiple times
+    // within the lifetime of a single ArcUsbHostBridge instance.
+    virtual void AttachDevicesToArcVm() = 0;
+  };
+
   // Returns singleton instance for the given BrowserContext,
   // or nullptr if the browser |context| is not allowed to use ARC.
   static ArcUsbHostBridge* GetForBrowserContext(
@@ -73,6 +84,9 @@ class ArcUsbHostBridge : public KeyedService,
 
   void SetUiDelegate(ArcUsbHostUiDelegate* ui_delegate);
 
+  // Sets the Delegate instance.
+  void SetDelegate(std::unique_ptr<Delegate> delegate);
+
  private:
   // Init |devices_| once the device list has been returned, so that we
   // can get UsbDeviceInfo from |guid| for other methods.
@@ -104,6 +118,7 @@ class ArcUsbHostBridge : public KeyedService,
   std::map<std::string, device::mojom::UsbDeviceInfoPtr> devices_;
 
   ArcUsbHostUiDelegate* ui_delegate_ = nullptr;
+  std::unique_ptr<Delegate> delegate_;
 
   // WeakPtrFactory to use for callbacks.
   base::WeakPtrFactory<ArcUsbHostBridge> weak_factory_{this};