Ensure device choosers are closed on navigation

The requestDevice() IPCs can race with navigation. This change ensures
that choosers are closed on navigation and adds browser tests to
exercise this for Web Bluetooth and WebUSB.

Bug: 723503
Change-Id: I66760161220e17bd2be9309cca228d161fe76e9c
Reviewed-on: https://chromium-review.googlesource.com/1099961
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569900}

chrome/browser/ui/bluetooth/bluetooth_chooser_desktop.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "chrome/browser/ui/bluetooth/bluetooth_chooser_controller.h"
+#include "components/bubble/bubble_controller.h"
 
 BluetoothChooserDesktop::BluetoothChooserDesktop(
     BluetoothChooserController* bluetooth_chooser_controller)
@@ -18,6 +19,8 @@ BluetoothChooserDesktop::~BluetoothChooserDesktop() {
   // that the EventHandler can be destroyed any time after the BluetoothChooser
   // instance.
   bluetooth_chooser_controller_->ResetEventHandler();
+  if (bubble_)
+    bubble_->CloseBubble(BUBBLE_CLOSE_FORCED);
 }
 
 void BluetoothChooserDesktop::SetAdapterPresence(AdapterPresence presence) {

chrome/browser/ui/bluetooth/bluetooth_chooser_desktop.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_UI_BLUETOOTH_BLUETOOTH_CHOOSER_DESKTOP_H_
 
 #include "base/macros.h"
+#include "components/bubble/bubble_reference.h"
 #include "content/public/browser/bluetooth_chooser.h"
 
 class BluetoothChooserController;
@@ -29,10 +30,18 @@ class BluetoothChooserDesktop : public content::BluetoothChooser {
                          bool is_paired,
                          int signal_strength_level) override;
 
+  // Sets a reference to the bubble being displayed so that it can be closed if
+  // this object is destroyed.
+  void set_bubble(base::WeakPtr<BubbleController> bubble) {
+    bubble_ = std::move(bubble);
+  }
+
  private:
   // Weak. DeviceChooserContentView[Cocoa] owns it.
   BluetoothChooserController* bluetooth_chooser_controller_;
 
+  base::WeakPtr<BubbleController> bubble_;
+
   DISALLOW_COPY_AND_ASSIGN(BluetoothChooserDesktop);
 };
 

chrome/browser/ui/browser.cc

@@ -1350,6 +1350,7 @@ std::unique_ptr<content::BluetoothChooser> Browser::RunBluetoothChooser(
       WebContents::FromRenderFrameHost(frame));
   BubbleReference bubble_reference = browser->GetBubbleManager()->ShowBubble(
       std::move(chooser_bubble_delegate));
+  bluetooth_chooser_desktop->set_bubble(std::move(bubble_reference));
 
   return std::move(bluetooth_chooser_desktop);
 }

chrome/browser/usb/usb_browsertest.cc

@@ -21,6 +21,7 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "device/base/mock_device_client.h"
 #include "device/usb/mock_usb_device.h"
 #include "device/usb/mock_usb_service.h"
@@ -87,12 +88,21 @@ class TestContentBrowserClient : public ChromeContentBrowserClient {
   void CreateUsbChooserService(
       content::RenderFrameHost* render_frame_host,
       device::mojom::UsbChooserServiceRequest request) override {
-    mojo::MakeStrongBinding(
-        std::make_unique<FakeChooserService>(render_frame_host),
-        std::move(request));
+    if (use_real_chooser_) {
+      ChromeContentBrowserClient::CreateUsbChooserService(render_frame_host,
+                                                          std::move(request));
+    } else {
+      mojo::MakeStrongBinding(
+          std::make_unique<FakeChooserService>(render_frame_host),
+          std::move(request));
+    }
   }
 
+  void UseRealChooser() { use_real_chooser_ = true; }
+
  private:
+  bool use_real_chooser_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(TestContentBrowserClient);
 };
 
@@ -140,6 +150,8 @@ class WebUsbTest : public InProcessBrowserTest {
 
   const GURL& origin() { return origin_; }
 
+  void UseRealChooser() { test_content_browser_client_.UseRealChooser(); }
+
  private:
   std::unique_ptr<MockDeviceClient> device_client_;
   scoped_refptr<MockUsbDevice> mock_device_;
@@ -287,4 +299,22 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, AddRemoveDeviceEphemeral) {
   EXPECT_EQ("", result);
 }
 
+IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) {
+  UseRealChooser();
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  content::TestNavigationObserver observer(
+      web_contents, 1 /* number_of_navigations */,
+      content::MessageLoopRunner::QuitMode::DEFERRED);
+
+  EXPECT_TRUE(content::ExecuteScript(
+      web_contents,
+      "navigator.usb.requestDevice({ filters: [] });"
+      "document.location.href = \"https://google.com\";"));
+
+  observer.Wait();
+  EXPECT_EQ(0u, browser()->GetBubbleManager()->GetBubbleCountForTesting());
+}
+
 }  // namespace

chrome/browser/usb/usb_tab_helper.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/usb/web_usb_permission_provider.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/common/content_features.h"
 #include "device/usb/mojo/device_manager_impl.h"
@@ -113,6 +114,13 @@ void UsbTabHelper::RenderFrameDeleted(RenderFrameHost* render_frame_host) {
   NotifyTabStateChanged();
 }
 
+void UsbTabHelper::DidFinishNavigation(content::NavigationHandle* handle) {
+  if (handle->HasCommitted() && !handle->IsSameDocument()) {
+    frame_usb_services_.erase(handle->GetRenderFrameHost());
+    NotifyTabStateChanged();
+  }
+}
+
 FrameUsbServices* UsbTabHelper::GetFrameUsbService(
     content::RenderFrameHost* render_frame_host) {
   FrameUsbServicesMap::const_iterator it =

chrome/browser/usb/usb_tab_helper.h

@@ -55,6 +55,7 @@ class UsbTabHelper : public content::WebContentsObserver,
 
   // content::WebContentsObserver overrides:
   void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
+  void DidFinishNavigation(content::NavigationHandle* handle) override;
 
   FrameUsbServices* GetFrameUsbService(
       content::RenderFrameHost* render_frame_host);

chrome/browser/web_bluetooth_browsertest.cc

@@ -18,6 +18,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
 
@@ -178,4 +179,31 @@ IN_PROC_BROWSER_TEST_F(WebBluetoothTest, BlocklistShouldBlock) {
               testing::MatchesRegex("SecurityError: .*blocklisted UUID.*"));
 }
 
+IN_PROC_BROWSER_TEST_F(WebBluetoothTest, NavigateWithChooserCrossOrigin) {
+  // Fake the BluetoothAdapter to say it's present.
+  scoped_refptr<device::MockBluetoothAdapter> adapter =
+      new testing::NiceMock<device::MockBluetoothAdapter>;
+  EXPECT_CALL(*adapter, IsPresent()).WillRepeatedly(Return(true));
+  auto bt_global_values =
+      device::BluetoothAdapterFactory::Get().InitGlobalValuesForTesting();
+  bt_global_values->SetLESupported(true);
+  device::BluetoothAdapterFactory::SetAdapterForTesting(adapter);
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  content::TestNavigationObserver observer(
+      web_contents, 1 /* number_of_navigations */,
+      content::MessageLoopRunner::QuitMode::DEFERRED);
+
+  EXPECT_TRUE(content::ExecuteScript(
+      web_contents,
+      "navigator.bluetooth.requestDevice({filters: [{name: 'Hello'}]});"
+      "document.location.href = \"https://google.com\";"));
+
+  observer.Wait();
+  EXPECT_EQ(0u, browser()->GetBubbleManager()->GetBubbleCountForTesting());
+  EXPECT_EQ(GURL("https://google.com"), web_contents->GetLastCommittedURL());
+}
+
 }  // namespace

components/bubble/bubble_manager.cc

@@ -81,6 +81,10 @@ void BubbleManager::RemoveBubbleManagerObserver(
   observers_.RemoveObserver(observer);
 }
 
+size_t BubbleManager::GetBubbleCountForTesting() const {
+  return controllers_.size();
+}
+
 void BubbleManager::FinalizePendingRequests() {
   // Return if already "Finalized".
   if (manager_state_ == NO_MORE_BUBBLES)

components/bubble/bubble_manager.h

@@ -70,6 +70,9 @@ class BubbleManager {
   // Remove an observer from this BubbleManager.
   void RemoveBubbleManagerObserver(BubbleManagerObserver* observer);
 
+  // Returns the number of bubbles currently being managed.
+  size_t GetBubbleCountForTesting() const;
+
  protected:
   // Will close any open bubbles and prevent new ones from being shown.
   void FinalizePendingRequests();

content/browser/bluetooth/web_bluetooth_service_impl.cc

@@ -1242,6 +1242,10 @@ BluetoothAllowedDevices& WebBluetoothServiceImpl::allowed_devices() {
 }
 
 void WebBluetoothServiceImpl::ClearState() {
+  // Releasing the adapter will drop references to callbacks that have not yet
+  // been executed. The binding must be closed first so that this is allowed.
+  binding_.Close();
+
   characteristic_id_to_notify_session_.clear();
   pending_primary_services_requests_.clear();
   descriptor_id_to_characteristic_id_.clear();