[Linux] Do not reuse cancellable object (#31828)

* [Linux] Do not reuse cancellable object

Per documentation for g_cancellable_reset():

> Note that it is generally not a good idea to reuse an existing
> cancellable for more operations after it has been cancelled once, as
> this function might tempt you to do. The recommended practice is to
> drop the reference to a cancellable after cancelling it, and let it
> die with the outstanding async operations.

* Update cancellable in ChipDeviceScanner

Author: arkq

src/platform/GLibTypeDeleter.h

@@ -108,6 +108,12 @@ struct GAutoPtrDeleter<GBytes>
     using deleter = GBytesDeleter;
 };
 
+template <>
+struct GAutoPtrDeleter<GCancellable>
+{
+    using deleter = GObjectDeleter;
+};
+
 template <>
 struct GAutoPtrDeleter<GDBusConnection>
 {

src/platform/Linux/BLEManagerImpl.cpp

@@ -571,7 +571,7 @@ void BLEManagerImpl::DriveBLEState()
     // Initializes the Bluez BLE layer if needed.
     if (mServiceMode == ConnectivityManager::kCHIPoBLEServiceMode_Enabled && !mFlags.Has(Flags::kBluezBLELayerInitialized))
     {
-        SuccessOrExit(err = mEndpoint.Init(mAdapterId, mIsCentral, nullptr));
+        SuccessOrExit(err = mEndpoint.Init(mIsCentral, mAdapterId));
         mFlags.Set(Flags::kBluezBLELayerInitialized);
     }
 

src/platform/Linux/bluez/BluezEndpoint.cpp

@@ -656,22 +656,14 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplication()
     return err;
 }
 
-CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr)
+CHIP_ERROR BluezEndpoint::Init(bool aIsCentral, uint32_t aAdapterId)
 {
-    CHIP_ERROR err;
+    VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
 
     mAdapterId = aAdapterId;
     mIsCentral = aIsCentral;
 
-    if (apBleAddr != nullptr)
-        mpAdapterAddr = g_strdup(apBleAddr);
-
-    if (aIsCentral)
-    {
-        mpConnectCancellable = g_cancellable_new();
-    }
-
-    err = PlatformMgrImpl().GLibMatterContextInvokeSync(
+    CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(
         +[](BluezEndpoint * self) { return self->StartupEndpointBindings(); }, this);
     VerifyOrReturnError(err == CHIP_NO_ERROR, err, ChipLogError(DeviceLayer, "Failed to schedule endpoint initialization"));
 
@@ -681,6 +673,13 @@ CHIP_ERROR BluezEndpoint::Init(uint32_t aAdapterId, bool aIsCentral, const char
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR BluezEndpoint::Init(bool aIsCentral, const char * apBleAddr)
+{
+    VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
+    mpAdapterAddr = g_strdup(apBleAddr);
+    return Init(aIsCentral, mAdapterId);
+}
+
 void BluezEndpoint::Shutdown()
 {
     VerifyOrReturn(mIsInitialized);
@@ -707,8 +706,6 @@ void BluezEndpoint::Shutdown()
                 g_object_unref(self->mpC2);
             if (self->mpC3 != nullptr)
                 g_object_unref(self->mpC3);
-            if (self->mpConnectCancellable != nullptr)
-                g_object_unref(self->mpConnectCancellable);
             return CHIP_NO_ERROR;
         },
         this);
@@ -744,7 +741,7 @@ void BluezEndpoint::ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult,
             g_clear_error(&MakeUniquePointerReceiver(error).Get());
 
             bluez_device1_call_disconnect_sync(device, nullptr, &MakeUniquePointerReceiver(error).Get());
-            bluez_device1_call_connect(device, params->mEndpoint.mpConnectCancellable, ConnectDeviceDone, params);
+            bluez_device1_call_connect(device, params->mEndpoint.mConnectCancellable.get(), ConnectDeviceDone, params);
             return;
         }
 
@@ -760,8 +757,7 @@ void BluezEndpoint::ConnectDeviceDone(GObject * aObject, GAsyncResult * aResult,
 
 CHIP_ERROR BluezEndpoint::ConnectDeviceImpl(ConnectParams * apParams)
 {
-    g_cancellable_reset(apParams->mEndpoint.mpConnectCancellable);
-    bluez_device1_call_connect(apParams->mpDevice, apParams->mEndpoint.mpConnectCancellable, ConnectDeviceDone, apParams);
+    bluez_device1_call_connect(apParams->mpDevice, apParams->mEndpoint.mConnectCancellable.get(), ConnectDeviceDone, apParams);
     return CHIP_NO_ERROR;
 }
 
@@ -770,6 +766,7 @@ CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice)
     auto params = chip::Platform::New<ConnectParams>(*this, &aDevice);
     VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY);
 
