[usb] Fix tab indicator behavior on device disconnection

In r604873 support for revoking USB permission in real time was added
but broke this tab indicator behavior by closing the mojo::Receiver for
the UsbDeviceClient interface on device disconnection without updating
the tab indicator state.

This change fixes that behavior by having WebUsbServiceImpl track the
open state of each UsbDevice pipe it creates and updating the tab
indicator state if one of them is forcibly closed while the device is
open.

This change allows connection state tracking in UsbTabHelper to be
simplified.

Additional unit tests for this behavior have been added.

Bug: 1064467
Change-Id: I9747d48428fc3daaf921b6763060ba16007004ba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2122461Reviewed-by: Ovidio de Jesús Ruiz-Henríquez <odejesush@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754751}

chrome/browser/resource_coordinator/tab_lifecycle_unit_unittest.cc

@@ -485,7 +485,7 @@ TEST_F(TabLifecycleUnitTest, CannotFreezeOrDiscardWebUsbConnectionsOpen) {
   EXPECT_TRUE(decision_details.IsPositive());
 
   // Open a USB connection. Shouldn't be freezable/discardable anymore.
-  usb_tab_helper->IncrementConnectionCount(web_contents_->GetMainFrame());
+  usb_tab_helper->IncrementConnectionCount();
   ExpectCanDiscardFalseAllReasons(
       &tab_lifecycle_unit, DecisionFailureReason::LIVE_STATE_USING_WEB_USB);
   decision_details = DecisionDetails();
@@ -495,7 +495,7 @@ TEST_F(TabLifecycleUnitTest, CannotFreezeOrDiscardWebUsbConnectionsOpen) {
             decision_details.FailureReason());
 
   // Close the USB connection. Should be freezable/discardable again.
-  usb_tab_helper->DecrementConnectionCount(web_contents_->GetMainFrame());
+  usb_tab_helper->DecrementConnectionCount();
   ExpectCanDiscardTrueAllReasons(&tab_lifecycle_unit);
   decision_details = DecisionDetails();
   EXPECT_TRUE(tab_lifecycle_unit.CanFreeze(&decision_details));

chrome/browser/usb/usb_tab_helper.cc

@@ -38,7 +38,6 @@ const char kFeaturePolicyViolation[] =
 struct FrameUsbServices {
   std::unique_ptr<WebUsbChooser> usb_chooser;
   std::unique_ptr<WebUsbServiceImpl> web_usb_service;
-  int device_connection_count_ = 0;
 };
 
 // static
@@ -52,7 +51,11 @@ UsbTabHelper* UsbTabHelper::GetOrCreateForWebContents(
   return tab_helper;
 }
 
-UsbTabHelper::~UsbTabHelper() {}
+UsbTabHelper::~UsbTabHelper() {
+  // All RenderFrameHosts should have been deleted before the WebContents.
+  DCHECK(frame_usb_services_.empty());
+  DCHECK_EQ(0, device_connection_count_);
+}
 
 void UsbTabHelper::CreateWebUsbService(
     RenderFrameHost* render_frame_host,
@@ -70,33 +73,21 @@ void UsbTabHelper::CreateWebUsbService(
   frame_usb_services->web_usb_service->BindReceiver(std::move(receiver));
 }
 
-void UsbTabHelper::IncrementConnectionCount(
-    RenderFrameHost* render_frame_host) {
-  const bool was_device_connected = IsDeviceConnected();
-  auto it = frame_usb_services_.find(render_frame_host);
-  DCHECK(it != frame_usb_services_.end());
-  it->second->device_connection_count_++;
-  if (!was_device_connected)
+void UsbTabHelper::IncrementConnectionCount() {
+  device_connection_count_++;
+  if (device_connection_count_ == 1)
     NotifyIsDeviceConnectedChanged(/*is_device_connected=*/true);
 }
 
-void UsbTabHelper::DecrementConnectionCount(
-    RenderFrameHost* render_frame_host) {
-  DCHECK(IsDeviceConnected());
-  auto it = frame_usb_services_.find(render_frame_host);
-  DCHECK(it != frame_usb_services_.end());
-  DCHECK_GT(it->second->device_connection_count_, 0);
-  it->second->device_connection_count_--;
-  if (!IsDeviceConnected())
+void UsbTabHelper::DecrementConnectionCount() {
+  DCHECK_GT(device_connection_count_, 0);
+  device_connection_count_--;
+  if (device_connection_count_ == 0)
     NotifyIsDeviceConnectedChanged(/*is_device_connected=*/false);
 }
 
 bool UsbTabHelper::IsDeviceConnected() const {
-  for (const auto& map_entry : frame_usb_services_) {
-    if (map_entry.second->device_connection_count_ > 0)
-      return true;
-  }
-  return false;
+  return device_connection_count_ > 0;
 }
 
 UsbTabHelper::UsbTabHelper(WebContents* web_contents)
@@ -136,10 +127,7 @@ FrameUsbServices* UsbTabHelper::GetFrameUsbService(
 }
 
 void UsbTabHelper::DeleteFrameServices(RenderFrameHost* render_frame_host) {
-  const bool was_device_connected = IsDeviceConnected();
   frame_usb_services_.erase(render_frame_host);
-  if (was_device_connected && !IsDeviceConnected())
-    NotifyIsDeviceConnectedChanged(/*is_device_connected=*/false);
 }
 
 base::WeakPtr<WebUsbChooser> UsbTabHelper::GetUsbChooser(

chrome/browser/usb/usb_tab_helper.h

@@ -39,8 +39,8 @@ class UsbTabHelper : public content::WebContentsObserver,
       content::RenderFrameHost* render_frame_host,
       mojo::PendingReceiver<blink::mojom::WebUsbService> receiver);
 
-  void IncrementConnectionCount(content::RenderFrameHost* render_frame_host);
-  void DecrementConnectionCount(content::RenderFrameHost* render_frame_host);
+  void IncrementConnectionCount();
+  void DecrementConnectionCount();
   bool IsDeviceConnected() const;
 
  private:
@@ -66,6 +66,7 @@ class UsbTabHelper : public content::WebContentsObserver,
       content::RenderFrameHost* render_frame_host) const;
 
   FrameUsbServicesMap frame_usb_services_;
+  int device_connection_count_ = 0;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

chrome/browser/usb/usb_tab_helper_unittest.cc

@@ -11,7 +11,6 @@
 #include "components/performance_manager/embedder/performance_manager_registry.h"
 #include "components/performance_manager/public/decorators/page_live_state_decorator.h"
 #include "components/performance_manager/test_support/decorators_utils.h"
-#include "content/public/test/web_contents_tester.h"
 #include "services/device/public/cpp/test/fake_usb_device_manager.h"
 #include "services/service_manager/public/cpp/test/test_service.h"
 #include "services/service_manager/public/cpp/test/test_service_manager.h"
@@ -36,6 +35,8 @@ class UsbTabHelperTest
 
     performance_manager::PerformanceManagerRegistry::GetInstance()
         ->CreatePageNodeForWebContents(web_contents());
+
+    NavigateAndCommit(GURL("https://www.google.com"));
   }
 
  private:
@@ -57,7 +58,7 @@ TEST_F(UsbTabHelperTest, IncrementDecrementConnectionCount) {
 
   // Increment the USB connection count. Expect USBTabHelper and
   // PerformanceManager to indicate that the tab is attached to USB.
-  helper->IncrementConnectionCount(main_rfh());
+  helper->IncrementConnectionCount();
   EXPECT_TRUE(helper->IsDeviceConnected());
   performance_manager::testing::TestPageNodePropertyOnPMSequence(
       web_contents(),
@@ -67,7 +68,7 @@ TEST_F(UsbTabHelperTest, IncrementDecrementConnectionCount) {
 
   // Increment the USB connection count again. State shouldn't change in
   // USBTabHelper and in the PerformanceManager.
-  helper->IncrementConnectionCount(main_rfh());
+  helper->IncrementConnectionCount();
   EXPECT_TRUE(helper->IsDeviceConnected());
   performance_manager::testing::TestPageNodePropertyOnPMSequence(
       web_contents(),
@@ -77,7 +78,7 @@ TEST_F(UsbTabHelperTest, IncrementDecrementConnectionCount) {
 
   // Decrement the USB connection count. State shouldn't change in USBTabHelper
   // and in the PerformanceManager as one connection remains.
-  helper->DecrementConnectionCount(main_rfh());
+  helper->DecrementConnectionCount();
   EXPECT_TRUE(helper->IsDeviceConnected());
   performance_manager::testing::TestPageNodePropertyOnPMSequence(
       web_contents(),
@@ -87,42 +88,7 @@ TEST_F(UsbTabHelperTest, IncrementDecrementConnectionCount) {
 
   // Decrement the USB connection count again. Expect USBTabHelper and
   // PerformanceManager to indicate that the tab is *not* attached to USB.
-  helper->DecrementConnectionCount(main_rfh());
-  EXPECT_FALSE(helper->IsDeviceConnected());
-  performance_manager::testing::TestPageNodePropertyOnPMSequence(
-      web_contents(),
-      &performance_manager::PageLiveStateDecorator::Data::
-          IsConnectedToUSBDevice,
-      false);
-}
-
-TEST_F(UsbTabHelperTest, Navigate) {
-  mojo::Remote<blink::mojom::WebUsbService> remote;
-
-  UsbTabHelper* helper =
-      UsbTabHelper::GetOrCreateForWebContents(web_contents());
-  helper->CreateWebUsbService(main_rfh(), remote.BindNewPipeAndPassReceiver());
-  EXPECT_FALSE(helper->IsDeviceConnected());
-  performance_manager::testing::TestPageNodePropertyOnPMSequence(
-      web_contents(),
-      &performance_manager::PageLiveStateDecorator::Data::
-          IsConnectedToUSBDevice,
-      false);
-
-  // Increment the USB connection count. Expect USBTabHelper and
-  // PerformanceManager to indicate that the tab is attached to USB.
-  helper->IncrementConnectionCount(main_rfh());
-  EXPECT_TRUE(helper->IsDeviceConnected());
-  performance_manager::testing::TestPageNodePropertyOnPMSequence(
-      web_contents(),
-      &performance_manager::PageLiveStateDecorator::Data::
-          IsConnectedToUSBDevice,
-      true);
-
-  // Navigate away. Expect USBTabHelper and PerformanceManager to indicate that
-  // the tab is no longer attached to USB.
-  content::WebContentsTester::For(web_contents())
-      ->NavigateAndCommit(GURL(url::kAboutBlankURL));
+  helper->DecrementConnectionCount();
   EXPECT_FALSE(helper->IsDeviceConnected());
   performance_manager::testing::TestPageNodePropertyOnPMSequence(
       web_contents(),

chrome/browser/usb/web_usb_service_impl.cc

@@ -16,8 +16,59 @@
 #include "content/public/browser/browser_thread.h"
 #include "media/mojo/mojom/remoting_common.mojom.h"
 #include "mojo/public/cpp/bindings/associated_remote.h"
+#include "services/device/public/mojom/usb_device.mojom.h"
 #include "services/device/public/mojom/usb_enumeration_options.mojom.h"
 
+// A UsbDeviceClient represents a UsbDevice pipe that has been passed to the
+// renderer process. The UsbDeviceClient pipe allows the browser process to
+// continue to monitor how the device is used and cause the connection to be
+// closed at will.
+class WebUsbServiceImpl::UsbDeviceClient
+    : public device::mojom::UsbDeviceClient {
+ public:
+  UsbDeviceClient(
+      WebUsbServiceImpl* service,
+      const std::string& device_guid,
+      mojo::PendingReceiver<device::mojom::UsbDeviceClient> receiver)
+      : service_(service),
+        device_guid_(device_guid),
+        receiver_(this, std::move(receiver)) {
+    receiver_.set_disconnect_handler(
+        base::BindOnce(&WebUsbServiceImpl::RemoveDeviceClient,
+                       base::Unretained(service_), base::Unretained(this)));
+  }
+
+  ~UsbDeviceClient() override {
+    if (opened_) {
+      // If the connection was opened destroying |receiver_| will cause it to
+      // be closed but that event won't be dispatched here because the receiver
+      // has been destroyed.
+      OnDeviceClosed();
+    }
+  }
+
+  const std::string& device_guid() const { return device_guid_; }
+
+  // device::mojom::UsbDeviceClient implementation:
+  void OnDeviceOpened() override {
+    DCHECK(!opened_);
+    opened_ = true;
+    service_->IncrementConnectionCount();
+  }
+
+  void OnDeviceClosed() override {
+    DCHECK(opened_);
+    opened_ = false;
+    service_->DecrementConnectionCount();
+  }
+
+ private:
+  WebUsbServiceImpl* const service_;
+  const std::string device_guid_;
+  bool opened_ = false;
+  mojo::Receiver<device::mojom::UsbDeviceClient> receiver_;
+};
+
 WebUsbServiceImpl::WebUsbServiceImpl(
     content::RenderFrameHost* render_frame_host,
     base::WeakPtr<WebUsbChooser> usb_chooser)
@@ -106,8 +157,8 @@ void WebUsbServiceImpl::GetDevice(
   // This receiver will also be closed to notify the device service to close
   // the connection if permission is revoked.
   mojo::PendingRemote<device::mojom::UsbDeviceClient> device_client;
-  device_client_receivers_[guid].Add(
-      this, device_client.InitWithNewPipeAndPassReceiver());
+  device_clients_.push_back(std::make_unique<UsbDeviceClient>(
+      this, guid, device_client.InitWithNewPipeAndPassReceiver()));
   chooser_context_->GetDevice(guid, std::move(device_receiver),
                               std::move(device_client));
 }
@@ -140,8 +191,8 @@ void WebUsbServiceImpl::OnPermissionRevoked(
 
   // Close the connection between Blink and the device if the device lost
   // permission.
-  base::EraseIf(device_client_receivers_, [this](const auto& entry) {
-    auto* device_info = chooser_context_->GetDeviceInfo(entry.first);
+  base::EraseIf(device_clients_, [this](const auto& client) {
+    auto* device_info = chooser_context_->GetDeviceInfo(client->device_guid());
     if (!device_info)
       return true;
 
@@ -160,7 +211,10 @@ void WebUsbServiceImpl::OnDeviceAdded(
 
 void WebUsbServiceImpl::OnDeviceRemoved(
     const device::mojom::UsbDeviceInfo& device_info) {
-  device_client_receivers_.erase(device_info.guid);
+  base::EraseIf(device_clients_, [&device_info](const auto& client) {
+    return device_info.guid == client->device_guid();
+  });
+
   if (!HasDevicePermission(device_info))
     return;
 
@@ -179,20 +233,26 @@ void WebUsbServiceImpl::OnDeviceManagerConnectionError() {
 }
 
 // device::mojom::UsbDeviceClient implementation:
-void WebUsbServiceImpl::OnDeviceOpened() {
+void WebUsbServiceImpl::IncrementConnectionCount() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(render_frame_host_);
   UsbTabHelper* tab_helper = UsbTabHelper::FromWebContents(web_contents);
-  tab_helper->IncrementConnectionCount(render_frame_host_);
+  tab_helper->IncrementConnectionCount();
 }
 
-void WebUsbServiceImpl::OnDeviceClosed() {
+void WebUsbServiceImpl::DecrementConnectionCount() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   content::WebContents* web_contents =
       content::WebContents::FromRenderFrameHost(render_frame_host_);
   UsbTabHelper* tab_helper = UsbTabHelper::FromWebContents(web_contents);
-  tab_helper->DecrementConnectionCount(render_frame_host_);
+  tab_helper->DecrementConnectionCount();
+}
+
+void WebUsbServiceImpl::RemoveDeviceClient(const UsbDeviceClient* client) {
+  base::EraseIf(device_clients_, [client](const auto& this_client) {
+    return client == this_client.get();
+  });
 }
 
 void WebUsbServiceImpl::OnConnectionError() {

chrome/browser/usb/web_usb_service_impl.h

@@ -21,7 +21,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote_set.h"
-#include "services/device/public/mojom/usb_device.mojom.h"
+#include "services/device/public/mojom/usb_device.mojom-forward.h"
 #include "third_party/blink/public/mojom/usb/web_usb_service.mojom-forward.h"
 #include "url/origin.h"
 
@@ -38,8 +38,7 @@ class UsbChooserContext;
 class WebUsbServiceImpl
     : public blink::mojom::WebUsbService,
       public permissions::ChooserContextBase::PermissionObserver,
-      public UsbChooserContext::DeviceObserver,
-      public device::mojom::UsbDeviceClient {
+      public UsbChooserContext::DeviceObserver {
  public:
   WebUsbServiceImpl(content::RenderFrameHost* render_frame_host,
                     base::WeakPtr<WebUsbChooser> usb_chooser);
@@ -49,6 +48,8 @@ class WebUsbServiceImpl
       mojo::PendingReceiver<blink::mojom::WebUsbService> receiver);
 
  private:
+  class UsbDeviceClient;
+
   bool HasDevicePermission(
       const device::mojom::UsbDeviceInfo& device_info) const;
 
@@ -78,9 +79,9 @@ class WebUsbServiceImpl
       const device::mojom::UsbDeviceInfo& device_info) override;
   void OnDeviceManagerConnectionError() override;
 
-  // device::mojom::UsbDeviceClient implementation:
-  void OnDeviceOpened() override;
-  void OnDeviceClosed() override;
+  void IncrementConnectionCount();
+  void DecrementConnectionCount();
+  void RemoveDeviceClient(const UsbDeviceClient* client);
 
   void OnConnectionError();
 
@@ -94,10 +95,8 @@ class WebUsbServiceImpl
   mojo::ReceiverSet<blink::mojom::WebUsbService> receivers_;
   mojo::AssociatedRemoteSet<device::mojom::UsbDeviceManagerClient> clients_;
 
-  // Tracks DeviceClient receivers for each device (by GUID).
-  std::unordered_map<std::string,
-                     mojo::ReceiverSet<device::mojom::UsbDeviceClient>>
-      device_client_receivers_;
+  // A UsbDeviceClient tracks a UsbDevice pipe that has been passed to Blink.
+  std::vector<std::unique_ptr<UsbDeviceClient>> device_clients_;
 
   ScopedObserver<UsbChooserContext, UsbChooserContext::DeviceObserver>
       device_observer_;

chrome/browser/usb/web_usb_service_impl_unittest.cc

@@ -16,9 +16,9 @@
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/usb/usb_chooser_context.h"
 #include "chrome/browser/usb/usb_chooser_context_factory.h"
+#include "chrome/browser/usb/usb_tab_helper.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
-#include "content/public/test/web_contents_tester.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -41,6 +41,7 @@ using device::mojom::UsbDeviceManagerClient;
 namespace {
 
 const char kDefaultTestUrl[] = "https://www.google.com/";
+const char kCrossOriginTestUrl[] = "https://www.chromium.org";
 
 ACTION_P2(ExpectGuidAndThen, expected_guid, callback) {
   ASSERT_TRUE(arg0);
@@ -55,9 +56,7 @@ class WebUsbServiceImplTest : public ChromeRenderViewHostTestHarness {
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
-    content::WebContentsTester* web_contents_tester =
-        content::WebContentsTester::For(web_contents());
-    web_contents_tester->NavigateAndCommit(GURL(kDefaultTestUrl));
+    NavigateAndCommit(GURL(kDefaultTestUrl));
   }
 
  protected:
@@ -75,10 +74,8 @@ class WebUsbServiceImplTest : public ChromeRenderViewHostTestHarness {
           std::move(pending_device_manager));
     }
 
-    if (!web_usb_service_)
-      web_usb_service_.reset(new WebUsbServiceImpl(main_rfh(), nullptr));
-
-    web_usb_service_->BindReceiver(std::move(receiver));
+    auto* tab_helper = UsbTabHelper::GetOrCreateForWebContents(web_contents());
+    tab_helper->CreateWebUsbService(main_rfh(), std::move(receiver));
   }
 
   UsbChooserContext* GetChooserContext() {
@@ -94,7 +91,6 @@ class WebUsbServiceImplTest : public ChromeRenderViewHostTestHarness {
 
  private:
   std::unique_ptr<device::FakeUsbDeviceManager> device_manager_;
-  std::unique_ptr<WebUsbServiceImpl> web_usb_service_;
   DISALLOW_COPY_AND_ASSIGN(WebUsbServiceImplTest);
 };
 
@@ -131,15 +127,29 @@ class MockDeviceManagerClient : public UsbDeviceManagerClient {
   mojo::AssociatedReceiver<UsbDeviceManagerClient> receiver_{this};
 };
 
-void ExpectDevicesAndThen(const std::set<std::string>& expected_guids,
-                          base::OnceClosure continuation,
-                          std::vector<UsbDeviceInfoPtr> results) {
-  EXPECT_EQ(expected_guids.size(), results.size());
-  std::set<std::string> actual_guids;
-  for (size_t i = 0; i < results.size(); ++i)
-    actual_guids.insert(results[i]->guid);
-  EXPECT_EQ(expected_guids, actual_guids);
-  std::move(continuation).Run();
+void GetDevicesBlocking(blink::mojom::WebUsbService* service,
+                        const std::set<std::string>& expected_guids) {
+  base::RunLoop run_loop;
+  service->GetDevices(
+      base::BindLambdaForTesting([&](std::vector<UsbDeviceInfoPtr> devices) {
+        EXPECT_EQ(expected_guids.size(), devices.size());
+        std::set<std::string> actual_guids;
+        for (const auto& device : devices)
+          actual_guids.insert(device->guid);
+        EXPECT_EQ(expected_guids, actual_guids);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
+}
+
+void OpenDeviceBlocking(device::mojom::UsbDevice* device) {
+  base::RunLoop run_loop;
+  device->Open(
+      base::BindLambdaForTesting([&](device::mojom::UsbOpenDeviceError error) {
+        EXPECT_EQ(device::mojom::UsbOpenDeviceError::OK, error);
+        run_loop.Quit();
+      }));
+  run_loop.Run();
 }
 
 }  // namespace
@@ -165,18 +175,11 @@ TEST_F(WebUsbServiceImplTest, NoPermissionDevice) {
   MockDeviceManagerClient mock_client;
   web_usb_service->SetClient(mock_client.CreateInterfacePtrAndBind());
 
-  {
-    // Call GetDevices once to make sure the WebUsbService is up and running
-    // and the client is set or else we could block forever waiting for calls.
-    // The site has no permission to access |no_permission_device1|, so result
-    // of GetDevices() should only contain the |guid| of |device1|.
-    std::set<std::string> guids;
-    guids.insert(device1->guid());
-    base::RunLoop loop;
-    web_usb_service->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
+  // Call GetDevices once to make sure the WebUsbService is up and running
+  // and the client is set or else we could block forever waiting for calls.
+  // The site has no permission to access |no_permission_device1|, so result
+  // of GetDevices() should only contain the |guid| of |device1|.
+  GetDevicesBlocking(web_usb_service.get(), {device1->guid()});
 
   auto device_info_2 = device_manager()->AddDevice(device2);
   GetChooserContext()->GrantDevicePermission(origin, origin, *device_info_2);
@@ -233,15 +236,8 @@ TEST_F(WebUsbServiceImplTest, ReconnectDeviceManager) {
   MockDeviceManagerClient mock_client;
   web_usb_service->SetClient(mock_client.CreateInterfacePtrAndBind());
 
-  {
-    std::set<std::string> guids;
-    guids.insert(device->guid());
-    guids.insert(ephemeral_device->guid());
-    base::RunLoop loop;
-    web_usb_service->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
+  GetDevicesBlocking(web_usb_service.get(),
+                     {device->guid(), ephemeral_device->guid()});
 
   EXPECT_TRUE(context->HasDevicePermission(origin, origin, *device_info));
   EXPECT_TRUE(
@@ -276,14 +272,7 @@ TEST_F(WebUsbServiceImplTest, ReconnectDeviceManager) {
   ConnectToService(web_usb_service.BindNewPipeAndPassReceiver());
   web_usb_service->SetClient(mock_client.CreateInterfacePtrAndBind());
 
-  {
-    std::set<std::string> guids;
-    guids.insert(another_device->guid());
-    base::RunLoop loop;
-    web_usb_service->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
+  GetDevicesBlocking(web_usb_service.get(), {another_device->guid()});
 
   EXPECT_TRUE(context->HasDevicePermission(origin, origin, *device_info));
   EXPECT_TRUE(
@@ -302,13 +291,7 @@ TEST_F(WebUsbServiceImplTest, RevokeDevicePermission) {
   mojo::Remote<WebUsbService> web_usb_service;
   ConnectToService(web_usb_service.BindNewPipeAndPassReceiver());
   base::RunLoop().RunUntilIdle();
-  {
-    std::set<std::string> guids;
-    base::RunLoop loop;
-    web_usb_service->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
+  GetDevicesBlocking(web_usb_service.get(), {});
 
   context->GrantDevicePermission(origin, origin, *device_info);
 
@@ -327,3 +310,89 @@ TEST_F(WebUsbServiceImplTest, RevokeDevicePermission) {
 
   EXPECT_FALSE(device);
 }
+
+TEST_F(WebUsbServiceImplTest, OpenAndCloseDevice) {
+  const auto origin = url::Origin::Create(GURL(kDefaultTestUrl));
+
+  auto* context = GetChooserContext();
+  auto device_info = device_manager()->CreateAndAddDevice(
+      0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF");
+  context->GrantDevicePermission(origin, origin, *device_info);
+
+  mojo::Remote<WebUsbService> service;
+  ConnectToService(service.BindNewPipeAndPassReceiver());
+  UsbTabHelper* tab_helper = UsbTabHelper::FromWebContents(web_contents());
+  ASSERT_TRUE(tab_helper);
+
+  GetDevicesBlocking(service.get(), {device_info->guid});
+
+  mojo::Remote<device::mojom::UsbDevice> device;
+  service->GetDevice(device_info->guid, device.BindNewPipeAndPassReceiver());
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+
+  OpenDeviceBlocking(device.get());
+  EXPECT_TRUE(tab_helper->IsDeviceConnected());
+
+  {
+    base::RunLoop run_loop;
+    device->Close(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+}
+
+TEST_F(WebUsbServiceImplTest, OpenAndDisconnectDevice) {
+  const auto origin = url::Origin::Create(GURL(kDefaultTestUrl));
+
+  auto* context = GetChooserContext();
+  auto fake_device = base::MakeRefCounted<FakeUsbDeviceInfo>(
+      0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF");
+  auto device_info = device_manager()->AddDevice(fake_device);
+  context->GrantDevicePermission(origin, origin, *device_info);
+
+  mojo::Remote<WebUsbService> service;
+  ConnectToService(service.BindNewPipeAndPassReceiver());
+  UsbTabHelper* tab_helper = UsbTabHelper::FromWebContents(web_contents());
+  ASSERT_TRUE(tab_helper);
+
+  GetDevicesBlocking(service.get(), {device_info->guid});
+
+  mojo::Remote<device::mojom::UsbDevice> device;
+  service->GetDevice(device_info->guid, device.BindNewPipeAndPassReceiver());
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+
+  OpenDeviceBlocking(device.get());
+  EXPECT_TRUE(tab_helper->IsDeviceConnected());
+
+  device_manager()->RemoveDevice(fake_device);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+}
+
+TEST_F(WebUsbServiceImplTest, OpenAndNavigateCrossOrigin) {
+  const auto origin = url::Origin::Create(GURL(kDefaultTestUrl));
+
+  auto* context = GetChooserContext();
+  auto fake_device = base::MakeRefCounted<FakeUsbDeviceInfo>(
+      0x1234, 0x5678, "ACME", "Frobinator", "ABCDEF");
+  auto device_info = device_manager()->AddDevice(fake_device);
+  context->GrantDevicePermission(origin, origin, *device_info);
+
+  mojo::Remote<WebUsbService> service;
+  ConnectToService(service.BindNewPipeAndPassReceiver());
+  UsbTabHelper* tab_helper = UsbTabHelper::FromWebContents(web_contents());
+  ASSERT_TRUE(tab_helper);
+
+  GetDevicesBlocking(service.get(), {device_info->guid});
+
+  mojo::Remote<device::mojom::UsbDevice> device;
+  service->GetDevice(device_info->guid, device.BindNewPipeAndPassReceiver());
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+
+  OpenDeviceBlocking(device.get());
+  EXPECT_TRUE(tab_helper->IsDeviceConnected());
+
+  NavigateAndCommit(GURL(kCrossOriginTestUrl));
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(tab_helper->IsDeviceConnected());
+}

services/device/usb/mojo/device_impl_unittest.cc

@@ -153,6 +153,8 @@ class MockUsbDeviceClient : public mojom::UsbDeviceClient {
     return receiver_.BindNewPipeAndPassRemote();
   }
 
+  void FlushForTesting() { receiver_.FlushForTesting(); }
+
   MOCK_METHOD0(OnDeviceOpened, void());
   MOCK_METHOD0(OnDeviceClosed, void());
 
@@ -448,12 +450,31 @@ class USBDeviceImplTest : public testing::Test {
 }  // namespace
 
 TEST_F(USBDeviceImplTest, Disconnect) {
-  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+  MockUsbDeviceClient device_client;
+  mojo::Remote<mojom::UsbDevice> device =
+      GetMockDeviceProxy(device_client.CreateInterfacePtrAndBind());
+
+  EXPECT_FALSE(is_device_open());
+
+  EXPECT_CALL(mock_device(), OpenInternal(_));
+  EXPECT_CALL(device_client, OnDeviceOpened());
+
+  {
+    base::RunLoop loop;
+    device->Open(base::BindOnce(
+        &ExpectOpenAndThen, mojom::UsbOpenDeviceError::OK, loop.QuitClosure()));
+    loop.Run();
+  }
+
+  EXPECT_CALL(mock_handle(), Close());
+  EXPECT_CALL(device_client, OnDeviceClosed());
 
   base::RunLoop loop;
   device.set_disconnect_handler(loop.QuitClosure());
   mock_device().NotifyDeviceRemoved();
   loop.Run();
+
+  device_client.FlushForTesting();
 }
 
 TEST_F(USBDeviceImplTest, Open) {
@@ -529,11 +550,14 @@ TEST_F(USBDeviceImplTest, OpenDelayedFailure) {
 }
 
 TEST_F(USBDeviceImplTest, Close) {
-  mojo::Remote<mojom::UsbDevice> device = GetMockDeviceProxy();
+  MockUsbDeviceClient device_client;
+  mojo::Remote<mojom::UsbDevice> device =
+      GetMockDeviceProxy(device_client.CreateInterfacePtrAndBind());
 
   EXPECT_FALSE(is_device_open());
 
   EXPECT_CALL(mock_device(), OpenInternal(_));
+  EXPECT_CALL(device_client, OnDeviceOpened());
 
   {
     base::RunLoop loop;
@@ -543,6 +567,7 @@ TEST_F(USBDeviceImplTest, Close) {
   }
 
   EXPECT_CALL(mock_handle(), Close());
+  EXPECT_CALL(device_client, OnDeviceClosed());
 
   {
     base::RunLoop loop;