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 6c3f46f..e24bcde 100644 (file)
--- 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<uint8_t> 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 296b9d7..8250c2a 100644 (file)
--- 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 78a3ebd..2dd6c32 100644 (file)
--- 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<CreatorParams>;
 
+BleAdvertisingManager* instance;
+base::WeakPtr<BleAdvertisingManagerImpl> 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<BleAdvertisingManagerImpl> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
  private:
   BleAdvertiserHciInterface* GetHciInterface() { return hci_interface; }
 
@@ -1015,23 +1021,22 @@ class BleAdvertisingManagerImpl
   base::WeakPtrFactory<BleAdvertisingManagerImpl> 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> 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 b539d29..f4fc81c 100644 (file)
--- a/stack/include/ble_advertiser.h
+++ b/stack/include/ble_advertiser.h
@@ -20,6 +20,7 @@
 #define BLE_ADVERTISER_H
 
 #include <base/bind.h>
+#include <base/memory/weak_ptr.h>
 #include <vector>
 #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<BleAdvertisingManager> Get();
 
   /* Register an advertising instance, status will be returned in |cb|
    * callback, with assigned id, if operation succeeds. Instance is freed when