Create ScopedLibusbDevice to simplify reference counting

This change wraps strong references to libusb_device objects in a
ScopedLibusbDevice class (specializing ScopedGeneric) in order to better
document when these references are expected to be dropped.

Bug: 819356
Change-Id: I9671d340f0d5353637405ddd72846456e23a5f1f
Reviewed-on: https://chromium-review.googlesource.com/972623Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545620}

device/usb/BUILD.gn

@@ -81,6 +81,7 @@ static_library("usb") {
 
   if (is_win || is_mac) {
     sources += [
+      "scoped_libusb_device_ref.h",
       "usb_context.cc",
       "usb_context.h",
       "usb_device_handle_impl.cc",

device/usb/scoped_libusb_device_ref.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef DEVICE_USB_SCOPED_LIBUSB_DEVICE_REF_H_
+#define DEVICE_USB_SCOPED_LIBUSB_DEVICE_REF_H_
+
+#include "base/scoped_generic.h"
+#include "third_party/libusb/src/libusb/libusb.h"
+
+namespace device {
+
+struct LibusbDeviceRefTraits {
+  static libusb_device* InvalidValue() { return nullptr; }
+
+  static void Free(libusb_device* device) { libusb_unref_device(device); }
+};
+
+using ScopedLibusbDeviceRef =
+    base::ScopedGeneric<libusb_device*, LibusbDeviceRefTraits>;
+
+}  // namespace device
+
+#endif  // DEVICE_USB_SCOPED_LIBUSB_DEVICE_REF_H_

device/usb/usb_device_handle_impl.cc

@@ -29,8 +29,6 @@
 
 namespace device {
 
-typedef libusb_device* PlatformUsbDevice;
-
 void HandleTransferCompletion(PlatformUsbTransferHandle transfer);
 
 namespace {

device/usb/usb_device_impl.cc

@@ -29,7 +29,7 @@
 namespace device {
 
 UsbDeviceImpl::UsbDeviceImpl(scoped_refptr<UsbContext> context,
-                             PlatformUsbDevice platform_device,
+                             ScopedLibusbDeviceRef platform_device,
                              const libusb_device_descriptor& descriptor)
     : UsbDevice(descriptor.bcdUSB,
                 descriptor.bDeviceClass,
@@ -41,17 +41,15 @@ UsbDeviceImpl::UsbDeviceImpl(scoped_refptr<UsbContext> context,
                 base::string16(),
                 base::string16(),
                 base::string16()),
-      platform_device_(platform_device),
-      context_(context) {
-  CHECK(platform_device) << "platform_device cannot be NULL";
-  libusb_ref_device(platform_device);
+      platform_device_(std::move(platform_device)),
+      context_(std::move(context)) {
+  CHECK(platform_device_.is_valid()) << "platform_device must be valid";
   ReadAllConfigurations();
   RefreshActiveConfiguration();
 }
 
 UsbDeviceImpl::~UsbDeviceImpl() {
   // The destructor must be safe to call from any thread.
-  libusb_unref_device(platform_device_);
 }
 
 void UsbDeviceImpl::Open(OpenCallback callback) {
@@ -68,11 +66,11 @@ void UsbDeviceImpl::Open(OpenCallback callback) {
 
 void UsbDeviceImpl::ReadAllConfigurations() {
   libusb_device_descriptor device_descriptor;
-  int rv = libusb_get_device_descriptor(platform_device_, &device_descriptor);
+  int rv = libusb_get_device_descriptor(platform_device(), &device_descriptor);
   if (rv == LIBUSB_SUCCESS) {
     for (uint8_t i = 0; i < device_descriptor.bNumConfigurations; ++i) {
       unsigned char* buffer;
-      rv = libusb_get_raw_config_descriptor(platform_device_, i, &buffer);
+      rv = libusb_get_raw_config_descriptor(platform_device(), i, &buffer);
       if (rv < 0) {
         USB_LOG(EVENT) << "Failed to get config descriptor: "
                        << ConvertPlatformUsbErrorToString(rv);
@@ -91,7 +89,7 @@ void UsbDeviceImpl::ReadAllConfigurations() {
 
 void UsbDeviceImpl::RefreshActiveConfiguration() {
   uint8_t config_value;
-  int rv = libusb_get_active_config_value(platform_device_, &config_value);
+  int rv = libusb_get_active_config_value(platform_device(), &config_value);
   if (rv != LIBUSB_SUCCESS) {
     USB_LOG(EVENT) << "Failed to get active configuration: "
                    << ConvertPlatformUsbErrorToString(rv);
@@ -107,7 +105,7 @@ void UsbDeviceImpl::OpenOnBlockingThread(
     scoped_refptr<base::SequencedTaskRunner> blocking_task_runner) {
   base::AssertBlockingAllowed();
   PlatformUsbDeviceHandle handle;
-  const int rv = libusb_open(platform_device_, &handle);
+  const int rv = libusb_open(platform_device(), &handle);
   if (LIBUSB_SUCCESS == rv) {
     task_runner->PostTask(
         FROM_HERE, base::BindOnce(&UsbDeviceImpl::Opened, this, handle,

device/usb/usb_device_impl.h

@@ -17,13 +17,13 @@
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
 #include "build/build_config.h"
+#include "device/usb/scoped_libusb_device_ref.h"
 #include "device/usb/usb_descriptors.h"
 #include "device/usb/usb_device.h"
 
 struct libusb_device;
 struct libusb_device_descriptor;
 struct libusb_device_handle;
-struct libusb_config_descriptor;
 
 namespace base {
 class SequencedTaskRunner;
@@ -34,12 +34,14 @@ namespace device {
 class UsbDeviceHandleImpl;
 class UsbContext;
 
-typedef struct libusb_device* PlatformUsbDevice;
-typedef struct libusb_config_descriptor* PlatformUsbConfigDescriptor;
 typedef struct libusb_device_handle* PlatformUsbDeviceHandle;
 
 class UsbDeviceImpl : public UsbDevice {
  public:
+  UsbDeviceImpl(scoped_refptr<UsbContext> context,
+                ScopedLibusbDeviceRef platform_device,
+                const libusb_device_descriptor& descriptor);
+
   // UsbDevice implementation:
   void Open(OpenCallback callback) override;
 
@@ -56,17 +58,12 @@ class UsbDeviceImpl : public UsbDevice {
   }
   void set_webusb_landing_page(const GURL& url) { webusb_landing_page_ = url; }
 
-  PlatformUsbDevice platform_device() const { return platform_device_; }
+  libusb_device* platform_device() const { return platform_device_.get(); }
 
  protected:
   friend class UsbServiceImpl;
   friend class UsbDeviceHandleImpl;
 
-  // Called by UsbServiceImpl only;
-  UsbDeviceImpl(scoped_refptr<UsbContext> context,
-                PlatformUsbDevice platform_device,
-                const libusb_device_descriptor& descriptor);
-
   ~UsbDeviceImpl() override;
 
   void ReadAllConfigurations();
@@ -87,11 +84,11 @@ class UsbDeviceImpl : public UsbDevice {
               scoped_refptr<base::SequencedTaskRunner> blocking_task_runner);
 
   base::ThreadChecker thread_checker_;
-  PlatformUsbDevice platform_device_;
+  const ScopedLibusbDeviceRef platform_device_;
   bool visited_ = false;
 
   // Retain the context so that it will not be released before UsbDevice.
-  scoped_refptr<UsbContext> context_;
+  const scoped_refptr<UsbContext> context_;
 
   DISALLOW_COPY_AND_ASSIGN(UsbDeviceImpl);
 };

device/usb/usb_service_impl.h

@@ -11,11 +11,14 @@
 
 #include <map>
 #include <set>
+#include <vector>
 
 #include "base/containers/queue.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "build/build_config.h"
+#include "device/usb/scoped_libusb_device_ref.h"
 #include "device/usb/usb_context.h"
 #include "device/usb/usb_device_impl.h"
 #include "third_party/libusb/src/libusb/libusb.h"
@@ -25,12 +28,10 @@
 #include "device/base/device_monitor_win.h"
 #endif  // OS_WIN
 
-struct libusb_device;
 struct libusb_context;
 
 namespace device {
 
-typedef struct libusb_device* PlatformUsbDevice;
 typedef struct libusb_context* PlatformUsbContext;
 
 class UsbDeviceImpl;
@@ -60,11 +61,12 @@ class UsbServiceImpl :
 
   // Enumerate USB devices from OS and update devices_ map.
   void RefreshDevices();
-  void OnDeviceList(libusb_device** platform_devices, size_t device_count);
+  void OnDeviceList(
+      base::Optional<std::vector<ScopedLibusbDeviceRef>> platform_devices);
   void RefreshDevicesComplete();
 
   // Creates a new UsbDevice based on the given libusb device.
-  void EnumerateDevice(PlatformUsbDevice platform_device,
+  void EnumerateDevice(ScopedLibusbDeviceRef platform_device,
                        const base::Closure& refresh_complete);
 
   void AddDevice(const base::Closure& refresh_complete,
@@ -73,16 +75,16 @@ class UsbServiceImpl :
 
   // Handle hotplug events from libusb.
   static int LIBUSB_CALL HotplugCallback(libusb_context* context,
-                                         PlatformUsbDevice device,
+                                         libusb_device* device,
                                          libusb_hotplug_event event,
                                          void* user_data);
   // These functions release a reference to the provided platform device.
-  void OnPlatformDeviceAdded(PlatformUsbDevice platform_device);
-  void OnPlatformDeviceRemoved(PlatformUsbDevice platform_device);
+  void OnPlatformDeviceAdded(ScopedLibusbDeviceRef platform_device);
+  void OnPlatformDeviceRemoved(ScopedLibusbDeviceRef platform_device);
 
   // Add |platform_device| to the |ignored_devices_| and
   // run |refresh_complete|.
-  void EnumerationFailed(PlatformUsbDevice platform_device,
+  void EnumerationFailed(ScopedLibusbDeviceRef platform_device,
                          const base::Closure& refresh_complete);
 
   scoped_refptr<UsbContext> context_;
@@ -100,18 +102,20 @@ class UsbServiceImpl :
   base::queue<std::string> pending_path_enumerations_;
   std::vector<GetDevicesCallback> pending_enumeration_callbacks_;
 
-  // The map from PlatformUsbDevices to UsbDevices.
-  typedef std::map<PlatformUsbDevice, scoped_refptr<UsbDeviceImpl>>
+  // The map from libusb_device to UsbDeviceImpl. The key is a weak pointer to
+  // the libusb_device object owned by the UsbDeviceImpl.
+  typedef std::map<libusb_device*, scoped_refptr<UsbDeviceImpl>>
       PlatformDeviceMap;
   PlatformDeviceMap platform_devices_;
 
   // The set of devices that only need to be enumerated once and then can be
   // ignored (for example, hub devices, devices that failed enumeration, etc.).
-  std::set<PlatformUsbDevice> ignored_devices_;
+  std::vector<ScopedLibusbDeviceRef> ignored_devices_;
 
-  // Tracks PlatformUsbDevices that might be removed while they are being
-  // enumerated.
-  std::set<PlatformUsbDevice> devices_being_enumerated_;
+  // Tracks libusb_devices that might be removed while they are being
+  // enumerated. This is a weak pointer to a libusb_device object owned by a
+  // UsbDeviceImpl.
+  std::set<libusb_device*> devices_being_enumerated_;
 
 #if defined(OS_WIN)
   ScopedObserver<DeviceMonitorWin, DeviceMonitorWin::Observer> device_observer_;