Apply Occam's razor to U2fDiscovery.

No production code is calling U2fDiscovery::Stop, and all observer
method implementations are no-ops.

Similarly, no U2fDiscovery has more than a single observer added, and
the only time that observer is removed is when both the discovery and
that observer (the owner) dies.

It sounds like we are not going to make use of this functionality in the
near future, so this CL removes:
 -- the ability to Stop() U2fDiscoveries, and
 -- the ability to have more than a single Observer per U2fDiscovery.

The CL also moves slightly more functionality into the U2fDiscovery base
class from derived classes, most importantly with the goal of making
sure the fake and production implementations function consistently in
the following areas:
 -- U2fDiscovery now keeps track of whether Start NotifyDiscoveryStarted
    have been called, and exposes corresponding getters.
 -- It is now enforced that Start() and NotifyDiscoveryStarted() can
    be called once each, and in this order, as well as that
    NotifyDeviceAdded cannot be called before Start() is called.
 -- It is now enforced that Observer::DiscoveryStarted is never invoked
    synchronously while Start() is still on the call stack.

Bug: 822244, 823686
Change-Id: Ia029646d43481a1a2ca58452577727f96d6aade1
Reviewed-on: https://chromium-review.googlesource.com/964301
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544379}

device/fido/fake_fido_discovery.cc

@@ -6,7 +6,11 @@
 
 #include <utility>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace device {
@@ -15,69 +19,32 @@ namespace test {
 // FakeFidoDiscovery ----------------------------------------------------------
 
 FakeFidoDiscovery::FakeFidoDiscovery(U2fTransportProtocol transport,
-                                     StartStopMode mode)
-    : transport_(transport),
-      mode_(mode),
-      start_called_callback_(wait_for_start_loop_.QuitClosure()),
-      stop_called_callback_(wait_for_stop_loop_.QuitClosure()) {}
+                                     StartMode mode)
+    : FidoDiscovery(transport), mode_(mode) {}
 FakeFidoDiscovery::~FakeFidoDiscovery() = default;
 
 void FakeFidoDiscovery::WaitForCallToStart() {
   wait_for_start_loop_.Run();
-  ASSERT_FALSE(start_called_callback_);
-}
-
-void FakeFidoDiscovery::WaitForCallToStop() {
-  wait_for_stop_loop_.Run();
-  ASSERT_FALSE(stop_called_callback_);
 }
 
 void FakeFidoDiscovery::SimulateStarted(bool success) {
-  ASSERT_FALSE(is_running_);
-  is_running_ = success;
+  ASSERT_FALSE(is_running());
   NotifyDiscoveryStarted(success);
 }
 
-void FakeFidoDiscovery::SimulateStopped(bool success) {
-  ASSERT_TRUE(is_running_);
-  is_running_ = !success;
-  NotifyDiscoveryStopped(success);
-}
-
 void FakeFidoDiscovery::WaitForCallToStartAndSimulateSuccess() {
   WaitForCallToStart();
   SimulateStarted(true /* success */);
 }
 
-void FakeFidoDiscovery::WaitForCallToStopAndSimulateSuccess() {
-  WaitForCallToStop();
-  SimulateStopped(true /* success */);
-}
-
-U2fTransportProtocol FakeFidoDiscovery::GetTransportProtocol() const {
-  return transport_;
-}
-
-void FakeFidoDiscovery::Start() {
-  if (is_running_)
-    return;
+void FakeFidoDiscovery::StartInternal() {
+  wait_for_start_loop_.Quit();
 
-  ASSERT_TRUE(start_called_callback_) << "Start called twice.";
-  std::move(start_called_callback_).Run();
-
-  if (mode_ == StartStopMode::kAutomatic)
-    SimulateStarted(true /* success */);
-}
-
-void FakeFidoDiscovery::Stop() {
-  if (!is_running_)
-    return;
-
-  ASSERT_TRUE(stop_called_callback_) << "Stop called twice.";
-  std::move(stop_called_callback_).Run();
-
-  if (mode_ == StartStopMode::kAutomatic)
-    SimulateStopped(true /* success */);
+  if (mode_ == StartMode::kAutomatic) {
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&FakeFidoDiscovery::SimulateStarted,
+                                  base::Unretained(this), true /* success */));
+  }
 }
 
 // ScopedFakeFidoDiscoveryFactory ---------------------------------------------
