From: Jakub Pawlowski Date: Thu, 25 Jan 2018 18:53:57 +0000 (-0800) Subject: Use weak pointer when refering to AdvertisingManager X-Git-Tag: android-x86-9.0-r1~135^2~9^2^2^2 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=ce0d65f4b96fd5d0dd53d998bea758874a37503e;p=android-x86%2Fsystem-bt.git Use weak pointer when refering to AdvertisingManager In btif layer, we schedule tasks for execution on AdvertisingManager on bta thread. Before the scheduling we check if AdvertisingManager is initialized. We don't check if AdvertisingManager is still valid, when the command execution is starting on the bta thread. This is a race condition, that can cause crashes. To fix that, always post task for execution using weak reference, rather than raw pointer. Thanks to it, the MessageLoop will check if the weak_ptr is valid before attempt to execute the task. The check happens on the target thread, right before the execution, eliminating all possible race conditions. Test: ran all advertising tests, no regression Bug: 71051865 Change-Id: I7fd8255879d5272d47aa79974bb79bdaacb55800 --- diff --git a/btif/src/btif_ble_advertiser.cc b/btif/src/btif_ble_advertiser.cc index 6c3f46f6b..e24bcde2b 100644 --- a/btif/src/btif_ble_advertiser.cc +++ b/btif/src/btif_ble_advertiser.cc @@ -95,7 +95,7 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { void RegisterAdvertiser(IdStatusCallback cb) override { do_in_bta_thread( FROM_HERE, Bind(&BleAdvertisingManager::RegisterAdvertiser, - base::Unretained(BleAdvertisingManager::Get()), + BleAdvertisingManager::Get(), Bind(&BleAdvertiserInterfaceImpl::RegisterAdvertiserCb, base::Unretained(this), cb))); } @@ -118,8 +118,8 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { if (!BleAdvertisingManager::IsInitialized()) return; do_in_bta_thread(FROM_HERE, Bind(&BleAdvertisingManager::GetOwnAddress, - base::Unretained(BleAdvertisingManager::Get()), - advertiser_id, jni_thread_wrapper(FROM_HERE, cb))); + BleAdvertisingManager::Get(), advertiser_id, + jni_thread_wrapper(FROM_HERE, cb))); } void SetParameters(uint8_t advertiser_id, AdvertiseParameters params, @@ -130,11 +130,10 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { tBTM_BLE_ADV_PARAMS* p_params = new tBTM_BLE_ADV_PARAMS; parseParams(p_params, params); - do_in_bta_thread( - FROM_HERE, - Bind(&BleAdvertisingManager::SetParameters, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, - base::Owned(p_params), jni_thread_wrapper(FROM_HERE, cb))); + do_in_bta_thread(FROM_HERE, Bind(&BleAdvertisingManager::SetParameters, + BleAdvertisingManager::Get(), + advertiser_id, base::Owned(p_params), + jni_thread_wrapper(FROM_HERE, cb))); } void SetData(int advertiser_id, bool set_scan_rsp, vector data, @@ -142,9 +141,9 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { if (!BleAdvertisingManager::IsInitialized()) return; do_in_bta_thread( FROM_HERE, - Bind(&BleAdvertisingManager::SetData, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, - set_scan_rsp, std::move(data), jni_thread_wrapper(FROM_HERE, cb))); + Bind(&BleAdvertisingManager::SetData, BleAdvertisingManager::Get(), + advertiser_id, set_scan_rsp, std::move(data), + jni_thread_wrapper(FROM_HERE, cb))); } void Enable(uint8_t advertiser_id, bool enable, StatusCallback cb, @@ -156,9 +155,8 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { if (!BleAdvertisingManager::IsInitialized()) return; do_in_bta_thread( FROM_HERE, - Bind(&BleAdvertisingManager::Enable, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, - enable, jni_thread_wrapper(FROM_HERE, cb), duration, + Bind(&BleAdvertisingManager::Enable, BleAdvertisingManager::Get(), + advertiser_id, enable, jni_thread_wrapper(FROM_HERE, cb), duration, maxExtAdvEvents, jni_thread_wrapper(FROM_HERE, timeout_cb))); } @@ -176,7 +174,7 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { do_in_bta_thread( FROM_HERE, Bind(&BleAdvertisingManager::StartAdvertising, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, + BleAdvertisingManager::Get(), advertiser_id, jni_thread_wrapper(FROM_HERE, cb), base::Owned(p_params), std::move(advertise_data), std::move(scan_response_data), timeout_s * 100, jni_thread_wrapper(FROM_HERE, timeout_cb))); @@ -202,11 +200,11 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { do_in_bta_thread( FROM_HERE, Bind(&BleAdvertisingManager::StartAdvertisingSet, - base::Unretained(BleAdvertisingManager::Get()), - jni_thread_wrapper(FROM_HERE, cb), base::Owned(p_params), - std::move(advertise_data), std::move(scan_response_data), - base::Owned(p_periodic_params), std::move(periodic_data), duration, - maxExtAdvEvents, jni_thread_wrapper(FROM_HERE, timeout_cb))); + BleAdvertisingManager::Get(), jni_thread_wrapper(FROM_HERE, cb), + base::Owned(p_params), std::move(advertise_data), + std::move(scan_response_data), base::Owned(p_periodic_params), + std::move(periodic_data), duration, maxExtAdvEvents, + jni_thread_wrapper(FROM_HERE, timeout_cb))); } void SetPeriodicAdvertisingParameters( @@ -221,7 +219,7 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { do_in_bta_thread( FROM_HERE, Bind(&BleAdvertisingManager::SetPeriodicAdvertisingParameters, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, + BleAdvertisingManager::Get(), advertiser_id, base::Owned(p_periodic_params), jni_thread_wrapper(FROM_HERE, cb))); } @@ -231,11 +229,10 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { VLOG(1) << __func__ << " advertiser_id: " << +advertiser_id; if (!BleAdvertisingManager::IsInitialized()) return; - do_in_bta_thread( - FROM_HERE, - Bind(&BleAdvertisingManager::SetPeriodicAdvertisingData, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, - std::move(data), jni_thread_wrapper(FROM_HERE, cb))); + do_in_bta_thread(FROM_HERE, + Bind(&BleAdvertisingManager::SetPeriodicAdvertisingData, + BleAdvertisingManager::Get(), advertiser_id, + std::move(data), jni_thread_wrapper(FROM_HERE, cb))); } void SetPeriodicAdvertisingEnable(int advertiser_id, bool enable, @@ -244,11 +241,10 @@ class BleAdvertiserInterfaceImpl : public BleAdvertiserInterface { << " ,enable: " << enable; if (!BleAdvertisingManager::IsInitialized()) return; - do_in_bta_thread( - FROM_HERE, - Bind(&BleAdvertisingManager::SetPeriodicAdvertisingEnable, - base::Unretained(BleAdvertisingManager::Get()), advertiser_id, - enable, jni_thread_wrapper(FROM_HERE, cb))); + do_in_bta_thread(FROM_HERE, + Bind(&BleAdvertisingManager::SetPeriodicAdvertisingEnable, + BleAdvertisingManager::Get(), advertiser_id, enable, + jni_thread_wrapper(FROM_HERE, cb))); } }; diff --git a/btif/src/btif_core.cc b/btif/src/btif_core.cc index 296b9d788..8250c2a9f 100644 --- a/btif/src/btif_core.cc +++ b/btif/src/btif_core.cc @@ -46,6 +46,7 @@ #include "bt_common.h" #include "bt_utils.h" #include "bta_api.h" +#include "bta_closure_api.h" #include "bte.h" #include "btif_api.h" #include "btif_av.h" @@ -469,7 +470,7 @@ void btif_enable_bluetooth_evt(tBTA_STATUS status) { bt_status_t btif_disable_bluetooth(void) { LOG_INFO(LOG_TAG, "%s entered", __func__); - btm_ble_multi_adv_cleanup(); + do_in_bta_thread(FROM_HERE, base::Bind(&btm_ble_multi_adv_cleanup)); // TODO(jpawlowski): this should do whole BTA_VendorCleanup(), but it would // kill the stack now. diff --git a/stack/btm/btm_ble_multi_adv.cc b/stack/btm/btm_ble_multi_adv.cc index 78a3ebdd1..2dd6c32ba 100644 --- a/stack/btm/btm_ble_multi_adv.cc +++ b/stack/btm/btm_ble_multi_adv.cc @@ -154,6 +154,9 @@ struct CreatorParams { using c_type = std::unique_ptr; +BleAdvertisingManager* instance; +base::WeakPtr instance_weakptr; + class BleAdvertisingManagerImpl : public BleAdvertisingManager, public BleAdvertiserHciInterface::AdvertisingEventObserver { @@ -233,9 +236,8 @@ class BleAdvertisingManagerImpl /* Connectable advertising set must be disabled when updating RPA */ bool restart = p_inst->IsEnabled() && p_inst->IsConnectable(); - auto hci_interface = - ((BleAdvertisingManagerImpl*)BleAdvertisingManager::Get()) - ->GetHciInterface(); + if (!instance_weakptr.get()) return; + auto hci_interface = instance_weakptr.get()->GetHciInterface(); if (restart) { p_inst->enable_status = false; @@ -1002,6 +1004,10 @@ class BleAdvertisingManagerImpl } } + base::WeakPtr GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + private: BleAdvertiserHciInterface* GetHciInterface() { return hci_interface; } @@ -1015,23 +1021,22 @@ class BleAdvertisingManagerImpl base::WeakPtrFactory weak_factory_; }; -BleAdvertisingManager* instance; - void btm_ble_adv_raddr_timer_timeout(void* data) { - ((BleAdvertisingManagerImpl*)BleAdvertisingManager::Get()) - ->ConfigureRpa((AdvertisingInstance*)data, base::Bind(DoNothing)); + BleAdvertisingManagerImpl* ptr = instance_weakptr.get(); + if (ptr) ptr->ConfigureRpa((AdvertisingInstance*)data, base::Bind(DoNothing)); } } // namespace void BleAdvertisingManager::Initialize(BleAdvertiserHciInterface* interface) { instance = new BleAdvertisingManagerImpl(interface); + instance_weakptr = ((BleAdvertisingManagerImpl*)instance)->GetWeakPtr(); } bool BleAdvertisingManager::IsInitialized() { return instance; } -BleAdvertisingManager* BleAdvertisingManager::Get() { +base::WeakPtr BleAdvertisingManager::Get() { CHECK(instance); - return instance; + return instance_weakptr; }; void BleAdvertisingManager::CleanUp() { @@ -1046,11 +1051,11 @@ void btm_ble_adv_init() { BleAdvertiserHciInterface::Initialize(); BleAdvertisingManager::Initialize(BleAdvertiserHciInterface::Get()); BleAdvertiserHciInterface::Get()->SetAdvertisingEventObserver( - (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get()); + (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get().get()); if (BleAdvertiserHciInterface::Get()->QuirkAdvertiserZeroHandle()) { // If handle 0 can't be used, register advertiser for it, but never use it. - BleAdvertisingManager::Get()->RegisterAdvertiser(Bind(DoNothing2)); + BleAdvertisingManager::Get().get()->RegisterAdvertiser(Bind(DoNothing2)); } } @@ -1077,7 +1082,7 @@ void test_timeout_cb(uint8_t status) { timeout_triggered = true; } // verify that if duration passed, or is about to pass, recomputation will shut // down the advertiser completly void testRecomputeTimeout1() { - auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get(); + auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get().get(); TimeTicks start = TimeTicks::Now(); TimeTicks end = start + TimeDelta::FromMilliseconds(111); @@ -1097,7 +1102,7 @@ void testRecomputeTimeout1() { // verify that duration and maxExtAdvEvents are properly adjusted when // recomputing. void testRecomputeTimeout2() { - auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get(); + auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get().get(); TimeTicks start = TimeTicks::Now(); TimeTicks end = start + TimeDelta::FromMilliseconds(250); @@ -1120,7 +1125,7 @@ void testRecomputeTimeout2() { // verify that if maxExtAdvEvents were sent, or are close to end, recomputation // wil shut down the advertiser completly void testRecomputeTimeout3() { - auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get(); + auto manager = (BleAdvertisingManagerImpl*)BleAdvertisingManager::Get().get(); TimeTicks start = TimeTicks::Now(); TimeTicks end = start + TimeDelta::FromMilliseconds(495); diff --git a/stack/include/ble_advertiser.h b/stack/include/ble_advertiser.h index b539d29ef..f4fc81c05 100644 --- a/stack/include/ble_advertiser.h +++ b/stack/include/ble_advertiser.h @@ -20,6 +20,7 @@ #define BLE_ADVERTISER_H #include +#include #include #include "btm_ble_api.h" @@ -72,7 +73,7 @@ class BleAdvertisingManager { static void Initialize(BleAdvertiserHciInterface* interface); static void CleanUp(); static bool IsInitialized(); - static BleAdvertisingManager* Get(); + static base::WeakPtr Get(); /* Register an advertising instance, status will be returned in |cb| * callback, with assigned id, if operation succeeds. Instance is freed when