Skip to content

Commit

Permalink
fido: fix a deadlock in waiting for BLE discovery to start
Browse files Browse the repository at this point in the history 
CL:1800725 changed FidoRequestHandlerBase's |notify_observer_callback_|
barrier to block on each of the discoveries sending DiscoveryStarted()
before reporting transport availability to the UI layer. However, on a
machine with BLE available but powered off, FidoBleDiscoveryBase will
never invoke DiscoveryStarted() until the adapter is powered on.
Powering on the adapter is triggered by the UI after receiving the
transport availability callback. Hence, with a powered-off BLE adapter,
on requests instantiating the BLE discovery transport availability
callback was never invoked, and as a result the UI never appeared.

As it happens, the DiscoveryStarted() callback is rather redundant
anyway. It's important for the platform discoveries (TouchID, Windows)
as a way of ensuring their authenticators are added to the request
handler, before signaling transport availability to the UI. But for
Bluetooth specifically, there is a separate callback for determining
Bluetooth availability and power status that the barrier waits on.
Nothing the Bluetooth discovery does actually requires the UI to block.

Therefore, this change modifies FidoBleDiscoveryBase to invoke
DiscoveryStarted() right after enumerating Bluetooth adapters. As a side
effect, the boolean |success| argument to DiscoveryStarted() now only
signals whether the discovery found an adapter, whereas previously it
also indicated e.g. whether caBLE advertising succeeded. But that
doesn't really seem to matter since that argument is only ever consumed
in unit tests anyway.

Fixed: 1018416
Change-Id: I33549a24b1c399fee61374e44f5478f10d9db71d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1881974
Reviewed-by: Adam Langley <agl@chromium.org>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#711893}
  • Loading branch information
@kreichgauer
kreichgauer authored and Commit Bot committed Nov 1, 2019
1 parent 1672223 commit a9dd7a5
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 32 deletions.
23 changes: 14 additions & 9 deletions device/fido/ble/fido_ble_discovery_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,15 @@ const BluetoothUUID& FidoBleDiscoveryBase::CableAdvertisementUUID() {
void FidoBleDiscoveryBase::OnStartDiscoverySessionWithFilter(
std::unique_ptr<BluetoothDiscoverySession> 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(
Expand All @@ -67,19 +64,27 @@ bool FidoBleDiscoveryBase::IsCableDevice(const BluetoothDevice* device) const {
void FidoBleDiscoveryBase::OnGetAdapter(
scoped_refptr<BluetoothAdapter> 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;
}

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() {
Expand Down
17 changes: 3 additions & 14 deletions device/fido/cable/fido_cable_discovery.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
74 changes: 66 additions & 8 deletions device/fido/cable/fido_cable_discovery_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
#include "testing/gtest/include/gtest/gtest.h"

using ::testing::_;
using ::testing::Sequence;
using ::testing::NiceMock;
using ::testing::Sequence;

namespace device {

Expand Down Expand Up @@ -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<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), false,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
cable_discovery->set_observer(&mock_observer);

auto mock_adapter =
base::MakeRefCounted<::testing::NiceMock<CableMockAdapter>>();
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<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _)).Times(0);
cable_discovery->set_observer(&mock_observer);

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);
cable_discovery->Start();
task_environment_.FastForwardUntilNoTasksRemain();
}

// Tests regular successful discovery flow for Cable device.
TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
cable_discovery->set_observer(&mock_observer);

auto mock_adapter =
Expand All @@ -354,7 +398,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewDevice) {
TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsNewAppleDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
cable_discovery->set_observer(&mock_observer);

auto mock_adapter =
Expand All @@ -376,7 +423,8 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsIncorrectDevice) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> 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 =
Expand Down Expand Up @@ -414,7 +462,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) {
mock_adapter->ExpectDiscoveryWithScanCallback(kAuthenticatorEid);

NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
cable_discovery->set_observer(&mock_observer);

Sequence sequence;
Expand Down Expand Up @@ -443,7 +494,10 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) {
auto cable_discovery =
std::make_unique<FakeFidoCableDiscovery>(std::move(discovery_data));
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
cable_discovery->set_observer(&mock_observer);

auto mock_adapter =
Expand Down Expand Up @@ -476,7 +530,8 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {

NiceMock<MockFidoDiscoveryObserver> 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 =
Expand Down Expand Up @@ -524,7 +579,10 @@ TEST_F(FidoCableDiscoveryTest, TestUnregisterAdvertisementUponDestruction) {
TEST_F(FidoCableDiscoveryTest, TestResumeDiscoveryAfterPoweredOn) {
auto cable_discovery = CreateDiscovery();
NiceMock<MockFidoDiscoveryObserver> mock_observer;
EXPECT_CALL(mock_observer, DiscoveryStarted(_, _, testing::SizeIs(1)));
EXPECT_CALL(mock_observer,
DiscoveryStarted(cable_discovery.get(), true,
std::vector<FidoAuthenticator*>()));
EXPECT_CALL(mock_observer, AuthenticatorAdded(_, _));
cable_discovery->set_observer(&mock_observer);

auto mock_adapter =
Expand Down
2 changes: 1 addition & 1 deletion device/fido/fido_request_handler_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit a9dd7a5

Please sign in to comment.