Add onDeviceAdded and onDeviceRemoved events to chrome.usb.

This changes creates a UsbAPI browser context-keyed service object that
waits for an app to register for device connection events and then
configures an observer on the UsbService.

BUG=411715

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

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

chrome/browser/extensions/api/device_permissions_manager_unittest.cc

@@ -107,6 +107,15 @@ void AllowUsbDevice(DevicePermissionsManager* manager,
       extension->id(), device, product, manufacturer, serial_number);
 }
 
+scoped_refptr<DevicePermissionEntry> FindEntry(
+    DevicePermissions* device_permissions,
+    scoped_refptr<UsbDevice> device) {
+  base::string16 serial_number;
+  device->GetSerialNumber(&serial_number);
+
+  return device_permissions->FindEntry(device, serial_number);
+}
+
 }  // namespace
 
 class DevicePermissionsManagerTest : public testing::Test {
@@ -158,13 +167,13 @@ TEST_F(DevicePermissionsManagerTest, AllowAndClearDevices) {
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
   scoped_refptr<DevicePermissionEntry> device0_entry =
-      device_permissions->FindEntry(device0);
+      FindEntry(device_permissions.get(), device0);
   ASSERT_TRUE(device0_entry.get());
   scoped_refptr<DevicePermissionEntry> device1_entry =
-      device_permissions->FindEntry(device1);
+      FindEntry(device_permissions.get(), device1);
   ASSERT_TRUE(device1_entry.get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
   ASSERT_EQ(2U, device_permissions->entries().size());
 
   ASSERT_EQ(base::ASCIIToUTF16(
@@ -176,21 +185,21 @@ TEST_F(DevicePermissionsManagerTest, AllowAndClearDevices) {
   manager->Clear(extension_->id());
 
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_FALSE(device_permissions->FindEntry(device0).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
   ASSERT_EQ(0U, device_permissions->entries().size());
 
   // After clearing device it should be possible to grant permission again.
   AllowUsbDevice(manager, extension_, device0);
   AllowUsbDevice(manager, extension_, device1);
 
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 }
 
 TEST_F(DevicePermissionsManagerTest, SuspendExtension) {
@@ -201,23 +210,23 @@ TEST_F(DevicePermissionsManagerTest, SuspendExtension) {
 
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 
   manager->OnBackgroundHostClose(extension_->id());
 
   device_permissions = manager->GetForExtension(extension_->id());
   // Device 0 is still registered because its serial number has been stored in
   // ExtensionPrefs, it is "persistent".
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
   // Device 1 does not have uniquely identifying traits and so permission to
   // open it has been dropped when the app's windows have closed and the
   // background page has been suspended.
-  ASSERT_FALSE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 }
 
 TEST_F(DevicePermissionsManagerTest, DisconnectDevice) {
@@ -228,24 +237,24 @@ TEST_F(DevicePermissionsManagerTest, DisconnectDevice) {
 
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 
   usb_service_->NotifyDeviceRemoved(device0);
   usb_service_->NotifyDeviceRemoved(device1);
 
   device_permissions = manager->GetForExtension(extension_->id());
   // Device 0 will be accessible when it is reconnected because it can be
   // recognized by its serial number.
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
   // Device 1 does not have a serial number and cannot be distinguished from
   // any other device of the same model so the app must request permission again
   // when it is reconnected.
-  ASSERT_FALSE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 }
 
 TEST_F(DevicePermissionsManagerTest, RevokeAndRegrantAccess) {
@@ -257,31 +266,31 @@ TEST_F(DevicePermissionsManagerTest, RevokeAndRegrantAccess) {
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
   scoped_refptr<DevicePermissionEntry> device0_entry =
-      device_permissions->FindEntry(device0);
+      FindEntry(device_permissions.get(), device0);
   ASSERT_TRUE(device0_entry.get());
   scoped_refptr<DevicePermissionEntry> device1_entry =
-      device_permissions->FindEntry(device1);
+      FindEntry(device_permissions.get(), device1);
   ASSERT_TRUE(device1_entry.get());
 
   manager->RemoveEntry(extension_->id(), device0_entry);
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_FALSE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
 
   AllowUsbDevice(manager, extension_, device0);
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
 
   manager->RemoveEntry(extension_->id(), device1_entry);
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device1).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device1).get());
 
   AllowUsbDevice(manager, extension_, device1);
   device_permissions = manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_TRUE(device_permissions->FindEntry(device1).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device1).get());
 }
 
 TEST_F(DevicePermissionsManagerTest, UpdateLastUsed) {
@@ -292,12 +301,12 @@ TEST_F(DevicePermissionsManagerTest, UpdateLastUsed) {
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
   scoped_refptr<DevicePermissionEntry> device0_entry =
-      device_permissions->FindEntry(device0);
+      FindEntry(device_permissions.get(), device0);
   ASSERT_TRUE(device0_entry->last_used().is_null());
 
   manager->UpdateLastUsed(extension_->id(), device0_entry);
   device_permissions = manager->GetForExtension(extension_->id());
-  device0_entry = device_permissions->FindEntry(device0);
+  device0_entry = FindEntry(device_permissions.get(), device0);
   ASSERT_FALSE(device0_entry->last_used().is_null());
 }
 
@@ -318,10 +327,10 @@ TEST_F(DevicePermissionsManagerTest, LoadPrefs) {
       DevicePermissionsManager::Get(env_->profile());
   scoped_ptr<DevicePermissions> device_permissions =
       manager->GetForExtension(extension_->id());
-  ASSERT_TRUE(device_permissions->FindEntry(device0).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device1).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device2).get());
-  ASSERT_FALSE(device_permissions->FindEntry(device3).get());
+  ASSERT_TRUE(FindEntry(device_permissions.get(), device0).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device1).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device2).get());
+  ASSERT_FALSE(FindEntry(device_permissions.get(), device3).get());
 }
 
 }  // namespace extensions

device/usb/usb_device.h

@@ -49,18 +49,18 @@ class UsbDevice : public base::RefCountedThreadSafe<UsbDevice> {
   // Blocking method. Must be called on FILE thread.
   virtual const UsbConfigDescriptor& GetConfiguration() = 0;
 
-  // Gets the manufacturer string of the device, or returns false.
-  // Blocking method. Must be called on FILE thread.
+  // Gets the manufacturer string of the device, or false and an empty
+  // string. This is a blocking method and must be called on FILE thread.
   // TODO(reillyg): Make this available from the UI thread. crbug.com/427985
   virtual bool GetManufacturer(base::string16* manufacturer) = 0;
 
-  // Gets the product string of the device, or returns false.
-  // Blocking method. Must be called on FILE thread.
+  // Gets the product string of the device, or returns false and an empty
+  // string. This is a blocking method and must be called on FILE thread.
   // TODO(reillyg): Make this available from the UI thread. crbug.com/427985
   virtual bool GetProduct(base::string16* product) = 0;
 
-  // Gets the serial number string of the device, or returns false.
-  // Blocking method. Must be called on FILE thread.
+  // Gets the serial number string of the device, or returns false and an empty
+  // string. This is a blocking method and must be called on FILE thread.
   // TODO(reillyg): Make this available from the UI thread. crbug.com/427985
   virtual bool GetSerialNumber(base::string16* serial) = 0;
 

device/usb/usb_service.h

@@ -24,18 +24,21 @@ class UsbDevice;
 // used to manage and dispatch USB events. It is also responsible for device
 // discovery on the system, which allows it to re-use device handles to prevent
 // competition for the same USB device.
+//
+// All functions on this object must be called from a thread with a
+// MessageLoopForIO (for example, BrowserThread::FILE).
 class UsbService : public base::NonThreadSafe {
  public:
   class Observer {
    public:
+    // These events are delivered from the thread on which the UsbService object
+    // was created.
     virtual void OnDeviceAdded(scoped_refptr<UsbDevice> device);
     virtual void OnDeviceRemoved(scoped_refptr<UsbDevice> device);
   };
 
-  // Must be called on a thread with a MessageLoopForIO (for example
-  // BrowserThread::FILE). The UI task runner reference is used to talk to the
-  // PermissionBrokerClient on ChromeOS (UI thread). Returns NULL when
-  // initialization fails.
+  // The UI task runner reference is used to talk to the PermissionBrokerClient
+  // on ChromeOS (UI thread). Returns NULL when initialization fails.
   static UsbService* GetInstance(
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
 
@@ -45,7 +48,7 @@ class UsbService : public base::NonThreadSafe {
 
   // Get all of the devices attached to the system, inserting them into
   // |devices|. Clears |devices| before use. The result will be sorted by id
-  // in increasing order. Must be called on FILE thread.
+  // in increasing order.
   virtual void GetDevices(std::vector<scoped_refptr<UsbDevice> >* devices) = 0;
 
   void AddObserver(Observer* observer);

device/usb/usb_service_impl.cc

@@ -328,18 +328,22 @@ UsbService::~UsbService() {
 }
 
 void UsbService::AddObserver(Observer* observer) {
+  DCHECK(CalledOnValidThread());
   observer_list_.AddObserver(observer);
 }
 
 void UsbService::RemoveObserver(Observer* observer) {
+  DCHECK(CalledOnValidThread());
   observer_list_.RemoveObserver(observer);
 }
 
 void UsbService::NotifyDeviceAdded(scoped_refptr<UsbDevice> device) {
+  DCHECK(CalledOnValidThread());
   FOR_EACH_OBSERVER(Observer, observer_list_, OnDeviceAdded(device));
 }
 
 void UsbService::NotifyDeviceRemoved(scoped_refptr<UsbDevice> device) {
+  DCHECK(CalledOnValidThread());
   FOR_EACH_OBSERVER(Observer, observer_list_, OnDeviceRemoved(device));
 }
 

extensions/browser/BUILD.gn

@@ -241,6 +241,8 @@ source_set("browser") {
       "api/test/test_api.h",
       "api/usb/usb_api.cc",
       "api/usb/usb_api.h",
+      "api/usb/usb_event_router.cc",
+      "api/usb/usb_event_router.h",
       "api/usb/usb_device_resource.cc",
       "api/usb/usb_device_resource.h",
       "api/virtual_keyboard_private/virtual_keyboard_private_api.cc",

extensions/browser/api/device_permissions_manager.cc

@@ -337,14 +337,17 @@ DevicePermissions::~DevicePermissions() {
 }
 
 scoped_refptr<DevicePermissionEntry> DevicePermissions::FindEntry(
-    scoped_refptr<device::UsbDevice> device) const {
+    scoped_refptr<device::UsbDevice> device,
+    const base::string16& serial_number) const {
   const auto& ephemeral_device_entry = ephemeral_devices_.find(device);
   if (ephemeral_device_entry != ephemeral_devices_.end()) {
     return ephemeral_device_entry->second;
   }
 
-  bool have_serial_number = false;
-  base::string16 serial_number;
+  if (serial_number.empty()) {
+    return nullptr;
+  }
+
   for (const auto& entry : entries_) {
     if (!entry->IsPersistent()) {
       continue;
@@ -355,12 +358,6 @@ scoped_refptr<DevicePermissionEntry> DevicePermissions::FindEntry(
     if (entry->product_id() != device->product_id()) {
       continue;
     }
-    if (!have_serial_number) {
-      if (!device->GetSerialNumber(&serial_number)) {
-        break;
-      }
-      have_serial_number = true;
-    }
     if (entry->serial_number() != serial_number) {
       continue;
     }

extensions/browser/api/device_permissions_manager.h

@@ -102,10 +102,13 @@ class DevicePermissions {
  public:
   virtual ~DevicePermissions();
 
-  // Attempts to find a permission entry matching the given device. This
-  // function must be called from the FILE thread. crbug.com/427985
+  // Attempts to find a permission entry matching the given device. The device
+  // serial number is presented separately so that this function does not need
+  // to call device->GetSerialNumber() which may not be possible on the
+  // current thread.
   scoped_refptr<DevicePermissionEntry> FindEntry(
-      scoped_refptr<device::UsbDevice> device) const;
+      scoped_refptr<device::UsbDevice> device,
+      const base::string16& serial_number) const;
 
   const std::set<scoped_refptr<DevicePermissionEntry>>& entries() const {
     return entries_;

extensions/browser/api/usb/usb_api.cc

@@ -466,8 +466,11 @@ bool UsbAsyncApiFunction::HasDevicePermission(scoped_refptr<UsbDevice> device) {
   DCHECK(device_permissions_);
 
   // Check the DevicePermissionsManager first so that if an entry is found
-  // it can be stored for later.
-  permission_entry_ = device_permissions_->FindEntry(device);
+  // it can be stored for later. This requires the serial number.
+  base::string16 serial_number;
+  device->GetSerialNumber(&serial_number);
+
+  permission_entry_ = device_permissions_->FindEntry(device, serial_number);
   if (permission_entry_.get()) {
     return true;
   }