+    mConnectCancellable.reset(g_cancellable_new());
     if (PlatformMgrImpl().GLibMatterContextInvokeSync(ConnectDeviceImpl, params) != CHIP_NO_ERROR)
     {
         ChipLogError(Ble, "Failed to schedule ConnectDeviceImpl() on CHIPoBluez thread");
@@ -782,8 +779,8 @@ CHIP_ERROR BluezEndpoint::ConnectDevice(BluezDevice1 & aDevice)
 
 void BluezEndpoint::CancelConnect()
 {
-    VerifyOrDie(mpConnectCancellable != nullptr);
-    g_cancellable_cancel(mpConnectCancellable);
+    g_cancellable_cancel(mConnectCancellable.get());
+    mConnectCancellable.reset();
 }
 
 } // namespace Internal

src/platform/Linux/bluez/BluezEndpoint.h

@@ -54,6 +54,7 @@
 
 #include <ble/CHIPBleServiceData.h>
 #include <lib/core/CHIPError.h>
+#include <platform/GLibTypeDeleter.h>
 #include <platform/Linux/dbus/bluez/DbusBluez.h>
 
 #include "BluezConnection.h"
@@ -69,7 +70,8 @@ class BluezEndpoint
     BluezEndpoint()  = default;
     ~BluezEndpoint() = default;
 
-    CHIP_ERROR Init(uint32_t aAdapterId, bool aIsCentral, const char * apBleAddr);
+    CHIP_ERROR Init(bool aIsCentral, uint32_t aAdapterId);
+    CHIP_ERROR Init(bool aIsCentral, const char * apBleAddr);
     void Shutdown();
 
     BluezAdapter1 * GetAdapter() const { return mpAdapter; }
@@ -148,8 +150,8 @@ class BluezEndpoint
     BluezGattCharacteristic1 * mpC3 = nullptr;
 
     std::unordered_map<std::string, BluezConnection *> mConnMap;
-    GCancellable * mpConnectCancellable = nullptr;
-    char * mpPeerDevicePath             = nullptr;
+    GAutoPtr<GCancellable> mConnectCancellable;
+    char * mpPeerDevicePath = nullptr;
 
     // Allow BluezConnection to access our private members
     friend class BluezConnection;

src/platform/Linux/bluez/ChipDeviceScanner.cpp

@@ -58,9 +58,8 @@ CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDel
     // Make this function idempotent by shutting down previously initialized state if any.
     Shutdown();
 
-    mAdapter     = BLUEZ_ADAPTER1(g_object_ref(adapter));
-    mCancellable = g_cancellable_new();
-    mDelegate    = delegate;
+    mAdapter  = BLUEZ_ADAPTER1(g_object_ref(adapter));
+    mDelegate = delegate;
 
     // Create the D-Bus object manager client object on the glib thread, so that all D-Bus signals
     // will be delivered to the glib thread.
