unity-notifications

Merge lp:~lukas-kde/unity-notifications/fix-1453958 into lp:unity-notifications

  1. fix-1453958
  2. Merge into trunk
Proposed by Lukáš Tinkl
Status: Merged
Approved by: Pete Woods
Approved revision: 231
Merged at revision: 236
Proposed branch: lp:~lukas-kde/unity-notifications/fix-1453958
Merge into: lp:unity-notifications
Diff against target: 422 lines (+145/-93)
9 files modified
include/ActionModel.h (+4/-4)
include/Notification.h (+3/-0)
include/NotificationModel.h (+3/-3)
include/NotificationPlugin.h (+2/-2)
src/Notification.cpp (+3/-3)
src/NotificationModel.cpp (+45/-37)
src/NotificationServer.cpp (+6/-6)
test/CMakeLists.txt (+0/-1)
test/notificationtest.cpp (+79/-37)
To merge this branch: bzr merge lp:~lukas-kde/unity-notifications/fix-1453958
Reviewer Review Type Date Requested Status
Pete Woods Approve
Review via email: mp+274163@code.launchpad.net

Commit message

Merge and rebase older code to fix notifications crashing on closing

Description of the change

Merge and rebase older code to fix notifications crashing on closing

Cf. https://bugs.launchpad.net/unity-notifications/+bug/1453958

Original message from lp:~macslow/unity-notifications/fix-1453958:

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.

Checklist:

* 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?

Yes

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

N/A

To post a comment you must log in.
Revision history for this message
Pete Woods (pete-woods) wrote :

When making a QSharedPointer use deleteLater, you should really pass in a custom deleter like so:

   QSharedPointer<MyClass> instance(new Myclass(param), &QObject::deleteLater)

i.e. at object creation time. It's risky to grab the raw pointer from a shared pointer and ask it to delete at some point.

review: Disapprove
Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

> When making a QSharedPointer use deleteLater, you should really pass in a
> custom deleter like so:
>
> QSharedPointer<MyClass> instance(new Myclass(param), &QObject::deleteLater)
>
> i.e. at object creation time. It's risky to grab the raw pointer from a shared
> pointer and ask it to delete at some point.

Ack, &QObject::deleteLater on construction and QSharedPointer::clear() when releasing it.

lp:~lukas-kde/unity-notifications/fix-1453958 updated
229. By Lukáš Tinkl

specify &QObject::deleteLater deleter at construction,
clear the shared pointer on destruction instead

Revision history for this message
Pete Woods (pete-woods) wrote :

Technically you don't even need the ::clear, as it will go out of scope naturally, (it's erased from the map, and the stack variable n goes out of scope). If you can be bothered, I would also remove all calls to n->clear();

lp:~lukas-kde/unity-notifications/fix-1453958 updated
230. By Lukáš Tinkl

remove the superfluous clear()

Revision history for this message
Lukáš Tinkl (lukas-kde) wrote :

> Technically you don't even need the ::clear, as it will go out of scope
> naturally, (it's erased from the map, and the stack variable n goes out of
> scope). If you can be bothered, I would also remove all calls to n->clear();

Done

lp:~lukas-kde/unity-notifications/fix-1453958 updated
231. By Lukáš Tinkl

add missing override keywords

Revision history for this message
Pete Woods (pete-woods) :
review: Approve
Revision history for this message
Pete Woods (pete-woods) wrote :

