Separate usb device handle from usb device. (deprecate)

chrome/browser/usb/usb_service.cc

Index: chrome/browser/usb/usb_service.cc
diff --git a/chrome/browser/usb/usb_service.cc b/chrome/browser/usb/usb_service.cc
index 038786debf85048856500ce8f184578faade605f..4e07541d4eef1bb5915cb2ab231c22d60e843081 100644
--- a/chrome/browser/usb/usb_service.cc
+++ b/chrome/browser/usb/usb_service.cc
@@ -4,13 +4,16 @@
 
 #include "chrome/browser/usb/usb_service.h"
 
+#include <set>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
-#include "chrome/browser/usb/usb_device.h"
+
+#include "chrome/browser/usb/usb_device_handle.h"
+#include "content/public/browser/browser_thread.h"
 #include "third_party/libusb/src/libusb/libusb.h"
 
 #if defined(OS_CHROMEOS)
@@ -20,61 +23,192 @@
 #endif  // defined(OS_CHROMEOS)
 
 using std::vector;
+using std::set;
+using content::BrowserThread;
+using base::PlatformThreadHandle;
 
-// The UsbEventHandler works around a design flaw in the libusb interface. There
-// is currently no way to signal to libusb that any caller into one of the event
+// The UsbEventHandler dispatches USB events on separate thread. There is
+// currently no way to signal to libusb that any caller into one of the event
 // handler calls should return without handling any events.
+//
+// This class in only visible to UsbContext. UsbContext manages its life cycle.
 class UsbEventHandler : public base::PlatformThread::Delegate {
- public:
-  explicit UsbEventHandler(PlatformUsbContext context)
-      : running_(true), context_(context) {
-    base::PlatformThread::CreateNonJoinable(0, this);
-  }
+ private:
+  friend class UsbContext;
+  friend struct base::DefaultDeleter<UsbEventHandler>;
+  explicit UsbEventHandler(PlatformUsbContext context);
+  virtual ~UsbEventHandler();
 
-  virtual ~UsbEventHandler() {}
+  void Stop() {
+    running_ = false;
+  }
 
   virtual void ThreadMain() OVERRIDE {
-    base::PlatformThread::SetName("UsbEventHandler");
-
-    DLOG(INFO) << "UsbEventHandler started.";
+    base::PlatformThread::SetName("UsbEventDispatcher");
+    VLOG(1) << "UsbEventDispatcher started.";
+    running_ = true;
     while (running_) {
       libusb_handle_events(context_);
     }
-    DLOG(INFO) << "UsbEventHandler shutting down.";
-    libusb_exit(context_);
-
-    delete this;
+    VLOG(1) << "UsbEventDispatcher shutting down.";
   }
 
+  volatile bool running_;

[akalin, 2013/06/13 21:53:12]

volatile isn't sufficient -- you need a memory barrier if you're using this
across threads (i.e., protect it with a lock)

[Bei Zhang, 2013/06/13 22:36:22]

I thought about this very carefully and yes you're right, this must be protected
with a lock.

At first I thought this was OK because libusb_handle_events and libusb_exit are
synced with an internal mutex. But after reading their code I found out that
they are not.

On 2013/06/13 21:53:12, akalin wrote:

[Bei Zhang, 2013/06/14 05:58:24]

Done.

+  PlatformUsbContext context_;
+  DISALLOW_COPY_AND_ASSIGN(UsbEventHandler);
+};
+
+UsbEventHandler::UsbEventHandler(PlatformUsbContext context)
+    : running_(true), context_(context) {
+  base::PlatformThread::CreateNonJoinable(0, this);
+}
+
+UsbEventHandler::~UsbEventHandler() {}
+
+// Ref-counted wrapper for PlatformUsbContext.
+class UsbContext : public base::RefCountedThreadSafe<UsbContext> {
+ public:
+  UsbContext();
+  PlatformUsbContext context() const { return context_; }
+
   void Stop() {
-    running_ = false;
+    event_handler_->Stop();
   }
 
  private:
-  bool running_;
+  friend class base::RefCountedThreadSafe<UsbContext>;
+  virtual ~UsbContext();
   PlatformUsbContext context_;
-
-  DISALLOW_EVIL_CONSTRUCTORS(UsbEventHandler);
+  scoped_ptr<UsbEventHandler> event_handler_;
 };
 