@@ -86,14 +53,14 @@ ScopedFakeFidoDiscoveryFactory::ScopedFakeFidoDiscoveryFactory() = default;
 ScopedFakeFidoDiscoveryFactory::~ScopedFakeFidoDiscoveryFactory() = default;
 
 FakeFidoDiscovery* ScopedFakeFidoDiscoveryFactory::ForgeNextHidDiscovery(
-    FakeFidoDiscovery::StartStopMode mode) {
+    FakeFidoDiscovery::StartMode mode) {
   next_hid_discovery_ = std::make_unique<FakeFidoDiscovery>(
       U2fTransportProtocol::kUsbHumanInterfaceDevice, mode);
   return next_hid_discovery_.get();
 }
 
 FakeFidoDiscovery* ScopedFakeFidoDiscoveryFactory::ForgeNextBleDiscovery(
-    FakeFidoDiscovery::StartStopMode mode) {
+    FakeFidoDiscovery::StartMode mode) {
   next_ble_discovery_ = std::make_unique<FakeFidoDiscovery>(
       U2fTransportProtocol::kBluetoothLowEnergy, mode);
   return next_ble_discovery_.get();

device/fido/fake_fido_discovery.h

@@ -51,66 +51,44 @@ namespace test {
 //   // Add devices discovered after doing some heavy lifting.
 //   fake_hid_discovery->AddDevice(std::make_unique<MockFidoDevice>(...));
 //
-//   // Run the production code that will eventually stop the discovery.
-//   //// hid_instance->Stop();
-//
-//   // Wait for discovery to be stopped by the production code, and simulate
-//   // the discovery starting successfully.
-//   fake_hid_discovery->WaitForCallToStopAndSimulateSuccess();
+//   // Destroy the production instance to eventually stop the discovery.
+//   // hid_instance.reset();
 //
 class FakeFidoDiscovery : public FidoDiscovery,
                           public base::SupportsWeakPtr<FakeFidoDiscovery> {
  public:
-  enum class StartStopMode {
-    // SimulateStarted()/SimualteStopped() needs to be called manually after the
-    // production code calls Start()/Stop().
+  enum class StartMode {
+    // SimulateStarted() needs to be called manually to finish starting the
+    // discovery after the production code calls Start().
     kManual,
-    // The discovery is automatically and successfully started/stopped once
-    // Start()/Stop() is called.
+    // The discovery is automatically (successfully) started after Start().
     kAutomatic
   };
 
   explicit FakeFidoDiscovery(U2fTransportProtocol transport,
-                             StartStopMode mode = StartStopMode::kManual);
+                             StartMode mode = StartMode::kManual);
   ~FakeFidoDiscovery() override;
 
-  // Blocks until start/stop is requested.
+  // Blocks until start is requested.
   void WaitForCallToStart();
-  void WaitForCallToStop();
 
-  // Simulates the discovery actually starting/stopping.
+  // Simulates the discovery actually starting.
   void SimulateStarted(bool success);
-  void SimulateStopped(bool success);
 
-  // Combines WaitForCallToStart/Stop + SimulateStarted/Stopped(true).
+  // Combines WaitForCallToStart + SimulateStarted(true).
   void WaitForCallToStartAndSimulateSuccess();
-  void WaitForCallToStopAndSimulateSuccess();
-
-  bool is_start_requested() const { return !start_called_callback_; }
-  bool is_stop_requested() const { return !stop_called_callback_; }
-  bool is_running() const { return is_running_; }
 
   // Tests are to directly call Add/RemoveDevice to simulate adding/removing
   // devices. Observers are automatically notified.
   using FidoDiscovery::AddDevice;
   using FidoDiscovery::RemoveDevice;
 
-  // FidoDiscovery:
-  U2fTransportProtocol GetTransportProtocol() const override;
-  void Start() override;
-  void Stop() override;
-
  private:
-  const U2fTransportProtocol transport_;
-
-  const StartStopMode mode_;
-  bool is_running_ = false;
+  // FidoDiscovery:
+  void StartInternal() override;
 
+  const StartMode mode_;
   base::RunLoop wait_for_start_loop_;
-  base::RunLoop wait_for_stop_loop_;
-
-  base::OnceClosure start_called_callback_;
-  base::OnceClosure stop_called_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeFidoDiscovery);
 };
@@ -120,7 +98,7 @@ class FakeFidoDiscovery : public FidoDiscovery,
 class ScopedFakeFidoDiscoveryFactory
     : public ::device::internal::ScopedFidoDiscoveryFactory {
  public:
-  using StartStopMode = FakeFidoDiscovery::StartStopMode;
+  using StartMode = FakeFidoDiscovery::StartMode;
 
   ScopedFakeFidoDiscoveryFactory();
   ~ScopedFakeFidoDiscoveryFactory() override;
@@ -131,10 +109,8 @@ class ScopedFakeFidoDiscoveryFactory
   //
   // It is an error not to call the relevant method prior to a call to
   // FidoDiscovery::Create with the respective transport.
-  FakeFidoDiscovery* ForgeNextHidDiscovery(
-      StartStopMode mode = StartStopMode::kManual);
-  FakeFidoDiscovery* ForgeNextBleDiscovery(
-      StartStopMode mode = StartStopMode::kManual);
+  FakeFidoDiscovery* ForgeNextHidDiscovery(StartMode mode = StartMode::kManual);
+  FakeFidoDiscovery* ForgeNextBleDiscovery(StartMode mode = StartMode::kManual);
 
  protected:
   std::unique_ptr<FidoDiscovery> CreateFidoDiscovery(

device/fido/fake_fido_discovery_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "device/fido/fake_fido_discovery.h"
 
+#include <utility>
+
 #include "base/macros.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_task_environment.h"
@@ -34,12 +36,12 @@ using ScopedFakeFidoDiscoveryFactoryTest = FakeFidoDiscoveryTest;
 TEST_F(FakeFidoDiscoveryTest, Transport) {
   FakeFidoDiscovery discovery_ble(U2fTransportProtocol::kBluetoothLowEnergy);
   EXPECT_EQ(U2fTransportProtocol::kBluetoothLowEnergy,
-            discovery_ble.GetTransportProtocol());
+            discovery_ble.transport());
 
   FakeFidoDiscovery discovery_hid(
       U2fTransportProtocol::kUsbHumanInterfaceDevice);
   EXPECT_EQ(U2fTransportProtocol::kUsbHumanInterfaceDevice,
-            discovery_hid.GetTransportProtocol());
+            discovery_hid.transport());
 }
 
 TEST_F(FakeFidoDiscoveryTest, InitialState) {
@@ -47,49 +49,30 @@ TEST_F(FakeFidoDiscoveryTest, InitialState) {
 
   ASSERT_FALSE(discovery.is_running());
   ASSERT_FALSE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
 }
 
-TEST_F(FakeFidoDiscoveryTest, StartStopDiscovery) {
+TEST_F(FakeFidoDiscoveryTest, StartDiscovery) {
   FakeFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
 
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   discovery.Start();
 
-  ASSERT_FALSE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
+  ASSERT_FALSE(discovery.is_running());
 
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
   discovery.WaitForCallToStartAndSimulateSuccess();
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
   ASSERT_TRUE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
-
-  discovery.Stop();
-
-  ASSERT_TRUE(discovery.is_running());
-  ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_TRUE(discovery.is_stop_requested());
-
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, true));
-  discovery.WaitForCallToStopAndSimulateSuccess();
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
-  ASSERT_FALSE(discovery.is_running());
-  ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_TRUE(discovery.is_stop_requested());
 }
 
 TEST_F(FakeFidoDiscoveryTest, WaitThenStartStopDiscovery) {
   FakeFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
 
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() { discovery.Start(); }));
@@ -98,32 +81,11 @@ TEST_F(FakeFidoDiscoveryTest, WaitThenStartStopDiscovery) {
 
   ASSERT_FALSE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
 
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
   discovery.SimulateStarted(true);
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
   ASSERT_TRUE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
-
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindLambdaForTesting([&]() { discovery.Stop(); }));
-
-  discovery.WaitForCallToStop();
-
-  ASSERT_TRUE(discovery.is_running());
-  ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_TRUE(discovery.is_stop_requested());
-
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, true));
-  discovery.SimulateStopped(true);
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
-  ASSERT_FALSE(discovery.is_running());
-  ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_TRUE(discovery.is_stop_requested());
 }
 
 // Starting discovery and failing: instance stays in "not running" state
