Usb backend refactor step 2: Separate Usb Device Handle and 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 e44c8aaae9dfa932383b12cbf173ba387e8b79bb..a0ddc3b737b8e4451fa80de7a2a64ac2347b6f09 100644
--- a/chrome/browser/usb/usb_service.cc
+++ b/chrome/browser/usb/usb_service.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/usb/usb_service.h"
 
+#include <set>
 #include <vector>
 
 #include "base/bind.h"
@@ -11,6 +12,8 @@
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "chrome/browser/usb/usb_device.h"
+#include "content/public/browser/browser_thread.h"
+#include "third_party/libusb/src/libusb/interrupt.h"
 #include "third_party/libusb/src/libusb/libusb.h"
 
 #if defined(OS_CHROMEOS)
@@ -19,16 +22,21 @@
 #include "chromeos/dbus/permission_broker_client.h"
 #endif  // defined(OS_CHROMEOS)
 
+using std::set;
 using std::vector;
+using content::BrowserThread;
 
-// 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 manages the polling thread and assures the thread exits safely.
+// This class is 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);
+      : running_(true), context_(context), thread_handle_(0) {
+    base::PlatformThread::Create(0, this, &thread_handle_);
   }
 
   virtual ~UsbEventHandler() {}
@@ -36,45 +44,146 @@ class UsbEventHandler : public base::PlatformThread::Delegate {
   virtual void ThreadMain() OVERRIDE {
     base::PlatformThread::SetName("UsbEventHandler");
 
-    DLOG(INFO) << "UsbEventHandler started.";
-    while (running_) {
+    VLOG(1) << "UsbEventHandler started.";
+
+    while (running_)
       libusb_handle_events(context_);
-    }
-    DLOG(INFO) << "UsbEventHandler shutting down.";
-    libusb_exit(context_);
 
-    delete this;
+    VLOG(1) << "UsbEventHandler shutting down.";
   }
 
   void Stop() {
     running_ = false;
+    base::subtle::MemoryBarrier();
+    libusb_interrupt_handle_event(context_);
+    base::PlatformThread::Join(thread_handle_);
   }
 
  private:
-  bool running_;
+  volatile bool running_;
   PlatformUsbContext context_;
+  base::PlatformThreadHandle thread_handle_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(UsbEventHandler);
+  DISALLOW_COPY_AND_ASSIGN(UsbEventHandler);
 };
 
-UsbService::UsbService() {
-  libusb_init(&context_);
-  event_handler_ = new UsbEventHandler(context_);
-}
+// Ref-counted wrapper for PlatformUsbContext.
+// It also manages the life-cycle of UsbEventHandler
+class UsbContext : public base::RefCountedThreadSafe<UsbContext> {

[pfeldman, 2013/07/22 08:39:45]

I think this tiny bit is worth landing separately.

[Bei Zhang, 2013/07/23 17:58:44]

Done.

+ public:
+  UsbContext() : context_(NULL) {
+    CHECK_EQ(0, libusb_init(&context_)) << "Cannot initialize libusb";
+    event_handler_.reset(new UsbEventHandler(context_));
+  }
+  PlatformUsbContext context() const { return context_; }
+
+ private:
+  friend class base::RefCountedThreadSafe<UsbContext>;
+
+  virtual ~UsbContext() {
+    event_handler_->Stop();
+    event_handler_.reset(NULL);
+    libusb_exit(context_);
+  }
+  PlatformUsbContext context_;
+  scoped_ptr<UsbEventHandler> event_handler_;
+};
+
+class UsbDeviceStub : public base::NonThreadSafe {
+ public:
+  explicit UsbDeviceStub(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_);
+  }
+
+  virtual ~UsbDeviceStub() {
+    DCHECK(CalledOnValidThread());
+    libusb_unref_device(device_);
+
+    // Device is lost.
+    // Invalidates all the opened handle.
+    for (vector<scoped_refptr<UsbDevice> >::iterator it = handles_.begin();
+         it != handles_.end(); ++it) {
+      it->get()->InternalClose();
+    }
+    STLClearObject(&handles_);
+  }
+
+  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<UsbDevice> OpenDevice(UsbService* service) {
+    DCHECK(CalledOnValidThread());
+    PlatformUsbDeviceHandle handle;
+
+    if (0 == libusb_open(device_, &handle)) {
+      scoped_refptr<UsbDevice> wrapper =
+          make_scoped_refptr(new UsbDevice(
+              service,
+              unique_id_,
+              handle));
+      handles_.push_back(wrapper);
+      return wrapper;
+    }
+    return scoped_refptr<UsbDevice>();

[pfeldman, 2013/07/22 18:23:13]

You could return NULL.

+  }
+
+  void CloseDeviceHandle(UsbDevice* device) {
+    DCHECK(CalledOnValidThread());
+    device->InternalClose();
+    for (vector<scoped_refptr<UsbDevice> >::iterator it = handles_.begin();
+        it != handles_.end(); ++it) {
+      if (it->get() == device) {
+        handles_.erase(it);
+        return;
+      }
+    }
+  }
+
+ private:
+  // Retain the context so it will not be release before the destruction
+  // of the UsbDevice object.
+  scoped_refptr<UsbContext> context_;
+  vector<scoped_refptr<UsbDevice> > handles_;
+  const PlatformUsbDevice device_;
+  const uint16 unique_id_;
+  const uint16 vendor_id_;
+  const int product_id_;
+  DISALLOW_COPY_AND_ASSIGN(UsbDeviceStub);
+};
+
+UsbService::UsbService()
+    : context_(new UsbContext()),
+      next_unique_id_(1),
+      device_enumeration_scheduled_(false) {}
 
 UsbService::~UsbService() {}
 
