[Linux] Release BLE scanner resources on glib thread (#30178)

* [Linux] Release BLE scanner resources on glib thread

* Update python scanner after API change

* Fix missing namespace

* Add missing license header

* Fix initialization check

src/controller/python/chip/ble/LinuxImpl.cpp

@@ -1,3 +1,20 @@
+/*
+ *
+ *    Copyright (c) 2020-2023 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
 
 #include <lib/support/CHIPMem.h>
 #include <platform/CHIPDeviceLayer.h>
@@ -76,7 +93,8 @@ class ScannerDelegateImpl : public ChipDeviceScannerDelegate
         mScanCallback(scanCallback), mCompleteCallback(completeCallback), mErrorCallback(errorCallback)
     {}
 
-    void SetScanner(std::unique_ptr<ChipDeviceScanner> scanner) { mScanner = std::move(scanner); }
+    CHIP_ERROR ScannerInit(BluezAdapter1 * adapter) { return mScanner.Init(adapter, this); }
+    CHIP_ERROR ScannerStartScan(chip::System::Clock::Timeout timeout) { return mScanner.StartScan(timeout); }
 
     void OnDeviceScanned(BluezDevice1 & device, const chip::Ble::ChipBLEDeviceIdentificationInfo & info) override
     {
@@ -106,7 +124,7 @@ class ScannerDelegateImpl : public ChipDeviceScannerDelegate
     }
 
 private:
-    std::unique_ptr<ChipDeviceScanner> mScanner;
+    ChipDeviceScanner mScanner;
     PyObject * const mContext;
     const DeviceScannedCallback mScanCallback;
     const ScanCompleteCallback mCompleteCallback;
@@ -123,23 +141,13 @@ extern "C" void * pychip_ble_start_scanning(PyObject * context, void * adapter,
     std::unique_ptr<ScannerDelegateImpl> delegate =
         std::make_unique<ScannerDelegateImpl>(context, scanCallback, completeCallback, errorCallback);
 
-    std::unique_ptr<ChipDeviceScanner> scanner = ChipDeviceScanner::Create(static_cast<BluezAdapter1 *>(adapter), delegate.get());
+    CHIP_ERROR err = delegate->ScannerInit(static_cast<BluezAdapter1 *>(adapter));
+    VerifyOrReturnError(err == CHIP_NO_ERROR, nullptr);
 
-    if (!scanner)
-    {
-        return nullptr;
-    }
-
-    CHIP_ERROR err = CHIP_NO_ERROR;
     chip::DeviceLayer::PlatformMgr().LockChipStack();
-    err = scanner->StartScan(chip::System::Clock::Milliseconds32(timeoutMs));
+    err = delegate->ScannerStartScan(chip::System::Clock::Milliseconds32(timeoutMs));
     chip::DeviceLayer::PlatformMgr().UnlockChipStack();
-    if (err != CHIP_NO_ERROR)
-    {
-        return nullptr;
-    }
-
-    delegate->SetScanner(std::move(scanner));
+    VerifyOrReturnError(err == CHIP_NO_ERROR, nullptr);
 
     return delegate.release();
 }

src/platform/Linux/BLEManagerImpl.cpp

@@ -102,7 +102,7 @@ CHIP_ERROR BLEManagerImpl::_Init()
 void BLEManagerImpl::_Shutdown()
 {
     // ensure scan resources are cleared (e.g. timeout timers)
-    mDeviceScanner.reset();
+    mDeviceScanner.Shutdown();
     // Release BLE connection resources (unregister from BlueZ).
     ShutdownBluezBleLayer(mpEndpoint);
     mFlags.Clear(Flags::kBluezBLELayerInitialized);
@@ -689,18 +689,18 @@ void BLEManagerImpl::InitiateScan(BleScanState scanType)
         return;
     }
 
-    mDeviceScanner               = Internal::ChipDeviceScanner::Create(mpEndpoint->mpAdapter, this);
     mBLEScanConfig.mBleScanState = scanType;
 
-    if (!mDeviceScanner)
+    CHIP_ERROR err = mDeviceScanner.Init(mpEndpoint->mpAdapter, this);
+    if (err != CHIP_NO_ERROR)
     {
         mBLEScanConfig.mBleScanState = BleScanState::kNotScanning;
         BleConnectionDelegate::OnConnectionError(mBLEScanConfig.mAppState, CHIP_ERROR_INTERNAL);
         ChipLogError(Ble, "Failed to create a BLE device scanner");
         return;
     }
 
-    CHIP_ERROR err = mDeviceScanner->StartScan(kNewConnectionScanTimeout);
+    err = mDeviceScanner.StartScan(kNewConnectionScanTimeout);
     if (err != CHIP_NO_ERROR)
     {
         mBLEScanConfig.mBleScanState = BleScanState::kNotScanning;
@@ -738,7 +738,7 @@ CHIP_ERROR BLEManagerImpl::CancelConnection()
         CancelConnect(mpEndpoint);
     // If in discovery mode, stop scan.
     else if (mBLEScanConfig.mBleScanState != BleScanState::kNotScanning)
-        mDeviceScanner->StopScan();
+        mDeviceScanner.StopScan();
     return CHIP_NO_ERROR;
 }
 
@@ -811,7 +811,7 @@ void BLEManagerImpl::OnDeviceScanned(BluezDevice1 & device, const chip::Ble::Chi
     DeviceLayer::SystemLayer().StartTimer(kConnectTimeout, HandleConnectTimeout, mpEndpoint);
     chip::DeviceLayer::PlatformMgr().UnlockChipStack();
 
-    mDeviceScanner->StopScan();
+    mDeviceScanner.StopScan();
 
     ConnectDevice(device, mpEndpoint);
 }

src/platform/Linux/BLEManagerImpl.h

@@ -204,7 +204,7 @@ class BLEManagerImpl final : public BLEManager,
     char mDeviceName[kMaxDeviceNameLength + 1];
     bool mIsCentral            = false;
     BluezEndpoint * mpEndpoint = nullptr;
-    std::unique_ptr<ChipDeviceScanner> mDeviceScanner;
+    ChipDeviceScanner mDeviceScanner;
 };
 
 /**

src/platform/Linux/bluez/ChipDeviceScanner.cpp

@@ -33,43 +33,6 @@ namespace Internal {
 
 namespace {
 
-// Helper context for creating GDBusObjectManager with
-// chip::DeviceLayer::GLibMatterContextInvokeSync()
-struct GDBusCreateObjectManagerContext
-{
-    GDBusObjectManager * object = nullptr;
-    // Cancellable passed to g_dbus_object_manager_client_new_for_bus_sync()
-    // which later can be used to cancel the scan operation.
-    GCancellable * cancellable = nullptr;
-
-    GDBusCreateObjectManagerContext() : cancellable(g_cancellable_new()) {}
-    ~GDBusCreateObjectManagerContext()
-    {
-        g_object_unref(cancellable);
-        if (object != nullptr)
-        {
-            g_object_unref(object);
-        }
-    }
-};
-
-CHIP_ERROR MainLoopCreateObjectManager(GDBusCreateObjectManagerContext * context)
-{
-    // When creating D-Bus proxy object, the thread default context must be initialized. Otherwise,
-    // all D-Bus signals will be delivered to the GLib global default main context.
-    VerifyOrDie(g_main_context_get_thread_default() != nullptr);
-
-    GAutoPtr<GError> err;
-    context->object = 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 */, context->cancellable, &MakeUniquePointerReceiver(err).Get());
-    VerifyOrReturnError(context->object != nullptr, CHIP_ERROR_INTERNAL,
-                        ChipLogError(Ble, "Failed to get DBUS object manager for device scanning: %s", err->message));
-
-    return CHIP_NO_ERROR;
-}
-
 /// Retrieve CHIP device identification info from the device advertising data
 bool BluezGetChipDeviceInfo(BluezDevice1 & aDevice, chip::Ble::ChipBLEDeviceIdentificationInfo & aDeviceInfo)
 {
@@ -89,18 +52,42 @@ bool BluezGetChipDeviceInfo(BluezDevice1 & aDevice, chip::Ble::ChipBLEDeviceIden
 
 } // namespace
 
-ChipDeviceScanner::ChipDeviceScanner(GDBusObjectManager * manager, BluezAdapter1 * adapter, GCancellable * cancellable,
-                                     ChipDeviceScannerDelegate * delegate) :
-    mManager(manager),
-    mAdapter(adapter), mCancellable(cancellable), mDelegate(delegate)
+CHIP_ERROR ChipDeviceScanner::Init(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate)
 {
-    g_object_ref(mAdapter);
-    g_object_ref(mCancellable);
-    g_object_ref(mManager);
+
+    // 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;
+
+    // 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.
+    ReturnErrorOnFailure(PlatformMgrImpl().GLibMatterContextInvokeSync(
+        +[](ChipDeviceScanner * self) {
+            // When creating D-Bus proxy object, the thread default context must be initialized.
+            VerifyOrDie(g_main_context_get_thread_default() != nullptr);
+
+            GAutoPtr<GError> err;
+            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());
+            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;
+        },
+        this));
+
+    mIsInitialized = true;
+    return CHIP_NO_ERROR;
 }
 