@@ -131,45 +93,17 @@ TEST_F(FakeFidoDiscoveryTest, StartFail) {
   FakeFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
 
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   discovery.Start();
 
-  EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
-  discovery.SimulateStarted(false);
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
   ASSERT_FALSE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
-}
-
-// Stopping discovery and failing: instance stays in a "running" state.
-TEST_F(FakeFidoDiscoveryTest, StopFail) {
-  FakeFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
-
-  MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
-
-  discovery.Start();
-
-  EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
-  discovery.SimulateStarted(true);
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
-  ASSERT_TRUE(discovery.is_running());
-  ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_FALSE(discovery.is_stop_requested());
-
-  discovery.Stop();
 
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, false));
-  discovery.SimulateStopped(false);
-  ::testing::Mock::VerifyAndClearExpectations(&observer);
-
-  ASSERT_TRUE(discovery.is_running());
+  EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
+  discovery.SimulateStarted(false);
+  ASSERT_FALSE(discovery.is_running());
   ASSERT_TRUE(discovery.is_start_requested());
-  ASSERT_TRUE(discovery.is_stop_requested());
 }
 
 // Adding device is possible both before and after discovery actually starts.
@@ -177,7 +111,7 @@ TEST_F(FakeFidoDiscoveryTest, AddDevice) {
   FakeFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
 
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   discovery.Start();
 
@@ -204,7 +138,7 @@ TEST_F(ScopedFakeFidoDiscoveryFactoryTest,
   ScopedFakeFidoDiscoveryFactory factory;
   auto* injected_fake_discovery = factory.ForgeNextHidDiscovery();
   ASSERT_EQ(U2fTransportProtocol::kUsbHumanInterfaceDevice,
-            injected_fake_discovery->GetTransportProtocol());
+            injected_fake_discovery->transport());
   auto produced_discovery = FidoDiscovery::Create(
       U2fTransportProtocol::kUsbHumanInterfaceDevice, nullptr);
   EXPECT_TRUE(produced_discovery);
@@ -218,14 +152,14 @@ TEST_F(ScopedFakeFidoDiscoveryFactoryTest,
 
   auto* injected_fake_discovery_1 = factory.ForgeNextBleDiscovery();
   ASSERT_EQ(U2fTransportProtocol::kBluetoothLowEnergy,
-            injected_fake_discovery_1->GetTransportProtocol());
+            injected_fake_discovery_1->transport());
   auto produced_discovery_1 =
       FidoDiscovery::Create(U2fTransportProtocol::kBluetoothLowEnergy, nullptr);
   EXPECT_EQ(injected_fake_discovery_1, produced_discovery_1.get());
 
   auto* injected_fake_discovery_2 = factory.ForgeNextBleDiscovery();
   ASSERT_EQ(U2fTransportProtocol::kBluetoothLowEnergy,
-            injected_fake_discovery_2->GetTransportProtocol());
+            injected_fake_discovery_2->transport());
   auto produced_discovery_2 =
       FidoDiscovery::Create(U2fTransportProtocol::kBluetoothLowEnergy, nullptr);
   EXPECT_EQ(injected_fake_discovery_2, produced_discovery_2.get());

device/fido/fido_ble_discovery.cc

@@ -8,8 +8,11 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/location.h"
 #include "base/stl_util.h"
 #include "base/strings/string_piece.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_common.h"
 #include "device/bluetooth/bluetooth_discovery_filter.h"
