fido: stop discoveries before the end of a WebAuthn request

This adds a FidoDiscovery::Stop() method that can be used to terminate
authenticator discovery without destroying the instance.

FidoDeviceDiscovery upon receiving Stop() will ignore all calls to
AddDevice() and RemoveDevice(). FidoCableDiscovery inherits this
behavior and also calls Unregister() on all pending BleAdvertisements.

Lastly, AuthenticatorCommon is changed to stop all discoveries of the
current FidoRequestHandler prior to signaling failure of the request to
the UI layer.

This fixes an issue where Chrome might continue to advertise caBLE EIDs
while the UI is showing an error sheet, e.g. because the request timed
out or because one of the authenticator returned an error that causes
the UI to show its retry error sheet.

Bug: 1046164
Change-Id: Ic5678f13a10efbb56277e6bd3485dd985c8ab0c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036266
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739073}

content/browser/webauth/authenticator_common.cc

@@ -1475,11 +1475,16 @@ void AuthenticatorCommon::SignalFailureToRequestDelegate(
     blink::mojom::AuthenticatorStatus status) {
   error_awaiting_user_acknowledgement_ = status;
 
+  // The request has failed, but the UI may delay resolution of the request
+  // callback and cleanup of the FidoRequestHandler and its associated
+  // discoveries and authenticators. Tell them to stop processing the request in
+  // the meantime.
+  request_->StopDiscoveries();
+  request_->CancelActiveAuthenticators();
+
   // If WebAuthnUi is enabled, this error blocks until after receiving user
   // acknowledgement. Otherwise, the error is returned right away.
   if (request_delegate_->DoesBlockRequestOnFailure(reason)) {
-    // Cancel pending authenticator requests before the error dialog is shown.
-    request_->CancelActiveAuthenticators();
     return;
   }
   CancelWithStatus(error_awaiting_user_acknowledgement_);

device/fido/cable/fido_cable_discovery.cc

@@ -304,10 +304,11 @@ FidoCableDiscovery::FidoCableDiscovery(
 #endif
 }
 
-// This is a workaround for https://crbug.com/846522
 FidoCableDiscovery::~FidoCableDiscovery() {
-  for (auto advertisement : advertisements_)
+  // Work around dangling advertisement references. (crbug/846522)
+  for (auto advertisement : advertisements_) {
     advertisement.second->Unregister(base::DoNothing(), base::DoNothing());
+  }
 }
 
 base::Optional<std::unique_ptr<FidoCableHandshakeHandler>>
@@ -462,17 +463,27 @@ void FidoCableDiscovery::StartAdvertisement() {
         base::AdaptCallbackForRepeating(
             base::BindOnce(&FidoCableDiscovery::OnAdvertisementRegistered,
                            weak_factory_.GetWeakPtr(), data.v1->client_eid)),
-        base::AdaptCallbackForRepeating(
-            base::BindOnce(&FidoCableDiscovery::OnAdvertisementRegisterError,
-                           weak_factory_.GetWeakPtr())));
+        base::BindRepeating([](BluetoothAdvertisement::ErrorCode error_code) {
+          FIDO_LOG(ERROR) << "Failed to register advertisement: " << error_code;
+        }));
   }
 }
 
 void FidoCableDiscovery::StopAdvertisements(base::OnceClosure callback) {
+  // Destructing a BluetoothAdvertisement invokes its Unregister() method, but
+  // there may be references to the advertisement outside this
+  // FidoCableDiscovery (see e.g. crbug/846522). Hence, merely clearing
+  // |advertisements_| is not sufficient; we need to manually invoke
+  // Unregister() for every advertisement in order to stop them. On the other
+  // hand, |advertisements_| must not be cleared before the Unregister()
+  // callbacks return either, in case we do hold the only reference to a
+  // BluetoothAdvertisement.
   FIDO_LOG(DEBUG) << "Stopping " << advertisements_.size()
                   << " caBLE advertisements";
-  auto barrier_closure =
-      base::BarrierClosure(advertisements_.size(), std::move(callback));
+  auto barrier_closure = base::BarrierClosure(
+      advertisements_.size(),
+      base::BindOnce(&FidoCableDiscovery::OnAdvertisementsStopped,
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
   auto error_closure = base::BindRepeating(
       [](base::RepeatingClosure cb, BluetoothAdvertisement::ErrorCode code) {
         FIDO_LOG(ERROR) << "BluetoothAdvertisement::Unregister() failed: "
@@ -483,27 +494,21 @@ void FidoCableDiscovery::StopAdvertisements(base::OnceClosure callback) {
   for (auto advertisement : advertisements_) {
     advertisement.second->Unregister(barrier_closure, error_closure);
   }
+}
 
-#if !defined(OS_WIN)
-  // On Windows the discovery is the only owner of the advertisements, meaning
-  // the advertisements would be destroyed before |barrier_closure| could be
-  // invoked.
+void FidoCableDiscovery::OnAdvertisementsStopped(base::OnceClosure callback) {
+  FIDO_LOG(DEBUG) << "Advertisements stopped";
   advertisements_.clear();
-#endif  // !defined(OS_WIN)
+  std::move(callback).Run();
 }
 
 void FidoCableDiscovery::OnAdvertisementRegistered(
     const CableEidArray& client_eid,
     scoped_refptr<BluetoothAdvertisement> advertisement) {
-  FIDO_LOG(DEBUG) << "Advertisement registered.";
+  FIDO_LOG(DEBUG) << "Advertisement registered";
   advertisements_.emplace(client_eid, std::move(advertisement));
 }
 
-void FidoCableDiscovery::OnAdvertisementRegisterError(
-    BluetoothAdvertisement::ErrorCode error_code) {
-  FIDO_LOG(ERROR) << "Failed to register advertisement: " << error_code;
-}
-
 void FidoCableDiscovery::CableDeviceFound(BluetoothAdapter* adapter,
                                           BluetoothDevice* device) {
   const std::string device_address = device->GetAddress();
@@ -784,4 +789,12 @@ std::string FidoCableDiscovery::ResultDebugString(
   return ret;
 }
 
+bool FidoCableDiscovery::MaybeStop() {
+  if (!FidoBleDiscoveryBase::MaybeStop()) {
+    NOTREACHED();
+  }
+  StopAdvertisements(base::DoNothing());
+  return true;
+}
+
 }  // namespace device

device/fido/cable/fido_cable_discovery.h

@@ -39,6 +39,14 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
           pairing_callback);
   ~FidoCableDiscovery() override;
 
+  // FidoDeviceDiscovery:
+  bool MaybeStop() override;
+
+  const std::map<CableEidArray, scoped_refptr<BluetoothAdvertisement>>&
+  AdvertisementsForTesting() const {
+    return advertisements_;
+  }
+
  protected:
   virtual base::Optional<std::unique_ptr<FidoCableHandshakeHandler>>
   CreateHandshakeHandler(FidoCableDevice* device,
@@ -78,11 +86,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
     std::vector<CableEidArray> uuids;
   };
 
-  FRIEND_TEST_ALL_PREFIXES(FidoCableDiscoveryTest,
-                           TestDiscoveryWithAdvertisementFailures);
-  FRIEND_TEST_ALL_PREFIXES(FidoCableDiscoveryTest,
-                           TestUnregisterAdvertisementUponDestruction);
-
   // BluetoothAdapter::Observer:
   void DeviceAdded(BluetoothAdapter* adapter, BluetoothDevice* device) override;
   void DeviceChanged(BluetoothAdapter* adapter,
@@ -101,12 +104,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
   void OnAdvertisementRegistered(
       const CableEidArray& client_eid,
       scoped_refptr<BluetoothAdvertisement> advertisement);
-  void OnAdvertisementRegisterError(
-      BluetoothAdvertisement::ErrorCode error_code);
+
   // Attempt to stop all on-going advertisements in best-effort basis.
   // Once all the callbacks for Unregister() function is received, invoke
   // |callback|.
   void StopAdvertisements(base::OnceClosure callback);
+  void OnAdvertisementsStopped(base::OnceClosure callback);
   void CableDeviceFound(BluetoothAdapter* adapter, BluetoothDevice* device);
   void ConductEncryptionHandshake(std::unique_ptr<FidoCableDevice> cable_device,
                                   Result discovery_data);
@@ -138,8 +141,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoCableDiscovery
   // not completely effective.
   std::set<std::string> active_devices_;
   base::Optional<QRGeneratorKey> qr_generator_key_;
+
+  // Note that on Windows, |advertisements_| is the only reference holder of
+  // BluetoothAdvertisement.
   std::map<CableEidArray, scoped_refptr<BluetoothAdvertisement>>
       advertisements_;
+
   std::vector<std::unique_ptr<FidoCableHandshakeHandler>>
       cable_handshake_handlers_;
   base::Optional<

device/fido/cable/fido_cable_discovery_unittest.cc

@@ -11,6 +11,8 @@
 #include "base/containers/span.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/test/task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/bluetooth_advertisement.h"
@@ -149,6 +151,15 @@ class CableMockBluetoothAdvertisement : public BluetoothAdvertisement {
                void(const SuccessCallback& success_callback,
                     const ErrorCallback& error_callback));
 
+  void ExpectUnregisterAndSucceed() {
+    EXPECT_CALL(*this, Unregister(_, _))
+        .WillOnce(
+            ::testing::WithArg<0>(::testing::Invoke([](const auto& success_cb) {
+              base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                            success_cb);
+            })));
+  }
+
  private:
   ~CableMockBluetoothAdvertisement() override = default;
 };
@@ -549,15 +560,14 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {
   BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter);
   cable_discovery->Start();
   task_environment_.FastForwardUntilNoTasksRemain();
-  EXPECT_TRUE(cable_discovery->advertisements_.empty());
+  EXPECT_TRUE(cable_discovery->AdvertisementsForTesting().empty());
 }
 #endif  // !defined(OS_WIN)
 
 TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
   auto cable_discovery = CreateDiscovery();
-  CableMockBluetoothAdvertisement* advertisement =
-      new CableMockBluetoothAdvertisement();
-  EXPECT_CALL(*advertisement, Unregister(_, _)).Times(1);
+  auto advertisement = base::MakeRefCounted<CableMockBluetoothAdvertisement>();
+  advertisement->ExpectUnregisterAndSucceed();
 
   auto mock_adapter =
       base::MakeRefCounted<::testing::NiceMock<CableMockAdapter>>();
@@ -565,16 +575,59 @@ TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
   mock_adapter->ExpectDiscoveryWithScanCallback();
   mock_adapter->ExpectRegisterAdvertisementWithResponse(
       true /* simulate_success */, kClientEid, kUuidFormattedClientEid,
-      Sequence(), base::WrapRefCounted(advertisement));
+      Sequence(), std::move(advertisement));
 
   BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter);
   cable_discovery->Start();
   task_environment_.FastForwardUntilNoTasksRemain();
 