-void UsbService::Cleanup() {
-  event_handler_->Stop();
-  event_handler_ = NULL;
+void UsbService::Shutdown() {
+  context_ = NULL;
+  for (DeviceMapById::iterator it = devices_by_id_.begin();
+      it != devices_by_id_.end(); ++it) {
+    BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, it->second);
+  }
+  devices_.clear();
+  devices_by_id_.clear();
 }
 
 void UsbService::FindDevices(const uint16 vendor_id,
                              const uint16 product_id,
                              int interface_id,
-                             vector<scoped_refptr<UsbDevice> >* devices,
+                             vector<int>* devices,
                              const base::Callback<void()>& callback) {
-  DCHECK(event_handler_) << "FindDevices called after event handler stopped.";
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 #if defined(OS_CHROMEOS)
   // ChromeOS builds on non-ChromeOS machines (dev) should not attempt to
   // use permission broker.
@@ -104,29 +213,40 @@ void UsbService::FindDevices(const uint16 vendor_id,
 #endif  // defined(OS_CHROMEOS)
 }
 
-void UsbService::EnumerateDevices(
-    std::vector<scoped_refptr<UsbDevice> >* devices) {
-  devices->clear();
+scoped_refptr<UsbDevice> UsbService::OpenDevice(int device) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  RefreshDevices();
+  if (ContainsKey(devices_by_id_, device)) {
+    return devices_by_id_[device]->OpenDevice(this);
+  }
+  return NULL;
+}
 
-  DeviceVector enumerated_devices;
-  EnumerateDevicesImpl(&enumerated_devices);
+void UsbService::CloseDevice(scoped_refptr<UsbDevice> device) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (ContainsKey(devices_by_id_, device->device())) {
+    return devices_by_id_[device->device()]->CloseDeviceHandle(device);
+  }
+}
 
-  for (DeviceVector::iterator it = enumerated_devices.begin();
-       it != enumerated_devices.end(); ++it) {
-    PlatformUsbDevice device = it->device();
-    UsbDevice* const wrapper = LookupOrCreateDevice(device);
-    if (wrapper)
-      devices->push_back(wrapper);
+void UsbService::GetDevices(std::vector<int>* devices) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  devices->clear();
+  RefreshDevices();
+  for (DeviceMapById::iterator it = devices_by_id_.begin();
+      it != devices_by_id_.end();
+      ++it) {
+    devices->push_back(it->first);
   }
 }
 
 void UsbService::FindDevicesImpl(const uint16 vendor_id,
                                  const uint16 product_id,
-                                 vector<scoped_refptr<UsbDevice> >* devices,
+                                 vector<int>* devices,
                                  const base::Callback<void()>& callback,
                                  bool success) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   base::ScopedClosureRunner run_callback(callback);