@@ -20,37 +23,14 @@
 
 namespace device {
 
-FidoBleDiscovery::FidoBleDiscovery() : weak_factory_(this) {}
-
+FidoBleDiscovery::FidoBleDiscovery()
+    : FidoDiscovery(U2fTransportProtocol::kBluetoothLowEnergy),
+      weak_factory_(this) {}
 FidoBleDiscovery::~FidoBleDiscovery() {
   if (adapter_)
     adapter_->RemoveObserver(this);
 
-  // Pretend we are able to successfully stop a discovery session in case it is
-  // still present.
-  if (discovery_session_)
-    OnStopped(true);
-}
-
-U2fTransportProtocol FidoBleDiscovery::GetTransportProtocol() const {
-  return U2fTransportProtocol::kBluetoothLowEnergy;
-}
-
-void FidoBleDiscovery::Start() {
-  auto& factory = BluetoothAdapterFactory::Get();
-  factory.GetAdapter(
-      base::Bind(&FidoBleDiscovery::OnGetAdapter, weak_factory_.GetWeakPtr()));
-}
-
-void FidoBleDiscovery::Stop() {
-  DCHECK(adapter_);
-  adapter_->RemoveObserver(this);
-
-  DCHECK(discovery_session_);
-  discovery_session_->Stop(base::Bind(&FidoBleDiscovery::OnStopped,
-                                      weak_factory_.GetWeakPtr(), true),
-                           base::Bind(&FidoBleDiscovery::OnStopped,
-                                      weak_factory_.GetWeakPtr(), false));
+  // Destroying |discovery_session_| will best-effort-stop discovering.
 }
 
 // static
@@ -117,6 +97,26 @@ void FidoBleDiscovery::OnStartDiscoverySessionWithFilterError() {
   NotifyDiscoveryStarted(false);
 }
 
+void FidoBleDiscovery::StartInternal() {
+  auto& factory = BluetoothAdapterFactory::Get();
+  auto callback = base::BindRepeating(&FidoBleDiscovery::OnGetAdapter,
+                                      weak_factory_.GetWeakPtr());
+#if defined(OS_MACOSX)
+  // BluetoothAdapter may invoke the callback synchronously on Mac, but
+  // StartInternal() never wants to invoke to NotifyDiscoveryStarted()
+  // immediately, so ensure there is at least post-task at this bottleneck.
+  // See: https://crbug.com/823686.
+  callback = base::BindRepeating(
+      [](BluetoothAdapterFactory::AdapterCallback callback,
+         scoped_refptr<BluetoothAdapter> adapter) {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::BindRepeating(callback, adapter));
+      },
+      std::move(callback));
+#endif  // defined(OS_MACOSX)
+  factory.GetAdapter(std::move(callback));
+}
+
 void FidoBleDiscovery::DeviceAdded(BluetoothAdapter* adapter,
                                    BluetoothDevice* device) {
   if (base::ContainsKey(device->GetUUIDs(), FidoServiceUUID())) {
@@ -143,9 +143,4 @@ void FidoBleDiscovery::DeviceRemoved(BluetoothAdapter* adapter,
   }
 }
 
-void FidoBleDiscovery::OnStopped(bool success) {
-  discovery_session_.reset();
-  NotifyDiscoveryStopped(success);
-}
-
 }  // namespace device

device/fido/fido_ble_discovery.h

@@ -27,11 +27,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscovery
   FidoBleDiscovery();
   ~FidoBleDiscovery() override;
 
-  // FidoDiscovery:
-  U2fTransportProtocol GetTransportProtocol() const override;
-  void Start() override;
-  void Stop() override;
-
  private:
   static const BluetoothUUID& FidoServiceUUID();
 
@@ -42,13 +37,15 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoBleDiscovery
       std::unique_ptr<BluetoothDiscoverySession>);
   void OnStartDiscoverySessionWithFilterError();
 
+  // FidoDiscovery:
+  void StartInternal() override;
+
   // BluetoothAdapter::Observer:
   void DeviceAdded(BluetoothAdapter* adapter, BluetoothDevice* device) override;
   void DeviceChanged(BluetoothAdapter* adapter,
                      BluetoothDevice* device) override;
   void DeviceRemoved(BluetoothAdapter* adapter,
                      BluetoothDevice* device) override;
-  void OnStopped(bool success);
 
   scoped_refptr<BluetoothAdapter> adapter_;
   std::unique_ptr<BluetoothDiscoverySession> discovery_session_;

device/fido/fido_ble_discovery_unittest.cc

@@ -45,9 +45,8 @@ TEST_F(BluetoothTest, FidoBleDiscoveryNoAdapter) {
 
   // We don't expect any calls to the notification methods.
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, _)).Times(0);
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, _)).Times(0);
   EXPECT_CALL(observer, DeviceAdded(&discovery, _)).Times(0);
   EXPECT_CALL(observer, DeviceRemoved(&discovery, _)).Times(0);
 }
@@ -63,8 +62,9 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsKnownDevice) {
   SimulateLowEnergyDevice(7);
 
   FidoBleDiscovery discovery;
+
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   {
     base::RunLoop run_loop;
@@ -78,19 +78,6 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsKnownDevice) {
     discovery.Start();
     run_loop.Run();
   }
-
-  // TODO(crbug/763303): Delete device and check OnDeviceDeleted invocation.
-
-  {
-    base::RunLoop run_loop;
-    auto quit = run_loop.QuitClosure();
-
-    EXPECT_CALL(observer, DiscoveryStopped(&discovery, true))
-        .WillOnce(ReturnFromAsyncCall(quit));
-
-    discovery.Stop();
-    run_loop.Run();
-  }
 }
 
 TEST_F(BluetoothTest, FidoBleDiscoveryFindsNewDevice) {
@@ -102,7 +89,7 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsNewDevice) {
 
   FidoBleDiscovery discovery;
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   {
     base::RunLoop run_loop;
@@ -127,17 +114,6 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsNewDevice) {
 
     run_loop.Run();
   }
-
-  // TODO(crbug/763303): Delete device and check OnDeviceDeleted invocation.
-
-  {
-    base::RunLoop run_loop;
-    auto quit = run_loop.QuitClosure();
-    EXPECT_CALL(observer, DiscoveryStopped(&discovery, true))
-        .WillOnce(ReturnFromAsyncCall(quit));
-    discovery.Stop();
-    run_loop.Run();
-  }
 }
 
 // Simulate the scenario where the BLE device is already known at start-up time,