-  EXPECT_EQ(1u, cable_discovery->advertisements_.size());
+  EXPECT_EQ(1u, cable_discovery->AdvertisementsForTesting().size());
   cable_discovery.reset();
 }
 
+TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponStop) {
+  auto cable_discovery = CreateDiscovery();
+  auto advertisement = base::MakeRefCounted<CableMockBluetoothAdvertisement>();
+  advertisement->ExpectUnregisterAndSucceed();
+
+  auto mock_adapter =
+      base::MakeRefCounted<::testing::NiceMock<CableMockAdapter>>();
+  mock_adapter->ExpectSuccessCallbackToIsPowered();
+  mock_adapter->ExpectDiscoveryWithScanCallback();
+  mock_adapter->ExpectRegisterAdvertisementWithResponse(
+      true /* simulate_success */, kClientEid, kUuidFormattedClientEid,
+      Sequence(), std::move(advertisement));
+
+  BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter);
+  cable_discovery->Start();
+  task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(1u, cable_discovery->AdvertisementsForTesting().size());
+
+  EXPECT_TRUE(cable_discovery->MaybeStop());
+  task_environment_.FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(0u, cable_discovery->AdvertisementsForTesting().size());
+}
+
+TEST_F(FidoCableDiscoveryTest, TestStopWithNoAdvertisementsSucceeds) {
+  auto mock_adapter =
+      base::MakeRefCounted<::testing::NiceMock<CableMockAdapter>>();
+  EXPECT_CALL(*mock_adapter, IsPresent()).WillOnce(::testing::Return(true));
+  EXPECT_CALL(*mock_adapter, IsPowered()).WillOnce(::testing::Return(false));
+  BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter);
+
+  auto cable_discovery = CreateDiscovery();
+  NiceMock<MockFidoDiscoveryObserver> mock_observer;
+  EXPECT_CALL(mock_observer,
+              DiscoveryStarted(cable_discovery.get(), true,
+                               std::vector<FidoAuthenticator*>()));
+  cable_discovery->set_observer(&mock_observer);
+  cable_discovery->Start();
+  task_environment_.FastForwardUntilNoTasksRemain();
+
+  EXPECT_EQ(0u, cable_discovery->AdvertisementsForTesting().size());
+  EXPECT_TRUE(cable_discovery->MaybeStop());
+}
+
 // Tests that cable discovery resumes after Bluetooth adapter is powered on.
 TEST_F(FidoCableDiscoveryTest, TestResumeDiscoveryAfterPoweredOn) {
   auto cable_discovery = CreateDiscovery();

device/fido/fido_device_discovery.cc

@@ -33,7 +33,10 @@ void FidoDeviceDiscovery::Start() {
 }
 
 void FidoDeviceDiscovery::NotifyDiscoveryStarted(bool success) {
-  DCHECK_EQ(state_, State::kStarting);
+  if (state_ == State::kStopped)
+    return;
+
+  DCHECK(state_ == State::kStarting);
   if (success)
     state_ = State::kRunning;
   if (!observer())
@@ -92,6 +95,9 @@ FidoDeviceAuthenticator* FidoDeviceDiscovery::GetAuthenticator(
 }
 
 bool FidoDeviceDiscovery::AddDevice(std::unique_ptr<FidoDevice> device) {
+  if (state_ == State::kStopped)
+    return false;
+
   auto authenticator =
       std::make_unique<FidoDeviceAuthenticator>(std::move(device));
   const auto result =
@@ -105,6 +111,9 @@ bool FidoDeviceDiscovery::AddDevice(std::unique_ptr<FidoDevice> device) {
 }
 
 bool FidoDeviceDiscovery::RemoveDevice(base::StringPiece device_id) {
+  if (state_ == State::kStopped)
+    return false;
+
   auto found = authenticators_.find(device_id);
   if (found == authenticators_.end())
     return false;
@@ -115,4 +124,9 @@ bool FidoDeviceDiscovery::RemoveDevice(base::StringPiece device_id) {
   return true;
 }
 
+bool FidoDeviceDiscovery::MaybeStop() {
+  state_ = State::kStopped;
+  return true;
+}
+
 }  // namespace device

device/fido/fido_device_discovery.h

@@ -32,6 +32,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceDiscovery
     kIdle,
     kStarting,
     kRunning,
+    kStopped,
   };
 
   ~FidoDeviceDiscovery() override;
@@ -47,13 +48,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceDiscovery
 
   // FidoDiscoveryBase:
   void Start() override;
+  bool MaybeStop() override;
 
  protected:
   FidoDeviceDiscovery(FidoTransportProtocol transport);
 
   void NotifyDiscoveryStarted(bool success);
-  void NotifyAuthenticatorAdded(FidoAuthenticator* authenticator);
-  void NotifyAuthenticatorRemoved(FidoAuthenticator* authenticator);
 
   bool AddDevice(std::unique_ptr<FidoDevice> device);
   bool RemoveDevice(base::StringPiece device_id);
@@ -73,6 +73,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDeviceDiscovery
       authenticators_;
 
  private:
+  void NotifyAuthenticatorAdded(FidoAuthenticator* authenticator);
+  void NotifyAuthenticatorRemoved(FidoAuthenticator* authenticator);
+
   State state_ = State::kIdle;
   base::WeakPtrFactory<FidoDeviceDiscovery> weak_factory_{this};
 

device/fido/fido_discovery_base.cc

@@ -10,4 +10,8 @@ FidoDiscoveryBase::FidoDiscoveryBase(FidoTransportProtocol transport)
     : transport_(transport) {}
 FidoDiscoveryBase::~FidoDiscoveryBase() = default;
 
+bool FidoDiscoveryBase::MaybeStop() {
+  return false;
+}
+
 }  // namespace device

device/fido/fido_discovery_base.h

@@ -56,6 +56,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscoveryBase {
   // this method.
   virtual void Start() = 0;
 
+  // Attempts to stop discovery and returns whether stopping was successful.
+  virtual bool MaybeStop();
+
   Observer* observer() const { return observer_; }
   void set_observer(Observer* observer) {
     DCHECK(!observer_ || !observer) << "Only one observer is supported.";

device/fido/fido_request_handler_base.cc

@@ -352,4 +352,10 @@ void FidoRequestHandlerBase::ConstructBleAdapterPowerManager() {
   bluetooth_adapter_manager_ = std::make_unique<BleAdapterManager>(this);
 }
 
+void FidoRequestHandlerBase::StopDiscoveries() {
+  for (const auto& discovery : discoveries_) {
+    discovery->MaybeStop();
+  }
+}
+
 }  // namespace device

device/fido/fido_request_handler_base.h

@@ -212,6 +212,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoRequestHandlerBase
     return bluetooth_adapter_manager_;
   }
 
+  void StopDiscoveries();
+
  protected:
   // Subclasses implement this method to dispatch their request onto the given
   // FidoAuthenticator. The FidoAuthenticator is owned by this