Thanks for the fixes!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'include/ActionModel.h'
--- include/ActionModel.h 2015-06-16 08:11:33 +0000
+++ include/ActionModel.h 2015-10-13 14:11:48 +0000
@@ -23,16 +23,16 @@
2323
24class ActionModel : public QStringListModel {24class ActionModel : public QStringListModel {
25 Q_OBJECT25 Q_OBJECT
26 Q_ENUMS(ActionsRoles)
2627
27public:28public:
28 ActionModel(QObject *parent=nullptr);29 ActionModel(QObject *parent=nullptr);
29 virtual ~ActionModel();30 virtual ~ActionModel();
3031
31 virtual int rowCount(const QModelIndex &index) const;32 virtual int rowCount(const QModelIndex &index) const override;
32 virtual QVariant data(const QModelIndex &index, int role) const;33 virtual QVariant data(const QModelIndex &index, int role) const override;
33 virtual QHash<int, QByteArray> roleNames() const;34 virtual QHash<int, QByteArray> roleNames() const override;
3435
35 Q_ENUMS(ActionsRoles)
36 enum ActionsRoles {36 enum ActionsRoles {
37 RoleActionLabel = Qt::UserRole + 1,37 RoleActionLabel = Qt::UserRole + 1,
38 RoleActionId = Qt::UserRole + 238 RoleActionId = Qt::UserRole + 2
3939
=== modified file 'include/Notification.h'
--- include/Notification.h 2015-06-23 09:16:23 +0000
+++ include/Notification.h 2015-10-13 14:11:48 +0000
@@ -125,4 +125,7 @@
125 QString filterText(const QString& text);125 QString filterText(const QString& text);
126};126};
127127
128Q_DECLARE_METATYPE(Notification::Urgency)
129Q_DECLARE_METATYPE(Notification::Type)
130
128#endif /* NOTIFICATION_HPP_ */131#endif /* NOTIFICATION_HPP_ */
129132
=== modified file 'include/NotificationModel.h'
--- include/NotificationModel.h 2015-06-23 09:18:43 +0000
+++ include/NotificationModel.h 2015-10-13 14:11:48 +0000
@@ -40,9 +40,9 @@
40 NotificationModel(QObject *parent=nullptr);40 NotificationModel(QObject *parent=nullptr);
41 virtual ~NotificationModel();41 virtual ~NotificationModel();
4242
43 virtual int rowCount(const QModelIndex &parent) const;43 virtual int rowCount(const QModelIndex &parent) const override;
44 virtual QVariant data(const QModelIndex &index, int role) const;44 virtual QVariant data(const QModelIndex &index, int role) const override;
45 virtual QHash<int, QByteArray> roleNames() const;45 virtual QHash<int, QByteArray> roleNames() const override;
4646
47 void insertNotification(const QSharedPointer<Notification> &n);47 void insertNotification(const QSharedPointer<Notification> &n);
48 QList<QSharedPointer<Notification>> getAllNotifications() const;48 QList<QSharedPointer<Notification>> getAllNotifications() const;
4949
=== modified file 'include/NotificationPlugin.h'
--- include/NotificationPlugin.h 2013-09-03 12:41:34 +0000
+++ include/NotificationPlugin.h 2015-10-13 14:11:48 +0000
@@ -28,8 +28,8 @@
28 Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QQmlExtensionInterface")28 Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QQmlExtensionInterface")
2929
30public:30public:
31 void registerTypes(const char *uri);31 void registerTypes(const char *uri) override;
32 void initializeEngine(QQmlEngine *engine, const char *uri);32 void initializeEngine(QQmlEngine *engine, const char *uri) override;
33};33};
3434
35#endif // NOTIFICATIONS_PLUGIN_H35#endif // NOTIFICATIONS_PLUGIN_H
3636
=== modified file 'src/Notification.cpp'
--- src/Notification.cpp 2015-07-06 18:13:16 +0000
+++ src/Notification.cpp 2015-10-13 14:11:48 +0000
@@ -53,7 +53,7 @@
53 p->body = "default text";53 p->body = "default text";
54 p->server = nullptr;54 p->server = nullptr;
55 p->value = -2;55 p->value = -2;
56 p->actionsModel = new ActionModel();56 p->actionsModel = new ActionModel(this);
57}57}
5858
59Notification::Notification(NotificationID id, int displayTime, const Urgency ur, const QString &text, Type type, NotificationServer *srv, QObject *parent) :59Notification::Notification(NotificationID id, int displayTime, const Urgency ur, const QString &text, Type type, NotificationServer *srv, QObject *parent) :
@@ -65,12 +65,12 @@
65 p->server = srv;65 p->server = srv;
66 p->value = -2;66 p->value = -2;
67 p->displayTime = displayTime;67 p->displayTime = displayTime;
68 p->actionsModel = new ActionModel();68 p->actionsModel = new ActionModel(this);
69}69}
7070
71Notification::Notification(NotificationID id, int displayTime, const Urgency ur, Type type, NotificationServer *srv, QObject *parent) :71Notification::Notification(NotificationID id, int displayTime, const Urgency ur, Type type, NotificationServer *srv, QObject *parent) :
72 Notification(id, displayTime, ur, QString(), type, srv, parent){72 Notification(id, displayTime, ur, QString(), type, srv, parent){
73 p->actionsModel = new ActionModel();73 p->actionsModel = new ActionModel(this);
74}74}
7575
76Notification::~Notification() {76Notification::~Notification() {
7777
=== modified file 'src/NotificationModel.cpp'
--- src/NotificationModel.cpp 2015-06-23 09:18:43 +0000
+++ src/NotificationModel.cpp 2015-10-13 14:11:48 +0000
@@ -46,7 +46,8 @@
46}46}
4747
48NotificationModel::NotificationModel(QObject *parent) : QAbstractListModel(parent), p(new NotificationModelPrivate) {48NotificationModel::NotificationModel(QObject *parent) : QAbstractListModel(parent), p(new NotificationModelPrivate) {
49 p->displayedNotifications.append(QSharedPointer<Notification>(new Notification(0, -1, Notification::Normal, QString(), Notification::PlaceHolder)));49 p->displayedNotifications.append(QSharedPointer<Notification>(new Notification(0, -1, Notification::Normal, QString(), Notification::PlaceHolder),
50 &QObject::deleteLater));
50 connect(&(p->timer), SIGNAL(timeout()), this, SLOT(timeout()));51 connect(&(p->timer), SIGNAL(timeout()), this, SLOT(timeout()));
51 p->timer.setSingleShot(true);52 p->timer.setSingleShot(true);
52}53}
@@ -74,41 +75,41 @@
74QVariant NotificationModel::data(const QModelIndex &index, int role) const {75QVariant NotificationModel::data(const QModelIndex &index, int role) const {
75 //printf("Data %d.\n", index.row());76 //printf("Data %d.\n", index.row());
76 if (!index.isValid())77 if (!index.isValid())
77 return QVariant();78 return QVariant();
7879
79 switch(role) {80 switch(role) {
80 case ModelInterface::RoleType:81 case ModelInterface::RoleType:
81 return QVariant(p->displayedNotifications[index.row()]->getType());82 return p->displayedNotifications[index.row()]->getType();
8283
83 case ModelInterface::RoleUrgency:84 case ModelInterface::RoleUrgency:
84 return QVariant(p->displayedNotifications[index.row()]->getUrgency());85 return p->displayedNotifications[index.row()]->getUrgency();
8586
86 case ModelInterface::RoleId:87 case ModelInterface::RoleId:
87 return QVariant(p->displayedNotifications[index.row()]->getID());88 return p->displayedNotifications[index.row()]->getID();
8889
89 case ModelInterface::RoleSummary:90 case ModelInterface::RoleSummary:
90 return QVariant(p->displayedNotifications[index.row()]->getSummary());91 return p->displayedNotifications[index.row()]->getSummary();
9192
92 case ModelInterface::RoleBody:93 case ModelInterface::RoleBody:
93 return QVariant(p->displayedNotifications[index.row()]->getBody());94 return p->displayedNotifications[index.row()]->getBody();
9495
95 case ModelInterface::RoleValue:96 case ModelInterface::RoleValue:
96 return QVariant(p->displayedNotifications[index.row()]->getValue());97 return p->displayedNotifications[index.row()]->getValue();
9798
98 case ModelInterface::RoleIcon:99 case ModelInterface::RoleIcon:
99 return QVariant(p->displayedNotifications[index.row()]->getIcon());100 return p->displayedNotifications[index.row()]->getIcon();
100101
101 case ModelInterface::RoleSecondaryIcon:102 case ModelInterface::RoleSecondaryIcon:
102 return QVariant(p->displayedNotifications[index.row()]->getSecondaryIcon());103 return p->displayedNotifications[index.row()]->getSecondaryIcon();
103104
104 case ModelInterface::RoleActions:105 case ModelInterface::RoleActions:
105 return QVariant::fromValue(p->displayedNotifications[index.row()]->getActions());106 return QVariant::fromValue(p->displayedNotifications[index.row()]->getActions());
106107
107 case ModelInterface::RoleHints:108 case ModelInterface::RoleHints:
108 return QVariant(p->displayedNotifications[index.row()]->getHints());109 return p->displayedNotifications[index.row()]->getHints();
109110
110 case ModelInterface::RoleNotification:111 case ModelInterface::RoleNotification:
111 return QVariant(p->displayedNotifications[index.row()]);112 return QVariant::fromValue(p->displayedNotifications[index.row()]);
112113
113 default:114 default:
114 return QVariant();115 return QVariant();
@@ -269,37 +270,44 @@
269}270}
270271
271void NotificationModel::removeNotification(const NotificationID id) {272void NotificationModel::removeNotification(const NotificationID id) {
272 for(int i=0; i<p->ephemeralQueue.size(); i++) {273 for (int i = 0; i < p->displayedNotifications.size(); i++) {
273 if(p->ephemeralQueue[i]->getID() == id) {
274 p->ephemeralQueue.erase(p->ephemeralQueue.begin() + i);
275 Q_EMIT queueSizeChanged(queued());
276 return;
277 }
278 }
279
280 for(int i=0; i<p->snapQueue.size(); i++) {
281 if(p->snapQueue[i]->getID() == id) {
282 p->snapQueue.erase(p->snapQueue.begin() + i);
283 Q_EMIT queueSizeChanged(queued());
284 return;
285 }
286 }
287
288 for(int i=0; i<p->interactiveQueue.size(); i++) {
289 if(p->interactiveQueue[i]->getID() == id) {
290 p->interactiveQueue.erase(p->interactiveQueue.begin() + i);
291 Q_EMIT queueSizeChanged(queued());
292 return;
293 }
294 }
295
296 for(int i=0; i<p->displayedNotifications.size(); i++) {
297 if(p->displayedNotifications[i]->getID() == id) {274 if(p->displayedNotifications[i]->getID() == id) {
298 deleteFromVisible(i);275 deleteFromVisible(i);
299 timeout(); // Simulate a timeout so visual state is updated.276 timeout(); // Simulate a timeout so visual state is updated.
300 return;277 return;
301 }278 }
302 }279 }
280
281 for (auto it = p->ephemeralQueue.begin(); it != p->ephemeralQueue.end(); ++it) {
282 QSharedPointer<Notification> n = *it;
283
284 if (n && n->getID() == id) {
285 p->ephemeralQueue.erase(it);
286 Q_EMIT queueSizeChanged(queued());
287 return;
288 }
289 }
290
291 for (auto it = p->snapQueue.begin(); it != p->snapQueue.end(); ++it) {
292 QSharedPointer<Notification> n = *it;
293
294 if (n && n->getID() == id) {
295 p->snapQueue.erase(it);
296 Q_EMIT queueSizeChanged(queued());
297 return;
298 }
299 }
300
301 for (auto it = p->interactiveQueue.begin(); it != p->interactiveQueue.end(); ++it) {
302 QSharedPointer<Notification> n = *it;
303
304 if (n && n->getID() == id) {
305 p->interactiveQueue.erase(it);
306 Q_EMIT queueSizeChanged(queued());
307 return;
308 }
309 }
310
303 // The ID was not found in any queue. Should it be an error case or not?311 // The ID was not found in any queue. Should it be an error case or not?
304}312}
305313
306314
=== modified file 'src/NotificationServer.cpp'
--- src/NotificationServer.cpp 2015-07-06 18:13:16 +0000
+++ src/NotificationServer.cpp 2015-10-13 14:11:48 +0000
@@ -144,7 +144,7 @@
144 expireTimeout = 5000;144 expireTimeout = 5000;
145 }145 }
146146
147 QSharedPointer<Notification> n(new Notification(id, expireTimeout, urg, ntype, this));147 QSharedPointer<Notification> n(new Notification(id, expireTimeout, urg, ntype, this), &QObject::deleteLater);
148 connect(n.data(), SIGNAL(dataChanged(unsigned int)), this, SLOT(onDataChanged(unsigned int)));148 connect(n.data(), SIGNAL(dataChanged(unsigned int)), this, SLOT(onDataChanged(unsigned int)));
149 connect(n.data(), SIGNAL(completed(unsigned int)), this, SLOT(onCompleted(unsigned int)));149 connect(n.data(), SIGNAL(completed(unsigned int)), this, SLOT(onCompleted(unsigned int)));
150150
@@ -156,15 +156,15 @@
156 // Spec forbids zero as return value.156 // Spec forbids zero as return value.
157 if(idCounter == 0) {157 if(idCounter == 0) {
158 idCounter = 1;158 idCounter = 1;
159}159 }
160}160}
161161
162QString NotificationServer::messageSender() {162QString NotificationServer::messageSender() {
163 QString sender(LOCAL_OWNER);163 QString sender(LOCAL_OWNER);
164 if (calledFromDBus()) {164 if (calledFromDBus()) {
165 sender = message().service();165 sender = message().service();
166 }166 }
167 return sender;167 return sender;
168}168}
169169
170bool NotificationServer::isCmdLine() {170bool NotificationServer::isCmdLine() {
171171
=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt 2015-06-01 09:42:45 +0000
+++ test/CMakeLists.txt 2015-10-13 14:11:48 +0000
@@ -1,4 +1,3 @@
1
2function(add_notification_test NAME)1function(add_notification_test NAME)
3 add_executable(${NAME} "${NAME}.cpp")2 add_executable(${NAME} "${NAME}.cpp")
43
54
=== modified file 'test/notificationtest.cpp'
--- test/notificationtest.cpp 2014-10-09 08:26:38 +0000
+++ test/notificationtest.cpp 2015-10-13 14:11:48 +0000
@@ -1,5 +1,6 @@
1#include "Notification.h"1#include "Notification.h"
2#include "NotificationModel.h"2#include "NotificationModel.h"
3#include "NotificationServer.h"
34
4#include <QtTest/QtTest>5#include <QtTest/QtTest>
56
@@ -24,6 +25,7 @@
24 void testConfirmationValue();25 void testConfirmationValue();
25 void testTextFilter();26 void testTextFilter();
26 void testTextFilter_data();27 void testTextFilter_data();
28 void testReverseClose();
27};29};
2830
29void TestNotifications::testSimpleInsertion() {31void TestNotifications::testSimpleInsertion() {
@@ -55,43 +57,50 @@
55}57}
5658
57void TestNotifications::testOrder() {59void TestNotifications::testOrder() {
58 const int timeout = 5000;60 const int timeout = 1000;
59 QSharedPointer<Notification> n1(new Notification(1, timeout, Notification::Low, "low", Notification::Ephemeral));61 static NotificationModel *m = new NotificationModel(this);
60 QSharedPointer<Notification> n2(new Notification(2, timeout, Notification::Normal, "high", Notification::Ephemeral));62 static NotificationServer *s = new NotificationServer(QDBusConnection::sessionBus(), *m);
61 QSharedPointer<Notification> n3(new Notification(3, timeout, Notification::Critical, "critical", Notification::Ephemeral));63 QStringList actions = QStringList();
62 NotificationModel m;64 QVariantMap hints;
6365 int id[3];
64 m.insertNotification(n1);66
65 QVERIFY(m.showingNotification(n1->getID()));67 hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Low);
6668 id[0] = s->Notify ("test-name-low", 0, "icon-low", "summary-low", "body-low", actions, hints, timeout);
67 m.insertNotification(n2);69 QVERIFY(m->showingNotification(id[0]));
68 QVERIFY(!m.showingNotification(n1->getID()));70
69 QVERIFY(m.showingNotification(n2->getID()));71 hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Normal);
70 QVERIFY(m.queued() == 1);72 id[1] = s->Notify ("test-name-normal", 0, "icon-normal", "summary-normal", "body-normal", actions, hints, timeout);
7173 QVERIFY(!m->showingNotification(id[0]));
72 m.insertNotification(n3);74 QVERIFY(m->showingNotification(id[1]));
73 QVERIFY(!m.showingNotification(n1->getID()));75 QVERIFY(m->queued() == 1);
74 QVERIFY(!m.showingNotification(n2->getID()));76
75 QVERIFY(m.showingNotification(n3->getID()));77 hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Critical);
76 QCOMPARE(m.queued(), 2);78 id[2] = s->Notify ("test-name-critical", 0, "icon-critical", "summary-critical", "body-critical", actions, hints, timeout);
7779 QVERIFY(!m->showingNotification(id[0]));
78 m.removeNotification(n3->getID());80 QVERIFY(!m->showingNotification(id[1]));
79 QVERIFY(!m.showingNotification(n1->getID()));81 QVERIFY(m->showingNotification(id[2]));
80 QVERIFY(m.showingNotification(n2->getID()));82 QCOMPARE(m->queued(), 2);
81 QVERIFY(!m.showingNotification(n3->getID()));83
82 QCOMPARE(m.queued(), 1);84 m->removeNotification(id[2]);
8385 QVERIFY(!m->showingNotification(id[0]));
84 m.removeNotification(n2->getID());86 QVERIFY(m->showingNotification(id[1]));
85 QVERIFY(m.showingNotification(n1->getID()));87 QVERIFY(!m->showingNotification(id[2]));
86 QVERIFY(!m.showingNotification(n2->getID()));88 QCOMPARE(m->queued(), 1);
87 QVERIFY(!m.showingNotification(n3->getID()));89
88 QCOMPARE(m.queued(), 0);90 m->removeNotification(id[1]);
8991 QVERIFY(m->showingNotification(id[0]));
90 m.removeNotification(n1->getID());92 QVERIFY(!m->showingNotification(id[1]));
91 QVERIFY(!m.showingNotification(n1->getID()));93 QVERIFY(!m->showingNotification(id[2]));
92 QVERIFY(!m.showingNotification(n2->getID()));94 QCOMPARE(m->queued(), 0);
93 QVERIFY(!m.showingNotification(n3->getID()));95
94 QCOMPARE(m.queued(), 0);96 m->removeNotification(id[0]);
97 QVERIFY(!m->showingNotification(id[0]));
98 QVERIFY(!m->showingNotification(id[1]));
99 QVERIFY(!m->showingNotification(id[2]));
100 QCOMPARE(m->queued(), 0);
101
102 delete s;
103 delete m;
95}104}
96105
97void TestNotifications::testHas() {106void TestNotifications::testHas() {
@@ -259,5 +268,38 @@
259 QCOMPARE(n.getBody(), result);268 QCOMPARE(n.getBody(), result);
260}269}
261270
271void TestNotifications::testReverseClose() {
272 const int timeout = 1000;
273 const int max = 20;
274 static NotificationModel *m = new NotificationModel();
275 static NotificationServer *s = new NotificationServer(QDBusConnection::sessionBus(), *m);
276 QStringList actions = QStringList();
277 QVariantMap hints;
278 int before = m->numNotifications();
279
280 for (int i = 1; i <= max; i++) {
281 s->Notify ("test-app-name",
282 0,
283 "test-icon",
284 "test-summary",
285 "test-body",
286 actions,
287 hints,
288 timeout);
289 }
290
291 QCOMPARE(m->numNotifications(), max+1);
292
293 for (int i = max; i >= 1; i--) {
294 m->getNotification(i)->close();
295 QCOMPARE(m->numNotifications(), i);
296 }
297
298 QCOMPARE(m->numNotifications(), before);
299
300 delete s;
301 delete m;
302}
303
262QTEST_MAIN(TestNotifications)304QTEST_MAIN(TestNotifications)
263#include "notificationtest.moc"305#include "notificationtest.moc"

Subscribers

People subscribed via source and target branches

to all changes: