Prevent chrome://inspect from creating duplicate USB connections

Recently chrome://inspect started to open USB devices multiple times
while probing for ADB devices. On Chrome OS this causes a problem
because the permission_broker will disconnect the previous connection in
favor of the new connection. This caused an endless cycle of connection
failures.

This patch maintains a list of the devices that are currently open and
refused to reopen them during probing until the previous connection is
fully closed.

Bug: 725320
Change-Id: Id9b01d8d14c55ee2576499e2f1c778fea6c7dedc
Reviewed-on: https://chromium-review.googlesource.com/572300
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488049}

chrome/browser/devtools/device/usb/android_usb_browsertest.cc

@@ -138,7 +138,10 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
     return device_;
   }
 
-  void Close() override { device_ = nullptr; }
+  void Close() override {
+    device_->set_open(false);
+    device_ = nullptr;
+  }
 
   void SetConfiguration(int configuration_value,
                         const ResultCallback& callback) override {
@@ -422,12 +425,21 @@ class MockUsbDevice : public UsbDevice {
   }
 
   void Open(const OpenCallback& callback) override {
+    // While most operating systems allow multiple applications to open a
+    // device simultaneously so that they may claim separate interfaces DevTools
+    // will always be trying to claim the same interface and so multiple
+    // connections are more likely to cause problems. https://crbug.com/725320
+    EXPECT_FALSE(open_);
+    open_ = true;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::BindOnce(callback,
                        make_scoped_refptr(new MockUsbDeviceHandle<T>(this))));
   }
 
+  void set_open(bool open) { open_ = open; }
+
+  bool open_ = false;
   std::set<int> claimed_interfaces_;
 
  protected:

chrome/browser/devtools/device/usb/android_usb_device.cc

@@ -64,6 +64,11 @@ typedef std::set<scoped_refptr<UsbDevice> > UsbDeviceSet;
 base::LazyInstance<std::vector<AndroidUsbDevice*>>::Leaky g_devices =
     LAZY_INSTANCE_INITIALIZER;
 
+// Stores the GUIDs of devices that are currently opened so that they are not
+// re-probed.
+base::LazyInstance<std::vector<std::string>>::Leaky g_open_devices =
+    LAZY_INSTANCE_INITIALIZER;
+
 bool IsAndroidInterface(const UsbInterfaceDescriptor& interface) {
   if (interface.alternate_setting != 0 ||
       interface.interface_class != kAdbClass ||
@@ -133,6 +138,7 @@ void DumpMessage(bool outgoing, const char* data, size_t length) {
 
 void CloseDevice(scoped_refptr<UsbDeviceHandle> usb_device,
                  bool release_successful) {
+  base::Erase(g_open_devices.Get(), usb_device->GetDevice()->guid());
   usb_device->Close();
 }
 
@@ -187,6 +193,7 @@ void CreateDeviceOnInterfaceClaimed(AndroidUsbDevices* devices,
 }
 
 void OnDeviceOpened(AndroidUsbDevices* devices,
+                    const std::string& device_guid,
                     crypto::RSAPrivateKey* rsa_key,
                     int inbound_address,
                     int outbound_address,
@@ -195,6 +202,7 @@ void OnDeviceOpened(AndroidUsbDevices* devices,
                     const base::Closure& barrier,
                     scoped_refptr<UsbDeviceHandle> usb_handle) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   if (usb_handle.get()) {
     usb_handle->ClaimInterface(
         interface_number,
@@ -202,6 +210,7 @@ void OnDeviceOpened(AndroidUsbDevices* devices,
                    usb_handle, inbound_address, outbound_address, zero_mask,
                    interface_number, barrier));
   } else {
+    base::Erase(g_open_devices.Get(), device_guid);
     barrier.Run();
   }
 }
@@ -243,8 +252,15 @@ void OpenAndroidDevice(AndroidUsbDevices* devices,
     return;
   }
 
-  device->Open(base::Bind(&OnDeviceOpened, devices, rsa_key, inbound_address,
-                          outbound_address, zero_mask,
+  if (base::ContainsValue(g_open_devices.Get(), device->guid())) {
+    // |device| is already open, do not make parallel attempts to connect to it.
+    barrier.Run();
+    return;
+  }
+
+  g_open_devices.Get().push_back(device->guid());
+  device->Open(base::Bind(&OnDeviceOpened, devices, device->guid(), rsa_key,
+                          inbound_address, outbound_address, zero_mask,
                           interface.interface_number, barrier));
 }
 
@@ -459,9 +475,8 @@ void AndroidUsbDevice::ProcessOutgoing() {
 void AndroidUsbDevice::OutgoingMessageSent(UsbTransferStatus status,
                                            scoped_refptr<net::IOBuffer> buffer,
                                            size_t result) {
-  if (status != UsbTransferStatus::COMPLETED) {
+  if (status != UsbTransferStatus::COMPLETED)
     return;
-  }
 
   task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&AndroidUsbDevice::ProcessOutgoing, this));