Merge "Make AdvertiseManager shutdown gracefully" (f57c2db3) · Commits · e / os / android_packages_modules_Bluetooth

system/stack/btm/btm_ble_multi_adv.cc

+124 −43

Original line number	Original line	Diff line number	Diff line
	@@ -20,6 +20,7 @@
	#include <base/bind.h>		#include <base/bind.h>
	#include <base/location.h>		#include <base/location.h>
	#include <base/logging.h>		#include <base/logging.h>
			#include <base/memory/weak_ptr.h>
	#include <base/strings/string_number_conversions.h>		#include <base/strings/string_number_conversions.h>
	#include <base/time/time.h>		#include <base/time/time.h>
	#include <string.h>		#include <string.h>
	@@ -139,7 +140,7 @@ class BleAdvertisingManagerImpl;
	/* a temporary type for holding all the data needed in callbacks below*/		/* a temporary type for holding all the data needed in callbacks below*/
	struct CreatorParams {		struct CreatorParams {
	uint8_t inst_id;		uint8_t inst_id;
	BleAdvertisingManagerImpl* self;		base::WeakPtr<BleAdvertisingManagerImpl> self;
	IdTxPowerStatusCb cb;		IdTxPowerStatusCb cb;
	tBTM_BLE_ADV_PARAMS params;		tBTM_BLE_ADV_PARAMS params;
	std::vector<uint8_t> advertise_data;		std::vector<uint8_t> advertise_data;
	@@ -157,11 +158,11 @@ class BleAdvertisingManagerImpl
	: public BleAdvertisingManager,		: public BleAdvertisingManager,
	public BleAdvertiserHciInterface::AdvertisingEventObserver {		public BleAdvertiserHciInterface::AdvertisingEventObserver {
	public:		public:
	BleAdvertisingManagerImpl(BleAdvertiserHciInterface* interface) {		BleAdvertisingManagerImpl(BleAdvertiserHciInterface* interface)
	this->hci_interface = interface;		: hci_interface(interface), weak_factory_(this) {
	hci_interface->ReadInstanceCount(		hci_interface->ReadInstanceCount(
	base::Bind(&BleAdvertisingManagerImpl::ReadInstanceCountCb,		base::Bind(&BleAdvertisingManagerImpl::ReadInstanceCountCb,
	base::Unretained(this)));		weak_factory_.GetWeakPtr()));
	}		}

	~BleAdvertisingManagerImpl() { adv_inst.clear(); }		~BleAdvertisingManagerImpl() { adv_inst.clear(); }
	@@ -210,7 +211,7 @@ class BleAdvertisingManagerImpl
	void GenerateRpa(base::Callback<void(RawAddress)> cb) {		void GenerateRpa(base::Callback<void(RawAddress)> cb) {
	btm_gen_resolvable_private_addr(		btm_gen_resolvable_private_addr(
	Bind(&BleAdvertisingManagerImpl::OnRpaGenerationComplete,		Bind(&BleAdvertisingManagerImpl::OnRpaGenerationComplete,
	base::Unretained(this), std::move(cb)));		weak_factory_.GetWeakPtr(), std::move(cb)));
	}		}

	void ConfigureRpa(AdvertisingInstance* p_inst, MultiAdvCb configuredCb) {		void ConfigureRpa(AdvertisingInstance* p_inst, MultiAdvCb configuredCb) {
	@@ -308,7 +309,7 @@ class BleAdvertisingManagerImpl
	/* a temporary type for holding all the data needed in callbacks below*/		/* a temporary type for holding all the data needed in callbacks below*/
	struct CreatorParams {		struct CreatorParams {
	uint8_t inst_id;		uint8_t inst_id;
	BleAdvertisingManagerImpl* self;		base::WeakPtr<BleAdvertisingManagerImpl> self;
	MultiAdvCb cb;		MultiAdvCb cb;
	tBTM_BLE_ADV_PARAMS params;		tBTM_BLE_ADV_PARAMS params;
	std::vector<uint8_t> advertise_data;		std::vector<uint8_t> advertise_data;
	@@ -320,7 +321,7 @@ class BleAdvertisingManagerImpl
	std::unique_ptr<CreatorParams> c;		std::unique_ptr<CreatorParams> c;
	c.reset(new CreatorParams());		c.reset(new CreatorParams());

	c->self = this;		c->self = weak_factory_.GetWeakPtr();
	c->cb = std::move(cb);		c->cb = std::move(cb);
	c->params = *params;		c->params = *params;
	c->advertise_data = std::move(advertise_data);		c->advertise_data = std::move(advertise_data);
	@@ -336,7 +337,12 @@ class BleAdvertisingManagerImpl
	// clang-format off		// clang-format off
	c->self->SetParameters(c->inst_id, &c->params, Bind(		c->self->SetParameters(c->inst_id, &c->params, Bind(
	[](c_type c, uint8_t status, int8_t tx_power) {		[](c_type c, uint8_t status, int8_t tx_power) {
	if (status != 0) {		if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

			if (status) {
	LOG(ERROR) << "setting parameters failed, status: " << +status;		LOG(ERROR) << "setting parameters failed, status: " << +status;
	c->cb.Run(status);		c->cb.Run(status);
	return;		return;
	@@ -347,6 +353,11 @@ class BleAdvertisingManagerImpl
	const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;		const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;
	c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(		c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	LOG(ERROR) << "setting random address failed, status: " << +status;		LOG(ERROR) << "setting random address failed, status: " << +status;
	c->cb.Run(status);		c->cb.Run(status);
	@@ -355,6 +366,11 @@ class BleAdvertisingManagerImpl

	c->self->SetData(c->inst_id, false, std::move(c->advertise_data), Bind(		c->self->SetData(c->inst_id, false, std::move(c->advertise_data), Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	LOG(ERROR) << "setting advertise data failed, status: " << +status;		LOG(ERROR) << "setting advertise data failed, status: " << +status;
	c->cb.Run(status);		c->cb.Run(status);
	@@ -363,6 +379,11 @@ class BleAdvertisingManagerImpl

	c->self->SetData(c->inst_id, true, std::move(c->scan_response_data), Bind(		c->self->SetData(c->inst_id, true, std::move(c->scan_response_data), Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	LOG(ERROR) << "setting scan response data failed, status: " << +status;		LOG(ERROR) << "setting scan response data failed, status: " << +status;
	c->cb.Run(status);		c->cb.Run(status);
	@@ -388,7 +409,7 @@ class BleAdvertisingManagerImpl
	std::unique_ptr<CreatorParams> c;		std::unique_ptr<CreatorParams> c;
	c.reset(new CreatorParams());		c.reset(new CreatorParams());

	c->self = this;		c->self = weak_factory_.GetWeakPtr();
	c->cb = std::move(cb);		c->cb = std::move(cb);
	c->params = *params;		c->params = *params;
	c->advertise_data = std::move(advertise_data);		c->advertise_data = std::move(advertise_data);
	@@ -404,8 +425,13 @@ class BleAdvertisingManagerImpl
	// clang-format off		// clang-format off
	c->self->RegisterAdvertiser(Bind(		c->self->RegisterAdvertiser(Bind(
	[](c_type c, uint8_t advertiser_id, uint8_t status) {		[](c_type c, uint8_t advertiser_id, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	LOG(ERROR) << "registering advertiser failed, status: " << +status;		LOG(ERROR) << " failed, status: " << +status;
	c->cb.Run(0, 0, status);		c->cb.Run(0, 0, status);
	return;		return;
	}		}
	@@ -414,6 +440,11 @@ class BleAdvertisingManagerImpl

	c->self->SetParameters(c->inst_id, &c->params, Bind(		c->self->SetParameters(c->inst_id, &c->params, Bind(
	[](c_type c, uint8_t status, int8_t tx_power) {		[](c_type c, uint8_t status, int8_t tx_power) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting parameters failed, status: " << +status;		LOG(ERROR) << "setting parameters failed, status: " << +status;
	@@ -432,6 +463,11 @@ class BleAdvertisingManagerImpl
	const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;		const RawAddress& rpa = c->self->adv_inst[c->inst_id].own_address;
	c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(		c->self->GetHciInterface()->SetRandomAddress(c->inst_id, rpa, Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting random address failed, status: " << +status;		LOG(ERROR) << "setting random address failed, status: " << +status;
	@@ -447,31 +483,51 @@ class BleAdvertisingManagerImpl
	}		}

	void StartAdvertisingSetAfterAddressPart(c_type c) {		void StartAdvertisingSetAfterAddressPart(c_type c) {
	c->self->SetData(c->inst_id, false, std::move(c->advertise_data), Bind(		c->self->SetData(
			c->inst_id, false, std::move(c->advertise_data),
			Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting advertise data failed, status: " << +status;		LOG(ERROR) << "setting advertise data failed, status: "
			<< +status;
	c->cb.Run(0, 0, status);		c->cb.Run(0, 0, status);
	return;		return;
	}		}

	c->self->SetData(c->inst_id, true, std::move(c->scan_response_data), Bind(		c->self->SetData(
			c->inst_id, true, std::move(c->scan_response_data),
			Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting scan response data failed, status: " << +status;		LOG(ERROR)
			<< "setting scan response data failed, status: "
			<< +status;
	c->cb.Run(0, 0, status);		c->cb.Run(0, 0, status);
	return;		return;
	}		}

	if (c->periodic_params.enable) {		if (c->periodic_params.enable) {
	c->self->StartAdvertisingSetPeriodicPart(std::move(c));		c->self->StartAdvertisingSetPeriodicPart(
			std::move(c));
	} else {		} else {
	c->self->StartAdvertisingSetFinish(std::move(c));		c->self->StartAdvertisingSetFinish(std::move(c));
	}		}
	}, base::Passed(&c)));		},
	}, base::Passed(&c)));		base::Passed(&c)));
			},
			base::Passed(&c)));
	}		}

	void StartAdvertisingSetPeriodicPart(c_type c) {		void StartAdvertisingSetPeriodicPart(c_type c) {
	@@ -480,6 +536,11 @@ class BleAdvertisingManagerImpl
	// clang-format off		// clang-format off
	c->self->SetPeriodicAdvertisingParameters(c->inst_id, &c->periodic_params, Bind(		c->self->SetPeriodicAdvertisingParameters(c->inst_id, &c->periodic_params, Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting periodic parameters failed, status: " << +status;		LOG(ERROR) << "setting periodic parameters failed, status: " << +status;
	@@ -489,6 +550,11 @@ class BleAdvertisingManagerImpl

	c->self->SetPeriodicAdvertisingData(c->inst_id, std::move(c->periodic_data), Bind(		c->self->SetPeriodicAdvertisingData(c->inst_id, std::move(c->periodic_data), Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "setting periodic parameters failed, status: " << +status;		LOG(ERROR) << "setting periodic parameters failed, status: " << +status;
	@@ -498,6 +564,11 @@ class BleAdvertisingManagerImpl

	c->self->SetPeriodicAdvertisingEnable(c->inst_id, true, Bind(		c->self->SetPeriodicAdvertisingEnable(c->inst_id, true, Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "enabling periodic advertising failed, status: " << +status;		LOG(ERROR) << "enabling periodic advertising failed, status: " << +status;
	@@ -518,9 +589,14 @@ class BleAdvertisingManagerImpl
	uint16_t duration = c->duration;		uint16_t duration = c->duration;
	uint8_t maxExtAdvEvents = c->maxExtAdvEvents;		uint8_t maxExtAdvEvents = c->maxExtAdvEvents;
	RegisterCb timeout_cb = std::move(c->timeout_cb);		RegisterCb timeout_cb = std::move(c->timeout_cb);
	BleAdvertisingManagerImpl* self = c->self;		base::WeakPtr<BleAdvertisingManagerImpl> self = c->self;
	MultiAdvCb enable_cb = Bind(		MultiAdvCb enable_cb = Bind(
	[](c_type c, uint8_t status) {		[](c_type c, uint8_t status) {
			if (!c->self) {
			LOG(INFO) << "Stack was shut down";
			return;
			}

	if (status != 0) {		if (status != 0) {
	c->self->Unregister(c->inst_id);		c->self->Unregister(c->inst_id);
	LOG(ERROR) << "enabling advertiser failed, status: " << +status;		LOG(ERROR) << "enabling advertiser failed, status: " << +status;
	@@ -546,9 +622,9 @@ class BleAdvertisingManagerImpl

	p_inst->timeout_timer = alarm_new("btm_ble.adv_timeout");		p_inst->timeout_timer = alarm_new("btm_ble.adv_timeout");

	base::Closure cb = Bind(&BleAdvertisingManagerImpl::Enable,		base::Closure cb = Bind(
	base::Unretained(this), inst_id, 0 /* disable */,		&BleAdvertisingManagerImpl::Enable, weak_factory_.GetWeakPtr(), inst_id,
	std::move(timeout_cb), 0, 0, base::Bind(DoNothing));		0 /* disable */, std::move(timeout_cb), 0, 0, base::Bind(DoNothing));

	// schedule disable when the timeout passes		// schedule disable when the timeout passes
	alarm_set_closure(FROM_HERE, p_inst->timeout_timer, duration * 10,		alarm_set_closure(FROM_HERE, p_inst->timeout_timer, duration * 10,
	@@ -581,8 +657,8 @@ class BleAdvertisingManagerImpl
	if (enable && p_inst->address_update_required) {		if (enable && p_inst->address_update_required) {
	p_inst->address_update_required = false;		p_inst->address_update_required = false;
	ConfigureRpa(p_inst, base::Bind(&BleAdvertisingManagerImpl::EnableFinish,		ConfigureRpa(p_inst, base::Bind(&BleAdvertisingManagerImpl::EnableFinish,
	base::Unretained(this), p_inst, enable,		weak_factory_.GetWeakPtr(), p_inst,
	std::move(cb)));		enable, std::move(cb)));
	return;		return;
	}		}

	@@ -596,7 +672,7 @@ class BleAdvertisingManagerImpl
	// TODO(jpawlowski): HCI implementation that can't do duration should		// TODO(jpawlowski): HCI implementation that can't do duration should
	// emulate it, not EnableWithTimerCb.		// emulate it, not EnableWithTimerCb.
	myCb = Bind(&BleAdvertisingManagerImpl::EnableWithTimerCb,		myCb = Bind(&BleAdvertisingManagerImpl::EnableWithTimerCb,
	base::Unretained(this), p_inst->inst_id, std::move(cb),		weak_factory_.GetWeakPtr(), p_inst->inst_id, std::move(cb),
	p_inst->duration, p_inst->timeout_cb);		p_inst->duration, p_inst->timeout_cb);
	} else {		} else {
	myCb = std::move(cb);		myCb = std::move(cb);
	@@ -694,7 +770,7 @@ class BleAdvertisingManagerImpl
	DivideAndSendData(		DivideAndSendData(
	inst_id, data, cb,		inst_id, data, cb,
	base::Bind(&BleAdvertisingManagerImpl::SetDataAdvDataSender,		base::Bind(&BleAdvertisingManagerImpl::SetDataAdvDataSender,
	base::Unretained(this), is_scan_rsp));		weak_factory_.GetWeakPtr(), is_scan_rsp));
	}		}

	void SetDataAdvDataSender(uint8_t is_scan_rsp, uint8_t inst_id,		void SetDataAdvDataSender(uint8_t is_scan_rsp, uint8_t inst_id,
	@@ -932,6 +1008,11 @@ class BleAdvertisingManagerImpl
	BleAdvertiserHciInterface* hci_interface = nullptr;		BleAdvertiserHciInterface* hci_interface = nullptr;
	std::vector<AdvertisingInstance> adv_inst;		std::vector<AdvertisingInstance> adv_inst;
	uint8_t inst_count;		uint8_t inst_count;

			// Member variables should appear before the WeakPtrFactory, to ensure
			// that any WeakPtrs are invalidated before its members
			// variable's destructors are executed, rendering them invalid.
			base::WeakPtrFactory<BleAdvertisingManagerImpl> weak_factory_;
	};		};

	BleAdvertisingManager* instance;		BleAdvertisingManager* instance;

system/stack/test/ble_advertiser_test.cc

+47 −0

Original line number	Original line	Diff line number	Diff line
	@@ -1064,6 +1064,53 @@ TEST_F(BleAdvertisingManagerTest, test_duration_update_during_timeout) {
	remove_cb.Run(0);		remove_cb.Run(0);
	}		}

			/* This test verifies that stack cleanup, and shutdown happening while there is
			* outstanding HCI command is not triggering the callback */
			TEST_F(BleAdvertisingManagerTest, test_cleanup_during_execution) {
			std::vector<uint8_t> adv_data;
			std::vector<uint8_t> scan_resp;
			tBTM_BLE_ADV_PARAMS params;
			tBLE_PERIODIC_ADV_PARAMS periodic_params;
			periodic_params.enable = false;
			std::vector<uint8_t> periodic_data;

			parameters_cb set_params_cb;
			status_cb set_address_cb;
			status_cb set_data_cb;
			EXPECT_CALL(*hci_mock, SetParameters1(_, _, _, _, _, _, _, _, _)).Times(1);
			EXPECT_CALL(*hci_mock, SetParameters2(_, _, _, _, _, _, _, _))
			.Times(1)
			.WillOnce(SaveArg<7>(&set_params_cb));
			EXPECT_CALL(*hci_mock, SetRandomAddress(_, _, _))
			.Times(1)
			.WillOnce(SaveArg<2>(&set_address_cb));
			EXPECT_CALL(*hci_mock, SetAdvertisingData(_, _, _, _, _, _))
			.Times(1)
			.WillOnce(SaveArg<5>(&set_data_cb));

			BleAdvertisingManager::Get()->StartAdvertisingSet(
			Bind(&BleAdvertisingManagerTest::StartAdvertisingSetCb,
			base::Unretained(this)),
			&params, adv_data, scan_resp, &periodic_params, periodic_data,
			0 /* duration /, 0 / maxExtAdvEvents */, Bind(DoNothing2));

			// we are a truly gracious fake controller, let the commands succeed!
			int selected_tx_power = -15;
			set_params_cb.Run(0, selected_tx_power);
			set_address_cb.Run(0);

			// Someone shut down the stack in the middle of flow, when the HCI Set
			// Advertise Data was scheduled!
			BleAdvertisingManager::Get()->CleanUp();

			// The HCI call returns with status, and tries to execute the callback. This
			// should just silently drop the call. If it got executed, we would get crash,
			// because BleAdvertisingManager object was already deleted.
			set_data_cb.Run(0);

			::testing::Mock::VerifyAndClearExpectations(hci_mock.get());
			}

	extern void testRecomputeTimeout1();		extern void testRecomputeTimeout1();
	extern void testRecomputeTimeout2();		extern void testRecomputeTimeout2();
	extern void testRecomputeTimeout3();		extern void testRecomputeTimeout3();