@@ -156,7 +132,7 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsUpdatedDevice) {
 
   FidoBleDiscovery discovery;
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
   {
     base::RunLoop run_loop;
@@ -188,17 +164,6 @@ TEST_F(BluetoothTest, FidoBleDiscoveryFindsUpdatedDevice) {
     EXPECT_EQ(FidoBleDevice::GetId(BluetoothTestBase::kTestDeviceAddress1),
               devices[0]->GetId());
   }
-
-  // TODO(crbug/763303): Delete device and check OnDeviceDeleted invocation.
-
-  {
-    base::RunLoop run_loop;
-    auto quit = run_loop.QuitClosure();
-    EXPECT_CALL(observer, DiscoveryStopped(&discovery, true))
-        .WillOnce(ReturnFromAsyncCall(quit));
-    discovery.Stop();
-    run_loop.Run();
-  }
 }
 
 }  // namespace device

device/fido/fido_discovery.cc

@@ -6,7 +6,6 @@
 
 #include <utility>
 
-#include "base/logging.h"
 #include "build/build_config.h"
 #include "device/fido/fido_ble_discovery.h"
 #include "device/fido/fido_device.h"
@@ -39,7 +38,7 @@ std::unique_ptr<FidoDiscovery> CreateFidoDiscoveryImpl(
   }
 
   DCHECK(discovery);
-  DCHECK_EQ(discovery->GetTransportProtocol(), transport);
+  DCHECK_EQ(discovery->transport(), transport);
   return discovery;
 }
 
@@ -58,36 +57,40 @@ std::unique_ptr<FidoDiscovery> FidoDiscovery::Create(
   return (*g_factory_func_)(transport, connector);
 }
 
-FidoDiscovery::FidoDiscovery() = default;
+FidoDiscovery::FidoDiscovery(U2fTransportProtocol transport)
+    : transport_(transport) {}
 
 FidoDiscovery::~FidoDiscovery() = default;
 
