Merge lp:~macslow/unity-notifications/fix-1453958 into lp:unity-notifications

[Mirco Müller]

Plugged memory-leaks causing the backend to crash the unity8-notification-plugin and updated unit-tests. Deleting notifications in reverse order of insertion no longer result the segfault forced by the provided python-script.

Plugged memory-leaks causing the backend to crash the unity8-notification-plugin and updated unit-tests. Deleting notifications in reverse order of insertion no longer result the segfault forced by the provided python-script.

* Are there any related MPs required for this MP to build/function as expected? Please list.
No.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* Did you make sure that your branch does not contain spurious tags?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

* If you changed the UI, has there been a design review?
Not applicable.

[Lukáš Tinkl (lukas-kde)]

Obviously +1 from me as I helped writing this code :)

[lp:~macslow/unity-notifications/fix-1453958]

226. By Mirco Müller on 2015-05-21

Make the notification object itself the parent of the ActionModel, not the Notification's parent.

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

review: Approve

[Michał Sawicz (saviq)]

Resubmitted as https://code.launchpad.net/~lukas-kde/unity-notifications/fix-1453958/+merge/274163

review: Disapprove

Unmerged revisions

=== modified file 'src/Notification.cpp'
--- src/Notification.cpp	2014-09-22 12:43:29 +0000
+++ src/Notification.cpp	2015-05-21 16:49:54 +0000
@@ -52,7 +52,7 @@
     p->body = "default text";
     p->server = nullptr;
     p->value = -2;
-    p->actionsModel = new ActionModel();
+    p->actionsModel = new ActionModel(this);
 }
 
 Notification::Notification(NotificationID id, int displayTime, const Urgency ur, const QString &text, Type type, NotificationServer *srv, QObject *parent) :
@@ -64,12 +64,12 @@
     p->server = srv;
     p->value = -2;
     p->displayTime = displayTime;
-    p->actionsModel = new ActionModel();
+    p->actionsModel = new ActionModel(this);
 }
 
 Notification::Notification(NotificationID id, int displayTime, const Urgency ur, Type type, NotificationServer *srv, QObject *parent) :
     Notification(id, displayTime, ur, QString(), type, srv, parent){
-    p->actionsModel = new ActionModel();
+    p->actionsModel = new ActionModel(this);
 }
 
 Notification::~Notification() {

=== modified file 'src/NotificationModel.cpp'
--- src/NotificationModel.cpp	2014-04-28 12:54:22 +0000
+++ src/NotificationModel.cpp	2015-05-21 16:49:54 +0000
@@ -202,37 +202,47 @@
 }
 
 void NotificationModel::removeNotification(const NotificationID id) {
-    for(int i=0; i<p->ephemeralQueue.size(); i++) {
-        if(p->ephemeralQueue[i]->getID() == id) {
-            p->ephemeralQueue.erase(p->ephemeralQueue.begin() + i);
-            Q_EMIT queueSizeChanged(queued());
-            return;
-        }
-    }
-
-    for(int i=0; i<p->snapQueue.size(); i++) {
-        if(p->snapQueue[i]->getID() == id) {
-            p->snapQueue.erase(p->snapQueue.begin() + i);
-            Q_EMIT queueSizeChanged(queued());
-            return;
-        }
-    }
-
-    for(int i=0; i<p->interactiveQueue.size(); i++) {
-        if(p->interactiveQueue[i]->getID() == id) {
-            p->interactiveQueue.erase(p->interactiveQueue.begin() + i);
-            Q_EMIT queueSizeChanged(queued());
-            return;
-        }
-    }
-
-    for(int i=0; i<p->displayedNotifications.size(); i++) {
+    for (int i = 0; i < p->displayedNotifications.size(); i++) {
         if(p->displayedNotifications[i]->getID() == id) {
             deleteFromVisible(i);
             timeout(); // Simulate a timeout so visual state is updated.
             return;
         }
     }
+
+    for (auto it = p->ephemeralQueue.begin(); it != p->ephemeralQueue.end(); ++it) {
+        QSharedPointer<Notification> n = *it;
+
+        if (n && n->getID() == id) {
+            p->ephemeralQueue.erase(it);
+            n.data()->deleteLater();
+            Q_EMIT queueSizeChanged(queued());
+            return;
+        }
+     }
+
+    for (auto it = p->snapQueue.begin(); it != p->snapQueue.end(); ++it) {
+        QSharedPointer<Notification> n = *it;
+
+        if (n && n->getID() == id) {
+            p->snapQueue.erase(it);
+            n.data()->deleteLater();
+            Q_EMIT queueSizeChanged(queued());
+            return;
+        }
+    }
+
+    for (auto it = p->interactiveQueue.begin(); it != p->interactiveQueue.end(); ++it) {
+        QSharedPointer<Notification> n = *it;
+
+        if (n && n->getID() == id) {
+            p->interactiveQueue.erase(it);
+            n.data()->deleteLater();
+            Q_EMIT queueSizeChanged(queued());
+            return;
+        }
+    }
+
     // The ID was not found in any queue. Should it be an error case or not?
 }
 
@@ -246,8 +256,9 @@
     QModelIndex deletePoint = QModelIndex();
     beginRemoveRows(deletePoint, loc, loc);
     QSharedPointer<Notification> n = p->displayedNotifications[loc];
-    p->displayTimes.erase(p->displayTimes.find(n->getID()));
-    p->displayedNotifications.erase(p->displayedNotifications.begin() + loc);
+    p->displayTimes.remove(n->getID());
+    p->displayedNotifications.removeAt(loc);
+    n.clear();
     endRemoveRows();
 }
 

