Put auto-attach USB device to ARCVM functionality behind ARCVM flag

There is a recent regression where USB serial devices (SuziQ) quickly
disappear from Crostini after being attached.

It's unclear yet whether the auto-attach to ARCVM, which is mostly
a no-op when ARCVM is not running modulo the extra device open prior to
that, is causing it. Nonetheless this ARCVM-specific functionality
should be put behind the ARCVM feature flag to avoid side effects when
ARCVM is not enabled.

Bug: 996113
Test: CrosUsbDetectorTest.DevicesDontGetAutoAttachedToArcVmWhenDisabled
Change-Id: Ib5f251402a27da95a28fb70a6b695741342ebc96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1763558Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Philippe Liard <pliard@google.com>
Auto-Submit: Philippe Liard <pliard@google.com>
Cr-Commit-Position: refs/heads/master@{#689725}

chrome/browser/chromeos/usb/cros_usb_detector.cc

@@ -38,18 +38,24 @@ static CrosUsbDetector* g_cros_usb_detector = nullptr;
 
 const char kNotifierUsb[] = "crosusb.connected";
 
-// ARCVM (Android VM) is considered as the "default" VM here 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.
+// 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();
@@ -353,11 +359,13 @@ void CrosUsbDetector::OnDeviceChecked(
   available_device_info_.emplace(device_info->guid, device_info.Clone());
   SignalUsbDeviceObservers();
 
-  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());
+  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.
@@ -447,8 +455,8 @@ void CrosUsbDetector::AttachUsbDeviceToVm(
       break;
     }
   }
-  if (vm_name == kDefaultVmName) {
-    AttachUsbDeviceToVmInternal(kDefaultVmName, guid, std::move(callback));
+  if (!IsDefaultVmEnabled() || vm_name == kDefaultVmName) {
+    AttachUsbDeviceToVmInternal(vm_name, guid, std::move(callback));
     return;
   }
   auto attach_internal = base::BindOnce(
@@ -483,7 +491,7 @@ void CrosUsbDetector::DetachUsbDeviceFromVm(
     const std::string& vm_name,
     const std::string& guid,
     base::OnceCallback<void(bool success)> callback) {
-  if (vm_name == kDefaultVmName) {
+  if (!IsDefaultVmEnabled() || vm_name == kDefaultVmName) {
     DetachUsbDeviceFromVmInternal(vm_name, guid, std::move(callback));
     return;
   }

chrome/browser/chromeos/usb/cros_usb_detector_unittest.cc

@@ -138,6 +138,7 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
         {chromeos::features::kCrostiniUsbSupport,
          chromeos::features::kCrostiniUsbAllowUnsupported},
         {});
+    arc::EnableArcVmForTesting();
 
     TestingBrowserProcess::GetGlobal()->SetSystemNotificationHelper(
         std::make_unique<SystemNotificationHelper>());
@@ -155,6 +156,7 @@ class CrosUsbDetectorTest : public BrowserWithTestWindowTest {
   }
 
   void TearDown() override {
+    arc::DisableArcVmForTesting();
     scoped_feature_list_.Reset();
     crostini_test_helper_.reset();
     BrowserWithTestWindowTest::TearDown();
@@ -818,3 +820,26 @@ TEST_F(CrosUsbDetectorTest, SharedDevicesGetAttachedOnStartup) {
   EXPECT_EQ(2, usb_device_observer_.notify_count());
   EXPECT_TRUE(GetSingleDeviceInfo().vm_sharing_info[arc::kArcVmName].shared);
 }
+
+TEST_F(CrosUsbDetectorTest, DevicesDontGetAutoAttachedToArcVmWhenDisabled) {
+  arc::DisableArcVmForTesting();
+  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.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());
+}

components/arc/arc_util.cc

@@ -108,6 +108,16 @@ 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,6 +43,11 @@ 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.