@@ -73,7 +72,7 @@ CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDel
             self->mManager = g_dbus_object_manager_client_new_for_bus_sync(
                 G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/",
                 bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */,
-                nullptr /* destroy notify */, self->mCancellable, &MakeUniquePointerReceiver(err).Get());
+                nullptr /* destroy notify */, nullptr /* cancellable */, &MakeUniquePointerReceiver(err).Get());
             VerifyOrReturnError(self->mManager != nullptr, CHIP_ERROR_INTERNAL,
                                 ChipLogError(Ble, "Failed to get D-Bus object manager for device scanning: %s", err->message));
             return CHIP_NO_ERROR;
@@ -100,8 +99,6 @@ void ChipDeviceScanner::Shutdown()
                 g_object_unref(self->mManager);
             if (self->mAdapter != nullptr)
                 g_object_unref(self->mAdapter);
-            if (self->mCancellable != nullptr)
-                g_object_unref(self->mCancellable);
             return CHIP_NO_ERROR;
         },
         this);
@@ -115,6 +112,7 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout)
     VerifyOrReturnError(mScannerState != ChipDeviceScannerState::SCANNER_SCANNING, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(mTimerState == ScannerTimerState::TIMER_CANCELED, CHIP_ERROR_INCORRECT_STATE);
 
+    mCancellable.reset(g_cancellable_new());
     if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR)
     {
         ChipLogError(Ble, "Failed to schedule BLE scan start.");
@@ -191,7 +189,9 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self)
 {
     GAutoPtr<GError> error;
 
-    g_cancellable_cancel(self->mCancellable); // in case we are currently running a scan
+    // In case we are currently running a scan
+    g_cancellable_cancel(self->mCancellable.get());
+    self->mCancellable.reset();
 
     if (self->mObjectAddedSignal)
     {
@@ -303,7 +303,7 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self)
     g_variant_builder_add(&filterBuilder, "{sv}", "Transport", g_variant_new_string("le"));
     GVariant * filter = g_variant_builder_end(&filterBuilder);
 
-    if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable,
+    if (!bluez_adapter1_call_set_discovery_filter_sync(self->mAdapter, filter, self->mCancellable.get(),
                                                        &MakeUniquePointerReceiver(error).Get()))
     {
         // Not critical: ignore if fails
@@ -312,7 +312,8 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStartScan(ChipDeviceScanner * self)
     }
 
     ChipLogProgress(Ble, "BLE initiating scan.");
-    if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable, &MakeUniquePointerReceiver(error).Get()))
+    if (!bluez_adapter1_call_start_discovery_sync(self->mAdapter, self->mCancellable.get(),
+                                                  &MakeUniquePointerReceiver(error).Get()))
     {
         ChipLogError(Ble, "Failed to start discovery: %s", error->message);
         return CHIP_ERROR_INTERNAL;

src/platform/Linux/bluez/ChipDeviceScanner.h

@@ -23,6 +23,7 @@
 
 #include <ble/CHIPBleServiceData.h>
 #include <lib/core/CHIPError.h>
+#include <platform/GLibTypeDeleter.h>
 #include <platform/Linux/dbus/bluez/DbusBluez.h>
 #include <system/SystemLayer.h>
 
@@ -106,13 +107,13 @@ class ChipDeviceScanner
 
     GDBusObjectManager * mManager         = nullptr;
     BluezAdapter1 * mAdapter              = nullptr;
-    GCancellable * mCancellable           = nullptr;
     ChipDeviceScannerDelegate * mDelegate = nullptr;
     gulong mObjectAddedSignal             = 0;
     gulong mInterfaceChangedSignal        = 0;
     ChipDeviceScannerState mScannerState  = ChipDeviceScannerState::SCANNER_UNINITIALIZED;
     /// Used to track if timer has already expired and doesn't need to be canceled.
     ScannerTimerState mTimerState = ScannerTimerState::TIMER_CANCELED;
+    GAutoPtr<GCancellable> mCancellable;
 };
 
 } // namespace Internal