Make UsbDeviceHandle::ReleaseInterface asynchronous.

This patch adds a callback to UsbDeviceHandle::ReleaseInterface that is
called when the interface has been released. Previously this result was
returned synchronously while the actual release operation happened in
the background.

In the process I've fixed an issue where libusb_release_interface could
have been called from the UI thread if there were no pending transfers
to delay it.

BUG=None

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

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

11 files changed, 99 insertions, 40 deletions

diff --git a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
index caa9950..2fac360 100644
--- a/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
+++ b/chrome/browser/devtools/device/usb/android_usb_browsertest.cc
@@ -153,13 +153,16 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
         FROM_HERE, base::Bind(callback, success));
   }
 
-  bool ReleaseInterface(int interface_number) override {
+  void ReleaseInterface(int interface_number,
+                        const ResultCallback& callback) override {
+    bool success = false;
     if (device_->claimed_interfaces_.find(interface_number) ==
         device_->claimed_interfaces_.end())
-      return false;
+      success = false;
 
     device_->claimed_interfaces_.erase(interface_number);
-    return true;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, success));
   }
 
   void SetInterfaceAlternateSetting(int interface_number,
diff --git a/chrome/browser/devtools/device/usb/android_usb_device.cc b/chrome/browser/devtools/device/usb/android_usb_device.cc
index fc8a274..8e655a6 100644
--- a/chrome/browser/devtools/device/usb/android_usb_device.cc
+++ b/chrome/browser/devtools/device/usb/android_usb_device.cc
@@ -128,10 +128,15 @@ void DumpMessage(bool outgoing, const char* data, size_t length) {
 #endif  // 0
 }
 
+void CloseDevice(scoped_refptr<UsbDeviceHandle> usb_device,
+                 bool release_successful) {
+  usb_device->Close();
+}
+
 void ReleaseInterface(scoped_refptr<UsbDeviceHandle> usb_device,
                       int interface_id) {
-  usb_device->ReleaseInterface(interface_id);
-  usb_device->Close();
+  usb_device->ReleaseInterface(interface_id,
+                               base::Bind(&CloseDevice, usb_device));
 }
 
 void RespondOnCallerThread(const AndroidUsbDevicesCallback& callback,
diff --git a/device/devices_app/usb/device_impl.cc b/device/devices_app/usb/device_impl.cc
index ae8ad40..2eb3ed5 100644
--- a/device/devices_app/usb/device_impl.cc
+++ b/device/devices_app/usb/device_impl.cc
@@ -303,7 +303,8 @@ void DeviceImpl::ReleaseInterface(uint8_t interface_number,
     return;
   }
 
-  callback.Run(device_handle_->ReleaseInterface(interface_number));
+  device_handle_->ReleaseInterface(interface_number,
+                                   WrapMojoCallback(callback));
 }
 
 void DeviceImpl::SetInterfaceAlternateSetting(
diff --git a/device/devices_app/usb/device_impl_unittest.cc b/device/devices_app/usb/device_impl_unittest.cc
index e5ee893..b98d330 100644
--- a/device/devices_app/usb/device_impl_unittest.cc
+++ b/device/devices_app/usb/device_impl_unittest.cc
@@ -192,7 +192,7 @@ class USBDeviceImplTest : public testing::Test {
         .WillByDefault(Invoke(this, &USBDeviceImplTest::SetConfiguration));
     ON_CALL(mock_handle(), ClaimInterface(_, _))
         .WillByDefault(Invoke(this, &USBDeviceImplTest::ClaimInterface));
-    ON_CALL(mock_handle(), ReleaseInterface(_))
+    ON_CALL(mock_handle(), ReleaseInterface(_, _))
         .WillByDefault(Invoke(this, &USBDeviceImplTest::ReleaseInterface));
     ON_CALL(mock_handle(), SetInterfaceAlternateSetting(_, _, _))
         .WillByDefault(
@@ -290,12 +290,14 @@ class USBDeviceImplTest : public testing::Test {
     callback.Run(false);
   }
 
-  bool ReleaseInterface(uint8_t interface_number) {
+  void ReleaseInterface(uint8_t interface_number,
+                        const UsbDeviceHandle::ResultCallback& callback) {
     if (ContainsKey(claimed_interfaces_, interface_number)) {
       claimed_interfaces_.erase(interface_number);
-      return true;
+      callback.Run(true);
+    } else {
+      callback.Run(false);
     }
-    return false;
   }
 
   void SetInterfaceAlternateSetting(
@@ -638,7 +640,7 @@ TEST_F(USBDeviceImplTest, ClaimAndReleaseInterface) {
     loop.Run();
   }
 
-  EXPECT_CALL(mock_handle(), ReleaseInterface(2));
+  EXPECT_CALL(mock_handle(), ReleaseInterface(2, _));
 
   {
     // Releasing a non-existent interface should fail.
@@ -648,7 +650,7 @@ TEST_F(USBDeviceImplTest, ClaimAndReleaseInterface) {
     loop.Run();
   }
 
-  EXPECT_CALL(mock_handle(), ReleaseInterface(1));
+  EXPECT_CALL(mock_handle(), ReleaseInterface(1, _));
 
   {
     // Now this should release the claimed interface and close the handle.
diff --git a/device/usb/mock_usb_device_handle.h b/device/usb/mock_usb_device_handle.h
index 26e6190..2cacafb 100644
--- a/device/usb/mock_usb_device_handle.h
+++ b/device/usb/mock_usb_device_handle.h
@@ -26,7 +26,8 @@ class MockUsbDeviceHandle : public UsbDeviceHandle {
                void(int configuration_value, const ResultCallback& callback));
   MOCK_METHOD2(ClaimInterface,
                void(int interface_number, const ResultCallback& callback));
-  MOCK_METHOD1(ReleaseInterface, bool(int interface_number));
+  MOCK_METHOD2(ReleaseInterface,
+               void(int interface_number, const ResultCallback& callback));
   MOCK_METHOD3(SetInterfaceAlternateSetting,
                void(int interface_number,
                     int alternate_setting,
diff --git a/device/usb/usb_device_handle.h b/device/usb/usb_device_handle.h
index b206cf1..31f0ec5 100644
--- a/device/usb/usb_device_handle.h
+++ b/device/usb/usb_device_handle.h
@@ -70,7 +70,8 @@ class UsbDeviceHandle : public base::RefCountedThreadSafe<UsbDeviceHandle> {
                                 const ResultCallback& callback) = 0;
   virtual void ClaimInterface(int interface_number,
                               const ResultCallback& callback) = 0;
-  virtual bool ReleaseInterface(int interface_number) = 0;
+  virtual void ReleaseInterface(int interface_number,
+                                const ResultCallback& callback) = 0;
   virtual void SetInterfaceAlternateSetting(int interface_number,
                                             int alternate_setting,
                                             const ResultCallback& callback) = 0;
diff --git a/device/usb/usb_device_handle_impl.cc b/device/usb/usb_device_handle_impl.cc
index fa4c085..16f343e 100644
--- a/device/usb/usb_device_handle_impl.cc
+++ b/device/usb/usb_device_handle_impl.cc
@@ -147,13 +147,19 @@ class UsbDeviceHandleImpl::InterfaceClaimer
     : public base::RefCountedThreadSafe<UsbDeviceHandleImpl::InterfaceClaimer> {
  public:
   InterfaceClaimer(scoped_refptr<UsbDeviceHandleImpl> handle,
-                   int interface_number);
+                   int interface_number,
+                   scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
+  int interface_number() const { return interface_number_; }
   int alternate_setting() const { return alternate_setting_; }
   void set_alternate_setting(const int alternate_setting) {
     alternate_setting_ = alternate_setting;
   }
 
+  void set_release_callback(const ResultCallback& callback) {
+    release_callback_ = callback;
+  }
+
  private:
   friend class base::RefCountedThreadSafe<InterfaceClaimer>;
   ~InterfaceClaimer();
@@ -161,19 +167,33 @@ class UsbDeviceHandleImpl::InterfaceClaimer
   const scoped_refptr<UsbDeviceHandleImpl> handle_;
   const int interface_number_;
   int alternate_setting_;
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  ResultCallback release_callback_;
+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(InterfaceClaimer);
 };
 
 UsbDeviceHandleImpl::InterfaceClaimer::InterfaceClaimer(
     scoped_refptr<UsbDeviceHandleImpl> handle,
-    int interface_number)
+    int interface_number,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : handle_(handle),
       interface_number_(interface_number),
-      alternate_setting_(0) {}
+      alternate_setting_(0),
+      task_runner_(task_runner) {}
 
 UsbDeviceHandleImpl::InterfaceClaimer::~InterfaceClaimer() {
-  libusb_release_interface(handle_->handle(), interface_number_);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  int rc = libusb_release_interface(handle_->handle(), interface_number_);
+  if (rc != LIBUSB_SUCCESS) {
+    USB_LOG(DEBUG) << "Failed to release interface: "
+                   << ConvertPlatformUsbErrorToString(rc);
+  }
+  if (!release_callback_.is_null()) {
+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(release_callback_, rc == LIBUSB_SUCCESS));
+  }
 }
 
 // This inner class owns the underlying libusb_transfer and may outlast
@@ -581,12 +601,13 @@ void UsbDeviceHandleImpl::ClaimInterface(int interface_number,
                  interface_number, callback));
 }
 
-bool UsbDeviceHandleImpl::ReleaseInterface(int interface_number) {
+void UsbDeviceHandleImpl::ReleaseInterface(int interface_number,
+                                           const ResultCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!device_)
-    return false;
-  if (!ContainsKey(claimed_interfaces_, interface_number))
-    return false;
+  if (!device_ || !ContainsKey(claimed_interfaces_, interface_number)) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(callback, false));
+    return;
+  }
 
   // Cancel all the transfers on that interface.
   InterfaceClaimer* interface_claimer =
@@ -596,10 +617,12 @@ bool UsbDeviceHandleImpl::ReleaseInterface(int interface_number) {
       transfer->Cancel();
     }
   }
+  interface_claimer->AddRef();
+  interface_claimer->set_release_callback(callback);
+  blocking_task_runner_->ReleaseSoon(FROM_HERE, interface_claimer);
   claimed_interfaces_.erase(interface_number);
 
   RefreshEndpointMap();
-  return true;
 }
 
 void UsbDeviceHandleImpl::SetInterfaceAlternateSetting(
@@ -790,25 +813,28 @@ void UsbDeviceHandleImpl::ClaimInterfaceOnBlockingThread(
     int interface_number,
     const ResultCallback& callback) {
   int rv = libusb_claim_interface(handle_, interface_number);
-  if (rv != LIBUSB_SUCCESS) {
+  scoped_refptr<InterfaceClaimer> interface_claimer;
+  if (rv == LIBUSB_SUCCESS) {
+    interface_claimer =
+        new InterfaceClaimer(this, interface_number, task_runner_);
+  } else {
     USB_LOG(EVENT) << "Failed to claim interface: "
                    << ConvertPlatformUsbErrorToString(rv);
   }
   task_runner_->PostTask(
       FROM_HERE, base::Bind(&UsbDeviceHandleImpl::ClaimInterfaceComplete, this,
-                            interface_number, rv == LIBUSB_SUCCESS, callback));
+                            interface_claimer, callback));
 }
 
 void UsbDeviceHandleImpl::ClaimInterfaceComplete(
-    int interface_number,
-    bool success,
+    scoped_refptr<InterfaceClaimer> interface_claimer,
     const ResultCallback& callback) {
-  if (success) {
-    claimed_interfaces_[interface_number] =
-        new InterfaceClaimer(this, interface_number);
+  if (interface_claimer) {
+    claimed_interfaces_[interface_claimer->interface_number()] =
+        interface_claimer;
     RefreshEndpointMap();
   }
-  callback.Run(success);
+  callback.Run(interface_claimer);
 }
 
 void UsbDeviceHandleImpl::SetInterfaceAlternateSettingOnBlockingThread(
diff --git a/device/usb/usb_device_handle_impl.h b/device/usb/usb_device_handle_impl.h
index ce2daff..0cb2311 100644
--- a/device/usb/usb_device_handle_impl.h
+++ b/device/usb/usb_device_handle_impl.h
@@ -53,7 +53,8 @@ class UsbDeviceHandleImpl : public UsbDeviceHandle {
                         const ResultCallback& callback) override;
   void ClaimInterface(int interface_number,
                       const ResultCallback& callback) override;
-  bool ReleaseInterface(int interface_number) override;
+  void ReleaseInterface(int interface_number,
+                        const ResultCallback& callback) override;
   void SetInterfaceAlternateSetting(int interface_number,
                                     int alternate_setting,
                                     const ResultCallback& callback) override;
@@ -116,8 +117,7 @@ class UsbDeviceHandleImpl : public UsbDeviceHandle {
   void SetConfigurationComplete(bool success, const ResultCallback& callback);
   void ClaimInterfaceOnBlockingThread(int interface_number,
                                       const ResultCallback& callback);
-  void ClaimInterfaceComplete(int interface_number,
-                              bool success,
+  void ClaimInterfaceComplete(scoped_refptr<InterfaceClaimer> interface_claimer,
                               const ResultCallback& callback);
   void SetInterfaceAlternateSettingOnBlockingThread(
       int interface_number,
diff --git a/device/usb/usb_device_handle_unittest.cc b/device/usb/usb_device_handle_unittest.cc
index 1c116fd..5fac40a 100644
--- a/device/usb/usb_device_handle_unittest.cc
+++ b/device/usb/usb_device_handle_unittest.cc
@@ -164,6 +164,10 @@ TEST_F(UsbDeviceHandleTest, InterruptTransfer) {
         << "Mismatch at index " << i << ".";
   }
 
+  TestResultCallback release_interface;
+  handle->ReleaseInterface(0, release_interface.callback());
+  ASSERT_TRUE(release_interface.WaitForResult());
+
   handle->Close();
 }
 
@@ -219,6 +223,10 @@ TEST_F(UsbDeviceHandleTest, BulkTransfer) {
         << "Mismatch at index " << i << ".";
   }
 
+  TestResultCallback release_interface;
+  handle->ReleaseInterface(1, release_interface.callback());
+  ASSERT_TRUE(release_interface.WaitForResult());
+
   handle->Close();
 }
 
@@ -245,6 +253,10 @@ TEST_F(UsbDeviceHandleTest, SetInterfaceAlternateSetting) {
   handle->SetInterfaceAlternateSetting(2, 1, set_interface.callback());
   ASSERT_TRUE(set_interface.WaitForResult());
 
+  TestResultCallback release_interface;
+  handle->ReleaseInterface(2, release_interface.callback());
+  ASSERT_TRUE(release_interface.WaitForResult());
+
   handle->Close();
 }
 
diff --git a/extensions/browser/api/usb/usb_api.cc b/extensions/browser/api/usb/usb_api.cc
index 3035239..73099f9 100644
--- a/extensions/browser/api/usb/usb_api.cc
+++ b/extensions/browser/api/usb/usb_api.cc
@@ -925,11 +925,17 @@ ExtensionFunction::ResponseAction UsbReleaseInterfaceFunction::Run() {
     return RespondNow(Error(kErrorNoConnection));
   }
 
-  if (device_handle->ReleaseInterface(parameters->interface_number)) {
-    return RespondNow(NoArguments());
-  } else {
-    return RespondNow(Error(kErrorCannotReleaseInterface));
-  }
+  device_handle->ReleaseInterface(
+      parameters->interface_number,
+      base::Bind(&UsbReleaseInterfaceFunction::OnComplete, this));
+  return RespondLater();
+}
+
+void UsbReleaseInterfaceFunction::OnComplete(bool success) {
+  if (success)
+    Respond(NoArguments());
+  else
+    Respond(Error(kErrorCannotReleaseInterface));
 }
 
 UsbSetInterfaceAlternateSettingFunction::
diff --git a/extensions/browser/api/usb/usb_api.h b/extensions/browser/api/usb/usb_api.h
index 3d1dec1..397b1ad 100644
--- a/extensions/browser/api/usb/usb_api.h
+++ b/extensions/browser/api/usb/usb_api.h
@@ -268,6 +268,8 @@ class UsbReleaseInterfaceFunction : public UsbConnectionFunction {
   // ExtensionFunction:
   ResponseAction Run() override;
 
+  void OnComplete(bool success);
+
   DISALLOW_COPY_AND_ASSIGN(UsbReleaseInterfaceFunction);
 };