-UsbService::UsbService() {
+UsbContext::UsbContext()
+    : context_(NULL) {
   libusb_init(&context_);
-  event_handler_ = new UsbEventHandler(context_);
+  event_handler_.reset(new UsbEventHandler(context_));
+}
+
+UsbContext::~UsbContext() {
+  Stop();
+  // The following statement will inform the event handler to stop waiting.
+  libusb_exit(context_);
 }
 
-UsbService::~UsbService() {}
+// UsbDevice class uniquely represents a USB devices recognized by libusb and
+// maintains all its opened handles. It is assigned with an unique id by
+// UsbService. Once the device is disconnected it will invalidate all the
+// UsbDeviceHandle objects attached to it. The class is only visible to
+// UsbService and other classes need to access the device using its unique id.
+//
+// This class can be only used on FILE thread. However, as UsbService calls
+// ReleaseSoon on this class, it must be RefCountedThreadSafe.
+class UsbDevice : public base::RefCountedThreadSafe<UsbDevice>,

[akalin, 2013/06/13 21:53:12]

this also doesn't make sense. Can you use DeleteSoon instead?

[Bei Zhang, 2013/06/13 22:36:22]

Both ReleaseSoon and DeleteSoon requires RefCountedThreadSafe<T>.

Is it that weird? I can find several examples in the code base but indeed not
many of them.

Maybe I should use base::ThreadChecker instead?

On 2013/06/13 21:53:12, akalin wrote:

+                  public base::NonThreadSafe {
+ public:
+  explicit UsbDevice(UsbContext* context, PlatformUsbDevice device,
+                     const int unique_id, const uint16 vendor_id,
+                     const uint16 product_id);
+  PlatformUsbDevice device() const { return device_; }
+  int unique_id() const { return unique_id_; }
+  int vendor_id() const { return vendor_id_; }
+  int product_id() const { return product_id_; }
+
+  scoped_refptr<UsbDeviceHandle> OpenDevice(UsbService* service);
+  void CloseDeviceHandle(UsbDeviceHandle* device);
 
-void UsbService::Cleanup() {
-  event_handler_->Stop();
-  event_handler_ = NULL;
+ private:
+  virtual ~UsbDevice();
+  friend class base::RefCountedThreadSafe<UsbDevice>;
+  // Retain the context so it will not be release before the destruction
+  // of the UsbDevice object.
+  scoped_refptr<UsbContext> context_;
+  vector<scoped_refptr<UsbDeviceHandle> > handles_;
+  const PlatformUsbDevice device_;
+  const uint16 unique_id_;
+  const uint16 vendor_id_;
+  const int product_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(UsbDevice);
+};
+
+UsbDevice::UsbDevice(UsbContext* context, PlatformUsbDevice device,
+                     const int unique_id, const uint16 vendor_id,
+                     const uint16 product_id)
+    : context_(context),
+      device_(device),
+      unique_id_(unique_id),
+      vendor_id_(vendor_id),
+      product_id_(product_id) {
+  DCHECK(CalledOnValidThread());
+  libusb_ref_device(device_);
+}
+
+UsbDevice::~UsbDevice() {
+  DCHECK(CalledOnValidThread());
+  libusb_unref_device(device_);
+
+  // Device is lost.
+  // Invalidates all the opened handle.
+  for (vector<scoped_refptr<UsbDeviceHandle> >::iterator it = handles_.begin();
+       it != handles_.end(); ++it) {
+    it->get()->InternalClose();
+  }
+  STLClearObject(&handles_);
+}
+
+scoped_refptr<UsbDeviceHandle> UsbDevice::OpenDevice(UsbService* service) {
+  DCHECK(CalledOnValidThread());
+  PlatformUsbDeviceHandle handle;
+  if (0 == libusb_open(device_, &handle)) {
+    scoped_refptr<UsbDeviceHandle> wrapper =
+        make_scoped_refptr(new UsbDeviceHandle(service, unique_id_, vendor_id_,
+                                               product_id_, handle));
+    handles_.push_back(wrapper);
+    return wrapper;
+  }
+  return scoped_refptr<UsbDeviceHandle>();
 }
 
-void UsbService::FindDevices(const uint16 vendor_id,
-                             const uint16 product_id,
-                             int interface_id,
-                             vector<scoped_refptr<UsbDevice> >* devices,
+void UsbDevice::CloseDeviceHandle(UsbDeviceHandle* device) {
+  DCHECK(CalledOnValidThread());
+  device->InternalClose();
+  for (vector<scoped_refptr<UsbDeviceHandle> >::iterator it = handles_.begin();
+       it != handles_.end(); ++it) {
+    if (it->get() == device) {
+      handles_.erase(it);
+      return;
+    }
+  }
+}
+
+UsbService::UsbService()
+    : context_(new UsbContext()),
+      next_unique_id_(1) {
+  // This class will be consequently called on FILE thread.
+  DetachFromThread();
+}
+
+UsbService::~UsbService() {
+  // The destructor will be called on UI thread.
+  DetachFromThread();
+}
+
+void UsbService::Shutdown() {
+  context_->Stop();
+  for (DeviceMap::iterator it = devices_.begin(); it != devices_.end(); ++it) {
+    scoped_refptr<UsbDevice> to_release;
+    to_release.swap(it->second);

[akalin, 2013/06/14 00:43:24]

What's going on here? You shouldn't have to do this.

In particular, calling AddRef() explicitly causes a leak; ReleaseSoon() takes a
ref if you pass it a pointer to a ref-counted class.

I think you should really find a way to remove ref-counting. :/

[Bei Zhang, 2013/06/14 05:06:02]

1. I carefully read the code of BrowserThread::ReleaseSoon and I'm positive that
BrowserThread::ReleaseSoon() will not manually increase the ref counter.
Specifically, it converts the pointer to void* when calling base::Bind:

https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...

Then it simply Releases it on the thread: 

https://code.google.com/p/chromium/codesearch#chromium/src/base/sequenced_tas...

As I tested, if I remove the AddRef, it will be deleted immediately and crashes
soon.

2. I swapped the scoped_refptr in case ReleaseSoon fires before I clear the
devices_ which will end up deleting the object on the current thread -- well, I
did this with a bug. I shall first exit the scope of to_release then use the
pointer to register the ReleaseSoon. But I believe 

I agree that is part of code is horrible :(. I will change the class to use
DeleteOnFileThread deletor instead, which will save all these code.

Your advice about removing the ref-counting is great and I tried very hard to do
it at the beginning. The problem is that I'm trying to hold a pointer to the
object and use it as the value of a map, which requires a operator= of the
object. I really need a map because otherwise an recurring enumeration will
become O(n^2) time. I can control the life-cycle using plain pointers but it'll
be more problematic AFAIK.

[akalin, 2013/06/14 06:24:38]

Ah, you're right in that ReleaseSoon doesn't take a ref. However, the code is
still problematic. There is still a race between the two threads as to which
thread will release the last ref. You encountered this problem when you didn't
do the swap, but it still exists even with the swap. It is possible that as soon
as you call ReleaseSoon, execution switches to the FILE thread, releases the
object, then switches back to this thread (the UI thread?) before to_release is
destroyed.

If you really have to stick with ref-counting, what you want is:

BrowserThread::ReleaseSoon(...::FILE, FROM_HERE, it->second->release());

Manual memory management is problematic, but trying to solve it with
ref-counting only introduces more problems, especially with multiple threads. I
think you're better off just storing a map with raw pointers, writing a utility
function to iterate through the map and call DeleteSoon on all objects, clearing
the map, and call that utility function from the destructor as well as any other
place you need it.

+    to_release->AddRef();
+    BrowserThread::ReleaseSoon(BrowserThread::FILE, FROM_HERE,
+                               to_release.get());
+  }
+  devices_.clear();
+}
+
+void UsbService::FindDevices(const uint16 vendor_id, const uint16 product_id,
+                             const int interface_id, vector<int>* devices,
                              const base::Callback<void()>& callback) {
-  DCHECK(event_handler_) << "FindDevices called after event handler stopped.";
+  DCHECK(CalledOnValidThread());
 #if defined(OS_CHROMEOS)
   // ChromeOS builds on non-ChromeOS machines (dev) should not attempt to
   // use permission broker.
@@ -94,6 +228,7 @@ void UsbService::FindDevices(const uint16 vendor_id,
                                         base::Unretained(this),
                                         vendor_id,
                                         product_id,
+                                        interface_id,
                                         devices,
                                         callback));
   } else {
@@ -105,10 +240,10 @@ void UsbService::FindDevices(const uint16 vendor_id,
 }
 
 void UsbService::FindDevicesImpl(const uint16 vendor_id,
-                                 const uint16 product_id,
-                                 vector<scoped_refptr<UsbDevice> >* devices,
+                                 const uint16 product_id, vector<int>* devices,
                                  const base::Callback<void()>& callback,
                                  bool success) {
+  DCHECK(CalledOnValidThread());
   base::ScopedClosureRunner run_callback(callback);
 
   devices->clear();
@@ -116,91 +251,87 @@ void UsbService::FindDevicesImpl(const uint16 vendor_id,
   // If the permission broker was unable to obtain permission for the specified
   // devices then there is no point in attempting to enumerate the devices. On
   // platforms without a permission broker, we assume permission is granted.
-  if (!success)
-    return;
-
-  DeviceVector enumerated_devices;
-  EnumerateDevices(&enumerated_devices);
-  if (enumerated_devices.empty())
-    return;
-
-  for (unsigned int i = 0; i < enumerated_devices.size(); ++i) {
-    PlatformUsbDevice device = enumerated_devices[i].device();
-    if (DeviceMatches(device, vendor_id, product_id)) {
-      UsbDevice* const wrapper = LookupOrCreateDevice(device);
-      if (wrapper)
-        devices->push_back(wrapper);
-    }
-  }
-}
+  if (!success) return;
 
-void UsbService::CloseDevice(scoped_refptr<UsbDevice> device) {
-  DCHECK(event_handler_) << "CloseDevice called after event handler stopped.";
+  EnumerateDevices();
 
-  PlatformUsbDevice platform_device = libusb_get_device(device->handle());
-  if (!ContainsKey(devices_, platform_device)) {
-    LOG(WARNING) << "CloseDevice called for device we're not tracking!";
-    return;
+  for (DeviceMap::iterator it = devices_.begin(); it != devices_.end(); ++it) {
+    if (DeviceMatches(it->second, vendor_id, product_id)) {
+      devices->push_back(it->second->unique_id());
+    }
   }
-
-  devices_.erase(platform_device);
-  libusb_close(device->handle());
 }
 
-UsbService::RefCountedPlatformUsbDevice::RefCountedPlatformUsbDevice(
-    PlatformUsbDevice device) : device_(device) {
-  libusb_ref_device(device_);
+scoped_refptr<UsbDeviceHandle> UsbService::OpenDevice(int device) {
+  DCHECK(CalledOnValidThread());
+  EnumerateDevices();
+  for (DeviceMap::iterator it = devices_.begin(); it != devices_.end(); ++it) {
+    if (it->second->unique_id() == device) {
+      return it->second->OpenDevice(this);
+    }
+  }
+  return NULL;
 }
 
-UsbService::RefCountedPlatformUsbDevice::RefCountedPlatformUsbDevice(
-    const RefCountedPlatformUsbDevice& other) : device_(other.device_) {
-  libusb_ref_device(device_);
-}
+void UsbService::CloseDeviceHandle(scoped_refptr<UsbDeviceHandle> device) {
+  DCHECK(CalledOnValidThread());
+  int id = device->device();
 
-UsbService::RefCountedPlatformUsbDevice::~RefCountedPlatformUsbDevice() {
-  libusb_unref_device(device_);
+  for (DeviceMap::iterator it = devices_.begin(); it != devices_.end(); ++it) {
+    if (it->second->unique_id() == id) {
+      it->second->CloseDeviceHandle(device);
+      break;
+    }
+  }
 }
 
-PlatformUsbDevice UsbService::RefCountedPlatformUsbDevice::device() {
-  return device_;
+void UsbService::ScheduleEnumerateDevice() {
+  // TODO(ikarienator): Throttle it.
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&UsbService::EnumerateDevices, base::Unretained(this)));
 }
 
-void UsbService::EnumerateDevices(DeviceVector* output) {
-  STLClearObject(output);
-
+void UsbService::EnumerateDevices() {
+  DCHECK(CalledOnValidThread());
   libusb_device** devices = NULL;
-  const ssize_t device_count = libusb_get_device_list(context_, &devices);
-  if (device_count < 0)
-    return;
-
-  for (int i = 0; i < device_count; ++i) {
-    libusb_device* device = devices[i];
-    libusb_ref_device(device);
-    output->push_back(RefCountedPlatformUsbDevice(device));
+  const ssize_t device_count =
+      libusb_get_device_list(context_->context(), &devices);
+  if (device_count < 0) return;
+
+  set<int> connected_devices;
+  vector<PlatformUsbDevice> disconnected_devices;
+
+  // Populates new devices.
+  for (ssize_t i = 0; i < device_count; ++i) {
+    if (!ContainsKey(devices_, devices[i])) {
+      libusb_device_descriptor descriptor;
+      if (0 != libusb_get_device_descriptor(devices[i], &descriptor)) continue;
+      devices_[devices[i]] = make_scoped_refptr(
+          new UsbDevice(context_.get(), devices[i], next_unique_id_,
+                        descriptor.idVendor, descriptor.idProduct));
+      ++next_unique_id_;
+    }
+    connected_devices.insert(devices_[devices[i]]->unique_id());
   }
-
   libusb_free_device_list(devices, true);
-}
 
-bool UsbService::DeviceMatches(PlatformUsbDevice device,
-                               const uint16 vendor_id,
-                               const uint16 product_id) {
-  libusb_device_descriptor descriptor;
-  if (libusb_get_device_descriptor(device, &descriptor))
-    return false;
-  return descriptor.idVendor == vendor_id && descriptor.idProduct == product_id;
-}
-
-UsbDevice* UsbService::LookupOrCreateDevice(PlatformUsbDevice device) {
-  if (!ContainsKey(devices_, device)) {
-    libusb_device_handle* handle = NULL;
-    if (libusb_open(device, &handle)) {
-      LOG(WARNING) << "Could not open device.";
-      return NULL;
+  // Find disconnected devices.
+  for (DeviceMap::iterator it = devices_.begin(); it != devices_.end(); ++it) {
+    if (!ContainsKey(connected_devices, it->second->unique_id())) {
+      disconnected_devices.push_back(it->first);
     }
+  }
 
-    UsbDevice* wrapper = new UsbDevice(this, handle);
-    devices_[device] = wrapper;
+  // Remove disconnected devices from devices_.
+  for (size_t i = 0; i < disconnected_devices.size(); ++i) {
+    // This should delete those devices and invalidate their handles.
+    // It might take long.
+    devices_.erase(disconnected_devices[i]);
   }
-  return devices_[device].get();
+}
+
+bool UsbService::DeviceMatches(const UsbDevice* device, const uint16 vendor_id,
+                               const uint16 product_id) {
+  return device->vendor_id() == vendor_id && device->product_id() == product_id;
 }