Start FidoDeviceDiscovery asynchronously

Callback for BluetoothAdapterFactory::GetAdapter() may be invoked
synchronously for ChromeOS devices. This results in discovery of
FidoBleDevices that were already known to BluetoothAdapter not being
notified to UI embedder, and WebAuthN UI would not show new Bluetooth
devices on device selection view --even when FidoBleDevices are
discovered. To avoid such hairpinning, invoke
FidoDeviceDiscovery::StartInternal asynchronously.

Bug: 823686
Change-Id: Idc6c4d1d12d76a15118f15366e734325ba1819c0
Reviewed-on: https://chromium-review.googlesource.com/c/1300798
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603216}

device/fido/ble/fido_ble_discovery_base.cc

@@ -83,23 +83,9 @@ void FidoBleDiscoveryBase::OnGetAdapter(
 }
 
 void FidoBleDiscoveryBase::StartInternal() {
-  auto& factory = BluetoothAdapterFactory::Get();
-  auto callback = base::BindOnce(&FidoBleDiscoveryBase::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::BindOnce(
-      [](BluetoothAdapterFactory::AdapterCallback callback,
-         scoped_refptr<BluetoothAdapter> adapter) {
-        base::ThreadTaskRunnerHandle::Get()->PostTask(
-            FROM_HERE, base::BindOnce(std::move(callback), adapter));
-      },
-      base::AdaptCallbackForRepeating(std::move(callback)));
-#endif  // defined(OS_MACOSX)
-  factory.GetAdapter(base::AdaptCallbackForRepeating(std::move(callback)));
+  BluetoothAdapterFactory::Get().GetAdapter(
+      base::AdaptCallbackForRepeating(base::BindOnce(
+          &FidoBleDiscoveryBase::OnGetAdapter, weak_factory_.GetWeakPtr())));
 }
 
 }  // namespace device

device/fido/ble/fido_ble_discovery_unittest.cc

@@ -120,6 +120,7 @@ TEST_F(FidoBleDiscoveryTest,
   EXPECT_CALL(*adapter(), SetPowered).Times(0);
   EXPECT_CALL(*observer(), DiscoveryStarted(discovery(), false));
   discovery()->Start();
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
 }
 
 TEST_F(FidoBleDiscoveryTest, FidoBleDiscoveryResumeScanningAfterPoweredOn) {
@@ -131,6 +132,7 @@ TEST_F(FidoBleDiscoveryTest, FidoBleDiscoveryResumeScanningAfterPoweredOn) {
   // starts again.
   EXPECT_CALL(*adapter(), StartDiscoverySessionWithFilterRaw);
   discovery()->Start();
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
   adapter()->NotifyAdapterPoweredChanged(true);
 }
 
@@ -305,6 +307,7 @@ TEST_F(FidoBleDiscoveryTest,
   EXPECT_CALL(*observer(), AuthenticatorIdChanged(discovery(), kAuthenticatorId,
                                                   kAuthenticatorChangedId));
   discovery()->Start();
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
 
   adapter()->NotifyDeviceChanged(mock_device.get());
   ASSERT_TRUE(::testing::Mock::VerifyAndClearExpectations(mock_device.get()));
@@ -328,6 +331,7 @@ TEST_F(FidoBleDiscoveryTest, DiscoveryNotifiesObserverWhenDeviceInPairingMode) {
 
   const auto device_id = FidoBleDevice::GetId(kDeviceAddress);
   discovery()->Start();
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
 
   ::testing::InSequence sequence;
   EXPECT_CALL(*observer(),
@@ -350,6 +354,7 @@ TEST_F(FidoBleDiscoveryTest,
 
   const auto device_id = FidoBleDevice::GetId(kDeviceAddress);
   discovery()->Start();
+  scoped_task_environment_.FastForwardUntilNoTasksRemain();
 
   ::testing::InSequence sequence;
   EXPECT_CALL(*observer(),

device/fido/fido_device_discovery.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "build/build_config.h"
 #include "device/fido/ble/fido_ble_discovery.h"
 #include "device/fido/cable/fido_cable_discovery.h"
@@ -90,10 +91,12 @@ FidoDeviceDiscovery::~FidoDeviceDiscovery() = default;
 void FidoDeviceDiscovery::Start() {
   DCHECK_EQ(state_, State::kIdle);
   state_ = State::kStarting;
-  // TODO(hongjunchoi): Fix so that NotifiyStarted() is never called
-  // synchronously after StartInternal().
-  // See: https://crbug.com/823686
-  StartInternal();
+
+  // To ensure that that NotifiyStarted() is never invoked synchronously,
+  // post task asynchronously.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(&FidoDeviceDiscovery::StartInternal,
+                                weak_factory_.GetWeakPtr()));
 }
 
 void FidoDeviceDiscovery::NotifyDiscoveryStarted(bool success) {

device/fido/fido_device_discovery_unittest.cc

@@ -59,6 +59,8 @@ TEST(FidoDiscoveryTest, TestAddAndRemoveObserver) {
 }
 
 TEST(FidoDiscoveryTest, TestNotificationsOnSuccessfulStart) {
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+
   ConcreteFidoDiscovery discovery(FidoTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
   discovery.set_observer(&observer);
@@ -68,6 +70,8 @@ TEST(FidoDiscoveryTest, TestNotificationsOnSuccessfulStart) {
 
   EXPECT_CALL(discovery, StartInternal());
   discovery.Start();
+  scoped_task_environment_.RunUntilIdle();
+
   EXPECT_TRUE(discovery.is_start_requested());
   EXPECT_FALSE(discovery.is_running());
   ::testing::Mock::VerifyAndClearExpectations(&discovery);
@@ -80,11 +84,14 @@ TEST(FidoDiscoveryTest, TestNotificationsOnSuccessfulStart) {
 }
 
 TEST(FidoDiscoveryTest, TestNotificationsOnFailedStart) {
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+
   ConcreteFidoDiscovery discovery(FidoTransportProtocol::kBluetoothLowEnergy);
   MockFidoDiscoveryObserver observer;
   discovery.set_observer(&observer);
 
   discovery.Start();
+  scoped_task_environment_.RunUntilIdle();
 
   EXPECT_CALL(observer, DiscoveryStarted(&discovery, false));
   discovery.NotifyDiscoveryStarted(false);