-ChipDeviceScanner::~ChipDeviceScanner()
+void ChipDeviceScanner::Shutdown()
 {
+    VerifyOrReturn(mIsInitialized);
+
     StopScan();
 
     // mTimerExpired should only be set to true in the TimerExpiredCallback, which means we are in that callback
@@ -110,34 +97,29 @@ ChipDeviceScanner::~ChipDeviceScanner()
         chip::DeviceLayer::SystemLayer().CancelTimer(TimerExpiredCallback, this);
     }
 
-    g_object_unref(mManager);
-    g_object_unref(mCancellable);
-    g_object_unref(mAdapter);
-
-    mManager     = nullptr;
-    mAdapter     = nullptr;
-    mCancellable = nullptr;
-    mDelegate    = nullptr;
-}
-
-std::unique_ptr<ChipDeviceScanner> ChipDeviceScanner::Create(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate)
-{
-    GDBusCreateObjectManagerContext context;
-    CHIP_ERROR err;
-
-    err = PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopCreateObjectManager, &context);
-    VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Ble, "Failed to create BLE object manager"));
-
-    return std::make_unique<ChipDeviceScanner>(context.object, adapter, context.cancellable, delegate);
-
-exit:
-    return std::unique_ptr<ChipDeviceScanner>();
+    // Release resources on the glib thread. This is necessary because the D-Bus manager client
+    // object handles D-Bus signals. Otherwise, we might face a race when the manager object is
+    // released during a D-Bus signal being processed.
+    PlatformMgrImpl().GLibMatterContextInvokeSync(
+        +[](ChipDeviceScanner * self) {
+            if (self->mManager != nullptr)
+                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);
+
+    mIsInitialized = false;
 }
 
 CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout)
 {
     assertChipStackLockedByCurrentThread();
-    ReturnErrorCodeIf(mIsScanning, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(!mIsScanning, CHIP_ERROR_INCORRECT_STATE);
 
     mIsScanning = true; // optimistic, to allow all callbacks to check this
     if (PlatformMgrImpl().GLibMatterContextInvokeSync(MainLoopStartScan, this) != CHIP_NO_ERROR)
@@ -169,15 +151,16 @@ CHIP_ERROR ChipDeviceScanner::StartScan(System::Clock::Timeout timeout)
 void ChipDeviceScanner::TimerExpiredCallback(chip::System::Layer * layer, void * appState)
 {
     ChipDeviceScanner * chipDeviceScanner = static_cast<ChipDeviceScanner *>(appState);
-    chipDeviceScanner->MarkTimerExpired();
+    chipDeviceScanner->mTimerExpired      = true;
     chipDeviceScanner->mDelegate->OnScanError(CHIP_ERROR_TIMEOUT);
     chipDeviceScanner->StopScan();
 }
 
 CHIP_ERROR ChipDeviceScanner::StopScan()
 {
-    ReturnErrorCodeIf(!mIsScanning, CHIP_NO_ERROR);
-    ReturnErrorCodeIf(mIsStopping, CHIP_NO_ERROR);
+    VerifyOrReturnError(mIsScanning, CHIP_NO_ERROR);
+    VerifyOrReturnError(!mIsStopping, CHIP_NO_ERROR);
+
     mIsStopping = true;
     g_cancellable_cancel(mCancellable); // in case we are currently running a scan
 

src/platform/Linux/bluez/ChipDeviceScanner.h

@@ -20,7 +20,6 @@
 #include <platform/CHIPDeviceConfig.h>
 
 #include <glib.h>
-#include <memory>
 
 #include <ble/CHIPBleServiceData.h>
 #include <lib/core/CHIPError.h>
@@ -53,15 +52,18 @@ class ChipDeviceScannerDelegate
 class ChipDeviceScanner
 {
 public:
-    /// NOTE: prefer to use the  ::Create method instead direct constructor calling.
-    ChipDeviceScanner(GDBusObjectManager * manager, BluezAdapter1 * adapter, GCancellable * cancellable,
-                      ChipDeviceScannerDelegate * delegate);
-
+    ChipDeviceScanner()                                      = default;
     ChipDeviceScanner(ChipDeviceScanner &&)                  = default;
     ChipDeviceScanner(const ChipDeviceScanner &)             = delete;
     ChipDeviceScanner & operator=(const ChipDeviceScanner &) = delete;
 
-    ~ChipDeviceScanner();
+    ~ChipDeviceScanner() { Shutdown(); }
+
+    /// Initialize the scanner.
+    CHIP_ERROR Init(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate);
+
+    /// Release any resources associated with the scanner.
+    void Shutdown();
 
     /// Initiate a scan for devices, with the given timeout
     ///
@@ -72,15 +74,6 @@ class ChipDeviceScanner
     /// Stop any currently running scan
     CHIP_ERROR StopScan();
 
-    /// Should only be called by TimerExpiredCallback.
-    void MarkTimerExpired() { mTimerExpired = true; }
-
-    /// Create a new device scanner
-    ///
-    /// Convenience method to allocate any required variables.
-    /// On success, maintains a reference to the provided adapter.
-    static std::unique_ptr<ChipDeviceScanner> Create(BluezAdapter1 * adapter, ChipDeviceScannerDelegate * delegate);
-
 private:
     static void TimerExpiredCallback(chip::System::Layer * layer, void * appState);
     static CHIP_ERROR MainLoopStartScan(ChipDeviceScanner * self);
@@ -103,6 +96,7 @@ class ChipDeviceScanner
     ChipDeviceScannerDelegate * mDelegate = nullptr;
     gulong mObjectAddedSignal             = 0;
     gulong mInterfaceChangedSignal        = 0;
+    bool mIsInitialized                   = false;
     bool mIsScanning                      = false;
     bool mIsStopping                      = false;
     /// Used to track if timer has already expired and doesn't need to be canceled.