Skip to content

Commit

Permalink
[QuickStart] Defer StartAdvertising if BT adapter not ready
Browse files Browse the repository at this point in the history 
TargetDeviceConnectionBroker::StartAdvertising() is getting called
before the FeatureSupportStatus is kSupported, causing a CHECK to fail.
In this case, defer StartAdvertising() until after the adapter is
initialized.

TEST=Manually confirmed that the Quick Start screen no longer crashes,
and added unit test to check for correct behavior before BT is
initialized.

Bug: None
Change-Id: I317eb96f1350a183aa654aa3e057c487462624d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803418
Reviewed-by: Michael Hansen <hansenmichael@google.com>
Commit-Queue: Curt Clemens <cclem@google.com>
Reviewed-by: Brittany Hartmire <bhartmire@google.com>
Cr-Commit-Position: refs/heads/main@{#1031620}
  • Loading branch information
Curt Clemens authored and Chromium LUCI CQ committed Aug 4, 2022
1 parent 88591ea commit 8e8c369
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 20 deletions.
56 changes: 47 additions & 9 deletions ...meos/ash/components/oobe_quick_start/connectivity/target_device_connection_broker_impl.cc
View file
Expand Up @@ -16,6 +16,27 @@

namespace ash::quick_start {

void TargetDeviceConnectionBrokerImpl::BluetoothAdapterFactoryWrapper::
GetAdapter(device::BluetoothAdapterFactory::AdapterCallback callback) {
if (bluetooth_adapter_factory_wrapper_for_testing_) {
bluetooth_adapter_factory_wrapper_for_testing_->GetAdapterImpl(
std::move(callback));
return;
}

device::BluetoothAdapterFactory* adapter_factory =
device::BluetoothAdapterFactory::Get();

// Bluetooth is always supported on the ChromeOS platform.
DCHECK(adapter_factory->IsBluetoothSupported());

adapter_factory->GetAdapter(std::move(callback));
}

TargetDeviceConnectionBrokerImpl::BluetoothAdapterFactoryWrapper*
TargetDeviceConnectionBrokerImpl::BluetoothAdapterFactoryWrapper::
bluetooth_adapter_factory_wrapper_for_testing_ = nullptr;

TargetDeviceConnectionBrokerImpl::TargetDeviceConnectionBrokerImpl() {
GetBluetoothAdapter();
}
Expand All @@ -24,7 +45,6 @@ TargetDeviceConnectionBrokerImpl::~TargetDeviceConnectionBrokerImpl() {}

TargetDeviceConnectionBrokerImpl::FeatureSupportStatus
TargetDeviceConnectionBrokerImpl::GetFeatureSupportStatus() const {
// TODO(b/234848503) Add unit test coverage for the kUndetermined case.
if (!bluetooth_adapter_)
return FeatureSupportStatus::kUndetermined;

Expand All @@ -35,19 +55,13 @@ TargetDeviceConnectionBrokerImpl::GetFeatureSupportStatus() const {
}

void TargetDeviceConnectionBrokerImpl::GetBluetoothAdapter() {
auto* adapter_factory = device::BluetoothAdapterFactory::Get();

// Bluetooth is always supported on the ChromeOS platform.
DCHECK(adapter_factory->IsBluetoothSupported());

// Because this will be called from the constructor, GetAdapter() may call
// OnGetBluetoothAdapter() immediately which can cause problems during tests
// since the class is not fully constructed yet.
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(
&device::BluetoothAdapterFactory::GetAdapter,
base::Unretained(adapter_factory),
&BluetoothAdapterFactoryWrapper::GetAdapter,
base::BindOnce(
&TargetDeviceConnectionBrokerImpl::OnGetBluetoothAdapter,
weak_ptr_factory_.GetWeakPtr())));
Expand All @@ -56,14 +70,34 @@ void TargetDeviceConnectionBrokerImpl::GetBluetoothAdapter() {
void TargetDeviceConnectionBrokerImpl::OnGetBluetoothAdapter(
scoped_refptr<device::BluetoothAdapter> adapter) {
bluetooth_adapter_ = adapter;

if (deferred_start_advertising_callback_) {
std::move(deferred_start_advertising_callback_).Run();
}
}

void TargetDeviceConnectionBrokerImpl::StartAdvertising(
ConnectionLifecycleListener* listener,
ResultCallback on_start_advertising_callback) {
// TODO(b/234655072): Notify client about incoming connections on the started
// advertisement via ConnectionLifecycleListener.
CHECK(GetFeatureSupportStatus() == FeatureSupportStatus::kSupported);
if (GetFeatureSupportStatus() == FeatureSupportStatus::kUndetermined) {
deferred_start_advertising_callback_ =
base::BindOnce(&TargetDeviceConnectionBroker::StartAdvertising,
weak_ptr_factory_.GetWeakPtr(), listener,
std::move(on_start_advertising_callback));
return;
}

if (GetFeatureSupportStatus() == FeatureSupportStatus::kNotSupported) {
LOG(ERROR)
<< __func__
<< " failed to start advertising because the feature is not supported.";
std::move(on_start_advertising_callback).Run(/*success=*/false);
return;
}

DCHECK(GetFeatureSupportStatus() == FeatureSupportStatus::kSupported);

if (!bluetooth_adapter_->IsPowered()) {
LOG(ERROR) << __func__
Expand Down Expand Up @@ -98,6 +132,10 @@ void TargetDeviceConnectionBrokerImpl::OnStartFastPairAdvertisingError(

void TargetDeviceConnectionBrokerImpl::StopAdvertising(
base::OnceClosure on_stop_advertising_callback) {
if (deferred_start_advertising_callback_) {
deferred_start_advertising_callback_.Reset();
}

if (!fast_pair_advertiser_) {
VLOG(1) << __func__ << " Not currently advertising, ignoring.";
std::move(on_stop_advertising_callback).Run();
Expand Down
26 changes: 22 additions & 4 deletions chromeos/ash/components/oobe_quick_start/connectivity/target_device_connection_broker_impl.h
View file
Expand Up @@ -8,13 +8,10 @@
#include "base/memory/weak_ptr.h"
#include "base/unguessable_token.h"
#include "chromeos/ash/components/oobe_quick_start/connectivity/target_device_connection_broker.h"
#include "device/bluetooth/bluetooth_adapter_factory.h"

class FastPairAdvertiser;

namespace device {
class BluetoothAdapter;
}

namespace ash::quick_start {

class TargetDeviceConnectionBrokerImpl : public TargetDeviceConnectionBroker {
Expand All @@ -23,6 +20,26 @@ class TargetDeviceConnectionBrokerImpl : public TargetDeviceConnectionBroker {
TargetDeviceConnectionBroker::FeatureSupportStatus;
using ResultCallback = TargetDeviceConnectionBroker::ResultCallback;

// Thin wrapper around BluetoothAdapterFactory to allow mocking GetAdapter()
// for unit tests.
class BluetoothAdapterFactoryWrapper {
public:
static void GetAdapter(
device::BluetoothAdapterFactory::AdapterCallback callback);

static void set_bluetooth_adapter_factory_wrapper_for_testing(
BluetoothAdapterFactoryWrapper* wrapper) {
bluetooth_adapter_factory_wrapper_for_testing_ = wrapper;
}

private:
virtual void GetAdapterImpl(
device::BluetoothAdapterFactory::AdapterCallback callback) = 0;

static BluetoothAdapterFactoryWrapper*
bluetooth_adapter_factory_wrapper_for_testing_;
};

TargetDeviceConnectionBrokerImpl();
TargetDeviceConnectionBrokerImpl(TargetDeviceConnectionBrokerImpl&) = delete;
TargetDeviceConnectionBrokerImpl& operator=(
Expand All @@ -42,6 +59,7 @@ class TargetDeviceConnectionBrokerImpl : public TargetDeviceConnectionBroker {
void OnStopFastPairAdvertising(base::OnceClosure callback);

scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
base::OnceClosure deferred_start_advertising_callback_;

std::unique_ptr<FastPairAdvertiser> fast_pair_advertiser_;
base::UnguessableToken random_session_id_;
Expand Down
106 changes: 99 additions & 7 deletions ...components/oobe_quick_start/connectivity/target_device_connection_broker_impl_unittest.cc
View file
Expand Up @@ -22,6 +22,29 @@ using TargetDeviceConnectionBroker =
using TargetDeviceConnectionBrokerImpl =
ash::quick_start::TargetDeviceConnectionBrokerImpl;

// Allows us to delay returning a Bluetooth adapter until after ReturnAdapter()
// is called. Used for testing how the connection broker behaves before the
// Bluetooth adapter is finished initializing
class DeferredBluetoothAdapterFactoryWrapper
: public TargetDeviceConnectionBrokerImpl::BluetoothAdapterFactoryWrapper {
public:
void ReturnAdapter() {
if (!adapter_callback_)
return;

device::BluetoothAdapterFactory::Get()->GetAdapter(
std::move(adapter_callback_));
}

private:
void GetAdapterImpl(
device::BluetoothAdapterFactory::AdapterCallback callback) override {
adapter_callback_ = std::move(callback);
}

device::BluetoothAdapterFactory::AdapterCallback adapter_callback_;
};

class FakeFastPairAdvertiser : public FastPairAdvertiser {
public:
explicit FakeFastPairAdvertiser(
Expand Down Expand Up @@ -136,18 +159,24 @@ class TargetDeviceConnectionBrokerImplTest : public testing::Test {
this, &TargetDeviceConnectionBrokerImplTest::IsBluetoothPowered));
device::BluetoothAdapterFactory::SetAdapterForTesting(
mock_bluetooth_adapter_);
TargetDeviceConnectionBrokerImpl::BluetoothAdapterFactoryWrapper::
set_bluetooth_adapter_factory_wrapper_for_testing(
&bluetooth_adapter_factory_wrapper_);

CreateConnectionBroker();
SetFakeFastPairAdvertiserFactory(/*should_succeed_on_start=*/true);
// Allow the Bluetooth adapter to be fetched.
base::RunLoop().RunUntilIdle();
}

void CreateConnectionBroker() {
connection_broker_ =
ash::quick_start::TargetDeviceConnectionBrokerFactory::Create();
}

void FinishFetchingBluetoothAdapter() {
base::RunLoop().RunUntilIdle();
bluetooth_adapter_factory_wrapper_.ReturnAdapter();
}

bool IsBluetoothPowered() { return is_bluetooth_powered_; }

bool IsBluetoothPresent() { return is_bluetooth_present_; }
Expand All @@ -166,11 +195,7 @@ class TargetDeviceConnectionBrokerImplTest : public testing::Test {

void StartAdvertisingResultCallback(bool success) {
start_advertising_callback_called_ = true;
if (success) {
start_advertising_callback_success_ = true;
return;
}
start_advertising_callback_success_ = false;
start_advertising_callback_success_ = success;
}

void StopAdvertisingCallback() { stop_advertising_callback_called_ = true; }
Expand All @@ -184,12 +209,19 @@ class TargetDeviceConnectionBrokerImplTest : public testing::Test {
scoped_refptr<NiceMock<device::MockBluetoothAdapter>> mock_bluetooth_adapter_;
std::unique_ptr<TargetDeviceConnectionBroker> connection_broker_;
std::unique_ptr<FakeFastPairAdvertiserFactory> fast_pair_advertiser_factory_;
DeferredBluetoothAdapterFactoryWrapper bluetooth_adapter_factory_wrapper_;
base::test::SingleThreadTaskEnvironment task_environment_;
base::WeakPtrFactory<TargetDeviceConnectionBrokerImplTest> weak_ptr_factory_{
this};
};

TEST_F(TargetDeviceConnectionBrokerImplTest, GetFeatureSupportStatus) {
EXPECT_EQ(
TargetDeviceConnectionBrokerImpl::FeatureSupportStatus::kUndetermined,
connection_broker_->GetFeatureSupportStatus());

FinishFetchingBluetoothAdapter();

SetBluetoothIsPresent(false);
EXPECT_EQ(
TargetDeviceConnectionBrokerImpl::FeatureSupportStatus::kNotSupported,
Expand All @@ -201,6 +233,7 @@ TEST_F(TargetDeviceConnectionBrokerImplTest, GetFeatureSupportStatus) {
}

TEST_F(TargetDeviceConnectionBrokerImplTest, StartFastPairAdvertising) {
FinishFetchingBluetoothAdapter();
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

connection_broker_->StartAdvertising(
Expand All @@ -213,8 +246,43 @@ TEST_F(TargetDeviceConnectionBrokerImplTest, StartFastPairAdvertising) {
EXPECT_TRUE(start_advertising_callback_success_);
}

TEST_F(TargetDeviceConnectionBrokerImplTest,
StartFastPairAdvertising_BeforeBTAdapterInitialized) {
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

connection_broker_->StartAdvertising(
nullptr,
base::BindOnce(
&TargetDeviceConnectionBrokerImplTest::StartAdvertisingResultCallback,
weak_ptr_factory_.GetWeakPtr()));
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

FinishFetchingBluetoothAdapter();

EXPECT_EQ(1u, fast_pair_advertiser_factory_->StartAdvertisingCount());
EXPECT_TRUE(start_advertising_callback_called_);
EXPECT_TRUE(start_advertising_callback_success_);
}

TEST_F(TargetDeviceConnectionBrokerImplTest,
StartFastPairAdvertisingError_BluetoothNotPresent) {
FinishFetchingBluetoothAdapter();
SetBluetoothIsPresent(false);
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

connection_broker_->StartAdvertising(
nullptr,
base::BindOnce(
&TargetDeviceConnectionBrokerImplTest::StartAdvertisingResultCallback,
weak_ptr_factory_.GetWeakPtr()));
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());
EXPECT_TRUE(start_advertising_callback_called_);
EXPECT_FALSE(start_advertising_callback_success_);
}

TEST_F(TargetDeviceConnectionBrokerImplTest,
StartFastPairAdvertisingError_BluetoothNotPowered) {
FinishFetchingBluetoothAdapter();
SetBluetoothIsPowered(false);
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

Expand All @@ -230,6 +298,7 @@ TEST_F(TargetDeviceConnectionBrokerImplTest,

TEST_F(TargetDeviceConnectionBrokerImplTest,
StartFastPairAdvertisingError_Unsuccessful) {
FinishFetchingBluetoothAdapter();
SetFakeFastPairAdvertiserFactory(/*should_succeed_on_start=*/false);
EXPECT_EQ(0u, fast_pair_advertiser_factory_->StartAdvertisingCount());

Expand All @@ -245,6 +314,8 @@ TEST_F(TargetDeviceConnectionBrokerImplTest,

TEST_F(TargetDeviceConnectionBrokerImplTest,
StopFastPairAdvertising_NeverStarted) {
FinishFetchingBluetoothAdapter();

// If StartAdvertising is never called, StopAdvertising should not propagate
// to the fast pair advertiser.
connection_broker_->StopAdvertising(base::BindOnce(
Expand All @@ -255,7 +326,28 @@ TEST_F(TargetDeviceConnectionBrokerImplTest,
EXPECT_FALSE(fast_pair_advertiser_factory_->StopAdvertisingCalled());
}

TEST_F(TargetDeviceConnectionBrokerImplTest,
StopFastPairAdvertising_BeforeBTAdapterInitialized) {
connection_broker_->StartAdvertising(
nullptr,
base::BindOnce(
&TargetDeviceConnectionBrokerImplTest::StartAdvertisingResultCallback,
weak_ptr_factory_.GetWeakPtr()));

// If the Bluetooth adapter hasn't finished initializing, then
// StartAdvertisings never completed, and StopAdvertising should not propagate
// to the fast pair advertiser.
connection_broker_->StopAdvertising(base::BindOnce(
&TargetDeviceConnectionBrokerImplTest::StopAdvertisingCallback,
weak_ptr_factory_.GetWeakPtr()));

EXPECT_TRUE(stop_advertising_callback_called_);
EXPECT_FALSE(fast_pair_advertiser_factory_->StopAdvertisingCalled());
}

TEST_F(TargetDeviceConnectionBrokerImplTest, StopFastPairAdvertising) {
FinishFetchingBluetoothAdapter();

connection_broker_->StartAdvertising(
nullptr,
base::BindOnce(
Expand Down

0 comments on commit 8e8c369

Please sign in to comment.