-void FidoDiscovery::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void FidoDiscovery::RemoveObserver(Observer* observer) {
-  observers_.RemoveObserver(observer);
+void FidoDiscovery::Start() {
+  DCHECK_EQ(state_, State::kIdle);
+  state_ = State::kStarting;
+  StartInternal();
+  // StartInternal should never synchronously call NotifyStarted().
+  DCHECK_EQ(state_, State::kStarting);
 }
 
 void FidoDiscovery::NotifyDiscoveryStarted(bool success) {
-  for (auto& observer : observers_)
-    observer.DiscoveryStarted(this, success);
-}
-
-void FidoDiscovery::NotifyDiscoveryStopped(bool success) {
-  for (auto& observer : observers_)
-    observer.DiscoveryStopped(this, success);
+  DCHECK_EQ(state_, State::kStarting);
+  if (success)
+    state_ = State::kRunning;
+  if (!observer_)
+    return;
+  observer_->DiscoveryStarted(this, success);
 }
 
 void FidoDiscovery::NotifyDeviceAdded(FidoDevice* device) {
-  for (auto& observer : observers_)
-    observer.DeviceAdded(this, device);
+  DCHECK_NE(state_, State::kIdle);
+  if (!observer_)
+    return;
+  observer_->DeviceAdded(this, device);
 }
 
 void FidoDiscovery::NotifyDeviceRemoved(FidoDevice* device) {
-  for (auto& observer : observers_)
-    observer.DeviceRemoved(this, device);
+  DCHECK_NE(state_, State::kIdle);
+  if (!observer_)
+    return;
+  observer_->DeviceRemoved(this, device);
 }
 
 std::vector<FidoDevice*> FidoDiscovery::GetDevices() {

device/fido/fido_discovery.h

@@ -12,9 +12,9 @@
 #include <vector>
 
 #include "base/component_export.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "base/observer_list.h"
 #include "base/strings/string_piece.h"
 #include "device/fido/u2f_transport_protocol.h"
 
@@ -32,11 +32,23 @@ class ScopedFidoDiscoveryFactory;
 
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
  public:
+  enum class State {
+    kIdle,
+    kStarting,
+    kRunning,
+  };
+
   class COMPONENT_EXPORT(DEVICE_FIDO) Observer {
    public:
     virtual ~Observer();
+
+    // It is guaranteed that this is never invoked synchronously from Start().
     virtual void DiscoveryStarted(FidoDiscovery* discovery, bool success) = 0;
-    virtual void DiscoveryStopped(FidoDiscovery* discovery, bool success) = 0;
+
+    // It is guaranteed that DeviceAdded/DeviceRemoved() will not be invoked
+    // before the client of FidoDiscovery calls FidoDiscovery::Start(). However,
+    // for devices already known to the system at that point, DeviceAdded()
+    // might already be called to reported already known devices.
     virtual void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) = 0;
     virtual void DeviceRemoved(FidoDiscovery* discovery,
                                FidoDevice* device) = 0;
@@ -53,17 +65,17 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
 
   virtual ~FidoDiscovery();
 
-  virtual U2fTransportProtocol GetTransportProtocol() const = 0;
-  virtual void Start() = 0;
-  virtual void Stop() = 0;
+  Observer* observer() const { return observer_; }
+  void set_observer(Observer* observer) {
+    DCHECK(!observer_ || !observer) << "Only one observer is supported.";
+    observer_ = observer;
+  }
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  U2fTransportProtocol transport() const { return transport_; }
+  bool is_start_requested() const { return state_ != State::kIdle; }
+  bool is_running() const { return state_ == State::kRunning; }
 
-  void NotifyDiscoveryStarted(bool success);
-  void NotifyDiscoveryStopped(bool success);
-  void NotifyDeviceAdded(FidoDevice* device);
-  void NotifyDeviceRemoved(FidoDevice* device);
+  void Start();
 
   std::vector<FidoDevice*> GetDevices();
   std::vector<const FidoDevice*> GetDevices() const;
@@ -72,13 +84,24 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
   const FidoDevice* GetDevice(base::StringPiece device_id) const;
 
  protected:
-  FidoDiscovery();
+  FidoDiscovery(U2fTransportProtocol transport);
 
-  virtual bool AddDevice(std::unique_ptr<FidoDevice> device);
-  virtual bool RemoveDevice(base::StringPiece device_id);
+  void NotifyDiscoveryStarted(bool success);
+  void NotifyDeviceAdded(FidoDevice* device);
+  void NotifyDeviceRemoved(FidoDevice* device);
+
+  bool AddDevice(std::unique_ptr<FidoDevice> device);
+  bool RemoveDevice(base::StringPiece device_id);
+
+  // Subclasses should implement this to actually start the discovery when it is
+  // requested.
+  //
+  // The implementation should asynchronously invoke NotifyDiscoveryStarted when
+  // the discovery is s tarted.
+  virtual void StartInternal() = 0;
 
   std::map<std::string, std::unique_ptr<FidoDevice>, std::less<>> devices_;
-  base::ObserverList<Observer> observers_;
+  Observer* observer_ = nullptr;
 
  private:
   friend class internal::ScopedFidoDiscoveryFactory;
@@ -87,6 +110,9 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoDiscovery {
   using FactoryFuncPtr = decltype(&Create);
   static FactoryFuncPtr g_factory_func_;
 
+  const U2fTransportProtocol transport_;
+  State state_ = State::kIdle;
+
   DISALLOW_COPY_AND_ASSIGN(FidoDiscovery);
 };
 

device/fido/fido_discovery_unittest.cc

@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "device/fido/fido_discovery.h"
+
 #include <utility>
 
 #include "base/macros.h"
-#include "device/fido/fido_discovery.h"
 #include "device/fido/mock_fido_device.h"
 #include "device/fido/mock_fido_discovery_observer.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -22,20 +23,18 @@ using ::testing::UnorderedElementsAre;
 // A minimal implementation of FidoDiscovery that is no longer abstract.
 class ConcreteFidoDiscovery : public FidoDiscovery {
  public:
-  ConcreteFidoDiscovery() = default;
+  explicit ConcreteFidoDiscovery(U2fTransportProtocol transport)
+      : FidoDiscovery(transport) {}
   ~ConcreteFidoDiscovery() override = default;
 
+  MOCK_METHOD0(StartInternal, void());
+
   using FidoDiscovery::AddDevice;
   using FidoDiscovery::RemoveDevice;
 
-  const base::ObserverList<Observer>& observers() const { return observers_; }
-
- protected:
-  U2fTransportProtocol GetTransportProtocol() const override {
-    return U2fTransportProtocol::kUsbHumanInterfaceDevice;
-  }
-  void Start() override {}
-  void Stop() override {}
+  using FidoDiscovery::NotifyDiscoveryStarted;
+  using FidoDiscovery::NotifyDeviceAdded;
+  using FidoDiscovery::NotifyDeviceRemoved;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ConcreteFidoDiscovery);
@@ -44,46 +43,57 @@ class ConcreteFidoDiscovery : public FidoDiscovery {
 }  // namespace
 
 TEST(FidoDiscoveryTest, TestAddAndRemoveObserver) {
-  ConcreteFidoDiscovery discovery;
+  ConcreteFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
-  EXPECT_FALSE(discovery.observers().HasObserver(&observer));
-  discovery.AddObserver(&observer);
+  EXPECT_EQ(nullptr, discovery.observer());
 
-  EXPECT_TRUE(discovery.observers().HasObserver(&observer));
+  discovery.set_observer(&observer);
+  EXPECT_EQ(&observer, discovery.observer());
 
-  discovery.RemoveObserver(&observer);
-  EXPECT_FALSE(discovery.observers().HasObserver(&observer));
+  discovery.set_observer(nullptr);
+  EXPECT_EQ(nullptr, discovery.observer());
 }
 
-TEST(FidoDiscoveryTest, TestNotifications) {
-  ConcreteFidoDiscovery discovery;
+TEST(FidoDiscoveryTest, TestNotificationsOnSuccessfulStart) {
+  ConcreteFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
 
-  EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
-  discovery.NotifyDiscoveryStarted(true);
+  EXPECT_FALSE(discovery.is_start_requested());
+  EXPECT_FALSE(discovery.is_running());
 
-  EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
-  discovery.NotifyDiscoveryStarted(false);
+  EXPECT_CALL(discovery, StartInternal());
+  discovery.Start();
+  EXPECT_TRUE(discovery.is_start_requested());
+  EXPECT_FALSE(discovery.is_running());
+  ::testing::Mock::VerifyAndClearExpectations(&discovery);
 
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, true));
-  discovery.NotifyDiscoveryStopped(true);
+  EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
+  discovery.NotifyDiscoveryStarted(true);
+  EXPECT_TRUE(discovery.is_start_requested());
+  EXPECT_TRUE(discovery.is_running());
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
+}
 
-  EXPECT_CALL(observer, DiscoveryStopped(&discovery, false));
-  discovery.NotifyDiscoveryStopped(false);
+TEST(FidoDiscoveryTest, TestNotificationsOnFailedStart) {
+  ConcreteFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
+  MockFidoDiscoveryObserver observer;
+  discovery.set_observer(&observer);
 
-  MockFidoDevice device;
-  EXPECT_CALL(observer, DeviceAdded(&discovery, &device));
-  discovery.NotifyDeviceAdded(&device);
+  discovery.Start();
 
-  EXPECT_CALL(observer, DeviceRemoved(&discovery, &device));
-  discovery.NotifyDeviceRemoved(&device);
+  EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
+  discovery.NotifyDiscoveryStarted(false);
+  EXPECT_TRUE(discovery.is_start_requested());
+  EXPECT_FALSE(discovery.is_running());
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
 TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
-  ConcreteFidoDiscovery discovery;
+  ConcreteFidoDiscovery discovery(U2fTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
+  discovery.Start();
 
   // Expect successful insertion.
   auto device0 = std::make_unique<MockFidoDevice>();
@@ -91,6 +101,7 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   EXPECT_CALL(observer, DeviceAdded(&discovery, device0_raw));
   EXPECT_CALL(*device0, GetId()).WillOnce(Return("device0"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device0)));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   // // Expect successful insertion.
   auto device1 = std::make_unique<MockFidoDevice>();
@@ -98,12 +109,14 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   EXPECT_CALL(observer, DeviceAdded(&discovery, device1_raw));
   EXPECT_CALL(*device1, GetId()).WillOnce(Return("device1"));
   EXPECT_TRUE(discovery.AddDevice(std::move(device1)));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   // Inserting a device with an already present id should be prevented.
   auto device1_dup = std::make_unique<MockFidoDevice>();
   EXPECT_CALL(observer, DeviceAdded(_, _)).Times(0);
   EXPECT_CALL(*device1_dup, GetId()).WillOnce(Return("device1"));
   EXPECT_FALSE(discovery.AddDevice(std::move(device1_dup)));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   EXPECT_EQ(device0_raw, discovery.GetDevice("device0"));
   EXPECT_EQ(device1_raw, discovery.GetDevice("device1"));
@@ -119,12 +132,15 @@ TEST(FidoDiscoveryTest, TestAddRemoveDevices) {
   // Trying to remove a non-present device should fail.
   EXPECT_CALL(observer, DeviceRemoved(_, _)).Times(0);
   EXPECT_FALSE(discovery.RemoveDevice("device2"));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   EXPECT_CALL(observer, DeviceRemoved(&discovery, device1_raw));
   EXPECT_TRUE(discovery.RemoveDevice("device1"));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   EXPECT_CALL(observer, DeviceRemoved(&discovery, device0_raw));
   EXPECT_TRUE(discovery.RemoveDevice("device0"));
+  ::testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
 }  // namespace device

device/fido/fido_hid_discovery.cc

@@ -14,18 +14,17 @@
 namespace device {
 
 FidoHidDiscovery::FidoHidDiscovery(::service_manager::Connector* connector)
-    : connector_(connector), binding_(this), weak_factory_(this) {
+    : FidoDiscovery(U2fTransportProtocol::kUsbHumanInterfaceDevice),
+      connector_(connector),
+      binding_(this),
+      weak_factory_(this) {
   // TODO(piperc@): Give this constant a name.
   filter_.SetUsagePage(0xf1d0);
 }
 
 FidoHidDiscovery::~FidoHidDiscovery() = default;
 
-U2fTransportProtocol FidoHidDiscovery::GetTransportProtocol() const {
-  return U2fTransportProtocol::kUsbHumanInterfaceDevice;
-}
-
-void FidoHidDiscovery::Start() {
+void FidoHidDiscovery::StartInternal() {
   DCHECK(connector_);
   connector_->BindInterface(device::mojom::kServiceName,
                             mojo::MakeRequest(&hid_manager_));
@@ -36,10 +35,6 @@ void FidoHidDiscovery::Start() {
       std::move(client), base::BindOnce(&FidoHidDiscovery::OnGetDevices,
                                         weak_factory_.GetWeakPtr()));
 }
-void FidoHidDiscovery::Stop() {
-  binding_.Unbind();
-  NotifyDiscoveryStopped(true);
-}
 
 void FidoHidDiscovery::DeviceAdded(
     device::mojom::HidDeviceInfoPtr device_info) {

device/fido/fido_hid_discovery.h

@@ -32,12 +32,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDiscovery
   explicit FidoHidDiscovery(::service_manager::Connector* connector);
   ~FidoHidDiscovery() override;
 
+ private:
   // FidoDiscovery:
-  U2fTransportProtocol GetTransportProtocol() const override;
-  void Start() override;
-  void Stop() override;
+  void StartInternal() override;
 
- private:
   // device::mojom::HidManagerClient implementation:
   void DeviceAdded(device::mojom::HidDeviceInfoPtr device_info) override;
   void DeviceRemoved(device::mojom::HidDeviceInfoPtr device_info) override;

device/fido/fido_hid_discovery_unittest.cc

@@ -86,7 +86,7 @@ TEST_F(FidoHidDiscoveryTest, TestAddRemoveDevice) {
   fake_hid_manager_->AddDevice(MakeFidoHidDevice("known"));
 
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, true));
-  discovery.AddObserver(&observer);
+  discovery.set_observer(&observer);
   discovery.Start();
 
   // Devices initially known to the service before discovery started should be

device/fido/scoped_virtual_u2f_device.cc

@@ -4,7 +4,13 @@
 
 #include "device/fido/scoped_virtual_u2f_device.h"
 
+#include <utility>
+
 #include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 
 namespace device {
 namespace test {
@@ -12,19 +18,22 @@ namespace test {
 // A FidoDiscovery that always vends a single |VirtualU2fDevice|.
 class VirtualU2fDeviceDiscovery : public FidoDiscovery {
  public:
+  VirtualU2fDeviceDiscovery()
+      : FidoDiscovery(U2fTransportProtocol::kUsbHumanInterfaceDevice) {}
   ~VirtualU2fDeviceDiscovery() override = default;
 
-  U2fTransportProtocol GetTransportProtocol() const override {
-    return U2fTransportProtocol::kUsbHumanInterfaceDevice;
-  }
-
-  void Start() override {
+ protected:
+  void StartInternal() override {
     auto device = std::make_unique<VirtualU2fDevice>();
     AddDevice(std::move(device));
-    NotifyDiscoveryStarted(true);
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&VirtualU2fDeviceDiscovery::NotifyDiscoveryStarted,
+                       base::Unretained(this), true /* success */));
   }
 
-  void Stop() override { NotifyDiscoveryStopped(true); }
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VirtualU2fDeviceDiscovery);
 };
 
 ScopedVirtualU2fDevice::ScopedVirtualU2fDevice() = default;

device/fido/u2f_request.cc

@@ -35,15 +35,12 @@ U2fRequest::U2fRequest(service_manager::Connector* connector,
       // non-HID transports are configured.
       continue;
     }
-    discovery->AddObserver(this);
+    discovery->set_observer(this);
     discoveries_.push_back(std::move(discovery));
   }
 }
 
-U2fRequest::~U2fRequest() {
-  for (auto& discovery : discoveries_)
-    discovery->RemoveObserver(this);
-}
+U2fRequest::~U2fRequest() = default;
 
 void U2fRequest::Start() {
   if (state_ == State::INIT) {
@@ -178,30 +175,28 @@ void U2fRequest::OnDeviceVersionRequest(
 }
 
 void U2fRequest::DiscoveryStarted(FidoDiscovery* discovery, bool success) {
+#if DCHECK_IS_ON()
   if (success) {
-    // The discovery might know about devices that have already been added to
-    // the system. Add them to the |devices_| list if we don't already know
-    // about them. This case could happen if a DeviceAdded() event is emitted
-    // before DiscoveryStarted() is invoked. In that case both the U2fRequest
-    // and the FidoDiscovery already know about the just added device.
-    std::set<std::string> current_device_ids;
+    // FidoDiscovery::Observer::DeviceAdded should have been already dispatched
+    // for each of devices already known by |discovery|, so we should never
+    // learn anything new here.
+    std::set<std::string> device_ids_known_to_request;
     for (const auto* device : attempted_devices_)
-      current_device_ids.insert(device->GetId());
+      device_ids_known_to_request.insert(device->GetId());
     if (current_device_)
-      current_device_ids.insert(current_device_->GetId());
+      device_ids_known_to_request.insert(current_device_->GetId());
     for (const auto* device : devices_)
-      current_device_ids.insert(device->GetId());
+      device_ids_known_to_request.insert(device->GetId());
 
-    auto new_devices = discovery->GetDevices();
-    std::copy_if(
-        new_devices.begin(), new_devices.end(), std::back_inserter(devices_),
-        [&current_device_ids](FidoDevice* device) {
-          return !base::ContainsKey(current_device_ids, device->GetId());
-        });
+    std::set<std::string> device_ids_from_newly_started_discovery;
+    for (const auto* device : discovery->GetDevices())
+      device_ids_from_newly_started_discovery.insert(device->GetId());
+    DCHECK(base::STLIncludes(device_ids_known_to_request,
+                             device_ids_from_newly_started_discovery));
   }
+#endif
 
   started_count_++;
-
   if ((state_ == State::IDLE || state_ == State::OFF) &&
       (success || started_count_ == discoveries_.size())) {
     state_ = State::IDLE;
@@ -209,8 +204,6 @@ void U2fRequest::DiscoveryStarted(FidoDiscovery* discovery, bool success) {
   }
 }
 
-void U2fRequest::DiscoveryStopped(FidoDiscovery* discovery, bool success) {}
-
 void U2fRequest::DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) {
   devices_.push_back(device);
 

device/fido/u2f_request.h

@@ -15,6 +15,7 @@
 #include "base/cancelable_callback.h"
 #include "base/component_export.h"
 #include "base/containers/flat_set.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "device/fido/fido_constants.h"
@@ -114,7 +115,6 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest
 
   // FidoDiscovery::Observer
   void DiscoveryStarted(FidoDiscovery* discovery, bool success) override;
-  void DiscoveryStopped(FidoDiscovery* discovery, bool success) override;
   void DeviceAdded(FidoDiscovery* discovery, FidoDevice* device) override;
   void DeviceRemoved(FidoDiscovery* discovery, FidoDevice* device) override;