Move permission checking in UsbDevice to WebUsbService.

This CL moves the permission checking in UsbDevice implementation
to WebUsbService by adjusting some mojo interfaces.

BUG=699790

Change-Id: Ie5842f1a433a8b05275b06f31f25320bcc10a4d7
Reviewed-on: https://chromium-review.googlesource.com/1166592
Commit-Queue: Donna Wu <donna.wu@intel.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582538}

chrome/browser/usb/web_usb_service_impl.cc

@@ -54,7 +54,12 @@ void WebUsbServiceImpl::GetDevices(GetDevicesCallback callback) {
 void WebUsbServiceImpl::GetDevice(
     const std::string& guid,
     device::mojom::UsbDeviceRequest device_request) {
-  device_manager_->GetDevice(guid, std::move(device_request));
+  // Try to bind with the new device to be created for DeviceOpened/Closed
+  // events.
+  device::mojom::UsbDeviceClientPtr device_client;
+  device_client_bindings_.AddBinding(this, mojo::MakeRequest(&device_client));
+  device_manager_->GetDevice(guid, std::move(device_request),
+                             std::move(device_client));
 }
 
 void WebUsbServiceImpl::SetClient(
@@ -77,6 +82,17 @@ void WebUsbServiceImpl::OnDeviceRemoved(
   }
 }
 
+// device::mojom::UsbDeviceClient implementation:
+void WebUsbServiceImpl::OnDeviceOpened() {
+  if (permission_provider_)
+    permission_provider_->IncrementConnectionCount();
+}
+
+void WebUsbServiceImpl::OnDeviceClosed() {
+  if (permission_provider_)
+    permission_provider_->DecrementConnectionCount();
+}
+
 void WebUsbServiceImpl::OnConnectionError() {
   device_manager_.reset();
 

chrome/browser/usb/web_usb_service_impl.h

@@ -14,6 +14,7 @@
 #include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/public/mojom/device_manager.mojom.h"
 #include "device/usb/usb_service.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "third_party/blink/public/mojom/usb/web_usb_service.mojom.h"
 
@@ -25,7 +26,8 @@ class UsbDevice;
 // another UsbDeviceManager instance and checking requests with the provided
 // device::usb::PermissionProvider.
 class WebUsbServiceImpl : public blink::mojom::WebUsbService,
-                          public device::UsbService::Observer {
+                          public device::UsbService::Observer,
+                          public device::mojom::UsbDeviceClient {
  public:
   static void Create(
       base::WeakPtr<device::usb::PermissionProvider> permission_provider,
@@ -46,6 +48,11 @@ class WebUsbServiceImpl : public blink::mojom::WebUsbService,
   // device::UsbService::Observer implementation:
   void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override;
   void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
+
+  // device::mojom::UsbDeviceClient implementation:
+  void OnDeviceOpened() override;
+  void OnDeviceClosed() override;
+
   void WillDestroyUsbService() override;
 
   void OnConnectionError();
@@ -56,6 +63,9 @@ class WebUsbServiceImpl : public blink::mojom::WebUsbService,
   mojo::StrongBindingPtr<blink::mojom::WebUsbService> binding_;
   device::mojom::UsbDeviceManagerClientPtr client_;
 
+  // Binding used to connect with USB devices for opened/closed events.
+  mojo::BindingSet<device::mojom::UsbDeviceClient> device_client_bindings_;
+
   device::mojom::UsbDeviceManagerPtr device_manager_;
   ScopedObserver<device::UsbService, device::UsbService::Observer> observer_;
 

device/usb/mojo/device_impl.cc

@@ -103,10 +103,9 @@ void OnIsochronousTransferOut(
 
 // static
 void DeviceImpl::Create(scoped_refptr<device::UsbDevice> device,
-                        base::WeakPtr<PermissionProvider> permission_provider,
-                        mojom::UsbDeviceRequest request) {
-  auto* device_impl =
-      new DeviceImpl(std::move(device), std::move(permission_provider));
+                        mojom::UsbDeviceRequest request,
+                        mojom::UsbDeviceClientPtr client) {
+  auto* device_impl = new DeviceImpl(std::move(device), std::move(client));
   device_impl->binding_ = mojo::MakeStrongBinding(base::WrapUnique(device_impl),
                                                   std::move(request));
 }
@@ -116,10 +115,10 @@ DeviceImpl::~DeviceImpl() {
 }
 
 DeviceImpl::DeviceImpl(scoped_refptr<device::UsbDevice> device,
-                       base::WeakPtr<PermissionProvider> permission_provider)
+                       mojom::UsbDeviceClientPtr client)
     : device_(std::move(device)),
-      permission_provider_(std::move(permission_provider)),
       observer_(this),
+      client_(std::move(client)),
       weak_factory_(this) {
   DCHECK(device_);
   observer_.Add(device_.get());
@@ -128,8 +127,8 @@ DeviceImpl::DeviceImpl(scoped_refptr<device::UsbDevice> device,
 void DeviceImpl::CloseHandle() {
   if (device_handle_) {
     device_handle_->Close();
-    if (permission_provider_)
-      permission_provider_->DecrementConnectionCount();
+    if (client_)
+      client_->OnDeviceClosed();
   }
   device_handle_ = nullptr;
 }
@@ -174,8 +173,9 @@ void DeviceImpl::OnOpen(base::WeakPtr<DeviceImpl> self,
   }
 
   self->device_handle_ = std::move(handle);
-  if (self->device_handle_ && self->permission_provider_)
-    self->permission_provider_->IncrementConnectionCount();
+  if (self->device_handle_ && self->client_)
+    self->client_->OnDeviceOpened();
+
   std::move(callback).Run(self->device_handle_
                               ? mojom::UsbOpenDeviceError::OK
                               : mojom::UsbOpenDeviceError::ACCESS_DENIED);

device/usb/mojo/device_impl.h

@@ -13,7 +13,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observer.h"
-#include "device/usb/mojo/permission_provider.h"
 #include "device/usb/public/mojom/device.mojom.h"
 #include "device/usb/usb_device.h"
 #include "device/usb/usb_device_handle.h"
@@ -22,22 +21,20 @@
 namespace device {
 namespace usb {
 
-class PermissionProvider;
-
 // Implementation of the public Device interface. Instances of this class are
 // constructed by DeviceManagerImpl and are strongly bound to their MessagePipe
 // lifetime.
 class DeviceImpl : public mojom::UsbDevice, public device::UsbDevice::Observer {
  public:
   static void Create(scoped_refptr<device::UsbDevice> device,
-                     base::WeakPtr<PermissionProvider> permission_provider,
-                     mojom::UsbDeviceRequest request);
+                     mojom::UsbDeviceRequest request,
+                     mojom::UsbDeviceClientPtr client);
 
   ~DeviceImpl() override;
 
  private:
   DeviceImpl(scoped_refptr<device::UsbDevice> device,
-             base::WeakPtr<PermissionProvider> permission_provider);
+             mojom::UsbDeviceClientPtr client);
 
   // Closes the device if it's open. This will always set |device_handle_| to
   // null.
@@ -99,7 +96,6 @@ class DeviceImpl : public mojom::UsbDevice, public device::UsbDevice::Observer {
   void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
 
   const scoped_refptr<device::UsbDevice> device_;
-  base::WeakPtr<PermissionProvider> permission_provider_;
   ScopedObserver<device::UsbDevice, device::UsbDevice::Observer> observer_;
 
   // The device handle. Will be null before the device is opened and after it
@@ -107,6 +103,7 @@ class DeviceImpl : public mojom::UsbDevice, public device::UsbDevice::Observer {
   scoped_refptr<UsbDeviceHandle> device_handle_;
 
   mojo::StrongBindingPtr<mojom::UsbDevice> binding_;
+  device::mojom::UsbDeviceClientPtr client_;
   base::WeakPtrFactory<DeviceImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceImpl);

device/usb/mojo/device_impl_unittest.cc

@@ -24,7 +24,6 @@
 #include "base/stl_util.h"
 #include "device/usb/mock_usb_device.h"
 #include "device/usb/mock_usb_device_handle.h"
-#include "device/usb/mojo/mock_permission_provider.h"
 #include "device/usb/mojo/type_converters.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
@@ -138,6 +137,24 @@ void ExpectTransferStatusAndThen(mojom::UsbTransferStatus expected_status,
   continuation.Run();
 }
 
+class MockUsbDeviceClient : public mojom::UsbDeviceClient {
+ public:
+  MockUsbDeviceClient() : binding_(this) {}
+  ~MockUsbDeviceClient() override = default;
+
+  mojom::UsbDeviceClientPtr CreateInterfacePtrAndBind() {
+    mojom::UsbDeviceClientPtr client;
+    binding_.Bind(mojo::MakeRequest(&client));
+    return client;
+  }
+
+  MOCK_METHOD0(OnDeviceOpened, void());
+  MOCK_METHOD0(OnDeviceClosed, void());
+
+ private:
+  mojo::Binding<mojom::UsbDeviceClient> binding_;
+};
+
 class USBDeviceImplTest : public testing::Test {
  public:
   USBDeviceImplTest()
@@ -150,7 +167,6 @@ class USBDeviceImplTest : public testing::Test {
   void TearDown() override { base::RunLoop().RunUntilIdle(); }
 
  protected:
-  MockPermissionProvider& permission_provider() { return permission_provider_; }
   MockUsbDevice& mock_device() { return *mock_device_.get(); }
   bool is_device_open() const { return is_device_open_; }
   MockUsbDeviceHandle& mock_handle() { return *mock_handle_.get(); }
@@ -163,14 +179,15 @@ class USBDeviceImplTest : public testing::Test {
                                   uint16_t product_id,
                                   const std::string& manufacturer,
                                   const std::string& product,
-                                  const std::string& serial) {
+                                  const std::string& serial,
+                                  mojom::UsbDeviceClientPtr client) {
     mock_device_ =
         new MockUsbDevice(vendor_id, product_id, manufacturer, product, serial);
     mock_handle_ = new MockUsbDeviceHandle(mock_device_.get());
 
     UsbDevicePtr proxy;
-    DeviceImpl::Create(mock_device_, permission_provider_.GetWeakPtr(),
-                       mojo::MakeRequest(&proxy));
+    DeviceImpl::Create(mock_device_, mojo::MakeRequest(&proxy),
+                       std::move(client));
 
     // Set up mock handle calls to respond based on mock device configs
     // established by the test.
@@ -202,10 +219,13 @@ class USBDeviceImplTest : public testing::Test {
     return proxy;
   }
 
-  UsbDevicePtr GetMockDeviceProxy() {
-    return GetMockDeviceProxy(0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF");
+  UsbDevicePtr GetMockDeviceProxy(mojom::UsbDeviceClientPtr client) {
+    return GetMockDeviceProxy(0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF",
+                              std::move(client));
   }
 
+  UsbDevicePtr GetMockDeviceProxy() { return GetMockDeviceProxy(nullptr); }
+
   void AddMockConfig(const ConfigBuilder& builder) {
     const UsbConfigDescriptor& config = builder.config();
     DCHECK(!base::ContainsKey(mock_configs_, config.configuration_value));
@@ -419,8 +439,6 @@ class USBDeviceImplTest : public testing::Test {
 
   std::set<uint8_t> claimed_interfaces_;
 
-  MockPermissionProvider permission_provider_;
-
   DISALLOW_COPY_AND_ASSIGN(USBDeviceImplTest);
 };
 
@@ -436,12 +454,14 @@ TEST_F(USBDeviceImplTest, Disconnect) {
 }
 
 TEST_F(USBDeviceImplTest, Open) {
-  UsbDevicePtr device = GetMockDeviceProxy();
+  MockUsbDeviceClient device_client;
+  UsbDevicePtr device =
+      GetMockDeviceProxy(device_client.CreateInterfacePtrAndBind());
 
   EXPECT_FALSE(is_device_open());
 
   EXPECT_CALL(mock_device(), OpenInternal(_));
-  EXPECT_CALL(permission_provider(), IncrementConnectionCount());
+  EXPECT_CALL(device_client, OnDeviceOpened());
 
   {
     base::RunLoop loop;
@@ -459,7 +479,10 @@ TEST_F(USBDeviceImplTest, Open) {
   }
 
   EXPECT_CALL(mock_handle(), Close());
-  EXPECT_CALL(permission_provider(), DecrementConnectionCount());
+  EXPECT_CALL(device_client, OnDeviceClosed());
+
+  device.reset();
+  base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(USBDeviceImplTest, Close) {

device/usb/mojo/device_manager_impl.cc

@@ -63,15 +63,16 @@ void DeviceManagerImpl::GetDevices(mojom::UsbEnumerationOptionsPtr options,
 }
 
 void DeviceManagerImpl::GetDevice(const std::string& guid,
-                                  mojom::UsbDeviceRequest device_request) {
+                                  mojom::UsbDeviceRequest device_request,
+                                  mojom::UsbDeviceClientPtr device_client) {
   scoped_refptr<UsbDevice> device = usb_service_->GetDevice(guid);
   if (!device)
     return;
 
   if (permission_provider_ &&
       permission_provider_->HasDevicePermission(device)) {
-    DeviceImpl::Create(std::move(device), permission_provider_,
-                       std::move(device_request));
+    DeviceImpl::Create(std::move(device), std::move(device_request),
+                       std::move(device_client));
   }
 }
 

device/usb/mojo/device_manager_impl.h

@@ -44,7 +44,8 @@ class DeviceManagerImpl : public mojom::UsbDeviceManager,
   void GetDevices(mojom::UsbEnumerationOptionsPtr options,
                   GetDevicesCallback callback) override;
   void GetDevice(const std::string& guid,
-                 mojom::UsbDeviceRequest device_request) override;
+                 mojom::UsbDeviceRequest device_request,
+                 mojom::UsbDeviceClientPtr device_client) override;
   void SetClient(mojom::UsbDeviceManagerClientPtr client) override;
 
   // Callbacks to handle the async responses from the underlying UsbService.

device/usb/mojo/device_manager_impl_unittest.cc

@@ -152,7 +152,8 @@ TEST_F(USBDeviceManagerImplTest, GetDevice) {
   {
     base::RunLoop loop;
     UsbDevicePtr device;
-    device_manager->GetDevice(mock_device->guid(), mojo::MakeRequest(&device));
+    device_manager->GetDevice(mock_device->guid(), mojo::MakeRequest(&device),
+                              /*device_client=*/nullptr);
     // Close is a no-op if the device hasn't been opened but ensures that the
     // pipe was successfully connected.
     device->Close(loop.QuitClosure());
@@ -160,7 +161,8 @@ TEST_F(USBDeviceManagerImplTest, GetDevice) {
   }
 
   UsbDevicePtr bad_device;
-  device_manager->GetDevice("not a real guid", mojo::MakeRequest(&bad_device));
+  device_manager->GetDevice("not a real guid", mojo::MakeRequest(&bad_device),
+                            /*device_client=*/nullptr);
 
   {
     base::RunLoop loop;

device/usb/public/mojom/device.mojom

@@ -271,3 +271,19 @@ interface UsbDevice {
                          uint32 timeout)
       => (array<UsbIsochronousPacket> packets);
 };
+
+// This client is introduced for keeping connection count for a tab through
+// WebUSBPermissionProvider. It will be implemented by WebUsbServiceImpl and
+// passed to the UsbDevice via UsbDeviceManager::GetDevice method.
+interface UsbDeviceClient {
+  // Called when a device is opened successfully.
+  // This event may be triggered once more than OnDeviceClosed while the device
+  // is in use.
+  OnDeviceOpened();
+
+  // Called when a device is closed successfully.
+  // This event may not be triggered when the device is in use. But eventually,
+  // after the whole lifecycle, it is expected to have the same number of this
+  // call as that of OnDeviceOpened.
+  OnDeviceClosed();
+};

device/usb/public/mojom/device_manager.mojom

@@ -36,7 +36,8 @@ interface UsbDeviceManager {
   GetDevices(UsbEnumerationOptions? options) => (array<UsbDeviceInfo> results);
 
   // Requests a device by guid.
-  GetDevice(string guid, UsbDevice& device_request);
+  GetDevice(string guid, UsbDevice& device_request,
+            UsbDeviceClient? device_client);
 
   // Sets the client for this DeviceManager service. The service will notify its
   // client of device events such as connection and disconnection.