-
   devices->clear();
 
   // If the permission broker was unable to obtain permission for the specified
@@ -135,87 +255,76 @@ void UsbService::FindDevicesImpl(const uint16 vendor_id,
   if (!success)
     return;
 
-  DeviceVector enumerated_devices;
-  EnumerateDevicesImpl(&enumerated_devices);
+  RefreshDevices();
 
-  for (DeviceVector::iterator it = enumerated_devices.begin();
-       it != enumerated_devices.end(); ++it) {
-    PlatformUsbDevice device = it->device();
-    if (DeviceMatches(device, vendor_id, product_id)) {
-      UsbDevice* const wrapper = LookupOrCreateDevice(device);
-      if (wrapper)
-        devices->push_back(wrapper);
-    }
+  for (DeviceMapById::iterator it = devices_by_id_.begin();
+      it != devices_by_id_.end();
+      ++it) {
+    if (DeviceMatches(it->second, vendor_id, product_id))
+      devices->push_back(it->first);
   }
 }
 
-void UsbService::CloseDevice(scoped_refptr<UsbDevice> device) {
-  DCHECK(event_handler_) << "CloseDevice called after event handler stopped.";
-
-  PlatformUsbDevice platform_device = libusb_get_device(device->handle());
-  if (!ContainsKey(devices_, platform_device)) {
-    LOG(WARNING) << "CloseDevice called for device we're not tracking!";
+void UsbService::ScheduleRefreshDevices() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  if (device_enumeration_scheduled_)
     return;
-  }
-
-  devices_.erase(platform_device);
-  libusb_close(device->handle());
-}
-
-UsbService::RefCountedPlatformUsbDevice::RefCountedPlatformUsbDevice(
-    PlatformUsbDevice device) : device_(device) {
-  libusb_ref_device(device_);
-}
-
-UsbService::RefCountedPlatformUsbDevice::RefCountedPlatformUsbDevice(
-    const RefCountedPlatformUsbDevice& other) : device_(other.device_) {
-  libusb_ref_device(device_);
+  device_enumeration_scheduled_ = true;
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&UsbService::RefreshDevices, base::Unretained(this)));
 }
 
-UsbService::RefCountedPlatformUsbDevice::~RefCountedPlatformUsbDevice() {
-  libusb_unref_device(device_);
-}
+void UsbService::RefreshDevices() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  libusb_device** devices = NULL;
+  const ssize_t device_count =
+      libusb_get_device_list(context_->context(), &devices);
+
+  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]] = new UsbDeviceStub(
+          context_.get(),
+          devices[i],
+          next_unique_id_,
+          descriptor.idVendor,
+          descriptor.idProduct);
+      devices_by_id_[next_unique_id_] = devices_[devices[i]];
+      ++next_unique_id_;
+    }
+    connected_devices.insert(devices_[devices[i]]->unique_id());
+  }
 
-PlatformUsbDevice UsbService::RefCountedPlatformUsbDevice::device() {
-  return device_;
-}
+  // 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);
+    }
+  }
 
-void UsbService::EnumerateDevicesImpl(DeviceVector* output) {
-  STLClearObject(output);
+  // Remove disconnected devices from devices_.
+  for (size_t i = 0; i < disconnected_devices.size(); ++i) {
+    UsbDeviceStub* device = devices_[disconnected_devices[i]];
+    devices_.erase(disconnected_devices[i]);
+    devices_by_id_.erase(device->unique_id());
 
-  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));
+    // This should delete those devices and invalidate their handles.
+    // It might take long.
+    delete device;
   }
 
   libusb_free_device_list(devices, true);
 }
 
-bool UsbService::DeviceMatches(PlatformUsbDevice device,
+bool UsbService::DeviceMatches(const UsbDeviceStub* 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;
-    }
-
-    UsbDevice* wrapper = new UsbDevice(this, handle);
-    devices_[device] = wrapper;
-  }
-  return devices_[device].get();
+  return device->vendor_id() == vendor_id && device->product_id() == product_id;
 }