Fix Android USB debugging through DevTools.

First, change 13cf2cf introduced a
logic error into AndroidUsbDevice::EnumerateOnFileThread that caused
enumerations to never complete because the BarrierClosure was not
called the correct number of times.

Second, this change may have also exposed an issue (investigated
before) where a libusb_transfer can be cancelled twice, once in
ReleaseInterface and again in CloseInternal. The second cancellation
occurs after the interface has been released and so an invalid handle
is passed to AbortPipe. A cancelled_ member has been added to Transfer
to prevent this from happening.

Some conversions to C++11 range loops (added during debugging to make
the loops clearer) are preserved in this change.

BUG=450007

Review URL: https://codereview.chromium.org/864183002

Cr-Commit-Position: refs/heads/master@{#312796}

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

@@ -35,25 +35,36 @@ using device::UsbUsageType;
 
 namespace {
 
+struct NoConfigTraits {
+  static const int kClass = 0xff;
+  static const int kSubclass = 0x42;
+  static const int kProtocol = 0x1;
+  static const bool kBreaks = false;
+  static const bool kConfigured = false;
+};
+
 struct AndroidTraits {
   static const int kClass = 0xff;
   static const int kSubclass = 0x42;
   static const int kProtocol = 0x1;
   static const bool kBreaks = false;
+  static const bool kConfigured = true;
 };
 
 struct NonAndroidTraits {
   static const int kClass = 0xf0;
   static const int kSubclass = 0x42;
   static const int kProtocol = 0x2;
   static const bool kBreaks = false;
+  static const bool kConfigured = true;
 };
 
 struct BreakingAndroidTraits {
   static const int kClass = 0xff;
   static const int kSubclass = 0x42;
   static const int kProtocol = 0x1;
   static const bool kBreaks = true;
+  static const bool kConfigured = true;
 };
 
 const uint32 kMaxPayload = 4096;
@@ -400,7 +411,7 @@ class MockUsbDevice : public UsbDevice {
   }
 
   virtual const UsbConfigDescriptor* GetConfiguration() override {
-    return &config_desc_;
+    return T::kConfigured ? &config_desc_ : nullptr;
   }
 
   virtual bool GetManufacturer(base::string16* manufacturer) override {
@@ -475,6 +486,20 @@ class MockBreakingUsbService : public UsbService {
   }
 };
 
+class MockNoConfigUsbService : public UsbService {
+ public:
+  scoped_refptr<UsbDevice> GetDeviceById(uint32 unique_id) override {
+    NOTIMPLEMENTED();
+    return nullptr;
+  }
+
+  void GetDevices(std::vector<scoped_refptr<UsbDevice>>* devices) override {
+    STLClearObject(devices);
+    devices->push_back(new MockUsbDevice<AndroidTraits>());
+    devices->push_back(new MockUsbDevice<NoConfigTraits>());
+  }
+};
+
 class MockUsbServiceForCheckingTraits : public UsbService {
  public:
   MockUsbServiceForCheckingTraits() : step_(0) {}
@@ -612,6 +637,13 @@ class AndroidBreakingUsbTest : public AndroidUsbDiscoveryTest {
   }
 };
 
+class AndroidNoConfigUsbTest : public AndroidUsbDiscoveryTest {
+ protected:
+  void SetUpService() override {
+    UsbService::SetInstanceForTest(new MockNoConfigUsbService());
+  }
+};
+
 class MockListListener : public DevToolsAndroidBridge::DeviceListListener {
  public:
   MockListListener(DevToolsAndroidBridge* adb_bridge,
@@ -787,6 +819,12 @@ IN_PROC_BROWSER_TEST_F(AndroidBreakingUsbTest, TestDeviceBreaking) {
   runner_->Run();
 }
 
+IN_PROC_BROWSER_TEST_F(AndroidNoConfigUsbTest, TestDeviceNoConfig) {
+  MockListListener listener(adb_bridge_, runner_->QuitClosure());
+  adb_bridge_->AddDeviceListListener(&listener);
+  runner_->Run();
+}
+
 IN_PROC_BROWSER_TEST_F(AndroidUsbCountTest,
                        TestNoMultipleCallsRemoveInCallback) {
   MockCountListener listener(adb_bridge_);

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

@@ -78,16 +78,14 @@ scoped_refptr<AndroidUsbDevice> ClaimInterface(
   int outbound_address = 0;
   int zero_mask = 0;
 
-  for (UsbEndpointDescriptor::Iterator endpointIt = interface.endpoints.begin();
-       endpointIt != interface.endpoints.end();
-       ++endpointIt) {
-    if (endpointIt->transfer_type != device::USB_TRANSFER_BULK)
+  for (const UsbEndpointDescriptor& endpoint : interface.endpoints) {
+    if (endpoint.transfer_type != device::USB_TRANSFER_BULK)
       continue;
-    if (endpointIt->direction == device::USB_DIRECTION_INBOUND)
-      inbound_address = endpointIt->address;
+    if (endpoint.direction == device::USB_DIRECTION_INBOUND)
+      inbound_address = endpoint.address;
     else
-      outbound_address = endpointIt->address;
-    zero_mask = endpointIt->maximum_packet_size - 1;
+      outbound_address = endpoint.address;
+    zero_mask = endpoint.maximum_packet_size - 1;
   }
 
   if (inbound_address == 0 || outbound_address == 0)
@@ -169,9 +167,8 @@ static void RespondOnCallerThread(const AndroidUsbDevicesCallback& callback,
   scoped_ptr<AndroidUsbDevices> devices(new_devices);
 
   // Add raw pointers to the newly claimed devices.
-  for (AndroidUsbDevices::iterator it = devices->begin(); it != devices->end();
-       ++it) {
-    g_devices.Get().push_back(it->get());
+  for (const scoped_refptr<AndroidUsbDevice>& device : *devices) {
+    g_devices.Get().push_back(device.get());
   }
 
   // Return all claimed devices.
@@ -207,7 +204,7 @@ static void OpenAndroidDeviceOnFileThread(
           scoped_refptr<AndroidUsbDevice> android_device = ClaimInterface(
               rsa_key, usb_handle, serial, config->interfaces[interface_id]);
           if (android_device.get())
-            devices->push_back(android_device.get());
+            devices->push_back(android_device);
           else
             usb_handle->Close();
         }
@@ -259,6 +256,7 @@ static void EnumerateOnFileThread(
   for (const scoped_refptr<UsbDevice>& device : usb_devices) {
     const UsbConfigDescriptor* config = device->GetConfiguration();
     if (!config) {
+      barrier.Run();
       continue;
     }
     bool has_android_interface = false;
@@ -299,12 +297,11 @@ void AndroidUsbDevice::Enumerate(crypto::RSAPrivateKey* rsa_key,
                                  const AndroidUsbDevicesCallback& callback) {
 
   // Collect devices with closed handles.
-  for (std::vector<AndroidUsbDevice*>::iterator it = g_devices.Get().begin();
-       it != g_devices.Get().end(); ++it) {
-    if ((*it)->usb_handle_.get()) {
+  for (AndroidUsbDevice* device : g_devices.Get()) {
+    if (device->usb_handle_.get()) {
       BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-          base::Bind(&AndroidUsbDevice::TerminateIfReleased, *it,
-                     (*it)->usb_handle_));
+          base::Bind(&AndroidUsbDevice::TerminateIfReleased, device,
+                     device->usb_handle_));
     }
   }
 

device/usb/usb_device_handle_impl.cc

@@ -211,6 +211,7 @@ class UsbDeviceHandleImpl::Transfer {
   scoped_refptr<UsbDeviceHandleImpl::InterfaceClaimer> claimed_interface_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   size_t length_;
+  bool cancelled_;
   UsbTransferCallback callback_;
   scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner_;
 };
@@ -340,6 +341,7 @@ UsbDeviceHandleImpl::Transfer::Transfer(UsbTransferType transfer_type,
     : transfer_type_(transfer_type),
       buffer_(buffer),
       length_(length),
+      cancelled_(false),
       callback_(callback) {
   // Remember the thread from which this transfer was created so that |callback|
   // can be dispatched there.
@@ -376,8 +378,11 @@ bool UsbDeviceHandleImpl::Transfer::Submit(
 }
 
 void UsbDeviceHandleImpl::Transfer::Cancel() {
-  libusb_cancel_transfer(platform_transfer_);
-  claimed_interface_ = nullptr;
+  if (!cancelled_) {
+    libusb_cancel_transfer(platform_transfer_);
+    claimed_interface_ = nullptr;
+  }
+  cancelled_ = true;
 }
 
 void UsbDeviceHandleImpl::Transfer::ProcessCompletion() {