Remove logic auto-attaching unclaimed USB devices to ARCVM

This behavior causes the attached bug where serial devices get detached
from the host. This behavior isn't going to work in general as unclaimed
devices on ChromeOS are still claimable through webusb for example.

This means that devices should get attached to ARCVM on demand/user
decision rather than on connection.

BUG=1763558, b:123374026
TEST=CrosUsbDetectorTest

Change-Id: I3aa40bfb08bb2ce61ddb6c3ff47efffe0781b728
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1772783Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Auto-Submit: Philippe Liard <pliard@google.com>
Commit-Queue: Philippe Liard <pliard@google.com>
Cr-Commit-Position: refs/heads/master@{#693026}

chrome/browser/chromeos/usb/cros_usb_detector.cc

@@ -38,29 +38,6 @@ static CrosUsbDetector* g_cros_usb_detector = nullptr;
 
 const char kNotifierUsb[] = "crosusb.connected";
 
-// ARCVM (Android VM) is considered as the "default" VM here, when it's enabled,
-// in the sense that the Android runtime is enabled by default on Chrome OS and
-// is always running. Android also has its own mechanisms/permissions ultimately
-// relying on Chrome OS for granting access to the USB devices attached in
-// ARCVM/ARC++. This means that unclaimed devices get auto-attached to ARCVM on
-// connect until the user decides otherwise by e.g. disconnecting them or
-// explicitly sharing them with another VM such as Crostini.
-// Note that attaching a device to a VM makes it visible by the guest kernel in
-// that VM but doesn't actually claim/open the device by itself, i.e. a device
-// attached in a VM (and not open there) can still be opened outside that VM.
-const char* const kDefaultVmName = arc::kArcVmName;
-
-// Returns whether the default VM is enabled. See declaration of
-// |kDefaultVmName|.
-bool IsDefaultVmEnabled() {
-  return arc::IsArcVmEnabled();
-}
-
-// Helper free function used with BindOnce() below for convenience.
-void IgnoreBool(base::OnceClosure closure, bool) {
-  std::move(closure).Run();
-}
-
 void RecordNotificationClosure(CrosUsbNotificationClosed disposition) {
   UMA_HISTOGRAM_ENUMERATION("CrosUsb.NotificationClosed", disposition);
 }
@@ -358,15 +335,6 @@ void CrosUsbDetector::OnDeviceChecked(
   available_device_info_.emplace(device_info->guid, device_info.Clone());
   SignalUsbDeviceObservers();
 
-  if (IsDefaultVmEnabled()) {
-    if (has_supported_interface || !has_blocked_interface) {
-      // USB devices not claimed by Chrome OS get automatically attached to the
-      // default VM. Note that this relies on the underlying VM (ARCVM) having
-      // its own permission model to restrict access to the device.
-      AttachUsbDeviceToVm(kDefaultVmName, new_device.guid, base::DoNothing());
-    }
-  }
-
   // Some devices should not trigger the notification.
   if (!new_device.sharable_with_crostini || hide_notification ||
       !ShouldShowNotification(*device_info)) {
@@ -397,8 +365,7 @@ void CrosUsbDetector::OnDeviceRemoved(
   for (const auto& device : usb_devices_) {
     if (device.guid == guid) {
       for (const auto& sharing_info_pair : device.vm_sharing_info) {
-        DetachUsbDeviceFromVmInternal(sharing_info_pair.first, guid,
-                                      base::DoNothing());
+        DetachUsbDeviceFromVm(sharing_info_pair.first, guid, base::DoNothing());
       }
     }
   }
@@ -422,20 +389,11 @@ void CrosUsbDetector::ConnectSharedDevicesOnVmStartup(
     const std::string& vm_name) {
   // Reattach shared devices when the VM becomes available.
   for (auto& device : usb_devices_) {
-    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) {
-        is_shared = true;
-        if (sharing_pair.first == vm_name) {
-          is_shared_with_this_vm = true;
-          break;
-        }
+      if (sharing_pair.second.shared && sharing_pair.first == vm_name) {
+        AttachUsbDeviceToVm(vm_name, device.guid, base::DoNothing());
       }
     }
-    if (is_shared_with_this_vm || (vm_name == kDefaultVmName && !is_shared)) {
-      AttachUsbDeviceToVm(vm_name, device.guid, base::DoNothing());
-    }
   }
 }
 
@@ -454,22 +412,6 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
       break;
     }
   }
-  if (!IsDefaultVmEnabled() || vm_name == kDefaultVmName) {
-    AttachUsbDeviceToVmInternal(vm_name, guid, std::move(callback));
-    return;
-  }
-  auto attach_internal = base::BindOnce(
-      &CrosUsbDetector::AttachUsbDeviceToVmInternal,
-      weak_ptr_factory_.GetWeakPtr(), vm_name, guid, std::move(callback));
-  DetachUsbDeviceFromVm(
-      kDefaultVmName, guid,
-      base::BindOnce(&IgnoreBool, std::move(attach_internal)));
-}
-
-void CrosUsbDetector::AttachUsbDeviceToVmInternal(
-    const std::string& vm_name,
-    const std::string& guid,
-    base::OnceCallback<void(bool success)> callback) {
   auto it = available_device_info_.find(guid);
   if (it == available_device_info_.end()) {
     return;
@@ -490,23 +432,6 @@ void CrosUsbDetector::DetachUsbDeviceFromVm(
     const std::string& vm_name,
     const std::string& guid,
     base::OnceCallback<void(bool success)> callback) {
-  if (!IsDefaultVmEnabled() || vm_name == kDefaultVmName) {
-    DetachUsbDeviceFromVmInternal(vm_name, guid, std::move(callback));
-    return;
-  }
-  auto attach_to_default_vm =
-      base::BindOnce(&CrosUsbDetector::AttachUsbDeviceToVmInternal,
-                     weak_ptr_factory_.GetWeakPtr(), kDefaultVmName, guid,
-                     std::move(callback));
-  DetachUsbDeviceFromVmInternal(
-      vm_name, guid,
-      base::BindOnce(&IgnoreBool, std::move(attach_to_default_vm)));
-}
-
-void CrosUsbDetector::DetachUsbDeviceFromVmInternal(
-    const std::string& vm_name,
-    const std::string& guid,
-    base::OnceCallback<void(bool success)> callback) {
   const auto& it = available_device_info_.find(guid);
   if (it == available_device_info_.end()) {
     // If there wasn't an existing attachment, then removal is a no-op and

chrome/browser/chromeos/usb/cros_usb_detector.h

@@ -106,15 +106,13 @@ class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
   void OnDeviceRemoved(device::mojom::UsbDeviceInfoPtr device) override;
 
   // Attaches the device identified by |guid| into the VM identified by
-  // |vm_name|. Note that this may detach the device from the default VM
-  // (=ARCVM) beforehand.
+  // |vm_name|.
   void AttachUsbDeviceToVm(const std::string& vm_name,
                            const std::string& guid,
                            base::OnceCallback<void(bool success)> callback);
 
   // Detaches the device identified by |guid| from the VM identified by
-  // |vm_name|. Note that this may attach the device (back) into the default VM
-  // (=ARCVM) as a result.
+  // |vm_name|.
   void DetachUsbDeviceFromVm(const std::string& vm_name,
                              const std::string& guid,
                              base::OnceCallback<void(bool success)> callback);
@@ -133,16 +131,6 @@ class CrosUsbDetector : public device::mojom::UsbDeviceManagerClient {
   std::vector<CrosUsbDeviceInfo> GetDevicesSharableWithCrostini() const;
 
  private:
-  void AttachUsbDeviceToVmInternal(
-      const std::string& vm_name,
-      const std::string& guid,
-      base::OnceCallback<void(bool success)> callback);
-
-  void DetachUsbDeviceFromVmInternal(
-      const std::string& vm_name,
-      const std::string& guid,
-      base::OnceCallback<void(bool success)> callback);
-
   // Called after USB device access has been checked.
   void OnDeviceChecked(device::mojom::UsbDeviceInfoPtr device,
                        bool hide_notification,

chrome/browser/chromeos/usb/cros_usb_detector_unittest.cc

@@ -138,7 +138,6 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
         {chromeos::features::kCrostiniUsbSupport,
          chromeos::features::kCrostiniUsbAllowUnsupported},
         {});
-    arc::EnableArcVmForTesting();
 
     TestingBrowserProcess::GetGlobal()->SetSystemNotificationHelper(
         std::make_unique<SystemNotificationHelper>());
@@ -157,7 +156,6 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
   }
 
   void TearDown() override {
-    arc::DisableArcVmForTesting();
     scoped_feature_list_.Reset();
     crostini_test_helper_.reset();
     BrowserWithTestWindowTest::TearDown();
@@ -196,6 +194,13 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
     return devices.front();
   }
 
+  static bool IsSharedWithCrostini(
+      const chromeos::CrosUsbDeviceInfo& device_info) {
+    const auto it = device_info.vm_sharing_info.find(
+        crostini::kCrostiniDefaultVmName);
+    return it != device_info.vm_sharing_info.end() && it->second.shared;
+  }
+
  protected:
   base::string16 connection_message(const char* product_name) {
     return base::ASCIIToUTF16(base::StringPrintf(
@@ -699,62 +704,6 @@ TEST_F(CrosUsbDetectorTest, AttachDeviceToVmSetsGuestPort) {
   EXPECT_EQ(0U, *crostini_info.guest_port);
 }
 
-TEST_F(CrosUsbDetectorTest, DeviceGetsAutoAttachedToArcVmOnConnect) {
-  cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
-  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();
-  EXPECT_EQ(2, usb_device_observer_.notify_count());
-  EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
-}
-
-TEST_F(CrosUsbDetectorTest, AttachingDeviceToCrostiniDetachesItFromArcVm) {
-  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();
-
-  auto device_info = GetSingleDeviceInfo();
-  EXPECT_TRUE(device_info.vm_sharing_info[arc::kArcVmName].shared);
-
-  cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
-  AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
-  EXPECT_EQ(2, usb_device_observer_.notify_count());
-
-  device_info = GetSingleDeviceInfo();
-  EXPECT_EQ(1U, device_info.vm_sharing_info.size());
-  EXPECT_TRUE(
-      device_info.vm_sharing_info[crostini::kCrostiniDefaultVmName].shared);
-}
-
-TEST_F(CrosUsbDetectorTest, DetachingDeviceFromCrostiniAttachesItToArcVm) {
-  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();
-
-  auto device_info = GetSingleDeviceInfo();
-  AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
-  cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
-  DetachDeviceFromVm(crostini::kCrostiniDefaultVmName, device_info.guid);
-  base::RunLoop().RunUntilIdle();
-
-  EXPECT_EQ(2, usb_device_observer_.notify_count());
-  device_info = GetSingleDeviceInfo();
-  EXPECT_EQ(1U, device_info.vm_sharing_info.size());
-  EXPECT_TRUE(device_info.vm_sharing_info[arc::kArcVmName].shared);
-}
-
 TEST_F(CrosUsbDetectorTest, AttachingAlreadyAttachedDeviceIsANoOp) {
   ConnectToDeviceManager();
   base::RunLoop().RunUntilIdle();
@@ -765,7 +714,7 @@ TEST_F(CrosUsbDetectorTest, AttachingAlreadyAttachedDeviceIsANoOp) {
   base::RunLoop().RunUntilIdle();
 
   auto device_info = GetSingleDeviceInfo();
-  EXPECT_TRUE(device_info.vm_sharing_info[arc::kArcVmName].shared);
+  EXPECT_EQ(0U, device_info.vm_sharing_info.size());
 
   AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
   cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
@@ -773,8 +722,7 @@ TEST_F(CrosUsbDetectorTest, AttachingAlreadyAttachedDeviceIsANoOp) {
   EXPECT_EQ(0, usb_device_observer_.notify_count());
   device_info = GetSingleDeviceInfo();
   EXPECT_EQ(1U, device_info.vm_sharing_info.size());
-  EXPECT_TRUE(
-      device_info.vm_sharing_info[crostini::kCrostiniDefaultVmName].shared);
+  EXPECT_TRUE(IsSharedWithCrostini(device_info));
 }
 
 TEST_F(CrosUsbDetectorTest, DeviceCanBeAttachedToArcVmWhenCrostiniIsDisabled) {
@@ -787,6 +735,7 @@ TEST_F(CrosUsbDetectorTest, DeviceCanBeAttachedToArcVmWhenCrostiniIsDisabled) {
   device_manager_.AddDevice(device_1);
   base::RunLoop().RunUntilIdle();
 
+  AttachDeviceToVm(arc::kArcVmName, GetSingleDeviceInfo().guid);
   EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
 }
 
@@ -799,48 +748,24 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
   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.
+  // No device is shared with Crostini, yet.
   EXPECT_EQ(0, usb_device_observer_.notify_count());
+  auto device = GetSingleDeviceInfo();
+  EXPECT_EQ(0U, device.vm_sharing_info.size());
 
-  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);
-}
-
-TEST_F(CrosUsbDetectorTest, DevicesDontGetAutoAttachedToArcVmWhenDisabled) {
-  arc::DisableArcVmForTesting();
-  ConnectToDeviceManager();
+  device = GetSingleDeviceInfo();
+  AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device.guid);
   base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
 
-  auto device_1 = base::MakeRefCounted<device::FakeUsbDeviceInfo>(
-      0, 1, kManufacturerName, kProductName_1, "002");
-  device_manager_.AddDevice(device_1);
+  cros_usb_detector_->AddUsbDeviceObserver(&usb_device_observer_);
+  cros_usb_detector_->ConnectSharedDevicesOnVmStartup(
+      crostini::kCrostiniDefaultVmName);
   base::RunLoop().RunUntilIdle();
-
-  auto device_info = GetSingleDeviceInfo();
-  EXPECT_TRUE(device_info.vm_sharing_info.empty());
-
-  AttachDeviceToVm(crostini::kCrostiniDefaultVmName, device_info.guid);
-  device_info = GetSingleDeviceInfo();
-  EXPECT_TRUE(
-      device_info.vm_sharing_info[crostini::kCrostiniDefaultVmName].shared);
-
-  // Detaching shouldn't auto-attach to ARCVM either when ARCVM is disabled.
-  DetachDeviceFromVm(crostini::kCrostiniDefaultVmName, device_info.guid);
-  EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info.empty());
+  // Attaching an already attached device is a no-op currently.
+  EXPECT_EQ(0, usb_device_observer_.notify_count());
+  EXPECT_TRUE(IsSharedWithCrostini(GetSingleDeviceInfo()));
 }

components/arc/arc_util.cc

@@ -120,16 +120,6 @@ bool IsArcVmEnabled() {
       chromeos::switches::kEnableArcVm);
 }
 
-void EnableArcVmForTesting() {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      chromeos::switches::kEnableArcVm);
-}
-
-void DisableArcVmForTesting() {
-  base::CommandLine::ForCurrentProcess()->RemoveSwitch(
-      chromeos::switches::kEnableArcVm);
-}
-
 bool ShouldArcAlwaysStart() {
   const auto* command_line = base::CommandLine::ForCurrentProcess();
   if (!command_line->HasSwitch(chromeos::switches::kArcStartMode))

components/arc/arc_util.h

@@ -43,11 +43,6 @@ bool IsArcAvailable();
 // Returns true if ARC VM is enabled.
 bool IsArcVmEnabled();
 
-// These two methods used for testing only add and remove the arcvm flag so that
-// IsArcVmEnabled() returns the corresponding result.
-void EnableArcVmForTesting();
-void DisableArcVmForTesting();
-
 // Returns true if ARC should always start within the primary user session
 // (opted in user or not), and other supported mode such as guest and Kiosk
 // mode.