=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt	2013-12-04 17:31:50 +0000
+++ test/CMakeLists.txt	2015-05-21 16:49:54 +0000
@@ -1,6 +1,6 @@
 add_executable(notificationtest notificationtest.cpp)
 target_link_libraries(notificationtest notifybackend)
-qt5_use_modules(notificationtest Gui Test Core Widgets)
+qt5_use_modules(notificationtest Gui Test Core DBus Widgets)
 
 add_test(notification notificationtest)
 set_tests_properties(notification PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=minimal")

=== modified file 'test/notificationtest.cpp'
--- test/notificationtest.cpp	2014-10-09 08:26:38 +0000
+++ test/notificationtest.cpp	2015-05-21 16:49:54 +0000
@@ -1,5 +1,6 @@
 #include "Notification.h"
 #include "NotificationModel.h"
+#include "NotificationServer.h"
 
 #include <QtTest/QtTest>
 
@@ -24,6 +25,7 @@
         void testConfirmationValue();
         void testTextFilter();
         void testTextFilter_data();
+        void testReverseClose();
 };
 
 void TestNotifications::testSimpleInsertion() {
@@ -55,43 +57,50 @@
 }
 
 void TestNotifications::testOrder() {
-    const int timeout = 5000;
-    QSharedPointer<Notification> n1(new Notification(1, timeout, Notification::Low, "low", Notification::Ephemeral));
-    QSharedPointer<Notification> n2(new Notification(2, timeout, Notification::Normal, "high", Notification::Ephemeral));
-    QSharedPointer<Notification> n3(new Notification(3, timeout, Notification::Critical, "critical", Notification::Ephemeral));
-    NotificationModel m;
-
-    m.insertNotification(n1);
-    QVERIFY(m.showingNotification(n1->getID()));
-
-    m.insertNotification(n2);
-    QVERIFY(!m.showingNotification(n1->getID()));
-    QVERIFY(m.showingNotification(n2->getID()));
-    QVERIFY(m.queued() == 1);
-
-    m.insertNotification(n3);
-    QVERIFY(!m.showingNotification(n1->getID()));
-    QVERIFY(!m.showingNotification(n2->getID()));
-    QVERIFY(m.showingNotification(n3->getID()));
-    QCOMPARE(m.queued(), 2);
-
-    m.removeNotification(n3->getID());
-    QVERIFY(!m.showingNotification(n1->getID()));
-    QVERIFY(m.showingNotification(n2->getID()));
-    QVERIFY(!m.showingNotification(n3->getID()));
-    QCOMPARE(m.queued(), 1);
-
-    m.removeNotification(n2->getID());
-    QVERIFY(m.showingNotification(n1->getID()));
-    QVERIFY(!m.showingNotification(n2->getID()));
-    QVERIFY(!m.showingNotification(n3->getID()));
-    QCOMPARE(m.queued(), 0);
-
-    m.removeNotification(n1->getID());
-    QVERIFY(!m.showingNotification(n1->getID()));
-    QVERIFY(!m.showingNotification(n2->getID()));
-    QVERIFY(!m.showingNotification(n3->getID()));
-    QCOMPARE(m.queued(), 0);
+    const int timeout = 1000;
+    static NotificationModel *m = new NotificationModel();
+    static NotificationServer *s = new NotificationServer(*m);
+    QStringList actions = QStringList();
+    Hints hints;
+    int id[3];
+
+    hints[URGENCY_HINT] = QDBusVariant(Notification::Urgency::Low);
+    id[0] = s->Notify ("test-name-low", 0, "icon-low", "summary-low", "body-low", actions, hints, timeout);
+    QVERIFY(m->showingNotification(id[0]));
+
+    hints[URGENCY_HINT] = QDBusVariant(Notification::Urgency::Normal);
+    id[1] = s->Notify ("test-name-normal", 0, "icon-normal", "summary-normal", "body-normal", actions, hints, timeout);
+    QVERIFY(!m->showingNotification(id[0]));
+    QVERIFY(m->showingNotification(id[1]));
+    QVERIFY(m->queued() == 1);
+
+    hints[URGENCY_HINT] = QDBusVariant(Notification::Urgency::Critical);
+    id[2] = s->Notify ("test-name-critical", 0, "icon-critical", "summary-critical", "body-critical", actions, hints, timeout);
+    QVERIFY(!m->showingNotification(id[0]));
+    QVERIFY(!m->showingNotification(id[1]));
+    QVERIFY(m->showingNotification(id[2]));
+    QCOMPARE(m->queued(), 2);
+
+    m->removeNotification(id[2]);
+    QVERIFY(!m->showingNotification(id[0]));
+    QVERIFY(m->showingNotification(id[1]));
+    QVERIFY(!m->showingNotification(id[2]));
+    QCOMPARE(m->queued(), 1);
+
+    m->removeNotification(id[1]);
+    QVERIFY(m->showingNotification(id[0]));
+    QVERIFY(!m->showingNotification(id[1]));
+    QVERIFY(!m->showingNotification(id[2]));
+    QCOMPARE(m->queued(), 0);
+
+    m->removeNotification(id[0]);
+    QVERIFY(!m->showingNotification(id[0]));
+    QVERIFY(!m->showingNotification(id[1]));
+    QVERIFY(!m->showingNotification(id[2]));
+    QCOMPARE(m->queued(), 0);
+
+    delete s;
+    delete m;
 }
 
 void TestNotifications::testHas() {
@@ -259,5 +268,38 @@
     QCOMPARE(n.getBody(), result);
 }
 
+void TestNotifications::testReverseClose() {
+    const int timeout = 1000;
+    const int max = 20;
+    static NotificationModel *m = new NotificationModel();
+    static NotificationServer *s = new NotificationServer(*m);
+    QStringList actions = QStringList();
+    Hints hints = Hints();
+    int before = m->numNotifications();
+
+    for (int i = 1; i <= max; i++) {
+        s->Notify ("test-app-name",
+                   0,
+                   "test-icon",
+                   "test-summary",
+                   "test-body",
+                   actions,
+                   hints,
+                   timeout);
+    }
+
+    QCOMPARE(m->numNotifications(), max+1);
+
+    for (int i = max; i >= 1; i--) {
+        m->getNotification(i)->close();
+        QCOMPARE(m->numNotifications(), i);
+    }
+
+    QCOMPARE(m->numNotifications(), before);
+
+    delete s;
+    delete m;
+}
+
 QTEST_MAIN(TestNotifications)
 #include "notificationtest.moc"