diff --git a/device/fido/ble/fido_ble_discovery_base.cc b/device/fido/ble/fido_ble_discovery_base.cc index 5b0945316cc2b2..22c41154304468 100644 --- a/device/fido/ble/fido_ble_discovery_base.cc +++ b/device/fido/ble/fido_ble_discovery_base.cc @@ -39,18 +39,15 @@ const BluetoothUUID& FidoBleDiscoveryBase::CableAdvertisementUUID() { void FidoBleDiscoveryBase::OnStartDiscoverySessionWithFilter( std::unique_ptr session) { SetDiscoverySession(std::move(session)); - FIDO_LOG(DEBUG) << "Discovery session started."; - NotifyDiscoveryStarted(true); + FIDO_LOG(DEBUG) << "BLE discovery session started"; } void FidoBleDiscoveryBase::OnSetPoweredError() { - FIDO_LOG(ERROR) << "Failed to power on the adapter."; - NotifyDiscoveryStarted(false); + FIDO_LOG(ERROR) << "Failed to power on BLE adapter"; } void FidoBleDiscoveryBase::OnStartDiscoverySessionError() { - FIDO_LOG(ERROR) << "Discovery session not started."; - NotifyDiscoveryStarted(false); + FIDO_LOG(ERROR) << "Failed to start BLE discovery"; } void FidoBleDiscoveryBase::SetDiscoverySession( @@ -67,7 +64,7 @@ bool FidoBleDiscoveryBase::IsCableDevice(const BluetoothDevice* device) const { void FidoBleDiscoveryBase::OnGetAdapter( scoped_refptr adapter) { if (!adapter->IsPresent()) { - FIDO_LOG(DEBUG) << "bluetooth adapter is not available in current system."; + FIDO_LOG(DEBUG) << "No BLE adapter present"; NotifyDiscoveryStarted(false); return; } @@ -75,11 +72,19 @@ void FidoBleDiscoveryBase::OnGetAdapter( DCHECK(!adapter_); adapter_ = std::move(adapter); DCHECK(adapter_); - FIDO_LOG(DEBUG) << "Got adapter " << adapter_->GetAddress(); + FIDO_LOG(DEBUG) << "BLE adapter address " << adapter_->GetAddress(); adapter_->AddObserver(this); - if (adapter_->IsPowered()) + if (adapter_->IsPowered()) { OnSetPowered(); + } + + // FidoRequestHandlerBase blocks its transport availability callback on the + // DiscoveryStarted() calls of all instantiated discoveries. Hence, this call + // must not be put behind the BLE adapter getting powered on (which is + // dependent on the UI), or else the UI and this discovery will wait on each + // other indefinitely (see crbug.com/1018416). + NotifyDiscoveryStarted(true); } void FidoBleDiscoveryBase::StartInternal() { diff --git a/device/fido/cable/fido_cable_discovery.cc b/device/fido/cable/fido_cable_discovery.cc index 79c4bf53da3a0d..dbfc006053c8ca 100644 --- a/device/fido/cable/fido_cable_discovery.cc +++ b/device/fido/cable/fido_cable_discovery.cc @@ -447,13 +447,6 @@ void FidoCableDiscovery::StartAdvertisement() { base::BindOnce(&FidoCableDiscovery::OnAdvertisementRegisterError, weak_factory_.GetWeakPtr()))); } - - if (!advertisements_pending) { - // If no V1 extensions were provided then this discovery is ready - // immediately. Otherwise |NotifyDiscoveryStarted| will be called once - // all the advertising requests have been resolved. - NotifyDiscoveryStarted(true); - } } void FidoCableDiscovery::StopAdvertisements(base::OnceClosure callback) { @@ -489,14 +482,10 @@ void FidoCableDiscovery::OnAdvertisementRegisterError( void FidoCableDiscovery::RecordAdvertisementResult(bool is_success) { // If at least one advertisement succeeds, then notify discovery start. if (is_success) { - if (!advertisement_success_counter_++) - NotifyDiscoveryStarted(true); - return; + advertisement_success_counter_++; + } else { + advertisement_failure_counter_++; } - - // No advertisements succeeded, no point in continuing with Cable discovery. - if (++advertisement_failure_counter_ == discovery_data_.size()) - NotifyDiscoveryStarted(false); } void FidoCableDiscovery::CableDeviceFound(BluetoothAdapter* adapter, diff --git a/device/fido/cable/fido_cable_discovery_unittest.cc b/device/fido/cable/fido_cable_discovery_unittest.cc index d9b568eda33b8a..24364aade0ee66 100644 --- a/device/fido/cable/fido_cable_discovery_unittest.cc +++ b/device/fido/cable/fido_cable_discovery_unittest.cc @@ -25,8 +25,8 @@ #include "testing/gtest/include/gtest/gtest.h" using ::testing::_; -using ::testing::Sequence; using ::testing::NiceMock; +using ::testing::Sequence; namespace device { @@ -331,11 +331,55 @@ class FidoCableDiscoveryTest : public ::testing::Test { base::test::TaskEnvironment::TimeSource::MOCK_TIME}; }; +// Tests discovery without a BLE adapter. +TEST_F(FidoCableDiscoveryTest, TestDiscoveryFails) { + auto cable_discovery = CreateDiscovery(); + NiceMock mock_observer; + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), false, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0); + cable_discovery->set_observer(&mock_observer); + + auto mock_adapter = + base::MakeRefCounted<::testing::NiceMock>(); + EXPECT_CALL(*mock_adapter, IsPresent()).WillOnce(::testing::Return(false)); + + BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter); + cable_discovery->Start(); + task_environment_.FastForwardUntilNoTasksRemain(); +} + +// Tests discovery with a powered-off BLE adapter. Not calling +// DiscoveryStarted() in the case of a present-but-unpowered adapter leads to a +// deadlock between the discovery and the UI (see crbug.com/1018416). +TEST_F(FidoCableDiscoveryTest, TestDiscoveryStartedWithUnpoweredAdapter) { + auto cable_discovery = CreateDiscovery(); + NiceMock mock_observer; + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0); + cable_discovery->set_observer(&mock_observer); + + auto mock_adapter = + base::MakeRefCounted<::testing::NiceMock>(); + EXPECT_CALL(*mock_adapter, IsPresent()).WillOnce(::testing::Return(true)); + EXPECT_CALL(*mock_adapter, IsPowered()).WillOnce(::testing::Return(false)); + + BluetoothAdapterFactory::SetAdapterForTesting(mock_adapter); + cable_discovery->Start(); + task_environment_.FastForwardUntilNoTasksRemain(); +} + // Tests regular successful discovery flow for Cable device. TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) { auto cable_discovery = CreateDiscovery(); NiceMock mock_observer; - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1))); + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)); cable_discovery->set_observer(&mock_observer); auto mock_adapter = @@ -354,7 +398,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) { TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewAppleDevice) { auto cable_discovery = CreateDiscovery(); NiceMock mock_observer; - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1))); + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)); cable_discovery->set_observer(&mock_observer); auto mock_adapter = @@ -376,7 +423,8 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsIncorrectDevice) { auto cable_discovery = CreateDiscovery(); NiceMock mock_observer; EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0); - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::IsEmpty())); + EXPECT_CALL(mock_observer, DiscoveryStarted(cable_discovery.get(), true, + testing::IsEmpty())); cable_discovery->set_observer(&mock_observer); auto mock_adapter = @@ -414,7 +462,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) { mock_adapter->ExpectDiscoveryWithScanCallback(kAuthenticatorEid); NiceMock mock_observer; - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1))); + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)); cable_discovery->set_observer(&mock_observer); Sequence sequence; @@ -443,7 +494,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) { auto cable_discovery = std::make_unique(std::move(discovery_data)); NiceMock mock_observer; - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1))); + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)); cable_discovery->set_observer(&mock_observer); auto mock_adapter = @@ -476,7 +530,8 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) { NiceMock mock_observer; EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0); - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::IsEmpty())); + EXPECT_CALL(mock_observer, DiscoveryStarted(cable_discovery.get(), true, + testing::IsEmpty())); cable_discovery->set_observer(&mock_observer); auto mock_adapter = @@ -524,7 +579,10 @@ TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) { TEST_F(FidoCableDiscoveryTest, TestResumeDiscoveryAfterPoweredOn) { auto cable_discovery = CreateDiscovery(); NiceMock mock_observer; - EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1))); + EXPECT_CALL(mock_observer, + DiscoveryStarted(cable_discovery.get(), true, + std::vector())); + EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)); cable_discovery->set_observer(&mock_observer); auto mock_adapter = diff --git a/device/fido/fido_request_handler_base.cc b/device/fido/fido_request_handler_base.cc index 42775f630ff84b..e6216746eb9646 100644 --- a/device/fido/fido_request_handler_base.cc +++ b/device/fido/fido_request_handler_base.cc @@ -96,7 +96,7 @@ void FidoRequestHandlerBase::InitDiscoveries( // |notify_observer_callback_| is invoked once for each discovery once it is // ready, and additionally: // - // 1) [If BLE or caBLE are enabled] if Bluetooth adapter is present. + // 1) [If BLE or caBLE are enabled] once BLE adapters have been enumerated // 2) When |observer_| is set, so that OnTransportAvailabilityEnumerated is // never called before it is set. notify_observer_callback_ = base::BarrierClosure(