diff --git a/panels/notification/server/notifyserverapplet.cpp b/panels/notification/server/notifyserverapplet.cpp index b4de43dfc..ce4387a97 100644 --- a/panels/notification/server/notifyserverapplet.cpp +++ b/panels/notification/server/notifyserverapplet.cpp @@ -7,6 +7,8 @@ #include "dbusadaptor.h" #include "pluginfactory.h" +#include +#include #include #include @@ -25,13 +27,37 @@ NotifyServerApplet::NotifyServerApplet(QObject *parent) NotifyServerApplet::~NotifyServerApplet() { qDebug(notifyLog) << "Exit notification server."; - if (m_manager) { - m_manager->deleteLater(); - } + + constexpr int kWaitTimeoutMs = 3000; + constexpr int kTerminateWaitMs = 1000; + if (m_worker) { - m_worker->exit(); - m_worker->wait(); - m_worker->deleteLater(); + if (m_worker->isRunning()) { + m_worker->quit(); + + if (!m_worker->wait(kWaitTimeoutMs)) { + qWarning(notifyLog) + << "Worker thread did not exit in time, terminating."; + + m_worker->terminate(); + + if (!m_worker->wait(kTerminateWaitMs)) { + qCritical(notifyLog) + << "Worker thread terminate timeout."; + } + } + } + + if (m_manager) { + delete m_manager; + m_manager = nullptr; + } + + delete m_worker; + m_worker = nullptr; + } else if (m_manager) { + delete m_manager; + m_manager = nullptr; } } @@ -42,12 +68,22 @@ bool NotifyServerApplet::load() bool NotifyServerApplet::init() { + // Reentrancy protection + if (m_manager || m_worker) { + qWarning(notifyLog) << "NotifyServerApplet is already initialized."; + return true; + } + DApplet::init(); m_manager = new NotificationManager(); if (!m_manager->registerDbusService()) { qWarning(notifyLog) << QString("Can't register Notifications to the D-Bus object."); + + delete m_manager; + m_manager = nullptr; + return false; } @@ -60,52 +96,66 @@ bool NotifyServerApplet::init() m_worker = new QThread(); m_manager->moveToThread(m_worker); + + // Register Qt asynchronous cleanup + // connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater); + // connect(m_worker, &QThread::finished, m_worker, &QObject::deleteLater); + m_worker->start(); return true; } void NotifyServerApplet::actionInvoked(qint64 id, uint bubbleId, const QString &actionKey) { + CHECK_MANAGER(); QMetaObject::invokeMethod(m_manager, "actionInvoked", Qt::DirectConnection, Q_ARG(qint64, id), Q_ARG(uint, bubbleId), Q_ARG(QString, actionKey)); } void NotifyServerApplet::actionInvoked(qint64 id, const QString &actionKey) { + CHECK_MANAGER(); QMetaObject::invokeMethod(m_manager, "actionInvoked", Qt::DirectConnection, Q_ARG(qint64, id), Q_ARG(QString, actionKey)); } void NotifyServerApplet::notificationClosed(qint64 id, uint bubbleId, uint reason) { + CHECK_MANAGER(); QMetaObject::invokeMethod(m_manager, "notificationClosed", Qt::DirectConnection, Q_ARG(qint64, id), Q_ARG(uint, bubbleId), Q_ARG(uint, reason)); } QVariant NotifyServerApplet::appValue(const QString &appId, int configItem) { + CHECK_MANAGER_RET({}); return m_manager->GetAppInfo(appId, configItem); } void NotifyServerApplet::removeNotification(qint64 id) { + CHECK_MANAGER(); m_manager->removeNotification(id); } void NotifyServerApplet::removeNotifications(const QString &appName) { + CHECK_MANAGER(); m_manager->removeNotifications(appName); } void NotifyServerApplet::removeNotifications() { + CHECK_MANAGER(); m_manager->removeNotifications(); } void NotifyServerApplet::removeExpiredNotifications() { + CHECK_MANAGER(); m_manager->removeExpiredNotifications(); } void NotifyServerApplet::setBlockClosedId(qint64 id) { + CHECK_MANAGER(); m_manager->setBlockClosedId(id); } diff --git a/panels/notification/server/notifyserverapplet.h b/panels/notification/server/notifyserverapplet.h index 20975e91d..88ce15133 100644 --- a/panels/notification/server/notifyserverapplet.h +++ b/panels/notification/server/notifyserverapplet.h @@ -4,8 +4,24 @@ #pragma once +#include #include "applet.h" +#define CHECK_MANAGER() \ + do { \ + if (!m_manager) { \ + qWarning(notifyLog) << "NotificationManager is null."; \ + return; \ + } \ + } while(0) + +#define CHECK_MANAGER_RET(val) \ + do { \ + if (!m_manager) { \ + qWarning(notifyLog) << "NotificationManager is null."; \ + return val; \ + } \ + } while(0) namespace notification { class NotificationManager; @@ -34,8 +50,8 @@ public Q_SLOTS: void setBlockClosedId(qint64 id); private: - NotificationManager *m_manager = nullptr; - QThread *m_worker = nullptr; + QPointer m_manager; + QPointer m_worker; }; } diff --git a/tests/panels/notification/server/notifyserverapplet_test.cpp b/tests/panels/notification/server/notifyserverapplet_test.cpp index 9a0463167..062a39f81 100644 --- a/tests/panels/notification/server/notifyserverapplet_test.cpp +++ b/tests/panels/notification/server/notifyserverapplet_test.cpp @@ -5,12 +5,17 @@ #include #include +#include #include +#include +#include #include #include #include +#define private public #include "notifyserverapplet.h" +#undef private #include "notificationmanager.h" using namespace notification; @@ -18,6 +23,22 @@ using ::testing::_; using ::testing::Return; using ::testing::Invoke; +class TrackableNotificationManager : public NotificationManager { + Q_OBJECT +public: + explicit TrackableNotificationManager(QObject *parent = nullptr) + : NotificationManager(parent) {} + + ~TrackableNotificationManager() override + { + ++destroyedCount; + } + + static std::atomic_int destroyedCount; +}; + +std::atomic_int TrackableNotificationManager::destroyedCount = 0; + // Mock class for NotificationManager class MockNotificationManager : public NotificationManager { Q_OBJECT @@ -62,7 +83,6 @@ class NotifyServerAppletTest : public ::testing::Test { // Test constructor TEST_F(NotifyServerAppletTest, ConstructorTest) { EXPECT_NE(applet, nullptr); - // Verify applet is created successfully EXPECT_TRUE(applet->inherits("ds::DApplet")); } @@ -72,6 +92,196 @@ TEST_F(NotifyServerAppletTest, DestructorTest) { EXPECT_NO_THROW(delete testApplet); } +// Test QPointer members are null by default after construction +TEST_F(NotifyServerAppletTest, QPointerNullByDefaultTest) { + auto *testApplet = new NotifyServerApplet(); + EXPECT_TRUE(testApplet->m_manager.isNull()); + EXPECT_TRUE(testApplet->m_worker.isNull()); + delete testApplet; +} + +// Test QPointer auto-null after explicit delete +TEST_F(NotifyServerAppletTest, QPointerAutoNullAfterDeleteTest) { + auto *testApplet = new NotifyServerApplet(); + auto *manager = new TrackableNotificationManager(); + + testApplet->m_manager = manager; + EXPECT_FALSE(testApplet->m_manager.isNull()); + + delete manager; + EXPECT_TRUE(testApplet->m_manager.isNull()); + + delete testApplet; +} + +// Test QPointer auto-null after deleteLater + sendPostedEvents +TEST_F(NotifyServerAppletTest, QPointerAutoNullAfterDeleteLaterTest) { + TrackableNotificationManager::destroyedCount = 0; + + auto *testApplet = new NotifyServerApplet(); + auto *manager = new TrackableNotificationManager(); + + testApplet->m_manager = manager; + EXPECT_FALSE(testApplet->m_manager.isNull()); + + manager->deleteLater(); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + + EXPECT_TRUE(testApplet->m_manager.isNull()); + EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); + + delete testApplet; +} + +// Test destructor when only manager exists (no worker thread) +TEST_F(NotifyServerAppletTest, DestructorWithManagerOnlyTest) { + TrackableNotificationManager::destroyedCount = 0; + + auto *testApplet = new NotifyServerApplet(); + auto *manager = new TrackableNotificationManager(); + QPointer managerGuard(manager); + + testApplet->m_manager = manager; + // m_worker remains nullptr + + delete testApplet; + + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + + EXPECT_TRUE(managerGuard.isNull()); + EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); +} + +// Test destructor when worker exists but manager is null +TEST_F(NotifyServerAppletTest, DestructorWithWorkerOnlyTest) { + auto *testApplet = new NotifyServerApplet(); + auto *worker = new QThread(); + QPointer workerGuard(worker); + + testApplet->m_worker = worker; + testApplet->m_manager = nullptr; + + QObject::connect(worker, &QThread::finished, worker, &QObject::deleteLater); + + worker->start(); + QTRY_VERIFY_WITH_TIMEOUT(worker->isRunning(), 1000); + + EXPECT_NO_THROW(delete testApplet); + + QTRY_VERIFY_WITH_TIMEOUT(workerGuard.isNull(), 3000); +} + +// Test QPointer auto-null for m_worker after running worker quit +TEST_F(NotifyServerAppletTest, QPointerWorkerAutoNullAfterQuitTest) { + auto *testApplet = new NotifyServerApplet(); + auto *worker = new QThread(); + + testApplet->m_worker = worker; + worker->start(); + QTRY_VERIFY_WITH_TIMEOUT(worker->isRunning(), 1000); + + worker->quit(); + worker->wait(); + delete worker; + + EXPECT_TRUE(testApplet->m_worker.isNull()); + + delete testApplet; +} + +// Test destructor with stopped worker and manager (else branch in destructor) +TEST_F(NotifyServerAppletTest, DestructorWithStoppedWorkerAndManagerTest) { + TrackableNotificationManager::destroyedCount = 0; + + auto *testApplet = new NotifyServerApplet(); + auto *worker = new QThread(); + auto *manager = new TrackableNotificationManager(); + QPointer managerGuard(manager); + QPointer workerGuard(worker); + + testApplet->m_manager = manager; + testApplet->m_worker = worker; + // worker is NOT started, so isRunning() returns false + + delete testApplet; + + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + + EXPECT_TRUE(managerGuard.isNull()); + EXPECT_TRUE(workerGuard.isNull()); + EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); +} + +TEST_F(NotifyServerAppletTest, DestructorDeletesManagerAfterWorkerThreadExit) { + TrackableNotificationManager::destroyedCount = 0; + + auto *testApplet = new NotifyServerApplet(); + auto *worker = new QThread(); + auto *manager = new TrackableNotificationManager(); + QPointer managerGuard(manager); + + testApplet->m_manager = manager; + testApplet->m_worker = worker; + + QObject::connect(worker, &QThread::finished, manager, &QObject::deleteLater); + QObject::connect(worker, &QThread::finished, worker, &QObject::deleteLater); + + manager->moveToThread(worker); + worker->start(); + + QTRY_VERIFY_WITH_TIMEOUT(worker->isRunning(), 1000); + + delete testApplet; + + QTRY_VERIFY_WITH_TIMEOUT(managerGuard.isNull(), 3000); + + EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); +} + +// Test QPointer m_worker auto-null after worker thread finished +TEST_F(NotifyServerAppletTest, QPointerWorkerAutoNullAfterThreadFinishedTest) { + TrackableNotificationManager::destroyedCount = 0; + + auto *testApplet = new NotifyServerApplet(); + auto *worker = new QThread(); + auto *manager = new TrackableNotificationManager(); + QPointer workerGuard(worker); + + testApplet->m_manager = manager; + testApplet->m_worker = worker; + + QObject::connect(worker, &QThread::finished, manager, &QObject::deleteLater); + QObject::connect(worker, &QThread::finished, worker, &QObject::deleteLater); + + manager->moveToThread(worker); + worker->start(); + QTRY_VERIFY_WITH_TIMEOUT(worker->isRunning(), 1000); + + worker->quit(); + QTRY_VERIFY_WITH_TIMEOUT(workerGuard.isNull(), 3000); + QTRY_VERIFY_WITH_TIMEOUT(testApplet->m_manager.isNull(), 3000); + + EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); + + delete testApplet; +} + +// Test QPointer members non-null after init() +TEST_F(NotifyServerAppletTest, QPointerNonNullAfterInitTest) { + auto *testApplet = new NotifyServerApplet(); + bool initResult = testApplet->init(); + + if (initResult) { + EXPECT_FALSE(testApplet->m_manager.isNull()); + EXPECT_FALSE(testApplet->m_worker.isNull()); + } else { + EXPECT_TRUE(testApplet->m_manager.isNull()); + EXPECT_TRUE(testApplet->m_worker.isNull()); + } + + EXPECT_NO_THROW(delete testApplet); +} + // Test memory leak detection for destructor with initialized applet TEST_F(NotifyServerAppletTest, DestructorMemoryLeakTest) { // This test verifies that all resources are properly cleaned up @@ -95,19 +305,13 @@ TEST_F(NotifyServerAppletTest, DestructorMemoryLeakTest) { // Test memory leak detection - multiple init calls TEST_F(NotifyServerAppletTest, MultipleInitMemoryLeakTest) { - // This test checks for memory leaks when init() is called multiple times - // Each init() creates new NotificationManager and QThread - // The old ones should be properly cleaned up or prevented - auto *testApplet = new NotifyServerApplet(); // First init testApplet->init(); - // Second init - this may create new objects without deleting old ones - // (Potential memory leak if not handled properly) - // fixme:(heysion) code dump because of this twice init - // testApplet->init(); + // Second init - after adding reentrancy prevention, no more crashes or leaks will occur + EXPECT_NO_THROW(testApplet->init()); EXPECT_NO_THROW({ delete testApplet;